Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: including public channels should check for compliance settings #102

Merged
merged 10 commits into from
Sep 17, 2024
Merged

fix: including public channels should check for compliance settings #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
071c0bb
validate in the api
fmartingr Sep 13, 2024
8ed9f62
validate the dates and the compliance status
fmartingr Sep 13, 2024
f6f2861
removed kvstore validation
fmartingr Sep 13, 2024
ba138a5
make check-style
fmartingr Sep 16, 2024
4d32047
fixed test defaults
fmartingr Sep 16, 2024
ec63c8f
pass only compliance instead of entire config
fmartingr Sep 16, 2024
eca5630
make check-style
fmartingr Sep 16, 2024
b609c39
compliance monitoring typo
fmartingr Sep 16, 2024
403c42b
removed redundant check
fmartingr Sep 16, 2024
01da804
updated tests
fmartingr Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions server/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,19 @@ func (p *Plugin) createLegalHold(w http.ResponseWriter, r *http.Request) {
}

legalHold := model.NewLegalHoldFromCreate(createLegalHold)
// TODO: Validate all the provided data here?

config := p.API.GetConfig()
if config == nil {
http.Error(w, "failed to get config", http.StatusInternalServerError)
return
}

mmConfigComplianceEnabled := config.ComplianceSettings.Enable != nil && *config.ComplianceSettings.Enable

if err := legalHold.IsValidForCreate(mmConfigComplianceEnabled); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}

savedLegalHold, err := p.KVStore.CreateLegalHold(legalHold)
if err != nil {
Expand Down Expand Up @@ -178,7 +190,15 @@ func (p *Plugin) updateLegalHold(w http.ResponseWriter, r *http.Request) {
return
}

if err = updateLegalHold.IsValid(); err != nil {
config := p.API.GetConfig()
if config == nil {
http.Error(w, "failed to get config", http.StatusInternalServerError)
return
}

mmConfigComplianceEnabled := config.ComplianceSettings.Enable != nil && *config.ComplianceSettings.Enable

if err = updateLegalHold.IsValid(mmConfigComplianceEnabled); err != nil {
http.Error(w, "LegalHold update data is not valid", http.StatusBadRequest)
p.Client.Log.Error(err.Error())
return
Expand Down
16 changes: 14 additions & 2 deletions server/model/legal_hold.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (lh *LegalHold) DeepCopy() LegalHold {
// failure. It does not guarantee that creation in the store will be successful,
// as other issues such as non-unique ID value can still cause the LegalHold to
// fail to save.
func (lh *LegalHold) IsValidForCreate() error {
func (lh *LegalHold) IsValidForCreate(mmConfigComplianceEnabled bool) error {
if !mattermostModel.IsValidId(lh.ID) {
return fmt.Errorf("LegalHold ID is not valid: %s", lh.ID)
}
Expand Down Expand Up @@ -93,6 +93,14 @@ func (lh *LegalHold) IsValidForCreate() error {
return errors.New("LegalHold must end at a valid time or zero")
}

if lh.EndsAt > 0 && lh.StartsAt > lh.EndsAt {
return errors.New("LegalHold must end after it starts")
}

if !lh.ExcludePublicChannels && !mmConfigComplianceEnabled {
return errors.New("Compliance monitoring must be enabled on the Mattermost server in order to include public channels in a LegalHold")
}

return nil
}

Expand Down Expand Up @@ -166,7 +174,7 @@ type UpdateLegalHold struct {
EndsAt int64 `json:"ends_at"`
}

func (ulh UpdateLegalHold) IsValid() error {
func (ulh UpdateLegalHold) IsValid(mmConfigComplianceEnabled bool) error {
if !mattermostModel.IsValidId(ulh.ID) {
return fmt.Errorf("LegalHold ID is not valid: %s", ulh.ID)
}
Expand All @@ -185,6 +193,10 @@ func (ulh UpdateLegalHold) IsValid() error {
}
}

if !ulh.ExcludePublicChannels && !mmConfigComplianceEnabled {
return errors.New("Compliance monitoring must be enabled on the Mattermost server in order to include public channels in a LegalHold")
}

if ulh.EndsAt < 0 {
return errors.New("LegalHold must end at a valid time or zero")
}
Expand Down
110 changes: 88 additions & 22 deletions server/model/legal_hold_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,21 @@ func TestModel_LegalHold_DeepCopy(t *testing.T) {

func TestModel_LegalHold_IsValidForCreate(t *testing.T) {
tests := []struct {
name string
lh *LegalHold
wantErr bool
name string
lh *LegalHold
wantErr bool
mmConfigComplianceEnabled bool
}{
{
name: "Valid",
lh: &LegalHold{
ID: mattermostModel.NewId(),
Name: "legalhold1",
DisplayName: "Test Legal Hold",
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 10,
EndsAt: 0,
ID: mattermostModel.NewId(),
Name: "legalhold1",
DisplayName: "Test Legal Hold",
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 10,
EndsAt: 0,
ExcludePublicChannels: true,
},
wantErr: false,
},
Expand Down Expand Up @@ -198,11 +200,51 @@ func TestModel_LegalHold_IsValidForCreate(t *testing.T) {
},
wantErr: true,
},
{
name: "EndsAt before StartsAt",
lh: &LegalHold{
ID: mattermostModel.NewId(),
Name: "legalhold1",
DisplayName: "EndsAt before StartsAt Test",
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 90,
EndsAt: 80,
},
wantErr: true,
},
{
name: "Include public channels compliance disabled",
lh: &LegalHold{
ID: mattermostModel.NewId(),
Name: "legalhold1",
DisplayName: "Exclude public channels without compliance enabled Test",
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 100,
EndsAt: 0,
ExcludePublicChannels: false,
},
wantErr: true,
mmConfigComplianceEnabled: false,
},
{
name: "Exclude public channels with compliance enabled",
lh: &LegalHold{
ID: mattermostModel.NewId(),
Name: "legalhold1",
DisplayName: "Exclude public channels without compliance enabled Test",
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 100,
EndsAt: 0,
ExcludePublicChannels: true,
},
wantErr: false,
mmConfigComplianceEnabled: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.lh.IsValidForCreate()
err := tt.lh.IsValidForCreate(tt.mmConfigComplianceEnabled)
if tt.wantErr {
require.Error(t, err)
} else {
Expand Down Expand Up @@ -419,17 +461,19 @@ func TestModel_LegalHold_BasePath(t *testing.T) {

func TestModel_UpdateLegalHold_IsValid(t *testing.T) {
testCases := []struct {
name string
ulh UpdateLegalHold
expected string
name string
ulh UpdateLegalHold
expected string
mmConfigComplianceEnabled bool
}{
{
name: "Valid",
ulh: UpdateLegalHold{
ID: model.NewId(),
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
EndsAt: 0,
ID: model.NewId(),
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
EndsAt: 0,
ExcludePublicChannels: true,
},
expected: "",
},
Expand Down Expand Up @@ -476,18 +520,40 @@ func TestModel_UpdateLegalHold_IsValid(t *testing.T) {
{
name: "NegativeEndsAt",
ulh: UpdateLegalHold{
ID: model.NewId(),
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
EndsAt: -1,
ID: model.NewId(),
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
ExcludePublicChannels: true,
EndsAt: -1,
},
expected: "LegalHold must end at a valid time or zero",
},
{
name: "Include public channels and compliance disabled",
ulh: UpdateLegalHold{
ID: mattermostModel.NewId(),
UserIDs: []string{mattermostModel.NewId()},
DisplayName: "TestName",
ExcludePublicChannels: false,
},
mmConfigComplianceEnabled: false,
expected: "Compliance monitoring must be enabled on the Mattermost server in order to include public channels in a LegalHold",
},
{
name: "Include public channels and compliance enabled",
ulh: UpdateLegalHold{
ID: mattermostModel.NewId(),
UserIDs: []string{mattermostModel.NewId()},
DisplayName: "TestName",
ExcludePublicChannels: false,
},
mmConfigComplianceEnabled: true,
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := testCase.ulh.IsValid()
err := testCase.ulh.IsValid(testCase.mmConfigComplianceEnabled)
if err != nil {
if err.Error() != testCase.expected {
t.Errorf("expected: %s, got: %s", testCase.expected, err.Error())
Expand Down
8 changes: 0 additions & 8 deletions server/store/kvstore/legal_hold.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ func NewKVStore(client *pluginapi.Client) KVStore {
}

func (kvs Impl) CreateLegalHold(lh model.LegalHold) (*model.LegalHold, error) {
if err := lh.IsValidForCreate(); err != nil {
return nil, errors.Wrap(err, "LegalHold is not valid for create")
}

lh.CreateAt = mattermostModel.GetMillis()
lh.UpdateAt = lh.CreateAt
lh.Secret = mattermostModel.NewId()
Expand Down Expand Up @@ -86,10 +82,6 @@ func (kvs Impl) GetLegalHoldByID(id string) (*model.LegalHold, error) {
}

func (kvs Impl) UpdateLegalHold(lh, oldValue model.LegalHold) (*model.LegalHold, error) {
if err := lh.IsValidForCreate(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would feel safer if we also kept the store level validation because we call this method from the job layer as well. We only call CreateLegalHold from the api layer but maybe we should also keep that validation in the store layer to protect ourselves from future modifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that we can reach this from the job layer, let me take a look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been checking the code, the job itself only updates one field currently outside of the scope of model.UpdateLegalHold struct, and the code does not follow the same logic path.

https://github.com/mattermost/mattermost-plugin-legal-hold/blob/main/server/jobs/legal_hold_job.go#L221

Do you think it would add value to add this check here considering we are updating the value atomically?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it just future proofs the code even if it doesn't make a difference right now. We do this in mattermost-server for a lot of our create/update store functions. @esarafianou what is our official stance on this? It's not entirely consistent in our server code either

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a ticket for this an address for subsequent release. We are in the final stage of releasing this to several customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #105

return nil, errors.Wrap(err, "LegalHold is not valid for create")
}

lh.UpdateAt = mattermostModel.GetMillis()

key := fmt.Sprintf("%s%s", legalHoldPrefix, lh.ID)
Expand Down
11 changes: 0 additions & 11 deletions server/store/kvstore/legal_hold_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,17 +201,6 @@ func TestKVStore_UpdateLegalHold(t *testing.T) {
assert.Equal(t, lh3.ExecutionLength, lh2.ExecutionLength)
assert.NotEqual(t, lh3.UpdateAt, lh2.UpdateAt)
assert.Equal(t, lh3.CreateAt, lh2.UpdateAt)

// Test updating a legal hold that does not exist
lh4 := model.LegalHold{
ID: "doesnotexist",
Name: "legal-hold-4",
DisplayName: "Legal Hold 4",
UserIDs: []string{mattermostModel.NewId()},
StartsAt: mattermostModel.GetMillis(),
}
_, err = kvstore.UpdateLegalHold(lh4, *lh3)
require.Error(t, err)
}

func TestKVStore_DeleteLegalHold(t *testing.T) {
Expand Down
Loading