Repository navigation
fix(auth-center): restrict role assignment in user create and update #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
78412ca
8e5329b
ed45e8d
f5052c0
9fd0ff9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,57 @@ 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 { | ||
| if currentRoleID == 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 newRoleID != token.Role.ID && newRoleID != currentRoleID { | ||
| return errcommon.StatusWithReason(codes.PermissionDenied, RoleAssignmentRestricted, | ||
| "can't assign a role other than your own").Err() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the point of such limitation? shouldn't admin be able to create users with other roles? p.s currently, admin and security engineer are allowed to create users. i suggest removing this permission from security engineer, since it doesn't make sense imo. after we do it, we can get rid of this check safely if i'm not mistaken. what do you think? |
||
| } | ||
|
|
||
| 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 { | ||
| 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 +243,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, | ||
|
|
@@ -247,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() | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| package service | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
| "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" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/metadata" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| 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. | ||
| 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") | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| callerRoleID uuid.UUID | ||
| currentRoleID uuid.UUID | ||
| newRoleID uuid.UUID | ||
| admins int | ||
| 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, | ||
| 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", | ||
| 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) | ||
| ug := &UserGeneric{TokenKey: key, UserRepository: &stubUserRepository{admins: tt.admins}} | ||
|
|
||
| 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()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last admin can be demoted here. need to additionally check isn't it a last admin user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!