From 78412ca008c54f8d9c8821860e17d27229f37d23 Mon Sep 17 00:00:00 2001 From: Ilya Danilov Date: Wed, 19 Aug 2026 01:35:27 +0300 Subject: [PATCH 1/5] fix(auth-center): restrict role assignment in user create and update --- auth-center/pkg/service/user_generic_test.go | 146 +++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 auth-center/pkg/service/user_generic_test.go diff --git a/auth-center/pkg/service/user_generic_test.go b/auth-center/pkg/service/user_generic_test.go new file mode 100644 index 00000000..5742a1f8 --- /dev/null +++ b/auth-center/pkg/service/user_generic_test.go @@ -0,0 +1,146 @@ +package service + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/runtime-radar/runtime-radar/auth-center/pkg/model" + "github.com/runtime-radar/runtime-radar/auth-center/pkg/tokens" + "github.com/runtime-radar/runtime-radar/lib/errcommon" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" +) + +var securityEngineerRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000002") + +// ctxWithCallerRole builds an incoming gRPC context carrying an access token issued for a user +// holding roleID, the way the interceptor would populate it for a real call. +func ctxWithCallerRole(t *testing.T, key []byte, roleID uuid.UUID) context.Context { + t.Helper() + + user := model.User{ + Base: model.Base{ID: uuid.New()}, + Username: "caller", + RoleID: roleID, + Role: model.Role{ID: roleID}, + LastPasswordChangedAt: time.Now(), + } + + pair, err := tokens.GenerateTokenPair(user, key, time.Minute, time.Hour) + if err != nil { + t.Fatalf("can't generate token pair: %v", err) + } + + md := metadata.Pairs(tokens.AuthorizationKey, "Bearer "+pair.AccessTokenHash) + + return metadata.NewIncomingContext(context.Background(), md) +} + +func TestVerifyRoleAssignment(t *testing.T) { + key := []byte("test-token-key") + ug := &UserGeneric{TokenKey: key} + + tests := []struct { + name string + callerRoleID uuid.UUID + currentRoleID uuid.UUID + newRoleID uuid.UUID + wantReason string + }{ + { + name: "admin grants admin", + callerRoleID: model.AdminRoleID, + currentRoleID: securityEngineerRoleID, + newRoleID: model.AdminRoleID, + }, + { + name: "admin demotes admin", + callerRoleID: model.AdminRoleID, + currentRoleID: model.AdminRoleID, + newRoleID: securityEngineerRoleID, + }, + { + name: "non-admin keeps role unchanged", + callerRoleID: securityEngineerRoleID, + currentRoleID: securityEngineerRoleID, + newRoleID: securityEngineerRoleID, + }, + { + name: "non-admin creates user with own role", + callerRoleID: securityEngineerRoleID, + currentRoleID: uuid.Nil, + newRoleID: securityEngineerRoleID, + }, + { + name: "non-admin escalates a user to admin", + callerRoleID: securityEngineerRoleID, + currentRoleID: securityEngineerRoleID, + newRoleID: model.AdminRoleID, + wantReason: RoleAssignmentRestricted, + }, + { + name: "non-admin creates an admin", + callerRoleID: securityEngineerRoleID, + currentRoleID: uuid.Nil, + newRoleID: model.AdminRoleID, + wantReason: RoleAssignmentRestricted, + }, + { + name: "non-admin modifies an admin account", + callerRoleID: securityEngineerRoleID, + currentRoleID: model.AdminRoleID, + newRoleID: model.AdminRoleID, + wantReason: RoleAssignmentRestricted, + }, + { + name: "non-admin demotes an admin", + callerRoleID: securityEngineerRoleID, + currentRoleID: model.AdminRoleID, + newRoleID: securityEngineerRoleID, + wantReason: RoleAssignmentRestricted, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := ctxWithCallerRole(t, key, tt.callerRoleID) + + err := ug.verifyRoleAssignment(ctx, tt.currentRoleID, tt.newRoleID) + + if tt.wantReason == "" { + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + return + } + + st, ok := status.FromError(err) + if !ok { + t.Fatalf("Expected a gRPC status error, got %v", err) + } + if st.Code() != codes.PermissionDenied { + t.Fatalf("Expected code %v, got %v", codes.PermissionDenied, st.Code()) + } + if reason, _ := errcommon.ReasonFromStatus(st); reason != tt.wantReason { + t.Fatalf("Expected reason %q, got %q", tt.wantReason, reason) + } + }) + } +} + +func TestVerifyRoleAssignmentWithoutToken(t *testing.T) { + ug := &UserGeneric{TokenKey: []byte("test-token-key")} + + err := ug.verifyRoleAssignment(context.Background(), uuid.Nil, model.AdminRoleID) + + st, ok := status.FromError(err) + if !ok { + t.Fatalf("Expected a gRPC status error, got %v", err) + } + if st.Code() != codes.Unauthenticated { + t.Fatalf("Expected code %v, got %v", codes.Unauthenticated, st.Code()) + } +} From 8e5329b5958c682afd894d23d81abfb49fa03aaa Mon Sep 17 00:00:00 2001 From: Ilya Danilov Date: Wed, 19 Aug 2026 01:36:04 +0300 Subject: [PATCH 2/5] fix(auth-center): restrict role assignment in user create and update --- auth-center/pkg/service/helpers.go | 5 ++++ auth-center/pkg/service/user_generic.go | 35 ++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/auth-center/pkg/service/helpers.go b/auth-center/pkg/service/helpers.go index fc8f0447..6112a130 100644 --- a/auth-center/pkg/service/helpers.go +++ b/auth-center/pkg/service/helpers.go @@ -12,6 +12,11 @@ import ( const maskedPassword = "******" +// gRPC errdetails.ErrorInfo.Reason codes used in service responses. +const ( + RoleAssignmentRestricted = "ROLE_ASSIGNMENT_RESTRICTED" +) + func haveUpper(s string) bool { for _, r := range s { if unicode.IsUpper(r) && unicode.IsLetter(r) { diff --git a/auth-center/pkg/service/user_generic.go b/auth-center/pkg/service/user_generic.go index 69006450..3fae055e 100644 --- a/auth-center/pkg/service/user_generic.go +++ b/auth-center/pkg/service/user_generic.go @@ -101,6 +101,10 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id: %v", err) } + if err := ug.verifyRoleAssignment(ctx, uuid.Nil, roleID); err != nil { + return nil, err + } + _, err = mail.ParseAddress(req.Email) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "can't parse email: %v", err) @@ -167,13 +171,38 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return resp, nil } +// verifyRoleAssignment checks that the caller may grant newRoleID; currentRoleID is uuid.Nil on create. +// Without it users:create and users:update are a privilege escalation path. +func (ug *UserGeneric) verifyRoleAssignment(ctx context.Context, currentRoleID, newRoleID uuid.UUID) error { + token, err := tokens.AccessTokenFromContext(ctx, ug.TokenKey) + if err != nil { + return status.Errorf(codes.Unauthenticated, "can't get token: %v", err) + } + + if token.Role.ID == model.AdminRoleID { + return nil + } + + if currentRoleID == model.AdminRoleID { + return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, + "can't modify an administrator account").Err() + } + + if newRoleID != token.Role.ID { + return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, + "can't assign a role other than your own").Err() + } + + return nil +} + func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp *api.UserResp, err error) { id, err := uuid.Parse(req.GetId()) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "can't parse id: %v", err) } - _, err = ug.UserRepository.GetByID(ctx, id) + current, err := ug.UserRepository.GetByID(ctx, id) if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return nil, status.Error(codes.NotFound, "user does not exist") @@ -195,6 +224,10 @@ func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id") } + if err := ug.verifyRoleAssignment(ctx, current.RoleID, roleID); err != nil { + return nil, err + } + user := &model.User{ Base: model.Base{ID: id}, Email: req.Email, From ed45e8d510110f55678bca7a3cf601ad3020bfdc Mon Sep 17 00:00:00 2001 From: Ilya Danilov Date: Thu, 20 Aug 2026 19:07:17 +0300 Subject: [PATCH 3/5] fix(auth-center): restrict role assignment in user create and update --- auth-center/pkg/service/helpers.go | 1 + auth-center/pkg/service/user_generic.go | 23 ++++++++++- auth-center/pkg/service/user_generic_test.go | 40 +++++++++++++++++++- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/auth-center/pkg/service/helpers.go b/auth-center/pkg/service/helpers.go index 6112a130..de29e9fc 100644 --- a/auth-center/pkg/service/helpers.go +++ b/auth-center/pkg/service/helpers.go @@ -15,6 +15,7 @@ const maskedPassword = "******" // gRPC errdetails.ErrorInfo.Reason codes used in service responses. const ( RoleAssignmentRestricted = "ROLE_ASSIGNMENT_RESTRICTED" + LastAdminRemovingDenied = "LAST_ADMIN_REMOVING_DENIED" ) func haveUpper(s string) bool { diff --git a/auth-center/pkg/service/user_generic.go b/auth-center/pkg/service/user_generic.go index 3fae055e..349380ad 100644 --- a/auth-center/pkg/service/user_generic.go +++ b/auth-center/pkg/service/user_generic.go @@ -180,6 +180,10 @@ func (ug *UserGeneric) verifyRoleAssignment(ctx context.Context, currentRoleID, } if token.Role.ID == model.AdminRoleID { + if currentRoleID == model.AdminRoleID && newRoleID != model.AdminRoleID { + return ug.verifyNotLastAdmin(ctx) + } + return nil } @@ -188,7 +192,7 @@ func (ug *UserGeneric) verifyRoleAssignment(ctx context.Context, currentRoleID, "can't modify an administrator account").Err() } - if newRoleID != token.Role.ID { + if newRoleID != token.Role.ID && newRoleID != currentRoleID { return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, "can't assign a role other than your own").Err() } @@ -196,6 +200,21 @@ func (ug *UserGeneric) verifyRoleAssignment(ctx context.Context, currentRoleID, return nil } +// verifyNotLastAdmin rejects demoting the only administrator left, the same way Delete guards the last one. +func (ug *UserGeneric) verifyNotLastAdmin(ctx context.Context) error { + adminUsers, err := ug.UserRepository.GetUsersByRoleID(ctx, model.AdminRoleID) + if err != nil { + return status.Error(codes.Internal, "internal error") + } + + if len(adminUsers) == 1 { + return errcommon.StatusWithReason(codes.PermissionDenied, LastAdminRemovingDenied, + "can't demote last administrator").Err() + } + + return nil +} + func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp *api.UserResp, err error) { id, err := uuid.Parse(req.GetId()) if err != nil { @@ -280,7 +299,7 @@ func (ug *UserGeneric) Delete(ctx context.Context, req *api.DeleteUserReq) (resp return nil, status.Error(codes.Internal, "internal error") } if len(adminUsers) == 1 { - return nil, errcommon.StatusWithReason(codes.PermissionDenied, "LAST_ADMIN_REMOVING_DENIED", "can't delete last administrator").Err() + return nil, errcommon.StatusWithReason(codes.PermissionDenied, LastAdminRemovingDenied, "can't delete last administrator").Err() } } diff --git a/auth-center/pkg/service/user_generic_test.go b/auth-center/pkg/service/user_generic_test.go index 5742a1f8..c151c4a1 100644 --- a/auth-center/pkg/service/user_generic_test.go +++ b/auth-center/pkg/service/user_generic_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/google/uuid" + "github.com/runtime-radar/runtime-radar/auth-center/pkg/database" "github.com/runtime-radar/runtime-radar/auth-center/pkg/model" "github.com/runtime-radar/runtime-radar/auth-center/pkg/tokens" "github.com/runtime-radar/runtime-radar/lib/errcommon" @@ -14,7 +15,26 @@ import ( "google.golang.org/grpc/status" ) -var securityEngineerRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000002") +var ( + securityEngineerRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000002") + analystRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000003") +) + +// stubUserRepository answers with a fixed number of administrators; the other methods are unused here. +type stubUserRepository struct { + database.UserRepository + + admins int +} + +func (s *stubUserRepository) GetUsersByRoleID(_ context.Context, roleID uuid.UUID) ([]*model.User, error) { + users := make([]*model.User, 0, s.admins) + for i := 0; i < s.admins; i++ { + users = append(users, &model.User{RoleID: roleID, Role: model.Role{ID: roleID}}) + } + + return users, nil +} // ctxWithCallerRole builds an incoming gRPC context carrying an access token issued for a user // holding roleID, the way the interceptor would populate it for a real call. @@ -41,13 +61,13 @@ func ctxWithCallerRole(t *testing.T, key []byte, roleID uuid.UUID) context.Conte func TestVerifyRoleAssignment(t *testing.T) { key := []byte("test-token-key") - ug := &UserGeneric{TokenKey: key} tests := []struct { name string callerRoleID uuid.UUID currentRoleID uuid.UUID newRoleID uuid.UUID + admins int wantReason string }{ { @@ -61,6 +81,21 @@ func TestVerifyRoleAssignment(t *testing.T) { callerRoleID: model.AdminRoleID, currentRoleID: model.AdminRoleID, newRoleID: securityEngineerRoleID, + admins: 2, + }, + { + name: "admin demotes the last admin", + callerRoleID: model.AdminRoleID, + currentRoleID: model.AdminRoleID, + newRoleID: securityEngineerRoleID, + admins: 1, + wantReason: LastAdminRemovingDenied, + }, + { + name: "non-admin keeps another user role unchanged", + callerRoleID: securityEngineerRoleID, + currentRoleID: analystRoleID, + newRoleID: analystRoleID, }, { name: "non-admin keeps role unchanged", @@ -107,6 +142,7 @@ func TestVerifyRoleAssignment(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := ctxWithCallerRole(t, key, tt.callerRoleID) + ug := &UserGeneric{TokenKey: key, UserRepository: &stubUserRepository{admins: tt.admins}} err := ug.verifyRoleAssignment(ctx, tt.currentRoleID, tt.newRoleID) From f5052c03804584eaa02b76d335c4afdd00dd4757 Mon Sep 17 00:00:00 2001 From: Ilya Danilov Date: Fri, 28 Aug 2026 16:45:39 +0300 Subject: [PATCH 4/5] fix(auth-center): restrict user management to administrators --- auth-center/pkg/model/model.go | 2 +- auth-center/pkg/model/model_test.go | 43 +++++++ auth-center/pkg/service/helpers.go | 1 + auth-center/pkg/service/user_generic.go | 27 +++-- auth-center/pkg/service/user_generic_test.go | 116 +++++++++++-------- 5 files changed, 128 insertions(+), 61 deletions(-) create mode 100644 auth-center/pkg/model/model_test.go diff --git a/auth-center/pkg/model/model.go b/auth-center/pkg/model/model.go index 4ac86e2d..09e702b0 100644 --- a/auth-center/pkg/model/model.go +++ b/auth-center/pkg/model/model.go @@ -74,7 +74,7 @@ var ( RoleName: "Security engineer", RolePermissions: Permissions{ Users: &jwt.Permission{ - Actions: []jwt.Action{"read", "update", "create", "delete"}, + Actions: []jwt.Action{"read", "update"}, Description: "User management", }, Roles: &jwt.Permission{ diff --git a/auth-center/pkg/model/model_test.go b/auth-center/pkg/model/model_test.go new file mode 100644 index 00000000..2bcdd3d2 --- /dev/null +++ b/auth-center/pkg/model/model_test.go @@ -0,0 +1,43 @@ +package model + +import ( + "testing" + + "github.com/runtime-radar/runtime-radar/lib/security/jwt" +) + +// Creating and deleting users are administrator-only actions. Any other predeclared role holding +// them would let its holders manage accounts other than their own, which is what the check in the +// service layer exists to prevent. +func TestOnlyAdministratorManagesUsers(t *testing.T) { + restricted := []jwt.Action{"create", "delete"} + + for _, action := range restricted { + var adminHolds bool + + for _, role := range PredeclaredRoles { + users := role.RolePermissions.Users + if users == nil { + continue + } + + var holds bool + for _, granted := range users.Actions { + if granted == action { + holds = true + } + } + + switch { + case role.ID == AdminRoleID: + adminHolds = holds + case holds: + t.Errorf("Role %q must not hold users:%s", role.RoleName, action) + } + } + + if !adminHolds { + t.Errorf("Expected the administrator role to hold users:%s", action) + } + } +} diff --git a/auth-center/pkg/service/helpers.go b/auth-center/pkg/service/helpers.go index de29e9fc..ca06ef4d 100644 --- a/auth-center/pkg/service/helpers.go +++ b/auth-center/pkg/service/helpers.go @@ -15,6 +15,7 @@ const maskedPassword = "******" // gRPC errdetails.ErrorInfo.Reason codes used in service responses. const ( RoleAssignmentRestricted = "ROLE_ASSIGNMENT_RESTRICTED" + UserManagementRestricted = "USER_MANAGEMENT_RESTRICTED" LastAdminRemovingDenied = "LAST_ADMIN_REMOVING_DENIED" ) diff --git a/auth-center/pkg/service/user_generic.go b/auth-center/pkg/service/user_generic.go index 349380ad..a787b797 100644 --- a/auth-center/pkg/service/user_generic.go +++ b/auth-center/pkg/service/user_generic.go @@ -101,7 +101,7 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id: %v", err) } - if err := ug.verifyRoleAssignment(ctx, uuid.Nil, roleID); err != nil { + if err := ug.verifyUserModification(ctx, nil, roleID); err != nil { return nil, err } @@ -171,30 +171,35 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return resp, nil } -// verifyRoleAssignment checks that the caller may grant newRoleID; currentRoleID is uuid.Nil on create. -// Without it users:create and users:update are a privilege escalation path. -func (ug *UserGeneric) verifyRoleAssignment(ctx context.Context, currentRoleID, newRoleID uuid.UUID) error { +// verifyUserModification checks that the caller may apply the requested change; current is nil on create. +// Only an administrator manages other accounts and hands out roles, otherwise users:update escalates. +func (ug *UserGeneric) verifyUserModification(ctx context.Context, current *model.User, newRoleID uuid.UUID) error { token, err := tokens.AccessTokenFromContext(ctx, ug.TokenKey) if err != nil { return status.Errorf(codes.Unauthenticated, "can't get token: %v", err) } if token.Role.ID == model.AdminRoleID { - if currentRoleID == model.AdminRoleID && newRoleID != model.AdminRoleID { + if current != nil && current.RoleID == model.AdminRoleID && newRoleID != model.AdminRoleID { return ug.verifyNotLastAdmin(ctx) } return nil } - if currentRoleID == model.AdminRoleID { - return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, - "can't modify an administrator account").Err() + if current == nil { + return errcommon.StatusWithReason(codes.PermissionDenied, UserManagementRestricted, + "can't create users").Err() + } + + if token.UserID != current.ID.String() { + return errcommon.StatusWithReason(codes.PermissionDenied, UserManagementRestricted, + "can't modify another user").Err() } - if newRoleID != token.Role.ID && newRoleID != currentRoleID { + if newRoleID != current.RoleID { return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, - "can't assign a role other than your own").Err() + "can't change your own role").Err() } return nil @@ -243,7 +248,7 @@ func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id") } - if err := ug.verifyRoleAssignment(ctx, current.RoleID, roleID); err != nil { + if err := ug.verifyUserModification(ctx, current, roleID); err != nil { return nil, err } diff --git a/auth-center/pkg/service/user_generic_test.go b/auth-center/pkg/service/user_generic_test.go index c151c4a1..35f140e6 100644 --- a/auth-center/pkg/service/user_generic_test.go +++ b/auth-center/pkg/service/user_generic_test.go @@ -17,7 +17,7 @@ import ( var ( securityEngineerRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000002") - analystRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000003") + cicdRoleID = uuid.MustParse("00000000-0000-0000-0000-000000000003") ) // stubUserRepository answers with a fixed number of administrators; the other methods are unused here. @@ -36,13 +36,13 @@ func (s *stubUserRepository) GetUsersByRoleID(_ context.Context, roleID uuid.UUI return users, nil } -// ctxWithCallerRole builds an incoming gRPC context carrying an access token issued for a user -// holding roleID, the way the interceptor would populate it for a real call. -func ctxWithCallerRole(t *testing.T, key []byte, roleID uuid.UUID) context.Context { +// ctxWithCaller builds an incoming gRPC context carrying an access token issued for the given user, +// the way the interceptor would populate it for a real call. +func ctxWithCaller(t *testing.T, key []byte, userID, roleID uuid.UUID) context.Context { t.Helper() user := model.User{ - Base: model.Base{ID: uuid.New()}, + Base: model.Base{ID: userID}, Username: "caller", RoleID: roleID, Role: model.Role{ID: roleID}, @@ -59,25 +59,35 @@ func ctxWithCallerRole(t *testing.T, key []byte, roleID uuid.UUID) context.Conte return metadata.NewIncomingContext(context.Background(), md) } -func TestVerifyRoleAssignment(t *testing.T) { +func TestVerifyUserModification(t *testing.T) { key := []byte("test-token-key") tests := []struct { - name string - callerRoleID uuid.UUID - currentRoleID uuid.UUID - newRoleID uuid.UUID - admins int - wantReason string + name string + callerRoleID uuid.UUID + // create leaves the request without an existing user; targetIsCaller tells whether the + // updated account is the caller's own one. + create bool + targetIsCaller bool + currentRoleID uuid.UUID + newRoleID uuid.UUID + admins int + wantReason string }{ { - name: "admin grants admin", + name: "admin creates an admin", + callerRoleID: model.AdminRoleID, + create: true, + newRoleID: model.AdminRoleID, + }, + { + name: "admin promotes another user", callerRoleID: model.AdminRoleID, currentRoleID: securityEngineerRoleID, newRoleID: model.AdminRoleID, }, { - name: "admin demotes admin", + name: "admin demotes another admin", callerRoleID: model.AdminRoleID, currentRoleID: model.AdminRoleID, newRoleID: securityEngineerRoleID, @@ -92,59 +102,67 @@ func TestVerifyRoleAssignment(t *testing.T) { wantReason: LastAdminRemovingDenied, }, { - name: "non-admin keeps another user role unchanged", - callerRoleID: securityEngineerRoleID, - currentRoleID: analystRoleID, - newRoleID: analystRoleID, + name: "non-admin edits its own account", + callerRoleID: securityEngineerRoleID, + targetIsCaller: true, + currentRoleID: securityEngineerRoleID, + newRoleID: securityEngineerRoleID, }, { - name: "non-admin keeps role unchanged", - callerRoleID: securityEngineerRoleID, - currentRoleID: securityEngineerRoleID, - newRoleID: securityEngineerRoleID, + name: "non-admin creates a user", + callerRoleID: securityEngineerRoleID, + create: true, + newRoleID: securityEngineerRoleID, + wantReason: UserManagementRestricted, }, { - name: "non-admin creates user with own role", + name: "non-admin edits another user", callerRoleID: securityEngineerRoleID, - currentRoleID: uuid.Nil, - newRoleID: securityEngineerRoleID, - }, - { - name: "non-admin escalates a user to admin", - callerRoleID: securityEngineerRoleID, - currentRoleID: securityEngineerRoleID, - newRoleID: model.AdminRoleID, - wantReason: RoleAssignmentRestricted, + currentRoleID: cicdRoleID, + newRoleID: cicdRoleID, + wantReason: UserManagementRestricted, }, { - name: "non-admin creates an admin", + name: "non-admin edits an admin account", callerRoleID: securityEngineerRoleID, - currentRoleID: uuid.Nil, + currentRoleID: model.AdminRoleID, newRoleID: model.AdminRoleID, - wantReason: RoleAssignmentRestricted, + wantReason: UserManagementRestricted, }, { - name: "non-admin modifies an admin account", - callerRoleID: securityEngineerRoleID, - currentRoleID: model.AdminRoleID, - newRoleID: model.AdminRoleID, - wantReason: RoleAssignmentRestricted, + name: "non-admin escalates itself to admin", + callerRoleID: securityEngineerRoleID, + targetIsCaller: true, + currentRoleID: securityEngineerRoleID, + newRoleID: model.AdminRoleID, + wantReason: RoleAssignmentRestricted, }, { - name: "non-admin demotes an admin", - callerRoleID: securityEngineerRoleID, - currentRoleID: model.AdminRoleID, - newRoleID: securityEngineerRoleID, - wantReason: RoleAssignmentRestricted, + name: "non-admin changes its own role", + callerRoleID: securityEngineerRoleID, + targetIsCaller: true, + currentRoleID: securityEngineerRoleID, + newRoleID: cicdRoleID, + wantReason: RoleAssignmentRestricted, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := ctxWithCallerRole(t, key, tt.callerRoleID) + callerID := uuid.New() + ctx := ctxWithCaller(t, key, callerID, tt.callerRoleID) ug := &UserGeneric{TokenKey: key, UserRepository: &stubUserRepository{admins: tt.admins}} - err := ug.verifyRoleAssignment(ctx, tt.currentRoleID, tt.newRoleID) + var current *model.User + if !tt.create { + targetID := uuid.New() + if tt.targetIsCaller { + targetID = callerID + } + current = &model.User{Base: model.Base{ID: targetID}, RoleID: tt.currentRoleID} + } + + err := ug.verifyUserModification(ctx, current, tt.newRoleID) if tt.wantReason == "" { if err != nil { @@ -167,10 +185,10 @@ func TestVerifyRoleAssignment(t *testing.T) { } } -func TestVerifyRoleAssignmentWithoutToken(t *testing.T) { +func TestVerifyUserModificationWithoutToken(t *testing.T) { ug := &UserGeneric{TokenKey: []byte("test-token-key")} - err := ug.verifyRoleAssignment(context.Background(), uuid.Nil, model.AdminRoleID) + err := ug.verifyUserModification(context.Background(), nil, model.AdminRoleID) st, ok := status.FromError(err) if !ok { From 9fd0ff97968439083213612af7131bfbaffc52d1 Mon Sep 17 00:00:00 2001 From: Ilya Danilov Date: Fri, 28 Aug 2026 22:30:45 +0300 Subject: [PATCH 5/5] fix(auth-center): restrict user management to administrators --- auth-center/pkg/service/user_generic.go | 39 ++-- auth-center/pkg/service/user_generic_test.go | 178 +++++++++++-------- 2 files changed, 127 insertions(+), 90 deletions(-) diff --git a/auth-center/pkg/service/user_generic.go b/auth-center/pkg/service/user_generic.go index a787b797..d8c0c0ca 100644 --- a/auth-center/pkg/service/user_generic.go +++ b/auth-center/pkg/service/user_generic.go @@ -101,7 +101,7 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id: %v", err) } - if err := ug.verifyUserModification(ctx, nil, roleID); err != nil { + if err := ug.verifyUserCreation(ctx); err != nil { return nil, err } @@ -171,33 +171,44 @@ func (ug *UserGeneric) Create(ctx context.Context, req *api.CreateUserReq) (resp return resp, nil } -// verifyUserModification checks that the caller may apply the requested change; current is nil on create. -// Only an administrator manages other accounts and hands out roles, otherwise users:update escalates. -func (ug *UserGeneric) verifyUserModification(ctx context.Context, current *model.User, newRoleID uuid.UUID) error { +// verifyUserCreation checks that the caller may add an account. Only an administrator creates users, +// since creating one also assigns its role and would otherwise hand out administrator itself. +func (ug *UserGeneric) verifyUserCreation(ctx context.Context) error { + token, err := tokens.AccessTokenFromContext(ctx, ug.TokenKey) + if err != nil { + return status.Errorf(codes.Unauthenticated, "can't get token: %v", err) + } + + if token.Role.ID != model.AdminRoleID { + return errcommon.StatusWithReason(codes.PermissionDenied, UserManagementRestricted, + "can't create users").Err() + } + + return nil +} + +// verifyUserUpdate checks that the caller may apply the requested change to target. Only an +// administrator edits other accounts and hands out roles, otherwise users:update escalates. +func (ug *UserGeneric) verifyUserUpdate(ctx context.Context, target *model.User, newRoleID uuid.UUID) error { token, err := tokens.AccessTokenFromContext(ctx, ug.TokenKey) if err != nil { return status.Errorf(codes.Unauthenticated, "can't get token: %v", err) } if token.Role.ID == model.AdminRoleID { - if current != nil && current.RoleID == model.AdminRoleID && newRoleID != model.AdminRoleID { + if target.RoleID == model.AdminRoleID && newRoleID != model.AdminRoleID { return ug.verifyNotLastAdmin(ctx) } return nil } - if current == nil { - return errcommon.StatusWithReason(codes.PermissionDenied, UserManagementRestricted, - "can't create users").Err() - } - - if token.UserID != current.ID.String() { + if token.UserID != target.ID.String() { return errcommon.StatusWithReason(codes.PermissionDenied, UserManagementRestricted, "can't modify another user").Err() } - if newRoleID != current.RoleID { + if newRoleID != target.RoleID { return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, "can't change your own role").Err() } @@ -226,7 +237,7 @@ func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse id: %v", err) } - current, err := ug.UserRepository.GetByID(ctx, id) + target, err := ug.UserRepository.GetByID(ctx, id) if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return nil, status.Error(codes.NotFound, "user does not exist") @@ -248,7 +259,7 @@ func (ug *UserGeneric) Update(ctx context.Context, req *api.UpdateUserReq) (resp return nil, status.Errorf(codes.InvalidArgument, "can't parse role id") } - if err := ug.verifyUserModification(ctx, current, roleID); err != nil { + if err := ug.verifyUserUpdate(ctx, target, roleID); err != nil { return nil, err } diff --git a/auth-center/pkg/service/user_generic_test.go b/auth-center/pkg/service/user_generic_test.go index 35f140e6..95f620da 100644 --- a/auth-center/pkg/service/user_generic_test.go +++ b/auth-center/pkg/service/user_generic_test.go @@ -59,81 +59,120 @@ func ctxWithCaller(t *testing.T, key []byte, userID, roleID uuid.UUID) context.C return metadata.NewIncomingContext(context.Background(), md) } -func TestVerifyUserModification(t *testing.T) { +// requirePermission asserts that err carries the expected PermissionDenied reason, or that there is +// no error at all when wantReason is empty. +func requirePermission(t *testing.T, err error, wantReason string) { + t.Helper() + + if wantReason == "" { + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + + return + } + + st, ok := status.FromError(err) + if !ok { + t.Fatalf("Expected a gRPC status error, got %v", err) + } + if st.Code() != codes.PermissionDenied { + t.Fatalf("Expected code %v, got %v", codes.PermissionDenied, st.Code()) + } + if reason, _ := errcommon.ReasonFromStatus(st); reason != wantReason { + t.Fatalf("Expected reason %q, got %q", wantReason, reason) + } +} + +func TestVerifyUserCreation(t *testing.T) { key := []byte("test-token-key") tests := []struct { name string callerRoleID uuid.UUID - // create leaves the request without an existing user; targetIsCaller tells whether the - // updated account is the caller's own one. - create bool + wantReason string + }{ + { + name: "admin creates a user", + callerRoleID: model.AdminRoleID, + }, + { + name: "non-admin creates a user", + callerRoleID: securityEngineerRoleID, + wantReason: UserManagementRestricted, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := ctxWithCaller(t, key, uuid.New(), tt.callerRoleID) + ug := &UserGeneric{TokenKey: key} + + requirePermission(t, ug.verifyUserCreation(ctx), tt.wantReason) + }) + } +} + +func TestVerifyUserUpdate(t *testing.T) { + key := []byte("test-token-key") + + tests := []struct { + name string + callerRoleID uuid.UUID + // targetIsCaller tells whether the updated account is the caller's own one. targetIsCaller bool - currentRoleID uuid.UUID + targetRoleID uuid.UUID newRoleID uuid.UUID admins int wantReason string }{ { - name: "admin creates an admin", + name: "admin promotes another user", callerRoleID: model.AdminRoleID, - create: true, + targetRoleID: securityEngineerRoleID, newRoleID: model.AdminRoleID, }, { - name: "admin promotes another user", - callerRoleID: model.AdminRoleID, - currentRoleID: securityEngineerRoleID, - newRoleID: model.AdminRoleID, - }, - { - name: "admin demotes another admin", - callerRoleID: model.AdminRoleID, - currentRoleID: model.AdminRoleID, - newRoleID: securityEngineerRoleID, - admins: 2, + name: "admin demotes another admin", + callerRoleID: model.AdminRoleID, + targetRoleID: model.AdminRoleID, + newRoleID: securityEngineerRoleID, + admins: 2, }, { - name: "admin demotes the last admin", - callerRoleID: model.AdminRoleID, - currentRoleID: model.AdminRoleID, - newRoleID: securityEngineerRoleID, - admins: 1, - wantReason: LastAdminRemovingDenied, + name: "admin demotes the last admin", + callerRoleID: model.AdminRoleID, + targetRoleID: model.AdminRoleID, + newRoleID: securityEngineerRoleID, + admins: 1, + wantReason: LastAdminRemovingDenied, }, { name: "non-admin edits its own account", callerRoleID: securityEngineerRoleID, targetIsCaller: true, - currentRoleID: securityEngineerRoleID, + targetRoleID: securityEngineerRoleID, newRoleID: securityEngineerRoleID, }, { - name: "non-admin creates a user", + name: "non-admin edits another user", callerRoleID: securityEngineerRoleID, - create: true, - newRoleID: securityEngineerRoleID, + targetRoleID: cicdRoleID, + newRoleID: cicdRoleID, wantReason: UserManagementRestricted, }, { - name: "non-admin edits another user", - callerRoleID: securityEngineerRoleID, - currentRoleID: cicdRoleID, - newRoleID: cicdRoleID, - wantReason: UserManagementRestricted, - }, - { - name: "non-admin edits an admin account", - callerRoleID: securityEngineerRoleID, - currentRoleID: model.AdminRoleID, - newRoleID: model.AdminRoleID, - wantReason: UserManagementRestricted, + name: "non-admin edits an admin account", + callerRoleID: securityEngineerRoleID, + targetRoleID: model.AdminRoleID, + newRoleID: model.AdminRoleID, + wantReason: UserManagementRestricted, }, { name: "non-admin escalates itself to admin", callerRoleID: securityEngineerRoleID, targetIsCaller: true, - currentRoleID: securityEngineerRoleID, + targetRoleID: securityEngineerRoleID, newRoleID: model.AdminRoleID, wantReason: RoleAssignmentRestricted, }, @@ -141,7 +180,7 @@ func TestVerifyUserModification(t *testing.T) { name: "non-admin changes its own role", callerRoleID: securityEngineerRoleID, targetIsCaller: true, - currentRoleID: securityEngineerRoleID, + targetRoleID: securityEngineerRoleID, newRoleID: cicdRoleID, wantReason: RoleAssignmentRestricted, }, @@ -153,48 +192,35 @@ func TestVerifyUserModification(t *testing.T) { ctx := ctxWithCaller(t, key, callerID, tt.callerRoleID) ug := &UserGeneric{TokenKey: key, UserRepository: &stubUserRepository{admins: tt.admins}} - var current *model.User - if !tt.create { - targetID := uuid.New() - if tt.targetIsCaller { - targetID = callerID - } - current = &model.User{Base: model.Base{ID: targetID}, RoleID: tt.currentRoleID} + targetID := uuid.New() + if tt.targetIsCaller { + targetID = callerID } + target := &model.User{Base: model.Base{ID: targetID}, RoleID: tt.targetRoleID} - err := ug.verifyUserModification(ctx, current, tt.newRoleID) - - if tt.wantReason == "" { - if err != nil { - t.Fatalf("Expected no error, got %v", err) - } - return - } - - st, ok := status.FromError(err) - if !ok { - t.Fatalf("Expected a gRPC status error, got %v", err) - } - if st.Code() != codes.PermissionDenied { - t.Fatalf("Expected code %v, got %v", codes.PermissionDenied, st.Code()) - } - if reason, _ := errcommon.ReasonFromStatus(st); reason != tt.wantReason { - t.Fatalf("Expected reason %q, got %q", tt.wantReason, reason) - } + requirePermission(t, ug.verifyUserUpdate(ctx, target, tt.newRoleID), tt.wantReason) }) } } -func TestVerifyUserModificationWithoutToken(t *testing.T) { +func TestVerifyWithoutToken(t *testing.T) { ug := &UserGeneric{TokenKey: []byte("test-token-key")} + target := &model.User{Base: model.Base{ID: uuid.New()}, RoleID: model.AdminRoleID} - err := ug.verifyUserModification(context.Background(), nil, model.AdminRoleID) - - st, ok := status.FromError(err) - if !ok { - t.Fatalf("Expected a gRPC status error, got %v", err) + tests := map[string]func() error{ + "create": func() error { return ug.verifyUserCreation(context.Background()) }, + "update": func() error { return ug.verifyUserUpdate(context.Background(), target, model.AdminRoleID) }, } - if st.Code() != codes.Unauthenticated { - t.Fatalf("Expected code %v, got %v", codes.Unauthenticated, st.Code()) + + for name, verify := range tests { + t.Run(name, func(t *testing.T) { + st, ok := status.FromError(verify()) + if !ok { + t.Fatalf("Expected a gRPC status error, got %v", verify()) + } + if st.Code() != codes.Unauthenticated { + t.Fatalf("Expected code %v, got %v", codes.Unauthenticated, st.Code()) + } + }) } }