Repository navigation
fix(auth): propagate per-group fetch errors in GetUserPermissions - #920
Conversation
Any transient store error fetching a group in GetUserPermissions or collectGroupsAndAccounts is now returned immediately rather than logged and skipped. Callers therefore fail closed with an error instead of silently receiving a partial permission union computed from the remaining groups (closes #918). A deleted/missing group (store returns nil, nil) is still skipped without error, preserving the existing behavior for that case. Remove unused logging import; update existing test that asserted the old swallow-and-continue behavior.
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 51 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR hardens the authorization service to fail-closed with explicit errors when group fetches fail. ChangesFail-Closed Error Propagation for Group Lookups
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/auth/service_group_test.go`:
- Around line 409-435: Add sibling test cases next to the existing "propagates
per-group fetch error..." (for GetUserPermissions) and the BuildAuthContext
test: mock Store.GetGroup to return (nil, pgx.ErrNoRows) for the missing group
ID instead of assert.AnError; then assert the call does not return an error and
that the missing group is simply skipped (i.e. permissions/auth context reflect
only the present groups and no partial/error result is produced). Use the same
test setup symbols (MockStore, GetUserByID, GetGroup,
service.GetUserPermissions, and service.BuildAuthContext) and mirror the
existing assertions but expect no error and the returned permissions/auth
context exclude the pgx.ErrNoRows group.
In `@internal/auth/service_group.go`:
- Around line 73-79: collectGroupsAndAccounts is treating any non-nil error from
s.store.GetGroup as fatal even though PostgresStore.scanGroup may return (nil,
pgx.ErrNoRows) for deleted groups; update collectGroupsAndAccounts in
internal/auth/service_group.go to treat pgx.ErrNoRows as a non-fatal "skip" by
checking errors.Is(err, pgx.ErrNoRows) and continue, mirroring the
comment/behavior in Service.GetUserPermissions and existing tests, so only other
errors are returned.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e133e1-beee-49f6-8d1b-61ccffb24cba
📒 Files selected for processing (3)
internal/auth/service_group.gointernal/auth/service_group_test.gointernal/auth/service_test.go
…tGroupsAndAccounts PostgresStore.GetGroup returns (nil, pgx.ErrNoRows) when a group row no longer exists. Without an explicit check, that error was treated as a transient failure and propagated, causing the entire permission lookup to fail for users whose group list includes a deleted group. Add errors.Is(err, pgx.ErrNoRows) guards in both GetUserPermissions and collectGroupsAndAccounts so a deleted group is skipped (same as the existing nil-group path), while any other store error still propagates. Add pgx.ErrNoRows test cases for both functions to cover the deleted-group skip path explicitly (CR #920 minor finding).
|
Addressed both CR findings (commit 99a83d7): MAJOR (service_group.go): Added MINOR (service_group_test.go): Added two sibling test cases covering the
Both assert no error is returned and the result reflects only the groups that resolved successfully. All 1897 tests pass; @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest PR diff
gh pr view 920 --repo LeanerCloud/CUDly --json files,headRefName,baseRefName 2>/dev/null || echo "Not accessible"
# Get the PR diff
gh pr diff 920 --repo LeanerCloud/CUDly 2>/dev/null | head -300Length of output: 10506 🏁 Script executed: #!/bin/bash
# Check the current state of service_group.go
cat internal/auth/service_group.go 2>/dev/null || echo "File not found"Length of output: 7932 🏁 Script executed: #!/bin/bash
# Check service_group_test.go for new test cases
cat internal/auth/service_group_test.go 2>/dev/null || echo "File not found"
# Check if there's a store abstraction wrapping pgx errors
rg -n "ErrNoRows\|pgx" internal/auth/ --type go -C2 2>/dev/nullLength of output: 42091 🏁 Script executed: #!/bin/bash
# Check how PostgresStore.GetGroup returns errors - look for store interface/implementation
rg -rn "GetGroup\|ErrNoRows\|ErrNotFound" --type go -C3 internal/ 2>/dev/null | head -100Length of output: 436 🏁 Script executed: #!/bin/bash
# Find the store implementation to check GetGroup error behavior
fd -e go . internal/ | xargs grep -l "GetGroup\|ErrNoRows" 2>/dev/null
# Also check for any store interface or postgres store files
fd -e go . --search-path internal/ 2>/dev/null | head -50Length of output: 3521 🏁 Script executed: #!/bin/bash
# Search more broadly for ErrNoRows and pgx usage across the codebase
rg -rn "ErrNoRows" --type go -C3 2>/dev/null | head -80
# Also look for postgres store GetGroup implementation
rg -rn "func.*GetGroup" --type go -C5 2>/dev/null | head -60Length of output: 7909 🏁 Script executed: #!/bin/bash
# Look at the postgres store GetGroup implementation
cat internal/auth/store_postgres.go | grep -A 30 "func.*GetGroup"Length of output: 974 🏁 Script executed: #!/bin/bash
# Check for any store-level ErrNoRows handling in auth package
rg -n "ErrNoRows\|pgx\.Err" internal/auth/store_postgres.go -C3 2>/dev/null
# Also check the store interface
cat internal/auth/interfaces.go | grep -A 5 "GetGroup"Length of output: 633 🏁 Script executed: #!/bin/bash
# Find the scanGroup function to understand how ErrNoRows is handled
rg -n "func.*scanGroup\|ErrNoRows\|pgx\." internal/auth/store_postgres.go -C4 2>/dev/nullLength of output: 413 🏁 Script executed: #!/bin/bash
# Get the full store_postgres.go to find scanGroup
grep -n "scanGroup\|ErrNoRows\|pgx\." internal/auth/store_postgres.go | head -40Length of output: 887 🏁 Script executed: #!/bin/bash
# Get the scanGroup function implementation
awk '/func.*scanGroup/,/^func/' internal/auth/store_postgres.go | head -30Length of output: 244 🏁 Script executed: #!/bin/bash
# Get full scanGroup implementation
sed -n '916,960p' internal/auth/store_postgres.goLength of output: 1254
Dual guard is sound — keeping both the
Tests are thorough — the four added cases (two propagation, two ErrNoRows-skip) cover the critical paths with clear regression annotations pointing back to LGTM. Triggering a full review of the latest commit. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
GetUserPermissionsandcollectGroupsAndAccountsininternal/auth/service_group.gowere swallowing per-group store errors: on a transient DB error fetching one group they logged a warning andcontinued, then returned the permission union of the remaining groups.GetGroupimmediately so callers fail closed with an error. A nil group (store returnsnil, nilfor a deleted group) is still skipped without error.collectGroupsAndAccountsto returnerror;BuildAuthContextnow checks and propagates it.loggingimport.GetUserPermissions, one forBuildAuthContext) asserting that a per-group fetch error propagates.closes #918
Test plan
go test ./internal/auth/... ./internal/api/...- 1895 tests passgofmt -lclean on touched filesgo vet ./internal/auth/... ./internal/api/...cleanTestService_GetUserPermissions/propagates_per-group_fetch_error_instead_of_returning_partial_permissionsandTestService_BuildAuthContext/propagates_per-group_fetch_error_instead_of_returning_partial_contextTestService_ErrorPaths/GetUserPermissions_with_store_error_on_groupupdated to match new correct behaviorSummary by CodeRabbit
Bug Fixes
Tests