mirror of
https://github.com/mattermost/mattermost.git
synced 2026-08-03 09:53:10 -05:00
MM-69857 - Keep parent imports and team scope system-managed on the policy update endpoint (#37625)
Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Pablo Vélez
co-authored by
Mattermost Build
Mattermost Build
parent
bc3ad86a23
commit
1cc20031fe
@@ -6,6 +6,7 @@ package api4
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"slices"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
@@ -21,6 +22,28 @@ func shouldRedactExpressions(c *Context) bool {
|
||||
return c.App.Config().FeatureFlags.AttributeValueMasking
|
||||
}
|
||||
|
||||
// preserveSystemManagedFields pins parent imports and team-scope metadata to the stored values:
|
||||
// attaching/detaching parents belongs to the assign/unassign endpoints, so a channel/team admin
|
||||
// editing rules here can't change them. No stored policy (first-time create) means they start empty.
|
||||
func preserveSystemManagedFields(c *Context, policy *model.AccessControlPolicy) *model.AppError {
|
||||
stored, appErr := c.App.GetAccessControlPolicy(c.AppContext, policy.ID)
|
||||
if appErr != nil {
|
||||
if appErr.StatusCode == http.StatusNotFound {
|
||||
policy.Imports = nil
|
||||
policy.Scope = ""
|
||||
policy.ScopeID = ""
|
||||
return nil
|
||||
}
|
||||
return appErr
|
||||
}
|
||||
|
||||
// Clone so a later mutation of policy.Imports can't reach back into the stored object.
|
||||
policy.Imports = slices.Clone(stored.Imports)
|
||||
policy.Scope = stored.Scope
|
||||
policy.ScopeID = stored.ScopeID
|
||||
return nil
|
||||
}
|
||||
|
||||
func (api *API) InitAccessControlPolicy() {
|
||||
api.BaseRoutes.AccessControlPolicies.Handle("", api.APISessionRequired(createAccessControlPolicy)).Methods(http.MethodPut)
|
||||
api.BaseRoutes.AccessControlPolicies.Handle("/search", api.APISessionRequired(searchAccessControlPolicies)).Methods(http.MethodPost)
|
||||
@@ -135,6 +158,11 @@ func createAccessControlPolicy(c *Context, w http.ResponseWriter, r *http.Reques
|
||||
c.Err = appErr
|
||||
return
|
||||
}
|
||||
|
||||
if appErr := preserveSystemManagedFields(c, &policy); appErr != nil {
|
||||
c.Err = appErr
|
||||
return
|
||||
}
|
||||
}
|
||||
case model.AccessControlPolicyTypeTeam:
|
||||
// Team-type policies are keyed by the team ID, so policy.ID is the team.
|
||||
@@ -155,6 +183,11 @@ func createAccessControlPolicy(c *Context, w http.ResponseWriter, r *http.Reques
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
if appErr := preserveSystemManagedFields(c, &policy); appErr != nil {
|
||||
c.Err = appErr
|
||||
return
|
||||
}
|
||||
}
|
||||
default:
|
||||
c.SetInvalidParam("type")
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"slices"
|
||||
"testing"
|
||||
|
||||
"github.com/mattermost/mattermost/server/public/model"
|
||||
@@ -143,6 +144,8 @@ func TestCreateAccessControlPolicy(t *testing.T) {
|
||||
// Create and set up the mock
|
||||
mockAccessControlService := &mocks.AccessControlServiceInterface{}
|
||||
th.App.Srv().Channels().AccessControl = mockAccessControlService
|
||||
notFound := model.NewAppError("GetPolicy", "app.access_control.not_found.app_error", nil, "", http.StatusNotFound)
|
||||
mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), privateChannel.Id).Return(nil, notFound)
|
||||
mockAccessControlService.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.AccessControlPolicy")).Return(channelPolicy, nil).Times(1)
|
||||
|
||||
th.App.UpdateConfig(func(cfg *model.Config) {
|
||||
@@ -265,6 +268,8 @@ func TestCreateAccessControlPolicy(t *testing.T) {
|
||||
ch := th.CreatePrivateChannel(t)
|
||||
|
||||
// Set up mock expectations
|
||||
notFound := model.NewAppError("GetPolicy", "app.access_control.not_found.app_error", nil, "", http.StatusNotFound)
|
||||
mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(nil, notFound)
|
||||
mockAccessControlService.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.AccessControlPolicy")).Return(samplePolicy, nil).Times(1)
|
||||
|
||||
// Set the mock on the app
|
||||
@@ -513,6 +518,271 @@ func TestCreateAccessControlPolicy(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
// A channel/team admin can't change a policy's parent imports or team-scope through the general
|
||||
// update endpoint — those belong to the assign/unassign paths. System admins still can.
|
||||
func TestCreateAccessControlPolicyPreservesSystemManagedFields(t *testing.T) {
|
||||
th := SetupConfig(t, maskingOffTestConfig).InitBasic(t)
|
||||
|
||||
parentID := model.NewId()
|
||||
|
||||
membershipRule := func(expression string) []model.AccessControlPolicyRule {
|
||||
return []model.AccessControlPolicyRule{{Expression: expression, Actions: []string{"membership"}}}
|
||||
}
|
||||
|
||||
// Saved policy must have exactly these imports and no scope: channel/team policies never carry
|
||||
// scope, so a caller-supplied value must be dropped.
|
||||
savedWith := func(imports ...string) any {
|
||||
return mock.MatchedBy(func(p *model.AccessControlPolicy) bool {
|
||||
return slices.Equal(p.Imports, imports) && p.Scope == "" && p.ScopeID == ""
|
||||
})
|
||||
}
|
||||
|
||||
enableABAC := func() *mocks.AccessControlServiceInterface {
|
||||
mockACS := &mocks.AccessControlServiceInterface{}
|
||||
th.App.Srv().Channels().AccessControl = mockACS
|
||||
th.App.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.AccessControlSettings.EnableAttributeBasedAccessControl = new(true)
|
||||
})
|
||||
return mockACS
|
||||
}
|
||||
|
||||
setupChannelAdmin := func(t *testing.T) (*model.Channel, *model.Client4, *mocks.AccessControlServiceInterface) {
|
||||
t.Helper()
|
||||
ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced))
|
||||
require.True(t, ok, "SetLicense should return true")
|
||||
th.AddPermissionToRole(t, model.PermissionManageChannelAccessRules.Id, model.ChannelAdminRoleId)
|
||||
|
||||
privateChannel := th.CreatePrivateChannel(t)
|
||||
channelAdmin := th.CreateUser(t)
|
||||
th.LinkUserToTeam(t, channelAdmin, th.BasicTeam)
|
||||
th.AddUserToChannel(t, channelAdmin, privateChannel)
|
||||
th.MakeUserChannelAdmin(t, channelAdmin, privateChannel)
|
||||
client := th.CreateClient()
|
||||
_, _, err := client.Login(context.Background(), channelAdmin.Email, channelAdmin.Password)
|
||||
require.NoError(t, err)
|
||||
|
||||
return privateChannel, client, enableABAC()
|
||||
}
|
||||
|
||||
// Logs th.Client in as a team admin and stubs the per-rule self-inclusion check so the request
|
||||
// reaches the save. Caller must defer th.LoginBasic(t).
|
||||
setupTeamAdmin := func(t *testing.T) *mocks.AccessControlServiceInterface {
|
||||
t.Helper()
|
||||
ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced))
|
||||
require.True(t, ok, "SetLicense should return true")
|
||||
th.AddPermissionToRole(t, model.PermissionManageTeamAccessRules.Id, model.TeamAdminRoleId)
|
||||
|
||||
teamAdmin := th.CreateUser(t)
|
||||
makeTeamAdminAndLogin(t, th, teamAdmin, th.BasicTeam)
|
||||
|
||||
mockACS := enableABAC()
|
||||
mockACS.On("QueryUsersForExpression", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("string"), mock.AnythingOfType("model.SubjectSearchOptions")).
|
||||
Return([]*model.User{{Id: teamAdmin.Id}}, int64(1), nil)
|
||||
return mockACS
|
||||
}
|
||||
|
||||
// withCallerScope sets scope fields on the request body; the handler owns scope for
|
||||
// channel/team policies and must ignore what the caller sends.
|
||||
withCallerScope := func(p *model.AccessControlPolicy) *model.AccessControlPolicy {
|
||||
p.Scope = model.AccessControlPolicyScopeTeam
|
||||
p.ScopeID = model.NewId()
|
||||
return p
|
||||
}
|
||||
|
||||
notFound := model.NewAppError("GetPolicy", "app.access_control.not_found.app_error", nil, "", http.StatusNotFound)
|
||||
|
||||
t.Run("channel admin cannot detach a stored parent import", func(t *testing.T) {
|
||||
ch, client, mockACS := setupChannelAdmin(t)
|
||||
|
||||
stored := &model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("user.attributes.department == 'security'"),
|
||||
}
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(stored, nil)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith(parentID)).Return(stored, nil).Once()
|
||||
|
||||
req := withCallerScope(&model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{},
|
||||
Rules: membershipRule("user.attributes.department == 'finance'"),
|
||||
})
|
||||
_, resp, err := client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("channel admin cannot swap a stored parent import", func(t *testing.T) {
|
||||
ch, client, mockACS := setupChannelAdmin(t)
|
||||
|
||||
stored := &model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(stored, nil)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith(parentID)).Return(stored, nil).Once()
|
||||
|
||||
req := &model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{model.NewId()},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
_, resp, err := client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("channel admin cannot seed imports on create", func(t *testing.T) {
|
||||
ch, client, mockACS := setupChannelAdmin(t)
|
||||
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(nil, notFound)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith()).Return(&model.AccessControlPolicy{ID: ch.Id, Type: model.AccessControlPolicyTypeChannel}, nil).Once()
|
||||
|
||||
req := withCallerScope(&model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("true"),
|
||||
})
|
||||
_, resp, err := client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("channel admin gets an error when the stored policy lookup fails", func(t *testing.T) {
|
||||
ch, client, mockACS := setupChannelAdmin(t)
|
||||
|
||||
serverErr := model.NewAppError("GetPolicy", "app.pap.get_policy.app_error", nil, "", http.StatusInternalServerError)
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), ch.Id).Return(nil, serverErr)
|
||||
|
||||
req := &model.AccessControlPolicy{
|
||||
ID: ch.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
_, resp, err := client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.Error(t, err)
|
||||
CheckInternalErrorStatus(t, resp)
|
||||
mockACS.AssertNotCalled(t, "SavePolicy", mock.Anything, mock.Anything)
|
||||
})
|
||||
|
||||
t.Run("system admin retains full control over imports", func(t *testing.T) {
|
||||
ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced))
|
||||
require.True(t, ok, "SetLicense should return true")
|
||||
|
||||
privateChannel := th.CreatePrivateChannel(t)
|
||||
mockACS := enableABAC()
|
||||
|
||||
newParent := model.NewId()
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), mock.MatchedBy(func(p *model.AccessControlPolicy) bool {
|
||||
return len(p.Imports) == 1 && p.Imports[0] == newParent
|
||||
})).Return(&model.AccessControlPolicy{ID: privateChannel.Id, Type: model.AccessControlPolicyTypeChannel}, nil).Once()
|
||||
|
||||
policy := &model.AccessControlPolicy{
|
||||
ID: privateChannel.Id,
|
||||
Type: model.AccessControlPolicyTypeChannel,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{newParent},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
_, resp, err := th.SystemAdminClient.CreateAccessControlPolicy(context.Background(), policy)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
// The system-admin path must not consult the stored policy at all.
|
||||
mockACS.AssertNotCalled(t, "GetPolicy", mock.Anything, mock.Anything)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("team admin cannot detach a stored parent import", func(t *testing.T) {
|
||||
mockACS := setupTeamAdmin(t)
|
||||
defer th.LoginBasic(t)
|
||||
|
||||
stored := &model.AccessControlPolicy{
|
||||
ID: th.BasicTeam.Id,
|
||||
Type: model.AccessControlPolicyTypeTeam,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), th.BasicTeam.Id).Return(stored, nil)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith(parentID)).Return(stored, nil).Once()
|
||||
|
||||
req := &model.AccessControlPolicy{
|
||||
ID: th.BasicTeam.Id,
|
||||
Type: model.AccessControlPolicyTypeTeam,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{},
|
||||
Rules: membershipRule("user.attributes.department == 'finance'"),
|
||||
}
|
||||
_, resp, err := th.Client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("team admin cannot swap a stored parent import", func(t *testing.T) {
|
||||
mockACS := setupTeamAdmin(t)
|
||||
defer th.LoginBasic(t)
|
||||
|
||||
stored := &model.AccessControlPolicy{
|
||||
ID: th.BasicTeam.Id,
|
||||
Type: model.AccessControlPolicyTypeTeam,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), th.BasicTeam.Id).Return(stored, nil)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith(parentID)).Return(stored, nil).Once()
|
||||
|
||||
req := &model.AccessControlPolicy{
|
||||
ID: th.BasicTeam.Id,
|
||||
Type: model.AccessControlPolicyTypeTeam,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{model.NewId()},
|
||||
Rules: membershipRule("true"),
|
||||
}
|
||||
_, resp, err := th.Client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
|
||||
t.Run("team admin cannot seed imports on create", func(t *testing.T) {
|
||||
mockACS := setupTeamAdmin(t)
|
||||
defer th.LoginBasic(t)
|
||||
|
||||
mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), th.BasicTeam.Id).Return(nil, notFound)
|
||||
mockACS.On("SavePolicy", mock.AnythingOfType("*request.Context"), savedWith()).Return(&model.AccessControlPolicy{ID: th.BasicTeam.Id, Type: model.AccessControlPolicyTypeTeam}, nil).Once()
|
||||
|
||||
req := withCallerScope(&model.AccessControlPolicy{
|
||||
ID: th.BasicTeam.Id,
|
||||
Type: model.AccessControlPolicyTypeTeam,
|
||||
Version: model.AccessControlPolicyVersionV0_3,
|
||||
Imports: []string{parentID},
|
||||
Rules: membershipRule("true"),
|
||||
})
|
||||
_, resp, err := th.Client.CreateAccessControlPolicy(context.Background(), req)
|
||||
require.NoError(t, err)
|
||||
CheckOKStatus(t, resp)
|
||||
mockACS.AssertExpectations(t)
|
||||
})
|
||||
}
|
||||
|
||||
func TestGetAccessControlPolicy(t *testing.T) {
|
||||
th := SetupConfig(t, maskingOffTestConfig).InitBasic(t)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user