diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 09744567d87..34cd9859b12 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -79,6 +79,15 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. policy.ID = model.NewId() } + policy.Version = model.AccessControlPolicyVersionV0_3 + for i, rule := range policy.Rules { + for j, action := range rule.Actions { + if action == "*" { + policy.Rules[i].Actions[j] = model.AccessControlPolicyActionMembership + } + } + } + var appErr *model.AppError policy, appErr = acs.SavePolicy(rctx, policy) if appErr != nil { @@ -173,7 +182,7 @@ func (a *App) AssignAccessControlPolicyToChannels(rctx request.CTX, parentID str Props: map[string]any{}, } } - child.Version = model.AccessControlPolicyVersionV0_2 + child.Version = model.AccessControlPolicyVersionV0_3 appErr := child.Inherit(policy) if appErr != nil { diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 549d3e5bf36..24bde177173 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -17,6 +17,96 @@ import ( "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" ) +func TestCreateOrUpdateAccessControlPolicy(t *testing.T) { + th := Setup(t).InitBasic(t) + + t.Run("Feature not enabled", func(t *testing.T) { + th.App.Srv().ch.AccessControl = nil + + policy := &model.AccessControlPolicy{ + Type: model.AccessControlPolicyTypeParent, + Name: "test-policy", + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"*"}, Expression: "true"}, + }, + } + result, err := th.App.CreateOrUpdateAccessControlPolicy(th.Context, policy) + require.NotNil(t, err) + require.Nil(t, result) + }) + + t.Run("Wildcard actions rewritten to membership and version set to v0.3", func(t *testing.T) { + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + + policy := &model.AccessControlPolicy{ + Type: model.AccessControlPolicyTypeParent, + Name: "wildcard-rewrite", + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"*"}, Expression: "user.attributes.team == \"eng\""}, + }, + } + + mockAccessControl.On("SavePolicy", th.Context, mock.MatchedBy(func(p *model.AccessControlPolicy) bool { + return p.Version == model.AccessControlPolicyVersionV0_3 && + len(p.Rules) == 1 && + len(p.Rules[0].Actions) == 1 && + p.Rules[0].Actions[0] == model.AccessControlPolicyActionMembership + })).Return(policy, nil).Once() + + result, err := th.App.CreateOrUpdateAccessControlPolicy(th.Context, policy) + require.Nil(t, err) + require.NotNil(t, result) + mockAccessControl.AssertExpectations(t) + }) + + t.Run("Multiple rules with mixed actions", func(t *testing.T) { + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + + policy := &model.AccessControlPolicy{ + Type: model.AccessControlPolicyTypeParent, + Name: "mixed-actions", + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"*"}, Expression: "expr1"}, + {Actions: []string{model.AccessControlPolicyActionUploadFileAttachment}, Expression: "expr2"}, + }, + } + + mockAccessControl.On("SavePolicy", th.Context, mock.MatchedBy(func(p *model.AccessControlPolicy) bool { + return p.Rules[0].Actions[0] == model.AccessControlPolicyActionMembership && + p.Rules[1].Actions[0] == model.AccessControlPolicyActionUploadFileAttachment + })).Return(policy, nil).Once() + + result, err := th.App.CreateOrUpdateAccessControlPolicy(th.Context, policy) + require.Nil(t, err) + require.NotNil(t, result) + mockAccessControl.AssertExpectations(t) + }) + + t.Run("Generates ID when empty", func(t *testing.T) { + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + + policy := &model.AccessControlPolicy{ + Type: model.AccessControlPolicyTypeParent, + Name: "no-id", + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"}, + }, + } + + mockAccessControl.On("SavePolicy", th.Context, mock.MatchedBy(func(p *model.AccessControlPolicy) bool { + return p.ID != "" && model.IsValidId(p.ID) + })).Return(policy, nil).Once() + + result, err := th.App.CreateOrUpdateAccessControlPolicy(th.Context, policy) + require.Nil(t, err) + require.NotNil(t, result) + mockAccessControl.AssertExpectations(t) + }) +} + func TestGetChannelsForPolicy(t *testing.T) { th := Setup(t).InitBasic(t) diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 5514a4a75e5..57637883c18 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -1771,7 +1771,7 @@ func (a *App) addUserToChannel(rctx request.CTX, user *model.User, channel *mode Type: model.AccessControlPolicyTypeChannel, ID: channel.Id, }, - Action: "join_channel", + Action: "membership", }) if evalErr != nil { return nil, evalErr diff --git a/server/channels/app/migrations.go b/server/channels/app/migrations.go index 8d8efd27c5c..e898ff0b478 100644 --- a/server/channels/app/migrations.go +++ b/server/channels/app/migrations.go @@ -14,6 +14,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/public/utils" "github.com/mattermost/mattermost/server/v8/channels/store" ) @@ -907,6 +908,73 @@ func (s *Server) doDeleteDmsPreferencesMigration(rctx request.CTX) error { return nil } +func (s *Server) doAccessControlPolicyV0_3Migration(rctx request.CTX) error { + var nfErr *store.ErrNotFound + if _, err := s.Store().System().GetByName(model.MigrationKeyAccessControlPolicyV0_3); err == nil { + return nil + } else if !errors.As(err, &nfErr) { + return fmt.Errorf("could not query migration: %w", err) + } + + policyTypes := []string{model.AccessControlPolicyTypeParent, model.AccessControlPolicyTypeChannel} + + const pageSize = 100 + for _, policyType := range policyTypes { + cursor := model.AccessControlPolicyCursor{} + policies, err := utils.Pager(func(_ int) ([]*model.AccessControlPolicy, error) { + results, _, err := s.Store().AccessControlPolicy().SearchPolicies(rctx, model.AccessControlPolicySearch{ + Type: policyType, + Cursor: cursor, + Limit: pageSize, + }) + if err != nil { + return nil, err + } + if len(results) > 0 { + cursor = model.AccessControlPolicyCursor{ID: results[len(results)-1].ID} + } + return results, nil + }, pageSize) + if err != nil { + return fmt.Errorf("failed to search access control policies: %w", err) + } + + for _, policy := range policies { + if policy.Version != model.AccessControlPolicyVersionV0_2 { + continue + } + + policy.Version = model.AccessControlPolicyVersionV0_3 + for i, rule := range policy.Rules { + for j, action := range rule.Actions { + if action == "*" { + policy.Rules[i].Actions[j] = model.AccessControlPolicyActionMembership + } + } + } + + if _, err := s.Store().AccessControlPolicy().Save(rctx, policy); err != nil { + return fmt.Errorf("failed to save migrated access control policy id=%s: %w", policy.ID, err) + } + + if policy.Type == model.AccessControlPolicyTypeChannel { + s.Store().Channel().InvalidateChannel(policy.ID) + } + } + } + + system := model.System{ + Name: model.MigrationKeyAccessControlPolicyV0_3, + Value: "true", + } + + if err := s.Store().System().Save(&system); err != nil { + return fmt.Errorf("failed to mark access control policy v0.3 migration as completed: %w", err) + } + + return nil +} + func (a *App) DoAppMigrations() { a.Srv().doAppMigrations() } @@ -953,6 +1021,7 @@ func (s *Server) doAppMigrations() { {"Delete Empty Drafts Migration", s.doDeleteEmptyDraftsMigration}, {"Delete Orphan Drafts Migration", s.doDeleteOrphanDraftsMigration}, {"Delete Invalid Dms Preferences Migration", s.doDeleteDmsPreferencesMigration}, + {"Access Control Policy V0.3 Migration", s.doAccessControlPolicyV0_3Migration}, } rctx := request.EmptyContext(s.Log()) diff --git a/server/channels/store/sqlstore/access_control_policy_store.go b/server/channels/store/sqlstore/access_control_policy_store.go index af5cbc07070..0b14597c78e 100644 --- a/server/channels/store/sqlstore/access_control_policy_store.go +++ b/server/channels/store/sqlstore/access_control_policy_store.go @@ -54,16 +54,19 @@ func (s *storeAccessControlPolicy) toModel() (*model.AccessControlPolicy, error) Version: s.Version, } - var p accessControlPolicyV0_1 - if err := json.Unmarshal(s.Data, &p); err != nil { - return nil, err + if len(s.Data) > 0 { + var p accessControlPolicyV0_1 + if err := json.Unmarshal(s.Data, &p); err != nil { + return nil, err + } + policy.Imports = p.Imports + policy.Rules = p.Rules } - policy.Imports = p.Imports - policy.Rules = p.Rules - - if err := json.Unmarshal(s.Props, &policy.Props); err != nil { - return nil, err + if len(s.Props) > 0 { + if err := json.Unmarshal(s.Props, &policy.Props); err != nil { + return nil, err + } } return policy, nil @@ -640,6 +643,18 @@ func (s *SqlAccessControlPolicyStore) SearchPolicies(rctx request.CTX, opts mode count = count.Where(condition) } + if len(opts.Actions) > 0 { + or := sq.Or{} + for _, action := range opts.Actions { + or = append(or, sq.Expr( + "EXISTS (SELECT 1 FROM jsonb_array_elements(CASE WHEN jsonb_typeof(Data->'rules') = 'array' THEN Data->'rules' ELSE '[]'::jsonb END) AS rule WHERE rule->'actions' @> ?::jsonb)", + fmt.Sprintf("%q", action), + )) + } + query = query.Where(or) + count = count.Where(or) + } + if opts.Active { query = query.Where(sq.Eq{"Active": true}) count = count.Where(sq.Eq{"Active": true}) diff --git a/server/channels/store/storetest/access_control_policy_store.go b/server/channels/store/storetest/access_control_policy_store.go index 347950aeac1..928d19c79aa 100644 --- a/server/channels/store/storetest/access_control_policy_store.go +++ b/server/channels/store/storetest/access_control_policy_store.go @@ -21,6 +21,7 @@ func TestAccessControlPolicyStore(t *testing.T, rctx request.CTX, ss store.Store t.Run("SetActiveMultiple", func(t *testing.T) { testAccessControlPolicyStoreSetActiveMultiple(t, rctx, ss) }) t.Run("GetAll", func(t *testing.T) { testAccessControlPolicyStoreGetAll(t, rctx, ss) }) t.Run("Search", func(t *testing.T) { testAccessControlPolicyStoreSearch(t, rctx, ss) }) + t.Run("SearchByActions", func(t *testing.T) { testAccessControlPolicyStoreSearchByActions(t, rctx, ss) }) t.Run("GetPoliciesByFieldID", func(t *testing.T) { testAccessControlPolicyStoreGetPoliciesByFieldID(t, rctx, ss) }) } @@ -707,6 +708,134 @@ func testAccessControlPolicyStoreSearch(t *testing.T, rctx request.CTX, ss store }) } +func testAccessControlPolicyStoreSearchByActions(t *testing.T, rctx request.CTX, ss store.Store) { + membershipOnly := &model.AccessControlPolicy{ + ID: model.NewId(), + Name: "MembershipOnly " + model.NewId(), + Type: model.AccessControlPolicyTypeParent, + Active: true, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_3, + Rules: []model.AccessControlPolicyRule{{ + Actions: []string{model.AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + uploadOnly := &model.AccessControlPolicy{ + ID: model.NewId(), + Name: "UploadOnly " + model.NewId(), + Type: model.AccessControlPolicyTypeParent, + Active: true, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_3, + Rules: []model.AccessControlPolicyRule{{ + Actions: []string{model.AccessControlPolicyActionUploadFileAttachment}, + Expression: "true", + }}, + } + multiAction := &model.AccessControlPolicy{ + ID: model.NewId(), + Name: "Multi " + model.NewId(), + Type: model.AccessControlPolicyTypeParent, + Active: true, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_3, + Rules: []model.AccessControlPolicyRule{ + { + Actions: []string{model.AccessControlPolicyActionMembership}, + Expression: "true", + }, + { + Actions: []string{model.AccessControlPolicyActionDownloadFileAttachment}, + Expression: "true", + }, + }, + } + multiActionSingleRule := &model.AccessControlPolicy{ + ID: model.NewId(), + Name: "MultiSingleRule " + model.NewId(), + Type: model.AccessControlPolicyTypeParent, + Active: true, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_3, + Rules: []model.AccessControlPolicyRule{{ + Actions: []string{ + model.AccessControlPolicyActionMembership, + model.AccessControlPolicyActionDownloadFileAttachment, + }, + Expression: "true", + }}, + } + + for _, p := range []*model.AccessControlPolicy{membershipOnly, uploadOnly, multiAction, multiActionSingleRule} { + saved, err := ss.AccessControlPolicy().Save(rctx, p) + require.NoError(t, err) + require.NotNil(t, saved) + } + t.Cleanup(func() { + for _, p := range []*model.AccessControlPolicy{membershipOnly, uploadOnly, multiAction, multiActionSingleRule} { + _ = ss.AccessControlPolicy().Delete(rctx, p.ID) + } + }) + + t.Run("single action filter returns matching policies", func(t *testing.T) { + policies, _, err := ss.AccessControlPolicy().SearchPolicies(rctx, model.AccessControlPolicySearch{ + Actions: []string{model.AccessControlPolicyActionMembership}, + Limit: 10, + }) + require.NoError(t, err) + ids := make([]string, len(policies)) + for i, p := range policies { + ids[i] = p.ID + } + require.Contains(t, ids, membershipOnly.ID) + require.Contains(t, ids, multiAction.ID) + require.Contains(t, ids, multiActionSingleRule.ID) + require.NotContains(t, ids, uploadOnly.ID) + }) + + t.Run("upload action filter", func(t *testing.T) { + policies, _, err := ss.AccessControlPolicy().SearchPolicies(rctx, model.AccessControlPolicySearch{ + Actions: []string{model.AccessControlPolicyActionUploadFileAttachment}, + Limit: 10, + }) + require.NoError(t, err) + ids := make([]string, len(policies)) + for i, p := range policies { + ids[i] = p.ID + } + require.Contains(t, ids, uploadOnly.ID) + require.NotContains(t, ids, membershipOnly.ID) + require.NotContains(t, ids, multiAction.ID) + require.NotContains(t, ids, multiActionSingleRule.ID) + }) + + t.Run("multiple actions OR semantics", func(t *testing.T) { + policies, _, err := ss.AccessControlPolicy().SearchPolicies(rctx, model.AccessControlPolicySearch{ + Actions: []string{model.AccessControlPolicyActionUploadFileAttachment, model.AccessControlPolicyActionDownloadFileAttachment}, + Limit: 10, + }) + require.NoError(t, err) + ids := make([]string, len(policies)) + for i, p := range policies { + ids[i] = p.ID + } + require.Contains(t, ids, uploadOnly.ID) + require.Contains(t, ids, multiAction.ID) + require.Contains(t, ids, multiActionSingleRule.ID) + require.NotContains(t, ids, membershipOnly.ID) + }) + + t.Run("non-existent action returns nothing from scoped set", func(t *testing.T) { + policies, _, err := ss.AccessControlPolicy().SearchPolicies(rctx, model.AccessControlPolicySearch{ + Actions: []string{"nonexistent_action"}, + Limit: 10, + }) + require.NoError(t, err) + require.Len(t, policies, 0) + }) +} + func testAccessControlPolicyStoreSetActiveMultiple(t *testing.T, rctx request.CTX, ss store.Store) { t.Run("Set active status for multiple policies", func(t *testing.T) { policy1 := &model.AccessControlPolicy{ diff --git a/server/channels/testlib/store.go b/server/channels/testlib/store.go index 5bac4f46848..a5d05aeec0f 100644 --- a/server/channels/testlib/store.go +++ b/server/channels/testlib/store.go @@ -95,6 +95,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store { systemStore.On("GetByName", model.MigrationKeyAddChannelAccessRulesPermission).Return(&model.System{Name: model.MigrationKeyAddChannelAccessRulesPermission, Value: "true"}, nil) systemStore.On("GetByName", model.MigrationKeyAddChannelAutoTranslationPermissions).Return(&model.System{Name: model.MigrationKeyAddChannelAutoTranslationPermissions, Value: "true"}, nil) systemStore.On("GetByName", model.MigrationKeyRestoreManageOAuthPermission).Return(&model.System{Name: model.MigrationKeyRestoreManageOAuthPermission, Value: "true"}, nil) + systemStore.On("GetByName", model.MigrationKeyAccessControlPolicyV0_3).Return(&model.System{Name: model.MigrationKeyAccessControlPolicyV0_3, Value: "true"}, nil) systemStore.On("InsertIfExists", mock.AnythingOfType("*model.System")).Return(&model.System{}, nil).Once() systemStore.On("Save", mock.AnythingOfType("*model.System")).Return(nil) diff --git a/server/i18n/en.json b/server/i18n/en.json index 29c49554e9c..6238ae91d5e 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -10084,10 +10084,18 @@ "id": "model.access_policy.inherit.already_imported.app_error", "translation": "The parent is already imported." }, + { + "id": "model.access_policy.inherit.permission.app_error", + "translation": "Permission policies cannot inherit from other policies." + }, { "id": "model.access_policy.inherit.version.app_error", "translation": "Could not inherit access control policy." }, + { + "id": "model.access_policy.is_valid.actions.app_error", + "translation": "Action(s) is not valid." + }, { "id": "model.access_policy.is_valid.id.app_error", "translation": "Invalid policy id." @@ -10104,6 +10112,10 @@ "id": "model.access_policy.is_valid.revision.app_error", "translation": "Invalid policy revision." }, + { + "id": "model.access_policy.is_valid.roles.app_error", + "translation": "Permission policies must be applied to exactly one role." + }, { "id": "model.access_policy.is_valid.rules.app_error", "translation": "Rule(s) is not valid." diff --git a/server/public/model/access_policy.go b/server/public/model/access_policy.go index e3d2e2ca1e6..c648c38698b 100644 --- a/server/public/model/access_policy.go +++ b/server/public/model/access_policy.go @@ -6,21 +6,34 @@ package model import ( "fmt" "slices" + "strings" "github.com/pkg/errors" "golang.org/x/mod/semver" ) const ( - AccessControlPolicyTypeParent = "parent" - AccessControlPolicyTypeChannel = "channel" + AccessControlPolicyTypeParent = "parent" + AccessControlPolicyTypeChannel = "channel" + AccessControlPolicyTypePermission = "permission" MaxPolicyNameLength = 128 AccessControlPolicyVersionV0_1 = "v0.1" AccessControlPolicyVersionV0_2 = "v0.2" + AccessControlPolicyVersionV0_3 = "v0.3" + + AccessControlPolicyActionMembership = "membership" + AccessControlPolicyActionUploadFileAttachment = "upload_file_attachment" + AccessControlPolicyActionDownloadFileAttachment = "download_file_attachment" ) +var allowedActionsV0_3 = map[string]bool{ + AccessControlPolicyActionMembership: true, + AccessControlPolicyActionUploadFileAttachment: true, + AccessControlPolicyActionDownloadFileAttachment: true, +} + // AccessControlAttribute represents a user attribute with its name and possible values type AccessControlAttribute struct { Attribute PropertyField `json:"attribute"` @@ -48,6 +61,7 @@ type AccessControlPolicySearch struct { Limit int `json:"limit"` IncludeChildren bool `json:"include_children"` Active bool `json:"active"` + Actions []string `json:"actions"` } type AccessControlPolicyCursor struct { @@ -69,6 +83,7 @@ type AccessControlPolicy struct { Revision int `json:"revision"` Version string `json:"version"` + Roles []string `json:"roles"` Imports []string `json:"imports"` Rules []AccessControlPolicyRule `json:"rules"` @@ -120,6 +135,8 @@ func (p *AccessControlPolicy) IsValid() *AppError { return p.accessPolicyVersionV0_1() case AccessControlPolicyVersionV0_2: return p.accessPolicyVersionV0_2() + case AccessControlPolicyVersionV0_3: + return p.accessPolicyVersionV0_3() default: return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.version.app_error", nil, "", 400) } @@ -211,6 +228,75 @@ func (p *AccessControlPolicy) accessPolicyVersionV0_2() *AppError { return nil } +func (p *AccessControlPolicy) accessPolicyVersionV0_3() *AppError { + if !slices.Contains([]string{AccessControlPolicyTypeParent, AccessControlPolicyTypeChannel, AccessControlPolicyTypePermission}, p.Type) { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.type.app_error", nil, "", 400) + } + + if !IsValidId(p.ID) { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.id.app_error", nil, "", 400) + } + + if p.Type == AccessControlPolicyTypeParent && (p.Name == "" || len(p.Name) > MaxPolicyNameLength) { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.name.app_error", nil, "", 400) + } + + if p.Revision < 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.revision.app_error", nil, "", 400) + } + + if !semver.IsValid(p.Version) { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.version.app_error", nil, "", 400) + } + + switch p.Type { + case AccessControlPolicyTypeParent: + if len(p.Rules) == 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.rules.app_error", nil, "", 400) + } + + if len(p.Imports) > 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.imports.app_error", nil, "", 400) + } + case AccessControlPolicyTypeChannel: + if len(p.Rules) == 0 && len(p.Imports) == 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.rules_imports.app_error", nil, "", 400) + } + case AccessControlPolicyTypePermission: + if len(p.Rules) == 0 && len(p.Imports) == 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.rules_imports.app_error", nil, "", 400) + } + + // Permissions are only allowed to be applied to a single role as of v0.3 + // role hierarchy is resolved at the PDP + if len(p.Roles) != 1 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.roles.app_error", nil, "", 400) + } + for _, role := range p.Roles { + if strings.TrimSpace(role) == "" { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.roles.app_error", nil, "", 400) + } + } + + if len(p.Imports) > 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.imports.app_error", nil, "", 400) + } + } + + for _, rule := range p.Rules { + if len(rule.Actions) == 0 { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.actions.app_error", nil, "actions must not be empty", 400) + } + for _, action := range rule.Actions { + if !allowedActionsV0_3[action] { + return NewAppError("AccessControlPolicy.IsValid", "model.access_policy.is_valid.actions.app_error", nil, fmt.Sprintf("unrecognized action: %s", action), 400) + } + } + } + + return nil +} + func (p *AccessControlPolicy) Inherit(parent *AccessControlPolicy) *AppError { rules := make([]AccessControlPolicyRule, len(p.Rules)) @@ -230,7 +316,17 @@ func (p *AccessControlPolicy) Inherit(parent *AccessControlPolicy) *AppError { return NewAppError("AccessControlPolicy.Inherit", "model.access_policy.inherit.already_imported.app_error", nil, "", 400) } p.Imports = append(p.Imports, parent.ID) - + case AccessControlPolicyVersionV0_3: + if p.Type == AccessControlPolicyTypePermission || parent.Type == AccessControlPolicyTypePermission { + return NewAppError("AccessControlPolicy.Inherit", "model.access_policy.inherit.permission.app_error", nil, "", 400) + } + if parent.Version != AccessControlPolicyVersionV0_3 { + return NewAppError("AccessControlPolicy.Inherit", "model.access_policy.inherit.version.app_error", nil, "", 400) + } + if slices.Contains(p.Imports, parent.ID) { + return NewAppError("AccessControlPolicy.Inherit", "model.access_policy.inherit.already_imported.app_error", nil, "", 400) + } + p.Imports = append(p.Imports, parent.ID) default: return NewAppError("AccessControlPolicy.Inherit", "model.access_policy.inherit.version.app_error", nil, "", 400) } diff --git a/server/public/model/access_policy_test.go b/server/public/model/access_policy_test.go index eb6f3e2bb84..9d9fcc3bfa5 100644 --- a/server/public/model/access_policy_test.go +++ b/server/public/model/access_policy_test.go @@ -213,3 +213,345 @@ func TestAccessPolicyVersionV0_1(t *testing.T) { require.Nil(t, err, "Should not return error for valid channel policy") }) } + +func TestAccessPolicyVersionV0_3(t *testing.T) { + validRule := AccessControlPolicyRule{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "user.properties.dept == \"eng\"", + } + + t.Run("valid parent type", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{ + AccessControlPolicyActionMembership, + AccessControlPolicyActionUploadFileAttachment, + AccessControlPolicyActionDownloadFileAttachment, + }, + Expression: "user.properties.dept == \"eng\"", + }}, + } + require.Nil(t, policy.accessPolicyVersionV0_3()) + }) + + t.Run("valid channel type", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Imports: []string{NewId()}, + Rules: []AccessControlPolicyRule{validRule}, + } + require.Nil(t, policy.accessPolicyVersionV0_3()) + }) + + t.Run("valid permission type", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{"system_admin"}, + Rules: []AccessControlPolicyRule{validRule}, + } + require.Nil(t, policy.accessPolicyVersionV0_3()) + }) + + t.Run("invalid type", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: "unknown", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{validRule}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.type.app_error", err.Id) + }) + + t.Run("parent with no rules", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.rules.app_error", err.Id) + }) + + t.Run("parent with non-empty imports", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{validRule}, + Imports: []string{NewId()}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.imports.app_error", err.Id) + }) + + t.Run("permission with empty roles", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{}, + Rules: []AccessControlPolicyRule{validRule}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.roles.app_error", err.Id) + }) + + t.Run("permission with blank role string", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{""}, + Rules: []AccessControlPolicyRule{validRule}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.roles.app_error", err.Id) + }) + + t.Run("permission with multiple roles", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{"system_admin", "system_user"}, + Rules: []AccessControlPolicyRule{validRule}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.roles.app_error", err.Id) + }) + + t.Run("permission with imports", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{"system_admin"}, + Rules: []AccessControlPolicyRule{validRule}, + Imports: []string{NewId()}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.imports.app_error", err.Id) + }) + + t.Run("unrecognized action", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{"not_a_real_action"}, + Expression: "true", + }}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.actions.app_error", err.Id) + require.Contains(t, err.DetailedError, "not_a_real_action") + }) + + t.Run("empty actions slice", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{}, + Expression: "true", + }}, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.actions.app_error", err.Id) + }) + + t.Run("channel with no rules and no imports", func(t *testing.T) { + policy := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + } + err := policy.accessPolicyVersionV0_3() + require.NotNil(t, err) + require.Equal(t, "model.access_policy.is_valid.rules_imports.app_error", err.Id) + }) +} + +func TestInheritV0_3(t *testing.T) { + t.Run("successful inherit", func(t *testing.T) { + parentID := NewId() + parent := &AccessControlPolicy{ + ID: parentID, + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + child := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + + err := child.Inherit(parent) + require.Nil(t, err) + require.Contains(t, child.Imports, parentID) + }) + + t.Run("duplicate import guard", func(t *testing.T) { + parentID := NewId() + parent := &AccessControlPolicy{ + ID: parentID, + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + child := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Imports: []string{parentID}, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + + err := child.Inherit(parent) + require.NotNil(t, err) + require.Equal(t, "model.access_policy.inherit.already_imported.app_error", err.Id) + }) + + t.Run("permission type child rejected", func(t *testing.T) { + parent := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + child := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{"system_admin"}, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + + err := child.Inherit(parent) + require.NotNil(t, err) + require.Equal(t, "model.access_policy.inherit.permission.app_error", err.Id) + }) + + t.Run("permission type parent rejected", func(t *testing.T) { + parent := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypePermission, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Roles: []string{"system_admin"}, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + child := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + + err := child.Inherit(parent) + require.NotNil(t, err) + require.Equal(t, "model.access_policy.inherit.permission.app_error", err.Id) + }) + + t.Run("non-v0.3 parent version rejected", func(t *testing.T) { + parent := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeParent, + Name: "V01 Parent", + Revision: 0, + Version: AccessControlPolicyVersionV0_1, + Rules: []AccessControlPolicyRule{{ + Actions: []string{"read"}, + Expression: "true", + }}, + } + child := &AccessControlPolicy{ + ID: NewId(), + Type: AccessControlPolicyTypeChannel, + Revision: 0, + Version: AccessControlPolicyVersionV0_3, + Rules: []AccessControlPolicyRule{{ + Actions: []string{AccessControlPolicyActionMembership}, + Expression: "true", + }}, + } + + err := child.Inherit(parent) + require.NotNil(t, err) + require.Equal(t, "model.access_policy.inherit.version.app_error", err.Id) + }) +} diff --git a/server/public/model/migration.go b/server/public/model/migration.go index a027e4b4205..ffe3dd63cd4 100644 --- a/server/public/model/migration.go +++ b/server/public/model/migration.go @@ -60,4 +60,5 @@ const ( MigrationKeyAddChannelAutoTranslationPermissions = "add_channel_auto_translation_permissions" MigrationKeyAddSharedChannelManagerPermissions = "system_shared_channel_manager_permissions" MigrationKeyRestoreManageOAuthPermission = "restore_manage_oauth_permission" + MigrationKeyAccessControlPolicyV0_3 = "access_control_policy_v0_3_migration" ) diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx index d54dce548e5..8cb74819ecf 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx @@ -7,6 +7,7 @@ import {FormattedMessage, useIntl} from 'react-intl'; import {GenericModal} from '@mattermost/components'; import type {AccessControlPolicy, AccessControlPolicyActiveUpdate, AccessControlPolicyRule} from '@mattermost/types/access_control'; +import {getMembershipRule, buildRulesWithMembership} from '@mattermost/types/access_control'; import type {ChannelSearchOpts, ChannelWithTeamData} from '@mattermost/types/channels'; import type {AccessControlSettings} from '@mattermost/types/config'; import type {JobTypeBase} from '@mattermost/types/jobs'; @@ -74,7 +75,8 @@ function PolicyDetails({ accessControlSettings, }: PolicyDetailsProps): JSX.Element { const [policyName, setPolicyName] = useState(policy?.name || ''); - const [expression, setExpression] = useState(policy?.rules?.[0]?.expression || ''); + const [expression, setExpression] = useState(getMembershipRule(policy?.rules)?.expression || ''); + const [existingRules, setExistingRules] = useState(policy?.rules || []); const [autoSyncMembership, setAutoSyncMembership] = useState(policy?.active || false); const [serverError, setServerError] = useState(undefined); const [addChannelOpen, setAddChannelOpen] = useState(false); @@ -151,7 +153,8 @@ function PolicyDetails({ // For existing policies, fetch policy details and channels const policyPromise = actions.fetchPolicy(policyId).then((result) => { setPolicyName(result.data?.name || ''); - setExpression(result.data?.rules?.[0]?.expression || ''); + setExpression(getMembershipRule(result.data?.rules)?.expression || ''); + setExistingRules(result.data?.rules || []); setAutoSyncMembership(result.data?.active || false); }); @@ -197,9 +200,8 @@ function PolicyDetails({ await actions.createPolicy({ id: currentPolicyId || '', name: policyName, - rules: [{expression, actions: ['*']}] as AccessControlPolicyRule[], + rules: buildRulesWithMembership(existingRules, expression), type: 'parent', - version: 'v0.2', }).then((result) => { if (result.error) { if (result.error.server_error_id === 'app.pap.save_policy.name_exists.app_error') { @@ -213,7 +215,8 @@ function PolicyDetails({ } currentPolicyId = result.data?.id; setPolicyName(result.data?.name || ''); - setExpression(result.data?.rules?.[0]?.expression || ''); + setExpression(getMembershipRule(result.data?.rules)?.expression || ''); + setExistingRules(result.data?.rules || []); setAutoSyncMembership(result.data?.active || false); }); diff --git a/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.tsx b/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.tsx index 6085b45f574..b8457778b14 100644 --- a/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.tsx +++ b/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.tsx @@ -5,7 +5,8 @@ import cloneDeep from 'lodash/cloneDeep'; import React from 'react'; import {FormattedMessage} from 'react-intl'; -import type {AccessControlPolicy, AccessControlPolicyActiveUpdate} from '@mattermost/types/access_control'; +import type {AccessControlPolicy, AccessControlPolicyActiveUpdate, AccessControlPolicyRule} from '@mattermost/types/access_control'; +import {getMembershipRule, buildRulesWithMembership} from '@mattermost/types/access_control'; import type {Channel, ChannelModeration as ChannelPermissions, ChannelModerationPatch} from '@mattermost/types/channels'; import {SyncableType} from '@mattermost/types/groups'; import type {SyncablePatch, Group} from '@mattermost/types/groups'; @@ -95,6 +96,7 @@ interface ChannelDetailsState { // Channel-level access rules state channelRulesExpression: string; channelRulesOriginalExpression: string; + channelRulesExistingRules: AccessControlPolicyRule[]; channelRulesAutoSync: boolean; channelRulesOriginalAutoSync: boolean; channelRulesHaveChanges: boolean; @@ -179,6 +181,7 @@ export default class ChannelDetails extends React.PureComponent p.id), - // Add/update channel-level rules - rules: [{ - actions: ['*'], - expression: channelRulesExpression, - }], + rules: buildRulesWithMembership(this.state.channelRulesExistingRules, channelRulesExpression), }; // Save the channel-level policy using the existing action @@ -848,11 +846,10 @@ export default class ChannelDetails extends React.PureComponent p.id), // SAME LOGIC as Channel Settings Modal }; @@ -1165,11 +1162,12 @@ export default class ChannelDetails extends React.PureComponent 0) { - const rule = policy.rules[0]; + const rule = getMembershipRule(policy.rules); const autoSyncValue = policy.active === true; // Explicitly check for true this.setState({ - channelRulesExpression: rule.expression || '', - channelRulesOriginalExpression: rule.expression || '', + channelRulesExpression: rule?.expression || '', + channelRulesOriginalExpression: rule?.expression || '', + channelRulesExistingRules: policy.rules, channelRulesAutoSync: autoSyncValue, channelRulesOriginalAutoSync: autoSyncValue, channelRulesHaveChanges: false, @@ -1188,7 +1186,7 @@ export default class ChannelDetails extends React.PureComponent { // Get expressions from parent policies const parentExpressions = this.state.accessControlPolicies. - map((policy) => policy.rules?.[0]?.expression). + map((policy) => getMembershipRule(policy.rules)?.expression). filter((expr) => expr && expr.trim()); // Combine channel expression with parent expressions @@ -1208,7 +1206,7 @@ export default class ChannelDetails extends React.PureComponent { mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: true, }, }); @@ -274,7 +274,7 @@ describe('ChannelSettingsAccessRulesTab - Activity Warning Integration', () => { mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: true, }, }); diff --git a/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.test.tsx b/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.test.tsx index 6507c00a855..b9d7201b81f 100644 --- a/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.test.tsx +++ b/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.test.tsx @@ -909,12 +909,11 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = id: 'channel_id', name: 'Test Channel', type: 'channel', - version: 'v0.2', active: false, // Policy starts as inactive until job completes revision: 1, created_at: expect.any(Number), rules: [{ - actions: ['*'], + actions: ['membership'], expression: 'user.attributes.department == "Engineering"', }], imports: [], @@ -1192,13 +1191,13 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = id: 'system_policy_1', name: 'System Policy 1', type: 'parent', - version: 'v0.2', + version: 'v0.3', revision: 1, active: false, createAt: 1234567890, rules: [ { - actions: ['join_channel'], + actions: ['membership'], expression: 'user.attributes.Program == "test"', }, ], @@ -1208,13 +1207,13 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = id: 'system_policy_2', name: 'System Policy 2', type: 'parent', - version: 'v0.2', + version: 'v0.3', revision: 1, active: false, createAt: 1234567891, rules: [ { - actions: ['join_channel'], + actions: ['membership'], expression: 'user.attributes.Department == "Engineering"', }, ], @@ -1775,7 +1774,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: false, }, }); @@ -1818,7 +1817,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: false, }, }); @@ -1877,7 +1876,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: false, }, }); @@ -1938,7 +1937,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: true, }, }); @@ -2008,7 +2007,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: true, }, }); @@ -2062,7 +2061,7 @@ describe('components/channel_settings_modal/ChannelSettingsAccessRulesTab', () = mockActions.getChannelPolicy.mockResolvedValue({ data: { id: 'channel_id', - rules: [{expression: 'user.department == "Engineering"'}], + rules: [{actions: ['membership'], expression: 'user.department == "Engineering"'}], active: true, }, }); diff --git a/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.tsx b/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.tsx index c84009fd8e7..c03f2ad4e14 100644 --- a/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.tsx +++ b/webapp/channels/src/components/channel_settings_modal/channel_settings_access_rules_tab.tsx @@ -5,6 +5,8 @@ import React, {useState, useEffect, useCallback, useMemo, useRef} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {useSelector} from 'react-redux'; +import type {AccessControlPolicyRule} from '@mattermost/types/access_control'; +import {getMembershipRule, buildRulesWithMembership} from '@mattermost/types/access_control'; import type {Channel} from '@mattermost/types/channels'; import type {UserPropertyField} from '@mattermost/types/properties'; @@ -59,6 +61,7 @@ function ChannelSettingsAccessRulesTab({ // State for the access control expression and user attributes const [expression, setExpression] = useState(''); const [originalExpression, setOriginalExpression] = useState(''); + const [existingRules, setExistingRules] = useState([]); const [userAttributes, setUserAttributes] = useState([]); const [attributesLoaded, setAttributesLoaded] = useState(false); @@ -121,12 +124,12 @@ function ChannelSettingsAccessRulesTab({ try { const result = await actions.getChannelPolicy(channel.id); if (result.data) { - // Extract expression from the policy rules - const existingExpression = result.data.rules?.[0]?.expression || ''; + const existingExpression = getMembershipRule(result.data.rules)?.expression || ''; const existingAutoSync = result.data.active || false; setExpression(existingExpression); setOriginalExpression(existingExpression); + setExistingRules(result.data.rules || []); setAutoSyncMembers(existingAutoSync); setOriginalAutoSyncMembers(existingAutoSync); } @@ -205,7 +208,7 @@ function ChannelSettingsAccessRulesTab({ const combineSystemAndChannelExpressions = useCallback((channelExpression: string): string => { // Get expressions from system policies const systemExpressions = systemPolicies. - map((policy) => policy.rules?.[0]?.expression). + map((policy) => getMembershipRule(policy.rules)?.expression). filter((expr) => expr && expr.trim()); // Combine channel expression with system expressions @@ -225,7 +228,7 @@ function ChannelSettingsAccessRulesTab({ if (allExpressions.length === 0) { return ''; } else if (allExpressions.length === 1) { - return allExpressions[0]; + return allExpressions[0]!; } // Wrap each expression in parentheses and combine with && @@ -400,14 +403,10 @@ function ChannelSettingsAccessRulesTab({ id: channel.id, name: channel.display_name, type: 'channel', - version: 'v0.2', active: false, // Always save as false initially, then update separately revision: 1, created_at: Date.now(), - rules: expression.trim() ? [{ - actions: ['*'], - expression: expression.trim(), - }] : [], + rules: buildRulesWithMembership(existingRules, expression), imports: systemPolicies.map((p) => p.id), // Include existing parent policies }; diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 730e6ff654c..9c7cfbe3a32 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -4716,7 +4716,7 @@ export default class Client4 { getAccessControlPolicies = (after: string, limit: number) => { return this.doFetch( `${this.getBaseRoute()}/access_control_policies/search`, - {method: 'post', body: JSON.stringify({type: 'parent', cursor: {id: after}, limit})}, + {method: 'post', body: JSON.stringify({type: 'parent', cursor: {id: after}, limit, actions: ['membership']})}, ); }; diff --git a/webapp/platform/types/src/access_control.test.ts b/webapp/platform/types/src/access_control.test.ts new file mode 100644 index 00000000000..87e72d9a913 --- /dev/null +++ b/webapp/platform/types/src/access_control.test.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {getMembershipRule, buildRulesWithMembership} from './access_control'; +import type {AccessControlPolicyRule} from './access_control'; + +describe('getMembershipRule', () => { + test('returns the membership rule when present', () => { + const rules: AccessControlPolicyRule[] = [ + {actions: ['file_upload'], expression: 'upload_expr'}, + {actions: ['membership'], expression: 'membership_expr'}, + ]; + expect(getMembershipRule(rules)).toEqual({actions: ['membership'], expression: 'membership_expr'}); + }); + + test('falls back to rules[0] for legacy v0.2 single-rule policy with wildcard action', () => { + const rules: AccessControlPolicyRule[] = [ + {actions: ['*'], expression: 'legacy_expr'}, + ]; + expect(getMembershipRule(rules)).toEqual({actions: ['*'], expression: 'legacy_expr'}); + }); + + test('returns undefined when rules contain only non-membership, non-wildcard actions', () => { + const rules: AccessControlPolicyRule[] = [ + {actions: ['file_upload'], expression: 'upload_expr'}, + {actions: ['file_download'], expression: 'download_expr'}, + ]; + expect(getMembershipRule(rules)).toBeUndefined(); + }); + + test('returns undefined for empty rules array', () => { + expect(getMembershipRule([])).toBeUndefined(); + }); + + test('returns undefined for undefined input', () => { + expect(getMembershipRule(undefined)).toBeUndefined(); + }); +}); + +describe('buildRulesWithMembership', () => { + test('inserts membership rule and preserves non-membership rules', () => { + const existing: AccessControlPolicyRule[] = [ + {actions: ['file_upload'], expression: 'upload_expr'}, + {actions: ['file_download'], expression: 'download_expr'}, + ]; + const result = buildRulesWithMembership(existing, 'new_membership_expr'); + expect(result).toEqual([ + {actions: ['membership'], expression: 'new_membership_expr'}, + {actions: ['file_upload'], expression: 'upload_expr'}, + {actions: ['file_download'], expression: 'download_expr'}, + ]); + }); + + test('replaces existing membership rule while preserving others', () => { + const existing: AccessControlPolicyRule[] = [ + {actions: ['membership'], expression: 'old_expr'}, + {actions: ['file_upload'], expression: 'upload_expr'}, + ]; + const result = buildRulesWithMembership(existing, 'new_expr'); + expect(result).toEqual([ + {actions: ['membership'], expression: 'new_expr'}, + {actions: ['file_upload'], expression: 'upload_expr'}, + ]); + }); + + test('empty expression removes membership rule', () => { + const existing: AccessControlPolicyRule[] = [ + {actions: ['membership'], expression: 'old_expr'}, + {actions: ['file_upload'], expression: 'upload_expr'}, + ]; + const result = buildRulesWithMembership(existing, ''); + expect(result).toEqual([ + {actions: ['file_upload'], expression: 'upload_expr'}, + ]); + }); + + test('whitespace-only expression removes membership rule', () => { + const existing: AccessControlPolicyRule[] = [ + {actions: ['membership'], expression: 'old_expr'}, + ]; + const result = buildRulesWithMembership(existing, ' '); + expect(result).toEqual([]); + }); + + test('trims whitespace from expression', () => { + const result = buildRulesWithMembership([], ' some_expr '); + expect(result).toEqual([ + {actions: ['membership'], expression: 'some_expr'}, + ]); + }); + + test('empty existing rules with valid expression creates membership-only array', () => { + const result = buildRulesWithMembership([], 'expr'); + expect(result).toEqual([ + {actions: ['membership'], expression: 'expr'}, + ]); + }); +}); diff --git a/webapp/platform/types/src/access_control.ts b/webapp/platform/types/src/access_control.ts index bb159bfaba0..19b2520c6c7 100644 --- a/webapp/platform/types/src/access_control.ts +++ b/webapp/platform/types/src/access_control.ts @@ -42,6 +42,35 @@ export type AccessControlPolicyRule = { expression: string; } +/** + * Returns the first rule with a "membership" action, falling back to rules[0] + * only when it carries a wildcard action (legacy v0.2 policies). + */ +export function getMembershipRule(rules?: AccessControlPolicyRule[]): AccessControlPolicyRule | undefined { + const membership = rules?.find((r) => r.actions?.includes('membership')); + if (membership) { + return membership; + } + const first = rules?.[0]; + if (first?.actions?.includes('*')) { + return first; + } + return undefined; +} + +/** + * Replaces or inserts the membership rule in an existing rules array while + * preserving all non-membership rules (e.g. file_upload, file_download). + * If expression is empty the membership rule is removed. + */ +export function buildRulesWithMembership(existingRules: AccessControlPolicyRule[], expression: string): AccessControlPolicyRule[] { + const otherRules = existingRules.filter((r) => !r.actions?.includes('membership')); + if (!expression.trim()) { + return otherRules; + } + return [{actions: ['membership'], expression: expression.trim()}, ...otherRules]; +} + export type CELExpressionError = { message: string; line: number;