From ced9a56e3988fe9fd4559d45f9971dbd562e2218 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Tue, 27 Jan 2026 15:49:08 +0100 Subject: [PATCH] [MM-67126] Deprecate UpdateAccessControlPolicyActiveStatus API in favor of new one (#34940) --- api/v4/source/access_control.yaml | 5 ++ server/channels/api4/access_control.go | 26 ++++++-- server/channels/api4/access_control_local.go | 1 - server/channels/api4/access_control_test.go | 62 +++++++++++++++++++ server/channels/app/access_control.go | 9 --- server/i18n/en.json | 4 -- .../channel/details/channel_details.test.tsx | 6 +- .../channel/details/channel_details.tsx | 6 +- .../channel/details/index.ts | 4 +- ...ngs_access_rules_activity_warning.test.tsx | 3 +- .../src/actions/access_control.ts | 10 --- webapp/platform/client/src/client4.ts | 7 --- 12 files changed, 98 insertions(+), 45 deletions(-) diff --git a/api/v4/source/access_control.yaml b/api/v4/source/access_control.yaml index 7bd8ead80ad..02402b81703 100644 --- a/api/v4/source/access_control.yaml +++ b/api/v4/source/access_control.yaml @@ -283,11 +283,16 @@ $ref: "#/components/responses/InternalServerError" "/api/v4/access_control_policies/{policy_id}/activate": get: + deprecated: true tags: - access control summary: Activate or deactivate an access control policy description: | Updates the active status of an access control policy. + + **Deprecated:** This endpoint will be removed in a future release. Use the dedicated access control policy update endpoint instead. + Link: ; rel="successor-version" + ##### Permissions Must have the `manage_system` permission. operationId: UpdateAccessControlPolicyActiveStatus diff --git a/server/channels/api4/access_control.go b/server/channels/api4/access_control.go index 2d5d861c560..078391032df 100644 --- a/server/channels/api4/access_control.go +++ b/server/channels/api4/access_control.go @@ -382,12 +382,22 @@ func searchAccessControlPolicies(c *Context, w http.ResponseWriter, r *http.Requ } } +// updateActiveStatus updates the active status of a single access control policy. +// +// Deprecated: This endpoint is deprecated and will be removed in a future release. +// Use PUT /api/v4/access_control/policies/activate instead, which supports batch updates. func updateActiveStatus(c *Context, w http.ResponseWriter, r *http.Request) { c.RequirePolicyId() if c.Err != nil { return } + // CSRF barrier: only allow header-based auth (reject cookie-only sessions) + if r.Header.Get(model.HeaderAuth) == "" { + c.SetInvalidParam("Authorization") + return + } + policyID := c.Params.PolicyId // Check if user has system admin permission OR channel-specific permission for this policy @@ -416,7 +426,11 @@ func updateActiveStatus(c *Context, w http.ResponseWriter, r *http.Request) { } model.AddEventParameterToAuditRec(auditRec, "active", activeBool) - appErr := c.App.UpdateAccessControlPolicyActive(c.AppContext, policyID, activeBool) + // Wrap single update in slice to use the batch update method + updates := []model.AccessControlPolicyActiveUpdate{ + {ID: policyID, Active: activeBool}, + } + _, appErr := c.App.UpdateAccessControlPoliciesActive(c.AppContext, updates) if appErr != nil { c.Err = appErr return @@ -429,6 +443,9 @@ func updateActiveStatus(c *Context, w http.ResponseWriter, r *http.Request) { "status": "OK", } + // Set deprecation header to inform clients + w.Header().Set("Deprecation", "true") + w.Header().Set("Link", "; rel=\"successor-version\"") w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(response); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) @@ -446,12 +463,13 @@ func setActiveStatus(c *Context, w http.ResponseWriter, r *http.Request) { defer c.LogAuditRec(auditRec) model.AddEventParameterAuditableToAuditRec(auditRec, "requested", &list) - // Check if user has system admin permission OR channel-specific permission for this policy + // Check if user has system admin permission OR policy-specific permission hasManageSystemPermission := c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) if !hasManageSystemPermission { for _, entry := range list.Entries { - hasChannelPermission, _ := c.App.HasPermissionToChannel(c.AppContext, c.AppContext.Session().UserId, entry.ID, model.PermissionManageChannelAccessRules) - if !hasChannelPermission { + // Validate policy access permission - this fetches the policy first to verify it exists + // and is a channel-type policy before checking channel permissions + if appErr := c.App.ValidateAccessControlPolicyPermission(c.AppContext, c.AppContext.Session().UserId, entry.ID); appErr != nil { c.SetPermissionError(model.PermissionManageChannelAccessRules) return } diff --git a/server/channels/api4/access_control_local.go b/server/channels/api4/access_control_local.go index 2acc143da3c..2a69a5307d2 100644 --- a/server/channels/api4/access_control_local.go +++ b/server/channels/api4/access_control_local.go @@ -21,7 +21,6 @@ func (api *API) InitAccessControlPolicyLocal() { api.BaseRoutes.AccessControlPolicy.Handle("", api.APILocal(getAccessControlPolicy)).Methods(http.MethodGet) api.BaseRoutes.AccessControlPolicy.Handle("", api.APILocal(deleteAccessControlPolicy)).Methods(http.MethodDelete) - api.BaseRoutes.AccessControlPolicy.Handle("/activate", api.APILocal(updateActiveStatus)).Methods(http.MethodGet) api.BaseRoutes.AccessControlPolicy.Handle("/assign", api.APILocal(assignAccessPolicy)).Methods(http.MethodPost) api.BaseRoutes.AccessControlPolicy.Handle("/unassign", api.APILocal(unassignAccessPolicy)).Methods(http.MethodDelete) api.BaseRoutes.AccessControlPolicy.Handle("/resources/channels", api.APILocal(getChannelsForAccessControlPolicy)).Methods(http.MethodGet) diff --git a/server/channels/api4/access_control_test.go b/server/channels/api4/access_control_test.go index e7d11a1750c..e6e63b264dd 100644 --- a/server/channels/api4/access_control_test.go +++ b/server/channels/api4/access_control_test.go @@ -963,6 +963,7 @@ func TestSetActiveStatus(t *testing.T) { } mockAccessControlService := &mocks.AccessControlServiceInterface{} + mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), privateChannel.Id).Return(channelPolicy, nil) th.App.Srv().Channels().AccessControl = mockAccessControlService // Channel admin should be able to set active status for their channel @@ -974,4 +975,65 @@ func TestSetActiveStatus(t *testing.T) { require.Equal(t, channelPolicy.ID, policies[0].ID, "expected policy ID to match") require.True(t, policies[0].Active, "expected policy to be active") }) + + t.Run("SetActiveStatus with channel admin for another channel should fail", func(t *testing.T) { + // This test verifies the security fix: a channel admin cannot modify the active status + // of a policy for a channel they don't have permissions on, even if they attempt to + // use a policy ID that matches a channel they control. + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + + // Add permission to channel admin role + th.AddPermissionToRole(t, model.PermissionManageChannelAccessRules.Id, model.ChannelAdminRoleId) + + // Create two private channels + channelA := th.CreatePrivateChannel(t) + channelB := th.CreatePrivateChannel(t) + + // Create a channel admin who only has access to channel A + channelAdmin := th.CreateUser(t) + th.LinkUserToTeam(t, channelAdmin, th.BasicTeam) + th.AddUserToChannel(t, channelAdmin, channelA) + th.MakeUserChannelAdmin(t, channelAdmin, channelA) + + // Create a policy for channel B (which the channel admin does NOT have access to) + channelBPolicy := &model.AccessControlPolicy{ + ID: channelB.Id, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_2, + Revision: 1, + Rules: []model.AccessControlPolicyRule{ + { + Expression: "user.attributes.team == 'engineering'", + Actions: []string{"*"}, + }, + }, + } + _, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, channelBPolicy) + require.NoError(t, err) + + channelAdminClient := th.CreateClient() + _, _, err = channelAdminClient.Login(context.Background(), channelAdmin.Email, channelAdmin.Password) + require.NoError(t, err) + + mockAccessControlService := &mocks.AccessControlServiceInterface{} + th.App.Srv().Channels().AccessControl = mockAccessControlService + mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), channelB.Id).Return(channelBPolicy, nil) + + // Attempt to update the policy for channel B (which the admin doesn't have access to) + maliciousUpdateReq := model.AccessControlPolicyActiveUpdateRequest{ + Entries: []model.AccessControlPolicyActiveUpdate{ + {ID: channelB.Id, Active: true}, + }, + } + + // Channel admin should NOT be able to set active status for another channel's policy + _, resp, err := channelAdminClient.SetAccessControlPolicyActive(context.Background(), maliciousUpdateReq) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) } diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index b17b0b9ca6a..256b39e09df 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -302,15 +302,6 @@ func (a *App) GetAccessControlFieldsAutocomplete(rctx request.CTX, after string, return fields, nil } -func (a *App) UpdateAccessControlPolicyActive(rctx request.CTX, policyID string, active bool) *model.AppError { - _, err := a.Srv().Store().AccessControlPolicy().SetActiveStatus(rctx, policyID, active) - if err != nil { - return model.NewAppError("UpdateAccessControlPolicyActive", "app.pap.update_access_control_policy_active.app_error", nil, err.Error(), http.StatusInternalServerError) - } - - return nil -} - func (a *App) UpdateAccessControlPoliciesActive(rctx request.CTX, updates []model.AccessControlPolicyActiveUpdate) ([]*model.AccessControlPolicy, *model.AppError) { acs := a.Srv().ch.AccessControl if acs == nil { diff --git a/server/i18n/en.json b/server/i18n/en.json index 5932396ac80..f0d134ac93f 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -6918,10 +6918,6 @@ "id": "app.pap.update_access_control_policies_active.app_error", "translation": "Could not update active status of access control policies." }, - { - "id": "app.pap.update_access_control_policy_active.app_error", - "translation": "Could not change active status of access control policy." - }, { "id": "app.pdp.access_evaluation.app_error", "translation": "Failed evaluate access control policy." diff --git a/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.test.tsx b/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.test.tsx index 5dfe7e8b69e..6846e925f3b 100644 --- a/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.test.tsx +++ b/webapp/channels/src/components/admin_console/team_channel_settings/channel/details/channel_details.test.tsx @@ -105,7 +105,7 @@ describe('admin_console/team_channel_settings/channel/ChannelDetails', () => { saveChannelAccessPolicy: jest.fn().mockResolvedValue({data: {}}), validateChannelExpression: jest.fn().mockResolvedValue({data: {}}), createAccessControlSyncJob: jest.fn().mockResolvedValue({data: {}}), - updateAccessControlPolicyActive: jest.fn().mockResolvedValue({data: {}}), + updateAccessControlPoliciesActive: jest.fn().mockResolvedValue({data: {}}), searchUsersForExpression: jest.fn().mockResolvedValue({data: {users: [], total: 0}}), getChannelMembers: jest.fn().mockResolvedValue({data: []}), getProfilesByIds: jest.fn().mockResolvedValue({data: []}), @@ -246,7 +246,7 @@ describe('admin_console/team_channel_settings/channel/ChannelDetails', () => { saveChannelAccessPolicy: jest.fn().mockResolvedValue({data: {}}), validateChannelExpression: jest.fn().mockResolvedValue({data: {}}), createAccessControlSyncJob: jest.fn().mockResolvedValue({data: {}}), - updateAccessControlPolicyActive: jest.fn().mockResolvedValue({data: {}}), + updateAccessControlPoliciesActive: jest.fn().mockResolvedValue({data: {}}), searchUsersForExpression: jest.fn().mockResolvedValue({data: {users: [], total: 0}}), getChannelMembers: jest.fn().mockResolvedValue({data: []}), getProfilesByIds: jest.fn().mockResolvedValue({data: []}), @@ -388,7 +388,7 @@ describe('admin_console/team_channel_settings/channel/ChannelDetails', () => { saveChannelAccessPolicy: jest.fn().mockResolvedValue({data: {}}), validateChannelExpression: jest.fn().mockResolvedValue({data: {}}), createAccessControlSyncJob: jest.fn().mockResolvedValue({data: {}}), - updateAccessControlPolicyActive: jest.fn().mockResolvedValue({data: {}}), + updateAccessControlPoliciesActive: jest.fn().mockResolvedValue({data: {}}), searchUsersForExpression: jest.fn().mockResolvedValue({data: {users: [], total: 0}}), getChannelMembers: jest.fn().mockResolvedValue({data: []}), getProfilesByIds: jest.fn().mockResolvedValue({data: []}), 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 731ab499684..a7bcdc0e25b 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,7 @@ import cloneDeep from 'lodash/cloneDeep'; import React from 'react'; import {FormattedMessage} from 'react-intl'; -import type {AccessControlPolicy} from '@mattermost/types/access_control'; +import type {AccessControlPolicy, AccessControlPolicyActiveUpdate} 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'; @@ -139,7 +139,7 @@ export type ChannelDetailsActions = { saveChannelAccessPolicy: (policy: AccessControlPolicy) => Promise; validateChannelExpression: (expression: string, channelId: string) => Promise; createAccessControlSyncJob: (job: JobTypeBase & { data: any }) => Promise; - updateAccessControlPolicyActive: (policyId: string, active: boolean) => Promise; + updateAccessControlPoliciesActive: (states: AccessControlPolicyActiveUpdate[]) => Promise; searchUsersForExpression: (expression: string, term: string, after: string, limit: number, channelId?: string) => Promise; getChannelMembers: (channelId: string, page?: number, perPage?: number) => Promise; getProfilesByIds: (userIds: string[]) => Promise; @@ -802,7 +802,7 @@ export default class ChannelDetails extends React.PureComponent { getChannelMembers: jest.fn().mockResolvedValue({data: []}), createJob: jest.fn().mockResolvedValue({data: {}}), createAccessControlSyncJob: jest.fn().mockResolvedValue({data: {}}), - updateAccessControlPolicyActive: jest.fn().mockResolvedValue({data: {}}), + updateAccessControlPoliciesActive: jest.fn().mockResolvedValue({data: {}}), validateExpressionAgainstRequester: jest.fn().mockResolvedValue({data: {requester_matches: true}}), savePreferences: jest.fn().mockResolvedValue({data: {}}), - updateAccessControlPoliciesActive: jest.fn().mockResolvedValue({data: {}}), }; const defaultProps = { diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts index 89d3d482044..bdf72c12f61 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts @@ -125,16 +125,6 @@ export function getAccessControlFields(after: string, limit: number, channelId?: }); } -export function updateAccessControlPolicyActive(policyId: string, active: boolean) { - return bindClientFunc({ - clientFunc: Client4.updateAccessControlPolicyActive, - params: [ - policyId, - active, - ], - }); -} - export function searchUsersForExpression(expression: string, term: string, after: string, limit: number, channelId?: string): ActionFuncAsync { return async (dispatch, getState) => { let data; diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 7e9fbe77e44..32ce63d196d 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -4681,13 +4681,6 @@ export default class Client4 { ); }; - updateAccessControlPolicyActive = (policyId: string, active: boolean) => { - return this.doFetch( - `${this.getBaseRoute()}/access_control_policies/${policyId}/activate?active=${active}`, - {method: 'get'}, - ); - }; - assignChannelsToAccessControlPolicy = (policyId: string, channelIds: string[]) => { return this.doFetch( `${this.getBaseRoute()}/access_control_policies/${policyId}/assign`,