From bd1dbb68ba26390e9fa06887959aac90eeb7cc5a Mon Sep 17 00:00:00 2001 From: Misi Date: Mon, 13 Oct 2025 10:00:18 +0200 Subject: [PATCH] IAM: Add the implementation of the Update User API (#112054) * wip * Add validate, wire mutate, add tests * Address copilot's feedback * Address feedback --- .../apis/iam/legacy/service_account.go | 2 +- pkg/registry/apis/iam/legacy/sql.go | 1 + pkg/registry/apis/iam/legacy/sql_test.go | 39 +++ ...-update_org_user-update_org_user_basic.sql | 6 + .../mysql--update_user-update_user_basic.sql | 11 + ...-update_org_user-update_org_user_basic.sql | 6 + ...ostgres--update_user-update_user_basic.sql | 11 + ...-update_org_user-update_org_user_basic.sql | 6 + .../sqlite--update_user-update_user_basic.sql | 11 + .../apis/iam/legacy/update_org_user.sql | 6 + pkg/registry/apis/iam/legacy/update_user.sql | 11 + pkg/registry/apis/iam/legacy/user.go | 135 +++++++- pkg/registry/apis/iam/register.go | 13 +- pkg/registry/apis/iam/user/mutate.go | 6 +- pkg/registry/apis/iam/user/mutate_test.go | 64 +++- pkg/registry/apis/iam/user/store.go | 49 ++- pkg/registry/apis/iam/user/validate.go | 71 ++++ pkg/registry/apis/iam/user/validate_test.go | 311 +++++++++++++++++- .../iam/testdata/user-test-create-2-v0.yaml | 11 + .../iam/testdata/user-test-create-v0.yaml | 2 +- pkg/tests/apis/iam/user_integration_test.go | 70 +++- 21 files changed, 820 insertions(+), 22 deletions(-) create mode 100755 pkg/registry/apis/iam/legacy/testdata/mysql--update_org_user-update_org_user_basic.sql create mode 100755 pkg/registry/apis/iam/legacy/testdata/mysql--update_user-update_user_basic.sql create mode 100755 pkg/registry/apis/iam/legacy/testdata/postgres--update_org_user-update_org_user_basic.sql create mode 100755 pkg/registry/apis/iam/legacy/testdata/postgres--update_user-update_user_basic.sql create mode 100755 pkg/registry/apis/iam/legacy/testdata/sqlite--update_org_user-update_org_user_basic.sql create mode 100755 pkg/registry/apis/iam/legacy/testdata/sqlite--update_user-update_user_basic.sql create mode 100644 pkg/registry/apis/iam/legacy/update_org_user.sql create mode 100644 pkg/registry/apis/iam/legacy/update_user.sql create mode 100644 pkg/tests/apis/iam/testdata/user-test-create-2-v0.yaml diff --git a/pkg/registry/apis/iam/legacy/service_account.go b/pkg/registry/apis/iam/legacy/service_account.go index b97c54ae8d7..b3b47f25fe7 100644 --- a/pkg/registry/apis/iam/legacy/service_account.go +++ b/pkg/registry/apis/iam/legacy/service_account.go @@ -332,7 +332,7 @@ func (s *legacySQLStore) CreateServiceAccount(ctx context.Context, ns claims.Nam cmd.OrgID = ns.OrgID cmd.Email = cmd.Login - now := time.Now().UTC().Truncate(time.Second) + now := time.Now().UTC() lastSeenAt := now.AddDate(-10, 0, 0) // Set last seen 10 years ago like in user service cmd.Created = NewDBTime(now) diff --git a/pkg/registry/apis/iam/legacy/sql.go b/pkg/registry/apis/iam/legacy/sql.go index 58d7c9f1fd3..f5ef5722d23 100644 --- a/pkg/registry/apis/iam/legacy/sql.go +++ b/pkg/registry/apis/iam/legacy/sql.go @@ -20,6 +20,7 @@ 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) + UpdateUser(ctx context.Context, ns claims.NamespaceInfo, cmd UpdateUserCommand) (*UpdateUserResult, error) DeleteUser(ctx context.Context, ns claims.NamespaceInfo, cmd DeleteUserCommand) error GetServiceAccountInternalID(ctx context.Context, ns claims.NamespaceInfo, query GetServiceAccountInternalIDQuery) (*GetServiceAccountInternalIDResult, error) diff --git a/pkg/registry/apis/iam/legacy/sql_test.go b/pkg/registry/apis/iam/legacy/sql_test.go index 7c4fad74ba8..58c268b6d95 100644 --- a/pkg/registry/apis/iam/legacy/sql_test.go +++ b/pkg/registry/apis/iam/legacy/sql_test.go @@ -114,6 +114,18 @@ func TestIdentityQueries(t *testing.T) { return &v } + updateUser := func(cmd *UpdateUserCommand) sqltemplate.SQLTemplate { + v := newUpdateUser(nodb, cmd) + v.SQLTemplate = mocks.NewTestingSQLTemplate() + return &v + } + + updateOrgUser := func(cmd *UpdateOrgUserCommand) sqltemplate.SQLTemplate { + v := newUpdateOrgUser(nodb, cmd) + v.SQLTemplate = mocks.NewTestingSQLTemplate() + return &v + } + mocks.CheckQuerySnapshots(t, mocks.TemplateTestSetup{ RootDir: "testdata", SQLTemplatesFS: sqlTemplatesFS, @@ -514,6 +526,33 @@ func TestIdentityQueries(t *testing.T) { }), }, }, + sqlUpdateUserTemplate: { + { + Name: "update_user_basic", + Data: updateUser(&UpdateUserCommand{ + UID: "user-1", + Login: "newuser1", + Email: "newuser1@example.com", + Name: "New User One", + IsAdmin: true, + IsDisabled: true, + EmailVerified: false, + Role: "Editor", + Updated: NewDBTime(time.Date(2023, 1, 1, 13, 0, 0, 0, time.UTC)), + }), + }, + }, + sqlUpdateOrgUserTemplate: { + { + Name: "update_org_user_basic", + Data: updateOrgUser(&UpdateOrgUserCommand{ + OrgID: 1, + UserID: 123, + Role: "Admin", + Updated: NewDBTime(time.Date(2023, 1, 1, 14, 0, 0, 0, time.UTC)), + }), + }, + }, }, }) } diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--update_org_user-update_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--update_org_user-update_org_user_basic.sql new file mode 100755 index 00000000000..1939b9c18b4 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--update_org_user-update_org_user_basic.sql @@ -0,0 +1,6 @@ +-- name: update_org_user +UPDATE `grafana`.`org_user` +SET + role = 'Admin', + updated = '2023-01-01 14:00:00' +WHERE org_id = 1 AND user_id = 123 diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--update_user-update_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--update_user-update_user_basic.sql new file mode 100755 index 00000000000..ee94ab18dfd --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--update_user-update_user_basic.sql @@ -0,0 +1,11 @@ +-- name: update_user +UPDATE `grafana`.`user` +SET + login = 'newuser1', + email = 'newuser1@example.com', + name = 'New User One', + is_admin = TRUE, + is_disabled = TRUE, + email_verified = FALSE, + updated = '2023-01-01 13:00:00' +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--update_org_user-update_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--update_org_user-update_org_user_basic.sql new file mode 100755 index 00000000000..b3bec3c5fe8 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--update_org_user-update_org_user_basic.sql @@ -0,0 +1,6 @@ +-- name: update_org_user +UPDATE "grafana"."org_user" +SET + role = 'Admin', + updated = '2023-01-01 14:00:00' +WHERE org_id = 1 AND user_id = 123 diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--update_user-update_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--update_user-update_user_basic.sql new file mode 100755 index 00000000000..034971946bb --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--update_user-update_user_basic.sql @@ -0,0 +1,11 @@ +-- name: update_user +UPDATE "grafana"."user" +SET + login = 'newuser1', + email = 'newuser1@example.com', + name = 'New User One', + is_admin = TRUE, + is_disabled = TRUE, + email_verified = FALSE, + updated = '2023-01-01 13:00:00' +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--update_org_user-update_org_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--update_org_user-update_org_user_basic.sql new file mode 100755 index 00000000000..b3bec3c5fe8 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--update_org_user-update_org_user_basic.sql @@ -0,0 +1,6 @@ +-- name: update_org_user +UPDATE "grafana"."org_user" +SET + role = 'Admin', + updated = '2023-01-01 14:00:00' +WHERE org_id = 1 AND user_id = 123 diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--update_user-update_user_basic.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--update_user-update_user_basic.sql new file mode 100755 index 00000000000..034971946bb --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--update_user-update_user_basic.sql @@ -0,0 +1,11 @@ +-- name: update_user +UPDATE "grafana"."user" +SET + login = 'newuser1', + email = 'newuser1@example.com', + name = 'New User One', + is_admin = TRUE, + is_disabled = TRUE, + email_verified = FALSE, + updated = '2023-01-01 13:00:00' +WHERE uid = 'user-1' diff --git a/pkg/registry/apis/iam/legacy/update_org_user.sql b/pkg/registry/apis/iam/legacy/update_org_user.sql new file mode 100644 index 00000000000..4efa710546e --- /dev/null +++ b/pkg/registry/apis/iam/legacy/update_org_user.sql @@ -0,0 +1,6 @@ +-- name: update_org_user +UPDATE {{ .Ident .OrgUserTable }} +SET + role = {{ .Arg .Command.Role }}, + updated = {{ .Arg .Command.Updated }} +WHERE org_id = {{ .Arg .Command.OrgID }} AND user_id = {{ .Arg .Command.UserID }} diff --git a/pkg/registry/apis/iam/legacy/update_user.sql b/pkg/registry/apis/iam/legacy/update_user.sql new file mode 100644 index 00000000000..d1d67a6b61a --- /dev/null +++ b/pkg/registry/apis/iam/legacy/update_user.sql @@ -0,0 +1,11 @@ +-- name: update_user +UPDATE {{ .Ident .UserTable }} +SET + login = {{ .Arg .Command.Login }}, + email = {{ .Arg .Command.Email }}, + name = {{ .Arg .Command.Name }}, + is_admin = {{ .Arg .Command.IsAdmin }}, + is_disabled = {{ .Arg .Command.IsDisabled }}, + email_verified = {{ .Arg .Command.EmailVerified }}, + updated = {{ .Arg .Command.Updated }} +WHERE uid = {{ .Arg .Command.UID }} \ No newline at end of file diff --git a/pkg/registry/apis/iam/legacy/user.go b/pkg/registry/apis/iam/legacy/user.go index 358fbcfe71f..0e3ec62388f 100644 --- a/pkg/registry/apis/iam/legacy/user.go +++ b/pkg/registry/apis/iam/legacy/user.go @@ -321,6 +321,13 @@ type CreateOrgUserCommand struct { Updated DBTime } +type UpdateOrgUserCommand struct { + OrgID int64 + UserID int64 + Role string + Updated DBTime +} + type DeleteUserCommand struct { UID string } @@ -329,6 +336,8 @@ var sqlCreateUserTemplate = mustTemplate("create_user.sql") var sqlCreateOrgUserTemplate = mustTemplate("create_org_user.sql") var sqlDeleteUserTemplate = mustTemplate("delete_user.sql") var sqlDeleteOrgUserTemplate = mustTemplate("delete_org_user.sql") +var sqlUpdateUserTemplate = mustTemplate("update_user.sql") +var sqlUpdateOrgUserTemplate = mustTemplate("update_org_user.sql") func newCreateUser(sql *legacysql.LegacyDatabaseHelper, cmd *CreateUserCommand) createUserQuery { return createUserQuery{ @@ -381,7 +390,7 @@ func (s *legacySQLStore) CreateUser(ctx context.Context, ns claims.NamespaceInfo return nil, err } - now := time.Now() + now := time.Now().UTC() lastSeenAt := now.AddDate(-10, 0, 0) // Set last seen 10 years ago like in user service cmd.Salt = salt @@ -389,7 +398,6 @@ func (s *legacySQLStore) CreateUser(ctx context.Context, ns claims.NamespaceInfo cmd.Created = NewDBTime(now) cmd.Updated = NewDBTime(now) cmd.LastSeenAt = NewDBTime(lastSeenAt) - cmd.Role = "Viewer" // TODO: https://github.com/grafana/identity-access-team/issues/1552 sql, err := s.sql(ctx) if err != nil { @@ -598,3 +606,126 @@ func (s *legacySQLStore) DeleteUser(ctx context.Context, ns claims.NamespaceInfo return nil } + +type UpdateUserCommand struct { + UID string + Login string + Email string + Name string + IsAdmin bool + IsDisabled bool + EmailVerified bool + Role string + Updated DBTime +} + +type UpdateUserResult struct { + User common.UserWithRole +} + +func newUpdateUser(sql *legacysql.LegacyDatabaseHelper, cmd *UpdateUserCommand) updateUserQuery { + return updateUserQuery{ + SQLTemplate: sqltemplate.New(sql.DialectForDriver()), + UserTable: sql.Table("user"), + Command: cmd, + } +} + +type updateUserQuery struct { + sqltemplate.SQLTemplate + UserTable string + Command *UpdateUserCommand +} + +func (r updateUserQuery) Validate() error { + return nil +} + +func newUpdateOrgUser(sql *legacysql.LegacyDatabaseHelper, cmd *UpdateOrgUserCommand) updateOrgUserQuery { + return updateOrgUserQuery{ + SQLTemplate: sqltemplate.New(sql.DialectForDriver()), + OrgUserTable: sql.Table("org_user"), + Command: cmd, + } +} + +type updateOrgUserQuery struct { + sqltemplate.SQLTemplate + OrgUserTable string + Command *UpdateOrgUserCommand +} + +func (r updateOrgUserQuery) Validate() error { + return nil +} + +// UpdateUser implements LegacyIdentityStore. +func (s *legacySQLStore) UpdateUser(ctx context.Context, ns claims.NamespaceInfo, cmd UpdateUserCommand) (*UpdateUserResult, error) { + now := time.Now().UTC() + cmd.Updated = NewDBTime(now) + + sql, err := s.sql(ctx) + if err != nil { + return nil, err + } + + req := newUpdateUser(sql, &cmd) + + var updatedUser common.UserWithRole + err = sql.DB.GetSqlxSession().WithTransaction(ctx, func(st *session.SessionTx) error { + userInternalID, err := s.GetUserInternalID(ctx, ns, GetUserInternalIDQuery{UID: cmd.UID}) + if err != nil { + return fmt.Errorf("user not found: %w", err) + } + + userQuery, err := sqltemplate.Execute(sqlUpdateUserTemplate, req) + if err != nil { + return fmt.Errorf("execute user template %q: %w", sqlUpdateUserTemplate.Name(), err) + } + + _, err = st.Exec(ctx, userQuery, req.GetArgs()...) + if err != nil { + return fmt.Errorf("failed to update user: %w", err) + } + + orgUserCmd := &UpdateOrgUserCommand{ + OrgID: ns.OrgID, + UserID: userInternalID.ID, + Role: cmd.Role, + Updated: cmd.Updated, + } + orgUserReq := newUpdateOrgUser(sql, orgUserCmd) + orgUserQuery, err := sqltemplate.Execute(sqlUpdateOrgUserTemplate, orgUserReq) + if err != nil { + return fmt.Errorf("execute org_user update template %q: %w", sqlUpdateOrgUserTemplate.Name(), err) + } + _, err = st.Exec(ctx, orgUserQuery, orgUserReq.GetArgs()...) + if err != nil { + return fmt.Errorf("failed to update org_user relationship: %w", err) + } + + updatedUser = common.UserWithRole{ + User: user.User{ + ID: userInternalID.ID, + UID: cmd.UID, + Login: cmd.Login, + Email: cmd.Email, + Name: cmd.Name, + OrgID: ns.OrgID, + IsAdmin: cmd.IsAdmin, + IsDisabled: cmd.IsDisabled, + EmailVerified: cmd.EmailVerified, + Updated: cmd.Updated.Time, + }, + Role: cmd.Role, + } + + return nil + }) + + if err != nil { + return nil, err + } + + return &UpdateUserResult{User: updatedUser}, nil +} diff --git a/pkg/registry/apis/iam/register.go b/pkg/registry/apis/iam/register.go index 767ec450bae..495db9cd2c4 100644 --- a/pkg/registry/apis/iam/register.go +++ b/pkg/registry/apis/iam/register.go @@ -352,6 +352,12 @@ func (b *IdentityAccessManagementAPIBuilder) Validate(ctx context.Context, a adm return nil case admission.Update: switch typedObj := a.GetObject().(type) { + case *iamv0.User: + oldUserObj, ok := a.GetOldObject().(*iamv0.User) + if !ok { + return fmt.Errorf("expected old object to be a User, got %T", oldUserObj) + } + return user.ValidateOnUpdate(ctx, oldUserObj, typedObj) case *iamv0.ResourcePermission: return resourcepermission.ValidateCreateAndUpdateInput(ctx, typedObj) case *iamv0.Team: @@ -379,12 +385,15 @@ func (b *IdentityAccessManagementAPIBuilder) Mutate(ctx context.Context, a admis case admission.Create: switch typedObj := a.GetObject().(type) { case *iamv0.User: - return user.MutateOnCreate(ctx, typedObj) + return user.MutateOnCreateAndUpdate(ctx, typedObj) case *iamv0.ServiceAccount: return serviceaccount.MutateOnCreate(ctx, typedObj) } case admission.Update: - return nil + switch typedObj := a.GetObject().(type) { + case *iamv0.User: + return user.MutateOnCreateAndUpdate(ctx, typedObj) + } case admission.Delete: return nil case admission.Connect: diff --git a/pkg/registry/apis/iam/user/mutate.go b/pkg/registry/apis/iam/user/mutate.go index ca7a2558123..dd62ea469ec 100644 --- a/pkg/registry/apis/iam/user/mutate.go +++ b/pkg/registry/apis/iam/user/mutate.go @@ -5,9 +5,11 @@ import ( "strings" iamv0alpha1 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" + "golang.org/x/text/cases" + "golang.org/x/text/language" ) -func MutateOnCreate(ctx context.Context, obj *iamv0alpha1.User) error { +func MutateOnCreateAndUpdate(ctx context.Context, obj *iamv0alpha1.User) error { obj.Spec.Email = strings.ToLower(obj.Spec.Email) obj.Spec.Login = strings.ToLower(obj.Spec.Login) @@ -15,5 +17,7 @@ func MutateOnCreate(ctx context.Context, obj *iamv0alpha1.User) error { obj.Spec.Login = obj.Spec.Email } + obj.Spec.Role = cases.Title(language.Und).String(obj.Spec.Role) + return nil } diff --git a/pkg/registry/apis/iam/user/mutate_test.go b/pkg/registry/apis/iam/user/mutate_test.go index 0ee742902c1..888a424517d 100644 --- a/pkg/registry/apis/iam/user/mutate_test.go +++ b/pkg/registry/apis/iam/user/mutate_test.go @@ -68,10 +68,72 @@ func TestMutateOnCreate_LoginEmail(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err := MutateOnCreate(context.Background(), tc.inputUser) + err := MutateOnCreateAndUpdate(context.Background(), tc.inputUser) require.NoError(t, err) require.Equal(t, tc.expectedLogin, tc.inputUser.Spec.Login) require.Equal(t, tc.expectedEmail, tc.inputUser.Spec.Email) }) } } + +func TestMutateOnCreate_Role(t *testing.T) { + testCases := []struct { + name string + inputUser *iamv0alpha1.User + expectedRole string + }{ + { + name: "role is lowercase", + inputUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Role: "admin", + }, + }, + expectedRole: "Admin", + }, + { + name: "role is uppercase", + inputUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Role: "ADMIN", + }, + }, + expectedRole: "Admin", + }, + { + name: "role is mixed case", + inputUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Role: "aDmIn", + }, + }, + expectedRole: "Admin", + }, + { + name: "role is already title case", + inputUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Role: "Admin", + }, + }, + expectedRole: "Admin", + }, + { + name: "role is empty", + inputUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Role: "", + }, + }, + expectedRole: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := MutateOnCreateAndUpdate(context.Background(), tc.inputUser) + require.NoError(t, err) + require.Equal(t, tc.expectedRole, tc.inputUser.Spec.Role) + }) + } +} diff --git a/pkg/registry/apis/iam/user/store.go b/pkg/registry/apis/iam/user/store.go index 09ac48ac70c..83416208ca8 100644 --- a/pkg/registry/apis/iam/user/store.go +++ b/pkg/registry/apis/iam/user/store.go @@ -48,7 +48,54 @@ type LegacyStore struct { // Update implements rest.Updater. func (s *LegacyStore) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { - return nil, false, apierrors.NewMethodNotSupported(resource.GroupResource(), "update") + if !s.enableAuthnMutation { + return nil, false, apierrors.NewMethodNotSupported(resource.GroupResource(), "update") + } + + ns, err := request.NamespaceInfoFrom(ctx, true) + if err != nil { + return nil, false, err + } + + oldObj, err := s.Get(ctx, name, nil) + if err != nil { + return nil, false, err + } + + newObj, err := objInfo.UpdatedObject(ctx, oldObj) + if err != nil { + return nil, false, err + } + + if updateValidation != nil { + if err := updateValidation(ctx, newObj, oldObj); err != nil { + return nil, false, err + } + } + + userObj, ok := newObj.(*iamv0alpha1.User) + if !ok { + return nil, false, fmt.Errorf("expected User object, got %T", newObj) + } + + updateCmd := legacy.UpdateUserCommand{ + UID: name, + Login: userObj.Spec.Login, + Email: userObj.Spec.Email, + Name: userObj.Spec.Name, + IsAdmin: userObj.Spec.GrafanaAdmin, + IsDisabled: userObj.Spec.Disabled, + EmailVerified: userObj.Spec.EmailVerified, + Role: userObj.Spec.Role, + } + + result, err := s.store.UpdateUser(ctx, ns, updateCmd) + if err != nil { + return nil, false, err + } + + iamUser := toUserItem(&result.User, ns.Value) + return &iamUser, false, nil } // DeleteCollection implements rest.CollectionDeleter. diff --git a/pkg/registry/apis/iam/user/validate.go b/pkg/registry/apis/iam/user/validate.go index 444015cc8f9..5ead9e368b9 100644 --- a/pkg/registry/apis/iam/user/validate.go +++ b/pkg/registry/apis/iam/user/validate.go @@ -6,6 +6,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" + "github.com/grafana/authlib/types" iamv0alpha1 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/identity" ) @@ -27,5 +28,75 @@ func ValidateOnCreate(ctx context.Context, obj *iamv0alpha1.User) error { return apierrors.NewBadRequest("user must have either login or email") } + err = validateRole(obj) + if err != nil { + return err + } + + return nil +} + +func validateRole(obj *iamv0alpha1.User) error { + if obj.Spec.Role == "" { + return apierrors.NewBadRequest("role is required") + } + + if !identity.RoleType(obj.Spec.Role).IsValid() { + return apierrors.NewBadRequest(fmt.Sprintf("invalid role '%s'", obj.Spec.Role)) + } + + return nil +} + +func ValidateOnUpdate(ctx context.Context, oldObj, newObj *iamv0alpha1.User) error { + requester, err := identity.GetRequester(ctx) + if err != nil { + return apierrors.NewUnauthorized("no identity found") + } + + isGrafanaAdmin := requester.GetIsGrafanaAdmin() + isServiceUser := requester.IsIdentityType(types.TypeAccessPolicy) + + if !isGrafanaAdmin { + if newObj.Spec.Disabled != oldObj.Spec.Disabled { + return apierrors.NewForbidden(iamv0alpha1.UserResourceInfo.GroupResource(), + newObj.Name, + fmt.Errorf("only grafana admins can disable or enable a user")) + } + if newObj.Spec.GrafanaAdmin != oldObj.Spec.GrafanaAdmin { + return apierrors.NewForbidden(iamv0alpha1.UserResourceInfo.GroupResource(), + newObj.Name, + fmt.Errorf("only grafana admins can change grafana admin status")) + } + } + + if !newObj.Spec.Provisioned && oldObj.Spec.Provisioned { + return apierrors.NewForbidden(iamv0alpha1.UserResourceInfo.GroupResource(), + newObj.Name, + fmt.Errorf("provisioned user cannot be un-provisioned")) + } + + if !isServiceUser { + if newObj.Spec.Provisioned && !oldObj.Spec.Provisioned { + return apierrors.NewForbidden(iamv0alpha1.UserResourceInfo.GroupResource(), + newObj.Name, + fmt.Errorf("only service users can provision a user")) + } + if newObj.Spec.EmailVerified && !oldObj.Spec.EmailVerified { + return apierrors.NewForbidden(iamv0alpha1.UserResourceInfo.GroupResource(), + newObj.Name, + fmt.Errorf("only service users can verify email")) + } + } + + if newObj.Spec.Login == "" && newObj.Spec.Email == "" { + return apierrors.NewBadRequest("user must have either login or email") + } + + err = validateRole(newObj) + if err != nil { + return err + } + return nil } diff --git a/pkg/registry/apis/iam/user/validate_test.go b/pkg/registry/apis/iam/user/validate_test.go index 675b62255f1..058ff61c25e 100644 --- a/pkg/registry/apis/iam/user/validate_test.go +++ b/pkg/registry/apis/iam/user/validate_test.go @@ -24,6 +24,7 @@ func TestValidateOnCreate(t *testing.T) { user: &iamv0alpha1.User{ Spec: iamv0alpha1.UserSpec{ Login: "testuser", + Role: "Viewer", }, }, requester: &identity.StaticRequester{ @@ -38,6 +39,7 @@ func TestValidateOnCreate(t *testing.T) { Spec: iamv0alpha1.UserSpec{ Login: "newadmin", GrafanaAdmin: true, + Role: "Viewer", }, }, requester: &identity.StaticRequester{ @@ -52,6 +54,7 @@ func TestValidateOnCreate(t *testing.T) { Spec: iamv0alpha1.UserSpec{ Login: "newadmin", GrafanaAdmin: true, + Role: "Viewer", }, }, requester: &identity.StaticRequester{ @@ -64,7 +67,9 @@ func TestValidateOnCreate(t *testing.T) { { name: "user with empty login and email", user: &iamv0alpha1.User{ - Spec: iamv0alpha1.UserSpec{}, + Spec: iamv0alpha1.UserSpec{ + Role: "Viewer", + }, }, requester: &identity.StaticRequester{ Type: types.TypeUser, @@ -78,6 +83,7 @@ func TestValidateOnCreate(t *testing.T) { user: &iamv0alpha1.User{ Spec: iamv0alpha1.UserSpec{ Login: "testuser", + Role: "Viewer", }, }, requester: &identity.StaticRequester{ @@ -91,6 +97,7 @@ func TestValidateOnCreate(t *testing.T) { user: &iamv0alpha1.User{ Spec: iamv0alpha1.UserSpec{ Email: "test@test.com", + Role: "Viewer", }, }, requester: &identity.StaticRequester{ @@ -99,6 +106,49 @@ func TestValidateOnCreate(t *testing.T) { }, expectError: false, }, + { + name: "user with empty role", + user: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Login: "testuser", + }, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: false, + }, + expectError: true, + errorContains: "role is required", + }, + { + name: "user with invalid role", + user: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Login: "testuser", + Role: "InvalidRole", + }, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: false, + }, + expectError: true, + errorContains: "invalid role 'InvalidRole'", + }, + { + name: "user with valid role", + user: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{ + Login: "testuser", + Role: "Admin", + }, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, } for _, tt := range tests { @@ -121,3 +171,262 @@ func TestValidateOnCreate(t *testing.T) { }) } } + +func TestValidateOnUpdate(t *testing.T) { + tests := []struct { + name string + oldUser *iamv0alpha1.User + newUser *iamv0alpha1.User + requester *identity.StaticRequester + expectError bool + errorContains string + }{ + { + name: "un-provisioning a provisioned user", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: true, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: false, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: true, + errorContains: "provisioned user cannot be un-provisioned", + }, + { + name: "non-service user provisions a user", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: true, + errorContains: "only service users can provision a user", + }, + { + name: "service user provisions a user", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Provisioned: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeAccessPolicy, + }, + expectError: false, + }, + { + name: "no changes", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + }, + expectError: false, + }, + { + name: "update with empty login and email", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "", Email: "", Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + }, + expectError: true, + errorContains: "user must have either login or email", + }, + { + name: "update with only login", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Email: "test@test.com", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Email: "", Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, + { + name: "update with only email", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "", Email: "test@test.com", Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, + { + name: "service user verifies email", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", EmailVerified: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", EmailVerified: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeAccessPolicy, + }, + expectError: false, + }, + { + name: "non-service user verifies email", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", EmailVerified: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", EmailVerified: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + }, + expectError: true, + errorContains: "only service users can verify email", + }, + { + name: "grafana admin disables user", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Disabled: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Disabled: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, + { + name: "non-admin disables user", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Disabled: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Disabled: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: false, + }, + expectError: true, + errorContains: "only grafana admins can disable or enable a user", + }, + { + name: "grafana admin grants admin", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", GrafanaAdmin: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", GrafanaAdmin: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, + { + name: "non-admin grants admin", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", GrafanaAdmin: false, Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", GrafanaAdmin: true, Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: false, + }, + expectError: true, + errorContains: "only grafana admins can change grafana admin status", + }, + { + name: "update to empty role", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: ""}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: true, + errorContains: "role is required", + }, + { + name: "update to invalid role", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "InvalidRole"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: true, + errorContains: "invalid role 'InvalidRole'", + }, + { + name: "update to valid role", + oldUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Editor"}, + }, + newUser: &iamv0alpha1.User{ + Spec: iamv0alpha1.UserSpec{Login: "testuser", Role: "Viewer"}, + }, + requester: &identity.StaticRequester{ + Type: types.TypeUser, + IsGrafanaAdmin: true, + }, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := identity.WithRequester( + context.Background(), + tt.requester, + ) + + err := ValidateOnUpdate(ctx, tt.oldUser, tt.newUser) + + if tt.expectError { + require.Error(t, err) + if tt.errorContains != "" { + require.Contains(t, err.Error(), tt.errorContains) + } + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/tests/apis/iam/testdata/user-test-create-2-v0.yaml b/pkg/tests/apis/iam/testdata/user-test-create-2-v0.yaml new file mode 100644 index 00000000000..87783327fdb --- /dev/null +++ b/pkg/tests/apis/iam/testdata/user-test-create-2-v0.yaml @@ -0,0 +1,11 @@ +apiVersion: iam.grafana.app/v0alpha1 +kind: User +metadata: + namespace: default + name: user2 +spec: + email: testuser2@example.com + login: testuser2 + name: Test User 2 + provisioned: false + role: Viewer diff --git a/pkg/tests/apis/iam/testdata/user-test-create-v0.yaml b/pkg/tests/apis/iam/testdata/user-test-create-v0.yaml index cd2545e070f..669bc89a951 100644 --- a/pkg/tests/apis/iam/testdata/user-test-create-v0.yaml +++ b/pkg/tests/apis/iam/testdata/user-test-create-v0.yaml @@ -8,4 +8,4 @@ spec: login: testuser1 name: Test User 1 provisioned: false - \ No newline at end of file + role: None \ No newline at end of file diff --git a/pkg/tests/apis/iam/user_integration_test.go b/pkg/tests/apis/iam/user_integration_test.go index d24ac919b5d..b5379867a93 100644 --- a/pkg/tests/apis/iam/user_integration_test.go +++ b/pkg/tests/apis/iam/user_integration_test.go @@ -91,14 +91,60 @@ func doUserCRUDTestsUsingTheNewAPIs(t *testing.T, helper *apis.K8sTestHelper) { require.Equal(t, createdUID, fetched.GetName()) require.Equal(t, "default", fetched.GetNamespace()) - // TODO: Uncomment when we know how to handle global scope (global.users:) - // err = userClient.Resource.Delete(ctx, createdUID, metav1.DeleteOptions{}) - // require.NoError(t, err) + err = userClient.Resource.Delete(ctx, createdUID, metav1.DeleteOptions{}) + require.NoError(t, err) // Verify deletion - // _, err = userClient.Resource.Get(ctx, createdUID, metav1.GetOptions{}) - // require.Error(t, err) - // require.Contains(t, err.Error(), "not found") + _, err = userClient.Resource.Get(ctx, createdUID, metav1.GetOptions{}) + require.Error(t, err) + require.Contains(t, err.Error(), "not found") + }) + + t.Run("should update user using the new APIs as a GrafanaAdmin", func(t *testing.T) { + ctx := context.Background() + + userClient := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + Namespace: helper.Namespacer(helper.Org1.Admin.Identity.GetOrgID()), + GVR: gvrUsers, + }) + + // Create the user + created, err := userClient.Resource.Create(ctx, helper.LoadYAMLOrJSONFile("testdata/user-test-create-2-v0.yaml"), metav1.CreateOptions{}) + require.NoError(t, err) + require.NotNil(t, created) + + // Get the user to update + createdUID := created.GetName() + userToUpdate, err := userClient.Resource.Get(ctx, createdUID, metav1.GetOptions{}) + require.NoError(t, err) + + // Modify the user spec + spec := userToUpdate.Object["spec"].(map[string]interface{}) + spec["name"] = "Updated Test User" + spec["email"] = "updated.test.user@example.com" + userToUpdate.Object["spec"] = spec + + // Update the user + updated, err := userClient.Resource.Update(ctx, userToUpdate, metav1.UpdateOptions{}) + require.NoError(t, err) + require.NotNil(t, updated) + + // Verify the update response + updatedSpec := updated.Object["spec"].(map[string]interface{}) + require.Equal(t, "Updated Test User", updatedSpec["name"]) + require.Equal(t, "updated.test.user@example.com", updatedSpec["email"]) + + // Fetch again to confirm + fetched, err := userClient.Resource.Get(ctx, createdUID, metav1.GetOptions{}) + require.NoError(t, err) + fetchedSpec := fetched.Object["spec"].(map[string]interface{}) + require.Equal(t, "Updated Test User", fetchedSpec["name"]) + require.Equal(t, "updated.test.user@example.com", fetchedSpec["email"]) + + // Cleanup + err = userClient.Resource.Delete(ctx, fetched.GetName(), metav1.DeleteOptions{}) + require.NoError(t, err) }) t.Run("should not be able to create user when using a user with insufficient permissions", func(t *testing.T) { @@ -135,9 +181,9 @@ func doUserCRUDTestsUsingTheLegacyAPIs(t *testing.T, helper *apis.K8sTestHelper) }) legacyUserPayload := `{ - "name": "Test User 2", - "email": "testuser2@example.com", - "login": "testuser2", + "name": "Legacy User 3", + "email": "legacyuser3@example.com", + "login": "legacyuser3", "password": "password123" }` @@ -159,9 +205,9 @@ func doUserCRUDTestsUsingTheLegacyAPIs(t *testing.T, helper *apis.K8sTestHelper) // Verify fetched user matches created user userSpec := user.Object["spec"].(map[string]interface{}) - require.Equal(t, "testuser2@example.com", userSpec["email"]) - require.Equal(t, "testuser2", userSpec["login"]) - require.Equal(t, "Test User 2", userSpec["name"]) + require.Equal(t, "legacyuser3@example.com", userSpec["email"]) + require.Equal(t, "legacyuser3", userSpec["login"]) + require.Equal(t, "Legacy User 3", userSpec["name"]) require.Equal(t, false, userSpec["provisioned"]) // Verify metadata