diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index abdaaaa6d72..bb81fb700e2 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -927,8 +927,14 @@ func TestIntegrationService_SaveExternalServiceRole(t *testing.T) { // Check that the permissions and assignment are stored correctly perms, errGetPerms := ac.getUserPermissions(ctx, &user.SignedInUser{OrgID: r.cmd.AssignmentOrgID, UserID: 2}, accesscontrol.Options{}) require.NoError(t, errGetPerms) + + // Only compare action and scope + expPerms := make([]accesscontrol.Permission, len(r.cmd.Permissions)) + for i, p := range r.cmd.Permissions { + expPerms[i] = accesscontrol.Permission{Action: p.Action, Scope: p.Scope} + } // shared with me is added by default for all users in pkg/services/accesscontrol/acimpl/service.go - assert.Equal(t, append([]accesscontrol.Permission{{Action: "folders:read", Scope: "folders:uid:sharedwithme"}}, r.cmd.Permissions...), perms) + assert.Equal(t, append([]accesscontrol.Permission{{Action: "folders:read", Scope: "folders:uid:sharedwithme"}}, expPerms...), perms) } }) } diff --git a/pkg/services/accesscontrol/database/externalservices.go b/pkg/services/accesscontrol/database/externalservices.go index 2ced82d908b..e5af7113f46 100644 --- a/pkg/services/accesscontrol/database/externalservices.go +++ b/pkg/services/accesscontrol/database/externalservices.go @@ -148,7 +148,15 @@ func getRolePermissions(ctx context.Context, sess *db.Session, id int64) ([]acce func permissionDiff(previous, new []accesscontrol.Permission) (added, removed []accesscontrol.Permission) { type key struct{ Action, Scope string } prevMap := map[key]int64{} + unSplit := map[key]int64{} for i := range previous { + // FIXME: This can be removed after a few releases. + // Previously, external service accounts scopes weren't split. + // We need to remove any unsplit permissions. + if previous[i].Scope != "" && previous[i].Kind == "" { + unSplit[key{previous[i].Action, previous[i].Scope}] = previous[i].ID + continue + } prevMap[key{previous[i].Action, previous[i].Scope}] = previous[i].ID } newMap := map[key]int64{} @@ -168,6 +176,12 @@ func permissionDiff(previous, new []accesscontrol.Permission) (added, removed [] removed = append(removed, accesscontrol.Permission{ID: id, Action: p.Action, Scope: p.Scope}) } + // FIXME: This can be removed after a few releases. + // Remove any unsplit permissions + for p, id := range unSplit { + removed = append(removed, accesscontrol.Permission{ID: id, Action: p.Action, Scope: p.Scope}) + } + return added, removed } @@ -204,16 +218,6 @@ func (*AccessControlStore) savePermissions(ctx context.Context, sess *db.Session return err } added, removed := permissionDiff(storedPermissions, permissions) - if len(added) > 0 { - for i := range added { - added[i].RoleID = roleID - added[i].Created = now - added[i].Updated = now - } - if _, err := sess.Insert(&added); err != nil { - return err - } - } if len(removed) > 0 { ids := make([]int64, len(removed)) for i := range removed { @@ -227,6 +231,16 @@ func (*AccessControlStore) savePermissions(ctx context.Context, sess *db.Session return errors.New("failed to delete permissions that have been removed from role") } } + if len(added) > 0 { + for i := range added { + added[i].RoleID = roleID + added[i].Created = now + added[i].Updated = now + } + if _, err := sess.Insert(&added); err != nil { + return err + } + } return nil } diff --git a/pkg/services/accesscontrol/database/externalservices_test.go b/pkg/services/accesscontrol/database/externalservices_test.go index 8ef9964b19b..fc1ffc88db5 100644 --- a/pkg/services/accesscontrol/database/externalservices_test.go +++ b/pkg/services/accesscontrol/database/externalservices_test.go @@ -249,3 +249,95 @@ func TestIntegrationAccessControlStore_DeleteExternalServiceRole(t *testing.T) { }) } } + +func Test_permissionDiff(t *testing.T) { + tests := []struct { + name string + previous []accesscontrol.Permission + new []accesscontrol.Permission + wantAdded []accesscontrol.Permission + wantRemoved []accesscontrol.Permission + }{ + { + name: "no changes", + previous: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {ID: 2, Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + new: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + wantAdded: []accesscontrol.Permission{}, + wantRemoved: []accesscontrol.Permission{}, + }, + { + name: "add new permissions", + previous: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + }, + new: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + wantAdded: []accesscontrol.Permission{ + {Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + wantRemoved: []accesscontrol.Permission{}, + }, + { + name: "remove existing permissions", + previous: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {ID: 2, Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + new: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + }, + wantAdded: []accesscontrol.Permission{}, + wantRemoved: []accesscontrol.Permission{ + {ID: 2, Action: "users:write", Scope: "users:id:2"}, + }, + }, + { + name: "add and remove permissions", + previous: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {ID: 2, Action: "users:write", Scope: "users:id:2", Kind: "users", Attribute: "id", Identifier: "2"}, + }, + new: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + {Action: "users:delete", Scope: "users:id:3", Kind: "users", Attribute: "id", Identifier: "3"}, + }, + wantAdded: []accesscontrol.Permission{ + {Action: "users:delete", Scope: "users:id:3", Kind: "users", Attribute: "id", Identifier: "3"}, + }, + wantRemoved: []accesscontrol.Permission{ + {ID: 2, Action: "users:write", Scope: "users:id:2"}, + }, + }, + { + name: "recreate unsplit permissions", + previous: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1"}, + }, + new: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + }, + wantAdded: []accesscontrol.Permission{ + {Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}, + }, + wantRemoved: []accesscontrol.Permission{ + {ID: 1, Action: "users:read", Scope: "users:id:1"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotAdded, gotRemoved := permissionDiff(tt.previous, tt.new) + require.ElementsMatch(t, tt.wantAdded, gotAdded, "added permissions do not match") + require.ElementsMatch(t, tt.wantRemoved, gotRemoved, "removed permissions do not match") + }) + } +} diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index ad6894feb44..3f93ce6ad82 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -310,6 +310,7 @@ func (cmd *SaveExternalServiceRoleCommand) Validate() error { continue } dedupMap[cmd.Permissions[i]] = true + cmd.Permissions[i].Kind, cmd.Permissions[i].Attribute, cmd.Permissions[i].Identifier = SplitScope(cmd.Permissions[i].Scope) dedup = append(dedup, cmd.Permissions[i]) } cmd.Permissions = dedup diff --git a/pkg/services/accesscontrol/models_test.go b/pkg/services/accesscontrol/models_test.go index c46dd45500b..d4fadffc774 100644 --- a/pkg/services/accesscontrol/models_test.go +++ b/pkg/services/accesscontrol/models_test.go @@ -79,7 +79,7 @@ func TestSaveExternalServiceRoleCommand_Validate(t *testing.T) { }, wantErr: false, wantID: "app-1", - wantPermissions: []Permission{{Action: "users:read", Scope: "users:id:1"}}, + wantPermissions: []Permission{{Action: "users:read", Scope: "users:id:1", Kind: "users", Attribute: "id", Identifier: "1"}}, }, } for _, tt := range tests {