refactor(store): wave B.5 — role repo (#253)#254
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds a domain RoleRepo and Postgres implementation, wires it into Repos, and migrates handlers to use repository methods for role lookups, listings, counts, and membership across auth, SSO, TOTP, user, and role RPC flows. ChangesRole Repository Abstraction & Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/api/role_handler.go (1)
357-377:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck membership before enforcing the last-admin guard.
If the target user does not have the Admin role, this path can still return
ErrCannotRemoveLastAdminwhen only one admin exists, even though no revoke would occur. Gate the guard onhasRole(or check membership first) to keep idempotent no-op revokes successful.Proposed fix
- // Prevent removing the last user from the Admin system role - if role.IsSystem && role.Name == "Admin" { - userCount, err := h.store.Repos().Role.CountUsersWithRole(ctx, req.Msg.RoleId) - if err != nil { - return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to count users") - } - if userCount <= 1 { - return nil, apiErrorCtx(ctx, ErrCannotRemoveLastAdmin, connect.CodeFailedPrecondition, "cannot remove last user from Admin role") - } - } - userCtx, err := requireAuth(ctx) if err != nil { return nil, err } // Check if the user currently has the role — skip the event on retry/idempotent call hasRole, err := h.store.Repos().Role.UserHasRole(ctx, req.Msg.UserId, req.Msg.RoleId) if err != nil { return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to check role assignment") } + + // Prevent removing the last user from the Admin system role, + // but only when this request would actually revoke membership. + if hasRole && role.IsSystem && role.Name == "Admin" { + userCount, err := h.store.Repos().Role.CountUsersWithRole(ctx, req.Msg.RoleId) + if err != nil { + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to count users") + } + if userCount <= 1 { + return nil, apiErrorCtx(ctx, ErrCannotRemoveLastAdmin, connect.CodeFailedPrecondition, "cannot remove last user from Admin role") + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/role_handler.go` around lines 357 - 377, The last-admin guard runs before confirming the target user actually has the Admin role, which can incorrectly return ErrCannotRemoveLastAdmin for idempotent no-op revokes; first call h.store.Repos().Role.UserHasRole(ctx, req.Msg.UserId, req.Msg.RoleId) (or otherwise check membership) and only if hasRole is true enforce the block that checks role.IsSystem && role.Name == "Admin" and calls h.store.Repos().Role.CountUsersWithRole; if hasRole is false skip the last-admin check so revoke remains a harmless no-op.internal/api/sso_handler.go (1)
376-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
ListUserRoleserrors explicitly in SSO callback.This currently suppresses role-load failures and returns a degraded success response.
Suggested fix
- if roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID); err == nil { - for _, r := range roles { - protoUser.Roles = append(protoUser.Roles, roleToProto(r)) - } - } + roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID) + if err != nil { + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to load user roles") + } + for _, r := range roles { + protoUser.Roles = append(protoUser.Roles, roleToProto(r)) + }As per coding guidelines, "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/sso_handler.go` around lines 376 - 380, The SSO callback currently swallows errors from h.store.Repos().Role.ListUserRoles which yields a degraded success; change the block in the SSO callback handler to check the error return and, on non-nil error, return an appropriate Connect error using apiErrorCtx with a relevant code (e.g., internal or unavailable) and context message instead of proceeding — keep the successful path that maps roles with roleToProto into protoUser.Roles when err == nil, but explicitly handle and return apiErrorCtx(ctx, connect.CodeInternal, "failed to list user roles", err) (or another suitable connect.Code) so failures aren't silently ignored.internal/api/auth_handler.go (1)
132-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently swallow
ListUserRolesfailures in auth responses.Both blocks return success with empty roles when role loading fails, which hides repository errors and produces partial user payloads.
Suggested fix
- if roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID); err == nil { - for _, r := range roles { - protoUser.Roles = append(protoUser.Roles, roleToProto(r)) - } - } + roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID) + if err != nil { + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to load user roles") + } + for _, r := range roles { + protoUser.Roles = append(protoUser.Roles, roleToProto(r)) + }As per coding guidelines, "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
Also applies to: 255-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/auth_handler.go` around lines 132 - 136, The code currently swallows errors from h.store.Repos().Role.ListUserRoles and returns success with empty protoUser.Roles; instead, check the error returned by ListUserRoles and if non-nil return an apiErrorCtx with an appropriate Connect error code (e.g., internal or unavailable) and a descriptive message rather than silently continuing. Update both places that call h.store.Repos().Role.ListUserRoles to propagate repository errors via apiErrorCtx and Connect error codes, ensuring protoUser.Roles is only populated on success.internal/api/totp_handler.go (1)
433-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not ignore role-repo errors in TOTP login completion.
If role loading fails here, the RPC still succeeds with incomplete role data.
Suggested fix
- if roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID); err == nil { - for _, r := range roles { - protoUser.Roles = append(protoUser.Roles, roleToProto(r)) - } - } + roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.ID) + if err != nil { + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to load user roles") + } + for _, r := range roles { + protoUser.Roles = append(protoUser.Roles, roleToProto(r)) + }As per coding guidelines, "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/totp_handler.go` around lines 433 - 437, The Role.ListUserRoles call in the TOTP completion handler currently swallows errors and can return an incomplete protoUser.Roles; change this to check the error from h.store.Repos().Role.ListUserRoles and return an apiErrorCtx with an appropriate connect error code (e.g., codes.Internal or codes.NotFound as appropriate) instead of proceeding on nil handling, so the RPC fails on role-load errors; keep the existing loop that appends roleToProto(r) when err == nil but move it into the successful branch and handle the error branch by returning apiErrorCtx(ctx, connect.CodeInternal, "failed to list user roles", err) (or the correct connect.Code) from the TOTP login completion handler that constructs protoUser.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/user_handler.go`:
- Around line 813-816: The helper populateUserRoles currently swallows
repository errors; change its signature to return error and propagate any err
from h.store.Repos().Role.ListUserRoles (and any other failures) instead of
returning silently; update all call sites of populateUserRoles to check the
returned error and convert it to an API error using apiErrorCtx (e.g., return
nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to load user
roles")) so repository failures are surfaced and handled consistently.
---
Outside diff comments:
In `@internal/api/auth_handler.go`:
- Around line 132-136: The code currently swallows errors from
h.store.Repos().Role.ListUserRoles and returns success with empty
protoUser.Roles; instead, check the error returned by ListUserRoles and if
non-nil return an apiErrorCtx with an appropriate Connect error code (e.g.,
internal or unavailable) and a descriptive message rather than silently
continuing. Update both places that call h.store.Repos().Role.ListUserRoles to
propagate repository errors via apiErrorCtx and Connect error codes, ensuring
protoUser.Roles is only populated on success.
In `@internal/api/role_handler.go`:
- Around line 357-377: The last-admin guard runs before confirming the target
user actually has the Admin role, which can incorrectly return
ErrCannotRemoveLastAdmin for idempotent no-op revokes; first call
h.store.Repos().Role.UserHasRole(ctx, req.Msg.UserId, req.Msg.RoleId) (or
otherwise check membership) and only if hasRole is true enforce the block that
checks role.IsSystem && role.Name == "Admin" and calls
h.store.Repos().Role.CountUsersWithRole; if hasRole is false skip the last-admin
check so revoke remains a harmless no-op.
In `@internal/api/sso_handler.go`:
- Around line 376-380: The SSO callback currently swallows errors from
h.store.Repos().Role.ListUserRoles which yields a degraded success; change the
block in the SSO callback handler to check the error return and, on non-nil
error, return an appropriate Connect error using apiErrorCtx with a relevant
code (e.g., internal or unavailable) and context message instead of proceeding —
keep the successful path that maps roles with roleToProto into protoUser.Roles
when err == nil, but explicitly handle and return apiErrorCtx(ctx,
connect.CodeInternal, "failed to list user roles", err) (or another suitable
connect.Code) so failures aren't silently ignored.
In `@internal/api/totp_handler.go`:
- Around line 433-437: The Role.ListUserRoles call in the TOTP completion
handler currently swallows errors and can return an incomplete protoUser.Roles;
change this to check the error from h.store.Repos().Role.ListUserRoles and
return an apiErrorCtx with an appropriate connect error code (e.g.,
codes.Internal or codes.NotFound as appropriate) instead of proceeding on nil
handling, so the RPC fails on role-load errors; keep the existing loop that
appends roleToProto(r) when err == nil but move it into the successful branch
and handle the error branch by returning apiErrorCtx(ctx, connect.CodeInternal,
"failed to list user roles", err) (or the correct connect.Code) from the TOTP
login completion handler that constructs protoUser.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43eb2a13-17a0-4dae-b35a-e8eed97a941e
📒 Files selected for processing (11)
cmd/control/admin_user.gointernal/api/auth_handler.gointernal/api/role_handler.gointernal/api/sso_handler.gointernal/api/totp_handler.gointernal/api/user_group_handler.gointernal/api/user_handler.gointernal/store/postgres/role.gointernal/store/postgres/wire.gointernal/store/repos.gointernal/store/role.go
Comment on lines
+813
to
816
| roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.Id) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Refactor populateUserRoles to return errors instead of silently returning.
Current helper behavior hides repository failures and blocks proper apiErrorCtx handling at handler boundaries.
Suggested refactor
-func (h *UserHandler) populateUserRoles(ctx context.Context, user *pm.User) {
+func (h *UserHandler) populateUserRoles(ctx context.Context, user *pm.User) error {
roles, err := h.store.Repos().Role.ListUserRoles(ctx, user.Id)
if err != nil {
- return
+ return err
}
for _, r := range roles {
user.Roles = append(user.Roles, roleToProto(r))
}
+ return nil
}Then at call sites:
if err := h.populateUserRoles(ctx, protoUser); err != nil {
return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to load user roles")
}As per coding guidelines, "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/user_handler.go` around lines 813 - 816, The helper
populateUserRoles currently swallows repository errors; change its signature to
return error and propagate any err from h.store.Repos().Role.ListUserRoles (and
any other failures) instead of returning silently; update all call sites of
populateUserRoles to check the returned error and convert it to an API error
using apiErrorCtx (e.g., return nil, apiErrorCtx(ctx, ErrInternal,
connect.CodeInternal, "failed to load user roles")) so repository failures are
surfaced and handled consistently.
Biggest single-domain migration so far. RoleRepo covers both the role catalog (Get / GetByName / List / Count) and the per-user role-membership graph (ListUserRoles / UserHasRole / CountUsersWithRole / CountGroupsWithRole / ListUserIDsWithRole / ListUserIDsWithGroupRole). Both halves share the projection pipeline and are queried together from the role handler. Call sites migrated (22 across 6 files): - role_handler.go (17) - auth_handler.go (2) — login response role population - sso_handler.go (1) - totp_handler.go (1) - user_handler.go (1) — populateUserRoles - cmd/control/admin_user.go (1) — admin bootstrap roleToProto's signature shifted from generated.RolesProjection to store.Role. The single roleToProto consumer in user_group_handler.go (still on Queries().GetUserGroupRoles until UserGroup migrates) uses a transitional inline conversion. Non-goals: - GetUserGroupRoles / UserGroupHasRole — UserGroup domain, stays. - ListInheritedRolesByUserIDs — bulk user-page join shape, handled when User migrates. Closes #253.
PaulDotterer
force-pushed
the
feat/253-role-repo
branch
from
eda4ff0 to
0d2476e
Compare
This was referenced May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
`RoleRepo` — biggest single-domain migration so far. 10 methods, 22 call sites across 6 files. Covers both the role catalog and the per-user role membership graph; both share the projection pipeline.
Branches off main directly (independent of #250 / #252 — different files).
Methods
Call sites migrated (22)
`roleToProto` signature shifted from `generated.RolesProjection` → `store.Role`. `user_group_handler.go` still uses the sqlc shape (UserGroup domain stays on `Queries()` until its migration) and does a transitional inline conversion at the one `roleToProto` call.
Non-goals
Acceptance
Part of #242. Closes #253.
Summary by CodeRabbit