Skip to content

Commit

Permalink
Remove compliance check from the code (#116)
Browse files Browse the repository at this point in the history 
* remove compliance check
* renamed exclude public channels to include public channels
  • Loading branch information
@fmartingr
fmartingr authored Oct 1, 2024
1 parent 1b4c8c0 commit 30cf4d6
Show file tree
Hide file tree
Showing 12 changed files with 92 additions and 131 deletions.
10 changes: 3 additions & 7 deletions server/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ func (p *Plugin) createLegalHold(w http.ResponseWriter, r *http.Request) {
return
}

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

if err := legalHold.IsValidForCreate(mmConfigComplianceEnabled); err != nil {
if err := legalHold.IsValidForCreate(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
Expand Down Expand Up @@ -196,9 +194,7 @@ func (p *Plugin) updateLegalHold(w http.ResponseWriter, r *http.Request) {
return
}

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

if err = updateLegalHold.IsValid(mmConfigComplianceEnabled); err != nil {
if err = updateLegalHold.IsValid(); err != nil {
http.Error(w, "LegalHold update data is not valid", http.StatusBadRequest)
p.Client.Log.Error(err.Error())
return
Expand All @@ -207,7 +203,7 @@ func (p *Plugin) updateLegalHold(w http.ResponseWriter, r *http.Request) {
// Retrieve the legal hold we are modifying
originalLegalHold, err := p.KVStore.GetLegalHoldByID(legalholdID)
if err != nil {
http.Error(w, "failed to update legal hold", http.StatusInternalServerError)
http.Error(w, err.Error(), http.StatusInternalServerError)
p.Client.Log.Error(err.Error())
return
}
Expand Down
2 changes: 1 addition & 1 deletion server/legalhold/legal_hold.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (ex *Execution) GetChannels() error {
return appErr
}

channelIDs, err := ex.store.GetChannelIDsForUserDuring(userID, ex.ExecutionStartTime, ex.ExecutionEndTime, ex.LegalHold.ExcludePublicChannels)
channelIDs, err := ex.store.GetChannelIDsForUserDuring(userID, ex.ExecutionStartTime, ex.ExecutionEndTime, ex.LegalHold.IncludePublicChannels)
if err != nil {
return err
}
Expand Down
24 changes: 8 additions & 16 deletions server/model/legal_hold.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type LegalHold struct {
UserIDs []string `json:"user_ids"`
StartsAt int64 `json:"starts_at"`
EndsAt int64 `json:"ends_at"`
ExcludePublicChannels bool `json:"exclude_public_channels"`
IncludePublicChannels bool `json:"include_public_channels"`
LastExecutionEndedAt int64 `json:"last_execution_ended_at"`
ExecutionLength int64 `json:"execution_length"`
Secret string `json:"secret"`
Expand All @@ -39,7 +39,7 @@ func (lh *LegalHold) DeepCopy() LegalHold {
UpdateAt: lh.UpdateAt,
StartsAt: lh.StartsAt,
EndsAt: lh.EndsAt,
ExcludePublicChannels: lh.ExcludePublicChannels,
IncludePublicChannels: lh.IncludePublicChannels,
LastExecutionEndedAt: lh.LastExecutionEndedAt,
ExecutionLength: lh.ExecutionLength,
Secret: lh.Secret,
Expand All @@ -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(mmConfigComplianceEnabled bool) error {
func (lh *LegalHold) IsValidForCreate() error {
if !mattermostModel.IsValidId(lh.ID) {
return fmt.Errorf("LegalHold ID is not valid: %s", lh.ID)
}
Expand Down Expand Up @@ -97,10 +97,6 @@ func (lh *LegalHold) IsValidForCreate(mmConfigComplianceEnabled bool) error {
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 @@ -146,7 +142,7 @@ type CreateLegalHold struct {
UserIDs []string `json:"user_ids"`
StartsAt int64 `json:"starts_at"`
EndsAt int64 `json:"ends_at"`
ExcludePublicChannels bool `json:"exclude_public_channels"`
IncludePublicChannels bool `json:"include_public_channels"`
}

// NewLegalHoldFromCreate creates and populates a new LegalHold instance from
Expand All @@ -159,7 +155,7 @@ func NewLegalHoldFromCreate(lhc CreateLegalHold) LegalHold {
UserIDs: lhc.UserIDs,
StartsAt: lhc.StartsAt,
EndsAt: lhc.EndsAt,
ExcludePublicChannels: lhc.ExcludePublicChannels,
IncludePublicChannels: lhc.IncludePublicChannels,
LastExecutionEndedAt: 0,
ExecutionLength: 86400000,
}
Expand All @@ -170,11 +166,11 @@ type UpdateLegalHold struct {
ID string `json:"id"`
DisplayName string `json:"display_name"`
UserIDs []string `json:"user_ids"`
ExcludePublicChannels bool `json:"exclude_public_channels"`
IncludePublicChannels bool `json:"include_public_channels"`
EndsAt int64 `json:"ends_at"`
}

func (ulh UpdateLegalHold) IsValid(mmConfigComplianceEnabled bool) error {
func (ulh UpdateLegalHold) IsValid() error {
if !mattermostModel.IsValidId(ulh.ID) {
return fmt.Errorf("LegalHold ID is not valid: %s", ulh.ID)
}
Expand All @@ -193,10 +189,6 @@ func (ulh UpdateLegalHold) IsValid(mmConfigComplianceEnabled bool) 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 All @@ -208,5 +200,5 @@ func (lh *LegalHold) ApplyUpdates(updates UpdateLegalHold) {
lh.DisplayName = updates.DisplayName
lh.UserIDs = updates.UserIDs
lh.EndsAt = updates.EndsAt
lh.ExcludePublicChannels = updates.ExcludePublicChannels
lh.IncludePublicChannels = updates.IncludePublicChannels
}
73 changes: 11 additions & 62 deletions server/model/legal_hold_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,9 @@ func TestModel_LegalHold_DeepCopy(t *testing.T) {

func TestModel_LegalHold_IsValidForCreate(t *testing.T) {
tests := []struct {
name string
lh *LegalHold
wantErr bool
mmConfigComplianceEnabled bool
name string
lh *LegalHold
wantErr bool
}{
{
name: "Valid",
Expand All @@ -76,7 +75,7 @@ func TestModel_LegalHold_IsValidForCreate(t *testing.T) {
UserIDs: []string{mattermostModel.NewId(), mattermostModel.NewId()},
StartsAt: 10,
EndsAt: 0,
ExcludePublicChannels: true,
IncludePublicChannels: false,
},
wantErr: false,
},
Expand Down Expand Up @@ -212,39 +211,11 @@ func TestModel_LegalHold_IsValidForCreate(t *testing.T) {
},
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(tt.mmConfigComplianceEnabled)
err := tt.lh.IsValidForCreate()
if tt.wantErr {
require.Error(t, err)
} else {
Expand Down Expand Up @@ -461,10 +432,9 @@ func TestModel_LegalHold_BasePath(t *testing.T) {

func TestModel_UpdateLegalHold_IsValid(t *testing.T) {
testCases := []struct {
name string
ulh UpdateLegalHold
expected string
mmConfigComplianceEnabled bool
name string
ulh UpdateLegalHold
expected string
}{
{
name: "Valid",
Expand All @@ -473,7 +443,7 @@ func TestModel_UpdateLegalHold_IsValid(t *testing.T) {
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
EndsAt: 0,
ExcludePublicChannels: true,
IncludePublicChannels: false,
},
expected: "",
},
Expand Down Expand Up @@ -523,37 +493,16 @@ func TestModel_UpdateLegalHold_IsValid(t *testing.T) {
ID: model.NewId(),
DisplayName: "TestName",
UserIDs: []string{model.NewId()},
ExcludePublicChannels: true,
IncludePublicChannels: false,
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(testCase.mmConfigComplianceEnabled)
err := testCase.ulh.IsValid()
if err != nil {
if err.Error() != testCase.expected {
t.Errorf("expected: %s, got: %s", testCase.expected, err.Error())
Expand Down
6 changes: 3 additions & 3 deletions server/store/sqlstore/legal_hold.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,16 @@ func (ss SQLStore) GetPostsBatch(channelID string, endTime int64, cursor model.L
// GetChannelIDsForUserDuring gets the channel IDs for all channels that the user indicated by userID is
// a member of during the time period from (and including) the startTime up until (but not including) the
// endTime.
func (ss SQLStore) GetChannelIDsForUserDuring(userID string, startTime int64, endTime int64, excludePublic bool) ([]string, error) {
func (ss SQLStore) GetChannelIDsForUserDuring(userID string, startTime int64, endTime int64, includePublic bool) ([]string, error) {
query := ss.replicaBuilder.
Select("distinct(cmh.channelid)").
From("ChannelMemberHistory as cmh").
Where(sq.Lt{"cmh.jointime": endTime}).
Where(sq.Or{sq.Eq{"cmh.leavetime": nil}, sq.GtOrEq{"cmh.leavetime": startTime}}).
Where(sq.Eq{"cmh.userid": userID})

// Exclude all public channels from the results
if excludePublic {
// Exclude all public channels from the results if includePublic is false.
if !includePublic {
query = query.Join("Channels on cmh.channelid = Channels.id").
Where(sq.NotEq{"Channels.type": mattermostModel.ChannelTypeOpen})
}
Expand Down
6 changes: 3 additions & 3 deletions server/store/sqlstore/legal_hold_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func TestSQLStore_LegalHold_GetChannelIDsForUserDuring(t *testing.T) {
require.NoError(t, th.mmStore.ChannelMemberHistory().LogLeaveEvent(th.User1.Id, channels[9].Id, endTwo-1000))

// Check channel IDs for first window.
firstWindowChannelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, startOne, endOne, false)
firstWindowChannelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, startOne, endOne, true)
expectedOne := []string{
channels[1].Id,
channels[2].Id,
Expand All @@ -133,7 +133,7 @@ func TestSQLStore_LegalHold_GetChannelIDsForUserDuring(t *testing.T) {
require.ElementsMatch(t, firstWindowChannelIDs, expectedOne)

// Check channel IDs for second window.
secondWindowChannelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, startTwo, endTwo, false)
secondWindowChannelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, startTwo, endTwo, true)
expectedTwo := []string{
channels[3].Id,
channels[4].Id,
Expand Down Expand Up @@ -168,7 +168,7 @@ func TestLegalHold_GetChannelIDsForUserDuring_ExcludePublic(t *testing.T) {
require.NoError(t, th.mmStore.ChannelMemberHistory().LogJoinEvent(th.User1.Id, dmChannel.Id, start+1000))

// Check channel IDs
channelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, start, end, true)
channelIDs, err := th.Store.GetChannelIDsForUserDuring(th.User1.Id, start, end, false)
require.NoError(t, err)
require.ElementsMatch(t, channelIDs, []string{privateChannel.Id, dmChannel.Id, groupDM.Id})
}
Expand Down
29 changes: 20 additions & 9 deletions webapp/src/components/create_legal_hold_form.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const CreateLegalHoldForm = (props: CreateLegalHoldFormProps) => {
const [startsAt, setStartsAt] = useState('');
const [endsAt, setEndsAt] = useState('');
const [saving, setSaving] = useState(false);
const [excludePublicChannels, setExcludePublicChannels] = useState(false);
const [includePublicChannels, setIncludePublicChannels] = useState(false);
const [serverError, setServerError] = useState('');

const displayNameChanged = (e: React.ChangeEvent<HTMLInputElement>) => {
Expand All @@ -36,8 +36,8 @@ const CreateLegalHoldForm = (props: CreateLegalHoldFormProps) => {
setEndsAt(e.target.value);
};

const excludePublicChannelsChanged: (e: React.ChangeEvent<HTMLInputElement>) => void = (e) => {
setExcludePublicChannels(e.target.checked);
const includePublicChannelsChanged: (e: React.ChangeEvent<HTMLInputElement>) => void = (e) => {
setIncludePublicChannels(e.target.checked);
};

const resetForm = () => {
Expand All @@ -46,6 +46,7 @@ const CreateLegalHoldForm = (props: CreateLegalHoldFormProps) => {
setEndsAt('');
setUsers([]);
setSaving(false);
setIncludePublicChannels(false);
setServerError('');
};

Expand All @@ -60,7 +61,7 @@ const CreateLegalHoldForm = (props: CreateLegalHoldFormProps) => {
ends_at: (new Date(endsAt)).getTime(),
starts_at: (new Date(startsAt)).getTime(),
display_name: displayName,
exclude_public_channels: excludePublicChannels,
include_public_channels: includePublicChannels,
name: slugify(displayName),
};

Expand Down Expand Up @@ -152,15 +153,25 @@ const CreateLegalHoldForm = (props: CreateLegalHoldFormProps) => {
>
<input
type='checkbox'
id='legal-hold-exclude-public-channels'
checked={excludePublicChannels}
onChange={excludePublicChannelsChanged}
id='legal-hold-include-public-channels'
checked={includePublicChannels}
onChange={includePublicChannelsChanged}
className={'create-legal-hold-checkbox'}
/>
<label htmlFor={'legal-hold-exclude-public-channels'}>
{'Exclude public channels'}
<label htmlFor={'legal-hold-include-public-channels'}>
{'Include public channels'}
</label>
</div>
<div
style={{
display: (includePublicChannels) ? 'block' : 'none',
marginTop: '-20px',
marginBottom: '20px',
}}
>
<i className='icon icon-alert-outline'/>
<span>{'It is possible for users to access public content without becoming members of a public channel. This setting only captures public channels that users are members of.'}</span>
</div>
<div
style={{
display: 'flex',
Expand Down
Loading

0 comments on commit 30cf4d6

Please sign in to comment.