From 29551a6edf3128a41216e5645b9e516dd5826c94 Mon Sep 17 00:00:00 2001 From: Misi Date: Tue, 16 Sep 2025 15:39:01 +0200 Subject: [PATCH] IAM: Implement Delete in Service Account API (#110584) * wip * IAM: Create Service Account * Add dual writer * Update openapi_test.go * Add integration tests * Add sql tests * Add Role to SA spec, add validation, add DBTime, add tests * Format, update test * Fixes * Add check for External * wip * Fix merge * wip * Use plugin name instead of title for ext svc account login Co-authored-by: Gabriel MABILLE * Remove OrgID from DeleteUserCommand * Use the new authorizer * Fix tests * cleanup * Move test to enterprise * Revert unnecessary change * Address feedback * Revert "Address feedback" This reverts commit 8ab9559076443bc285de1a082bb8cb98a54e2502. --------- Co-authored-by: Gabriel MABILLE --- pkg/registry/apis/iam/authorizer.go | 2 +- pkg/registry/apis/iam/legacy/delete_user.sql | 3 +- .../apis/iam/legacy/service_account.go | 82 ++++++++++++++++++ pkg/registry/apis/iam/legacy/sql.go | 3 +- pkg/registry/apis/iam/legacy/sql_test.go | 18 ++-- ...-delete_org_user-delete_org_user_basic.sql | 2 +- ..._org_user-delete_org_user_different_id.sql | 2 +- .../mysql--delete_user-delete_user_basic.sql | 3 +- ...-delete_user-delete_user_different_org.sql | 3 +- ...-delete_org_user-delete_org_user_basic.sql | 2 +- ..._org_user-delete_org_user_different_id.sql | 2 +- ...ostgres--delete_user-delete_user_basic.sql | 3 +- ...-delete_user-delete_user_different_org.sql | 3 +- ...-delete_org_user-delete_org_user_basic.sql | 2 +- ..._org_user-delete_org_user_different_id.sql | 2 +- .../sqlite--delete_user-delete_user_basic.sql | 3 +- ...-delete_user-delete_user_different_org.sql | 3 +- pkg/registry/apis/iam/legacy/user.go | 76 +++++++--------- pkg/registry/apis/iam/register.go | 1 - pkg/registry/apis/iam/serviceaccount/store.go | 37 +++++++- .../apis/iam/serviceaccount/validate.go | 2 +- .../apis/iam/serviceaccount/validate_test.go | 2 +- pkg/registry/apis/iam/user/store.go | 2 +- pkg/services/authz/rbac/mapper.go | 1 + pkg/services/authz/rbac/resolver.go | 41 +++++++++ .../iam/service_account_integration_test.go | 86 ++++++++++++------- .../serviceaccount-test-higher-role-v0.yaml | 7 -- 27 files changed, 266 insertions(+), 127 deletions(-) delete mode 100644 pkg/tests/apis/iam/testdata/serviceaccount-test-higher-role-v0.yaml diff --git a/pkg/registry/apis/iam/authorizer.go b/pkg/registry/apis/iam/authorizer.go index 9d28ea8aa43..4fd5544bb56 100644 --- a/pkg/registry/apis/iam/authorizer.go +++ b/pkg/registry/apis/iam/authorizer.go @@ -24,7 +24,6 @@ func newIAMAuthorizer(accessClient authlib.AccessClient, legacyAccessClient auth // Identity specific resources legacyAuthorizer := gfauthorizer.NewResourceAuthorizer(legacyAccessClient) resourceAuthorizer[iamv0.UserResourceInfo.GetName()] = legacyAuthorizer - resourceAuthorizer[iamv0.ServiceAccountResourceInfo.GetName()] = legacyAuthorizer resourceAuthorizer[iamv0.TeamResourceInfo.GetName()] = legacyAuthorizer resourceAuthorizer["display"] = legacyAuthorizer @@ -33,6 +32,7 @@ func newIAMAuthorizer(accessClient authlib.AccessClient, legacyAccessClient auth resourceAuthorizer[iamv0.CoreRoleInfo.GetName()] = authorizer resourceAuthorizer[iamv0.RoleInfo.GetName()] = authorizer resourceAuthorizer[iamv0.ResourcePermissionInfo.GetName()] = authorizer + resourceAuthorizer[iamv0.ServiceAccountResourceInfo.GetName()] = authorizer return &iamAuthorizer{resourceAuthorizer: resourceAuthorizer} } diff --git a/pkg/registry/apis/iam/legacy/delete_user.sql b/pkg/registry/apis/iam/legacy/delete_user.sql index 3c090010cf1..87881a028f4 100644 --- a/pkg/registry/apis/iam/legacy/delete_user.sql +++ b/pkg/registry/apis/iam/legacy/delete_user.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM {{ .Ident .UserTable }} -WHERE uid = {{ .Arg .Query.UID }} - AND org_id = {{ .Arg .Query.OrgID }} +WHERE uid = {{ .Arg .Command.UID }} diff --git a/pkg/registry/apis/iam/legacy/service_account.go b/pkg/registry/apis/iam/legacy/service_account.go index feae6927e6a..b97c54ae8d7 100644 --- a/pkg/registry/apis/iam/legacy/service_account.go +++ b/pkg/registry/apis/iam/legacy/service_account.go @@ -400,3 +400,85 @@ func (s *legacySQLStore) CreateServiceAccount(ctx context.Context, ns claims.Nam return &CreateServiceAccountResult{ServiceAccount: createdSA}, nil } + +func (s *legacySQLStore) DeleteServiceAccount(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) error { + sql, err := s.sql(ctx) + if err != nil { + return err + } + + req := newDeleteUser(sql, &cmd) + if err := req.Validate(); err != nil { + return err + } + + err = sql.DB.GetSqlxSession().WithTransaction(ctx, func(st *session.SessionTx) error { + userLookupReq := newGetServiceAccountInternalID(sql, &GetServiceAccountInternalIDQuery{ + OrgID: ns.OrgID, + UID: cmd.UID, + }) + + userQuery, err := sqltemplate.Execute(sqlQueryServiceAccountInternalIDTemplate, userLookupReq) + if err != nil { + return fmt.Errorf("execute user lookup template: %w", err) + } + + rows, err := st.Query(ctx, userQuery, userLookupReq.GetArgs()...) + if err != nil { + return fmt.Errorf("failed to check if user exists: %w", err) + } + defer func() { + if rows != nil { + _ = rows.Close() + } + }() + + var userID int64 + if !rows.Next() { + if err := rows.Err(); err != nil { + return fmt.Errorf("failed to read user lookup rows: %w", err) + } + return fmt.Errorf("user not found") + } + + if err := rows.Scan(&userID); err != nil { + return fmt.Errorf("failed to scan user ID: %w", err) + } + + // Close rows to avoid the bad connection error + if rows != nil { + _ = rows.Close() + } + + orgUserReq := newDeleteOrgUser(sql, userID) + if err := orgUserReq.Validate(); err != nil { + return err + } + + orgUserDeleteQuery, err := sqltemplate.Execute(sqlDeleteOrgUserTemplate, orgUserReq) + if err != nil { + return fmt.Errorf("execute org_user delete template: %w", err) + } + + if _, err := st.Exec(ctx, orgUserDeleteQuery, orgUserReq.GetArgs()...); err != nil { + return fmt.Errorf("failed to delete org_user relationship: %w", err) + } + + deleteQuery, err := sqltemplate.Execute(sqlDeleteUserTemplate, req) + if err != nil { + return fmt.Errorf("execute service account template %q: %w", sqlDeleteUserTemplate.Name(), err) + } + + if _, err := st.Exec(ctx, deleteQuery, req.GetArgs()...); err != nil { + return fmt.Errorf("failed to delete service account: %w", err) + } + + return nil + }) + + if err != nil { + return err + } + + return nil +} diff --git a/pkg/registry/apis/iam/legacy/sql.go b/pkg/registry/apis/iam/legacy/sql.go index 1f77d18e6ed..71e127faeef 100644 --- a/pkg/registry/apis/iam/legacy/sql.go +++ b/pkg/registry/apis/iam/legacy/sql.go @@ -20,11 +20,12 @@ type LegacyIdentityStore interface { ListUsers(ctx context.Context, ns claims.NamespaceInfo, query ListUserQuery) (*ListUserResult, error) ListUserTeams(ctx context.Context, ns claims.NamespaceInfo, query ListUserTeamsQuery) (*ListUserTeamsResult, error) CreateUser(ctx context.Context, ns claims.NamespaceInfo, cmd CreateUserCommand) (*CreateUserResult, error) - DeleteUser(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) (*DeleteUserResult, error) + DeleteUser(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) error GetServiceAccountInternalID(ctx context.Context, ns claims.NamespaceInfo, query GetServiceAccountInternalIDQuery) (*GetServiceAccountInternalIDResult, error) ListServiceAccounts(ctx context.Context, ns claims.NamespaceInfo, query ListServiceAccountsQuery) (*ListServiceAccountResult, error) CreateServiceAccount(ctx context.Context, ns claims.NamespaceInfo, cmd CreateServiceAccountCommand) (*CreateServiceAccountResult, error) + DeleteServiceAccount(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) error ListServiceAccountTokens(ctx context.Context, ns claims.NamespaceInfo, query ListServiceAccountTokenQuery) (*ListServiceAccountTokenResult, error) diff --git a/pkg/registry/apis/iam/legacy/sql_test.go b/pkg/registry/apis/iam/legacy/sql_test.go index 332f7f7697a..daa737aacfb 100644 --- a/pkg/registry/apis/iam/legacy/sql_test.go +++ b/pkg/registry/apis/iam/legacy/sql_test.go @@ -31,18 +31,14 @@ func TestIdentityQueries(t *testing.T) { return &v } - deleteUser := func(q *DeleteUserQuery) sqltemplate.SQLTemplate { + deleteUser := func(q *DeleteUserCommand) sqltemplate.SQLTemplate { v := newDeleteUser(nodb, q) v.SQLTemplate = mocks.NewTestingSQLTemplate() return &v } deleteOrgUser := func(userID int64) sqltemplate.SQLTemplate { - v := deleteOrgUserQuery{ - SQLTemplate: mocks.NewTestingSQLTemplate(), - OrgUserTable: nodb.Table("org_user"), - UserID: userID, - } + v := newDeleteOrgUser(nodb, userID) return &v } @@ -337,16 +333,14 @@ func TestIdentityQueries(t *testing.T) { sqlDeleteUserTemplate: { { Name: "delete_user_basic", - Data: deleteUser(&DeleteUserQuery{ - OrgID: 1, - UID: "user-1", + Data: deleteUser(&DeleteUserCommand{ + UID: "user-1", }), }, { Name: "delete_user_different_org", - Data: deleteUser(&DeleteUserQuery{ - OrgID: 2, - UID: "user-abc", + Data: deleteUser(&DeleteUserCommand{ + UID: "user-abc", }), }, }, diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_basic.sql index c76fea1d97a..aeb73501b64 100755 --- a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_basic.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM `grafana`.`org_user` -WHERE user_id = 123 +WHERE user_id = ? diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_different_id.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_different_id.sql index 00f92d1f153..aeb73501b64 100755 --- a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_different_id.sql +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_org_user-delete_org_user_different_id.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM `grafana`.`org_user` -WHERE user_id = 456 +WHERE user_id = ? diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_basic.sql index 12e6b24c9f4..79e569b2bf5 100755 --- a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_basic.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM `grafana`.`user` -WHERE uid = 'user-1' - AND org_id = 1 +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_different_org.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_different_org.sql index 130bc314d90..a65cea40456 100755 --- a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_different_org.sql +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_user-delete_user_different_org.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM `grafana`.`user` -WHERE uid = 'user-abc' - AND org_id = 2 +WHERE uid = 'user-abc' diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_basic.sql index 100c57c58b0..573bf77ab84 100755 --- a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_basic.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM "grafana"."org_user" -WHERE user_id = 123 +WHERE user_id = $1 diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_different_id.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_different_id.sql index 4a4730c13a8..573bf77ab84 100755 --- a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_different_id.sql +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_org_user-delete_org_user_different_id.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM "grafana"."org_user" -WHERE user_id = 456 +WHERE user_id = $1 diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_basic.sql index 9b71e92959a..96a8824b003 100755 --- a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_basic.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM "grafana"."user" -WHERE uid = 'user-1' - AND org_id = 1 +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_different_org.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_different_org.sql index a4bdd5f5635..b8a63f2d0f1 100755 --- a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_different_org.sql +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_user-delete_user_different_org.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM "grafana"."user" -WHERE uid = 'user-abc' - AND org_id = 2 +WHERE uid = 'user-abc' diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_basic.sql index 100c57c58b0..9694d9151c3 100755 --- a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_basic.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM "grafana"."org_user" -WHERE user_id = 123 +WHERE user_id = ? diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_different_id.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_different_id.sql index 4a4730c13a8..9694d9151c3 100755 --- a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_different_id.sql +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_org_user-delete_org_user_different_id.sql @@ -1,3 +1,3 @@ -- Delete from org_user table for a specific user DELETE FROM "grafana"."org_user" -WHERE user_id = 456 +WHERE user_id = ? diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_basic.sql index 9b71e92959a..96a8824b003 100755 --- a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_basic.sql +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_basic.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM "grafana"."user" -WHERE uid = 'user-1' - AND org_id = 1 +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_different_org.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_different_org.sql index a4bdd5f5635..b8a63f2d0f1 100755 --- a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_different_org.sql +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_user-delete_user_different_org.sql @@ -1,4 +1,3 @@ -- Delete from user table (org_user will be handled separately to avoid locking) DELETE FROM "grafana"."user" -WHERE uid = 'user-abc' - AND org_id = 2 +WHERE uid = 'user-abc' diff --git a/pkg/registry/apis/iam/legacy/user.go b/pkg/registry/apis/iam/legacy/user.go index b1c656a4f83..733230b3266 100644 --- a/pkg/registry/apis/iam/legacy/user.go +++ b/pkg/registry/apis/iam/legacy/user.go @@ -325,15 +325,6 @@ type DeleteUserCommand struct { UID string } -type DeleteUserQuery struct { - OrgID int64 - UID string -} - -type DeleteUserResult struct { - Success bool -} - var sqlCreateUserTemplate = mustTemplate("create_user.sql") var sqlCreateOrgUserTemplate = mustTemplate("create_org_user.sql") var sqlDeleteUserTemplate = mustTemplate("delete_user.sql") @@ -467,20 +458,34 @@ func (s *legacySQLStore) CreateUser(ctx context.Context, ns claims.NamespaceInfo return &CreateUserResult{User: createdUser}, nil } -func newDeleteUser(sql *legacysql.LegacyDatabaseHelper, q *DeleteUserQuery) deleteUserQuery { +func newDeleteUser(sql *legacysql.LegacyDatabaseHelper, cmd *DeleteUserCommand) deleteUserQuery { return deleteUserQuery{ - SQLTemplate: sqltemplate.New(sql.DialectForDriver()), - UserTable: sql.Table("user"), - OrgUserTable: sql.Table("org_user"), - Query: q, + SQLTemplate: sqltemplate.New(sql.DialectForDriver()), + UserTable: sql.Table("user"), + Command: cmd, } } type deleteUserQuery struct { sqltemplate.SQLTemplate - UserTable string - OrgUserTable string - Query *DeleteUserQuery + UserTable string + Command *DeleteUserCommand +} + +func (r deleteUserQuery) Validate() error { + if r.Command.UID == "" { + return fmt.Errorf("user UID is required") + } + + return nil +} + +func newDeleteOrgUser(sql *legacysql.LegacyDatabaseHelper, userID int64) deleteOrgUserQuery { + return deleteOrgUserQuery{ + SQLTemplate: sqltemplate.New(sql.DialectForDriver()), + OrgUserTable: sql.Table("org_user"), + UserID: userID, + } } type deleteOrgUserQuery struct { @@ -496,45 +501,22 @@ func (r deleteOrgUserQuery) Validate() error { return nil } -func (r deleteUserQuery) Validate() error { - if r.Query.UID == "" { - return fmt.Errorf("user UID is required") - } - if r.Query.OrgID == 0 { - return fmt.Errorf("org ID is required") - } - return nil -} - // DeleteUser implements LegacyIdentityStore. -func (s *legacySQLStore) DeleteUser(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) (*DeleteUserResult, error) { - if ns.OrgID == 0 { - return nil, fmt.Errorf("expected non zero org id") - } - - if cmd.UID == "" { - return nil, fmt.Errorf("user UID is required") - } - +func (s *legacySQLStore) DeleteUser(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) error { sql, err := s.sql(ctx) if err != nil { - return nil, err + return err } - query := &DeleteUserQuery{ - OrgID: ns.OrgID, - UID: cmd.UID, - } - - req := newDeleteUser(sql, query) + req := newDeleteUser(sql, &cmd) if err := req.Validate(); err != nil { - return nil, err + return err } err = sql.DB.GetSqlxSession().WithTransaction(ctx, func(st *session.SessionTx) error { userLookupReq := newGetUserInternalID(sql, &GetUserInternalIDQuery{ OrgID: ns.OrgID, - UID: req.Query.UID, + UID: req.Command.UID, }) userQuery, err := sqltemplate.Execute(sqlQueryUserInternalIDTemplate, userLookupReq) @@ -608,8 +590,8 @@ func (s *legacySQLStore) DeleteUser(ctx context.Context, ns claims.NamespaceInfo }) if err != nil { - return nil, err + return err } - return &DeleteUserResult{Success: true}, nil + return nil } diff --git a/pkg/registry/apis/iam/register.go b/pkg/registry/apis/iam/register.go index a54a038a08e..f9f06e7580c 100644 --- a/pkg/registry/apis/iam/register.go +++ b/pkg/registry/apis/iam/register.go @@ -363,7 +363,6 @@ func (b *IdentityAccessManagementAPIBuilder) Mutate(ctx context.Context, a admis case *iamv0.ServiceAccount: return serviceaccount.MutateOnCreate(ctx, typedObj) } - return nil case admission.Update: return nil case admission.Delete: diff --git a/pkg/registry/apis/iam/serviceaccount/store.go b/pkg/registry/apis/iam/serviceaccount/store.go index 009635649d4..2980322cb27 100644 --- a/pkg/registry/apis/iam/serviceaccount/store.go +++ b/pkg/registry/apis/iam/serviceaccount/store.go @@ -52,7 +52,40 @@ func (s *LegacyStore) DeleteCollection(ctx context.Context, deleteValidation res // Delete implements rest.GracefulDeleter. func (s *LegacyStore) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { - return nil, false, apierrors.NewMethodNotSupported(resource.GroupResource(), "delete") + if !s.enableAuthnMutation { + return nil, false, apierrors.NewMethodNotSupported(resource.GroupResource(), "delete") + } + + ns, err := request.NamespaceInfoFrom(ctx, true) + if err != nil { + return nil, false, err + } + + toBeDeleted, err := s.Get(ctx, name, nil) + if err != nil { + return nil, false, err + } + + if deleteValidation != nil { + if err := deleteValidation(ctx, toBeDeleted); err != nil { + return nil, false, err + } + } + + err = s.store.DeleteServiceAccount(ctx, ns, legacy.DeleteUserCommand{ + UID: name, + }) + + if err != nil { + return nil, false, err + } + + return &iamv0alpha1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Value, + }, + }, true, nil } // Update implements rest.Updater. @@ -89,7 +122,7 @@ func (s *LegacyStore) Create(ctx context.Context, obj runtime.Object, createVali login := serviceaccounts.GenerateLogin(serviceaccounts.ServiceAccountPrefix, ns.OrgID, saObj.Spec.Title) if saObj.Spec.Plugin != "" { - login = serviceaccounts.ExtSvcLoginPrefix(ns.OrgID) + slugify.Slugify(saObj.Spec.Title) + login = serviceaccounts.ExtSvcLoginPrefix(ns.OrgID) + slugify.Slugify(saObj.Spec.Plugin) } createCmd := legacy.CreateServiceAccountCommand{ diff --git a/pkg/registry/apis/iam/serviceaccount/validate.go b/pkg/registry/apis/iam/serviceaccount/validate.go index 34e851461a0..72de42b7f35 100644 --- a/pkg/registry/apis/iam/serviceaccount/validate.go +++ b/pkg/registry/apis/iam/serviceaccount/validate.go @@ -51,7 +51,7 @@ func ValidateOnCreate(ctx context.Context, obj *iamv0alpha1.ServiceAccount) erro if !requester.HasRole(requestedRole) { return apierrors.NewForbidden(iamv0alpha1.ServiceAccountResourceInfo.GroupResource(), obj.Name, - fmt.Errorf("can not assign a role higher than user's role")) + fmt.Errorf("cannot assign a role higher than user's role")) } return nil diff --git a/pkg/registry/apis/iam/serviceaccount/validate_test.go b/pkg/registry/apis/iam/serviceaccount/validate_test.go index 39703b83c83..d19aae3f7c8 100644 --- a/pkg/registry/apis/iam/serviceaccount/validate_test.go +++ b/pkg/registry/apis/iam/serviceaccount/validate_test.go @@ -77,7 +77,7 @@ func TestValidateOnCreate(t *testing.T) { OrgRole: identity.RoleViewer, }, expectError: true, - errorContains: "can not assign a role higher than user's role", + errorContains: "cannot assign a role higher than user's role", }, { name: "external service account - valid", diff --git a/pkg/registry/apis/iam/user/store.go b/pkg/registry/apis/iam/user/store.go index 988bc24c469..1c1bf9eae67 100644 --- a/pkg/registry/apis/iam/user/store.go +++ b/pkg/registry/apis/iam/user/store.go @@ -92,7 +92,7 @@ func (s *LegacyStore) Delete(ctx context.Context, name string, deleteValidation UID: name, } - _, err = s.store.DeleteUser(ctx, ns, deleteCmd) + err = s.store.DeleteUser(ctx, ns, deleteCmd) if err != nil { return nil, false, fmt.Errorf("failed to delete user: %w", err) } diff --git a/pkg/services/authz/rbac/mapper.go b/pkg/services/authz/rbac/mapper.go index e15d336fbb7..950287a1059 100644 --- a/pkg/services/authz/rbac/mapper.go +++ b/pkg/services/authz/rbac/mapper.go @@ -110,6 +110,7 @@ func NewMapperRegistry() MapperRegistry { "folders": newResourceTranslation("folders", "uid", true, false), }, "iam.grafana.app": { + "serviceaccounts": newResourceTranslation("serviceaccounts", "uid", false, true), // Teams is a special case. We translate user permissions from id to uid based. "teams": newResourceTranslation("teams", "uid", false, true), // No need to skip scope on create for roles because we translate `permissions:type:delegate` to `roles:*`` diff --git a/pkg/services/authz/rbac/resolver.go b/pkg/services/authz/rbac/resolver.go index 0a774844c29..aaad1818852 100644 --- a/pkg/services/authz/rbac/resolver.go +++ b/pkg/services/authz/rbac/resolver.go @@ -13,6 +13,44 @@ import ( type ScopeResolverFunc func(scope string) (string, error) +func (s *Service) fetchServiceAccounts(ctx context.Context, ns types.NamespaceInfo) (map[int64]string, error) { + serviceAccounts, err := s.identityStore.ListServiceAccounts(ctx, ns, legacy.ListServiceAccountsQuery{}) + if err != nil { + return nil, fmt.Errorf("could not fetch service accounts: %w", err) + } + saIDs := make(map[int64]string, len(serviceAccounts.Items)) + for _, sa := range serviceAccounts.Items { + saIDs[sa.ID] = sa.UID + } + return saIDs, nil +} + +// Should return an error if we fail to build the resolver. +func (s *Service) newServiceAccountNameResolver(ctx context.Context, ns types.NamespaceInfo) (ScopeResolverFunc, error) { + return func(scope string) (string, error) { + saIDs, err := s.fetchServiceAccounts(ctx, ns) + if err != nil { + return "", fmt.Errorf("could not build resolver: %w", err) + } + + serviceAccountIDStr := strings.TrimPrefix(scope, "serviceaccounts:id:") + if serviceAccountIDStr == "" { + return "", fmt.Errorf("service account ID is empty") + } + if serviceAccountIDStr == "*" { + return "serviceaccounts:uid:*", nil + } + serviceAccountID, err := strconv.ParseInt(serviceAccountIDStr, 10, 64) + if err != nil { + return "", fmt.Errorf("invalid service account ID %s: %w", serviceAccountIDStr, err) + } + if serviceAccountName, ok := saIDs[serviceAccountID]; ok { + return "serviceaccounts:uid:" + serviceAccountName, nil + } + return "", fmt.Errorf("service account ID %s not found", serviceAccountIDStr) + }, nil +} + func (s *Service) fetchTeams(ctx context.Context, ns types.NamespaceInfo) (map[int64]string, error) { key := teamIDsCacheKey(ns.Value) res, err, _ := s.sf.Do(key, func() (any, error) { @@ -97,6 +135,9 @@ func (s *Service) nameResolver(ctx context.Context, ns types.NamespaceInfo, scop if scopePrefix == "permissions:type:" { return permissionsDelegateResolverFunc, nil } + if scopePrefix == "serviceaccounts:id:" { + return s.newServiceAccountNameResolver(ctx, ns) + } // No resolver found for the given scope prefix. return nil, nil } diff --git a/pkg/tests/apis/iam/service_account_integration_test.go b/pkg/tests/apis/iam/service_account_integration_test.go index 6117a56c2fa..929c66ba693 100644 --- a/pkg/tests/apis/iam/service_account_integration_test.go +++ b/pkg/tests/apis/iam/service_account_integration_test.go @@ -5,21 +5,18 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "github.com/grafana/grafana/pkg/apiserver/rest" - "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/testinfra" "github.com/grafana/grafana/pkg/util/testutil" - - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" ) var gvrServiceAccounts = schema.GroupVersionResource{ @@ -59,7 +56,7 @@ func TestIntegrationServiceAccounts(t *testing.T) { } func doServiceAccountCRUDTestsUsingTheNewAPIs(t *testing.T, helper *apis.K8sTestHelper) { - t.Run("should create service account and get it using the new APIs as a GrafanaAdmin", func(t *testing.T) { + t.Run("should create service account and get it using the new APIs as admin", func(t *testing.T) { ctx := context.Background() saClient := helper.GetResourceClient(apis.ResourceClientArgs{ @@ -94,6 +91,15 @@ func doServiceAccountCRUDTestsUsingTheNewAPIs(t *testing.T, helper *apis.K8sTest require.Equal(t, createdUID, fetched.GetName()) require.Equal(t, "default", fetched.GetNamespace()) + + err = saClient.Resource.Delete(ctx, createdUID, metav1.DeleteOptions{}) + require.NoError(t, err) + + _, err = saClient.Resource.Get(ctx, createdUID, metav1.GetOptions{}) + require.Error(t, err) + var statusErr *errors.StatusError + require.ErrorAs(t, err, &statusErr) + require.Equal(t, int32(404), statusErr.ErrStatus.Code) }) t.Run("should not be able to create service account when using a user with insufficient permissions", func(t *testing.T) { @@ -136,30 +142,6 @@ func doServiceAccountCRUDTestsUsingTheNewAPIs(t *testing.T, helper *apis.K8sTest require.Contains(t, statusErr.ErrStatus.Message, "invalid role: InvalidRole") }) - t.Run("should not be able to create service account with higher role than the user", func(t *testing.T) { - ctx := context.Background() - - editorWithSACreate := helper.CreateUser("custom-editor", apis.Org1, org.RoleEditor, - []resourcepermissions.SetResourcePermissionCommand{ - {Actions: []string{serviceaccounts.ActionCreate}}, - }) - - saClient := helper.GetResourceClient(apis.ResourceClientArgs{ - User: editorWithSACreate, - Namespace: helper.Namespacer(editorWithSACreate.Identity.GetOrgID()), - GVR: gvrServiceAccounts, - }) - - saToCreate := helper.LoadYAMLOrJSONFile("testdata/serviceaccount-test-higher-role-v0.yaml") - - _, err := saClient.Resource.Create(ctx, saToCreate, metav1.CreateOptions{}) - require.Error(t, err) - var statusErr *errors.StatusError - require.ErrorAs(t, err, &statusErr) - require.Equal(t, int32(403), statusErr.ErrStatus.Code) - require.Contains(t, statusErr.ErrStatus.Message, "can not assign a role higher than user's role") - }) - t.Run("should not be able to create service account without a title", func(t *testing.T) { ctx := context.Background() saClient := helper.GetResourceClient(apis.ResourceClientArgs{ @@ -239,8 +221,9 @@ func doServiceAccountCRUDTestsUsingTheLegacyAPIs(t *testing.T, helper *apis.K8sT t.Run("should create service account using legacy APIs and get it using the new APIs", func(t *testing.T) { ctx := context.Background() saClient := helper.GetResourceClient(apis.ResourceClientArgs{ - User: helper.Org1.Admin, - GVR: gvrServiceAccounts, + User: helper.Org1.Admin, + Namespace: helper.Namespacer(helper.Org1.Admin.Identity.GetOrgID()), + GVR: gvrServiceAccounts, }) legacySAPayload := `{ @@ -271,4 +254,41 @@ func doServiceAccountCRUDTestsUsingTheLegacyAPIs(t *testing.T, helper *apis.K8sT require.Equal(t, rsp.Result.UID, sa.GetName()) require.Equal(t, "default", sa.GetNamespace()) }) + + t.Run("should create service account using legacy APIs and delete it using the new APIs", func(t *testing.T) { + ctx := context.Background() + saClient := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + Namespace: helper.Namespacer(helper.Org1.Admin.Identity.GetOrgID()), + GVR: gvrServiceAccounts, + }) + + legacySAPayload := `{ + "name": "Test Service Account to delete", + "role": "Editor" + }` + + rsp := apis.DoRequest(helper, apis.RequestParams{ + User: helper.Org1.Admin, + Method: "POST", + Path: "/api/serviceaccounts", + Body: []byte(legacySAPayload), + }, &serviceaccounts.ServiceAccountDTO{}) + + require.NotNil(t, rsp) + require.Equal(t, 201, rsp.Response.StatusCode) + require.NotEmpty(t, rsp.Result.UID) + + _, err := saClient.Resource.Get(ctx, rsp.Result.UID, metav1.GetOptions{}) + require.NoError(t, err) + + err = saClient.Resource.Delete(ctx, rsp.Result.UID, metav1.DeleteOptions{}) + require.NoError(t, err) + + _, err = saClient.Resource.Get(ctx, rsp.Result.UID, metav1.GetOptions{}) + require.Error(t, err) + var statusErr *errors.StatusError + require.ErrorAs(t, err, &statusErr) + require.Equal(t, int32(404), statusErr.ErrStatus.Code) + }) } diff --git a/pkg/tests/apis/iam/testdata/serviceaccount-test-higher-role-v0.yaml b/pkg/tests/apis/iam/testdata/serviceaccount-test-higher-role-v0.yaml deleted file mode 100644 index 59512f73bfe..00000000000 --- a/pkg/tests/apis/iam/testdata/serviceaccount-test-higher-role-v0.yaml +++ /dev/null @@ -1,7 +0,0 @@ -apiVersion: iam.grafana.app/v0alpha1 -kind: ServiceAccount -metadata: - name: sa-with-higher-role -spec: - title: SA with higher role - role: Admin