Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 SummarySummary by CodeRabbit
Walkthrough
ChangesPolicy role locking
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🔵 Low · up to The role-locking change itself looks sound. One concurrency test could be flaky because it counts every lock waiter in the database. Fixing that is a small, test-only follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens role-lifecycle consistency without adding an endpoint or granting additional authority. Conflicting locks protect the policy write, and errors trigger rollback. Remaining uncertainty concerns production-path concurrency coverage, not an observed security defect. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Correct the missing-role error assertion. · policy_repository_test.go:197-199
internal/store/postgres/policy_repository_test.go:197-199
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the missing-role error assertion.
Upsertnow returnsrole.ErrNotExistfor the nonexistent role in the case at lines 175-182. That case still expectspolicy.ErrInvalidDetail. The conditionerrors.Is(tc.Err, err)also fails on a match and accepts a mismatch, so this test passes without checking the new contract. Set the expected error torole.ErrNotExistand asserts.Require().ErrorIs(err, tc.Err).
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: raystack/frontier/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
78913928-6bc8-4944-8b5a-a0eb9927133b
📒 Files selected for processing (2)
internal/store/postgres/policy_repository.gointernal/store/postgres/policy_repository_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
Coverage Report for CI Build 37429504705Coverage increased (+0.02%) to 54.369%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
060ff0e to
aa8b809
Compare
|
Fixed in aa8b809: the missing-role case now uses a random uuid and expects |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Give the lock-wait check more than one second. · policy_repository_test.go:265-269
internal/store/postgres/policy_repository_test.go:265-269
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the lock-wait check more than one second.
If a loaded CI worker delays the goroutine before its
FOR KEY SHAREquery reaches PostgreSQL, the one-secondEventuallydeadline can fail before the test observes the wait. The client also gives that query a one-second timeout, so extending only the polling window is not enough.Suggested timeout changes
+# In newTestClient's pgConfig: - MaxQueryTimeout: time.Millisecond * 1000, + MaxQueryTimeout: 10 * time.Second,- }, time.Second, 10*time.Millisecond, "the create did not wait for the role delete") + }, 5*time.Second, 10*time.Millisecond, "the create did not wait for the role delete")
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: raystack/frontier/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
68d2a8a1-db65-4aed-931d-83934d94529d
📒 Files selected for processing (1)
internal/store/postgres/policy_repository_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
…sert transaction A soft-deleted role keeps its row, so the foreign key on policies.role_id no longer proves the role is live. The insert transaction now reads the role from the live rows with FOR KEY SHARE before the insert. That read waits for a role delete that is still running and returns not found once it commits, and it does not block other policy creates for the same role.
The missing-role case now uses a random uuid and expects the not-found error, the namespace case carries valid ids so it reaches the foreign key, and the table asserts with ErrorIs instead of a check that passed on any error.
aa8b809 to
c006ab7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: raystack/frontier/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
501bf322-e01f-4e24-a2e4-f163c82d09ec
📒 Files selected for processing (1)
internal/store/postgres/policy_repository_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
| err := s.client.QueryRowxContext(s.ctx, "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock'").Scan(&waiting) | ||
| return err == nil && waiting == 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count waits on this transaction, not all database waits.
If another session waits for a lock in the same database, waiting == 1 can pass before this policy upsert blocks or fail when both sessions block. Record the deletion transaction’s backend PID. Then check whether the policy upsert is blocked by that PID, for example with pg_blocking_pids. (postgresql.org)
This branch was successfully deployed
2 participants
What
PolicyRepository.Upsertreads the role from the live rows withFOR KEY SHAREinside the insert transaction, before the insert. No row returnsrole.ErrNotExist, which the create handler already maps to not found.Why
A role delete now sets
deleted_atand keeps the row (#1964), so the foreign key onpolicies.role_idno longer proves the role is live. The service reads the live role before calling the repository, but that read runs outside the transaction. A policy insert that starts while a role delete is running waits for the delete and then succeeds, because the row still exists. The locked read closes that order: it waits for the delete and returns no row once the delete commits.FOR KEY SHAREis the lock the foreign key check itself takes, so policy creates for the same role do not block each other.Behaviour change
Creating a policy for a soft-deleted role returns not found instead of succeeding. Creates for live roles are unchanged.
Rollout
No migration, no config.
Tested
golangci-lintreports no issues on the package.SQL Safety
$1throughgoqu.Ex{}.ToSQL()params are forwarded unchanged.?inside quoted SQL literals.// nolinton a deferred rollback in a test, the same as the role suite. None in source.