feat(store): org scoped searches report nothing for a deleted organization - #1953
Conversation
…ation The org users, org projects, and user projects searches join organizations and filter it, so a soft-deleted organization answers the same way as an organization id nobody owns.
… scoped searches Seeds the same shape under a live organization and a deleted one, so an empty result cannot come from a broken seed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe three PostgreSQL search query paths now require a matching live organization. SQL expectations and database-backed tests cover live and soft-deleted organizations. ChangesLive organization search filtering
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The searches are set to exclude results for soft-deleted organizations, with tests covering live and deleted cases. No issue identified here prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed queries add a condition that removes results for deleted organizations while retaining their existing organization and policy filters. No expanded access was identified. Coverage of other organization-scoped paths remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Coverage Report for CI Build 36388354390Coverage increased (+0.02%) to 52.214%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Its base query at The description says why org service users and org tokens are left alone. I checked both and agree with the reasoning. Org pats needs the same note, or the same fix. One more thing in that file while you are in there. The |
|
Right on both, thanks. Org pats has the same gap. It was in an earlier revision of this branch and came out again deliberately, because this PR is scoped to the searches this series raised PRs for and org pats is not one of them. I should have said that in the description rather than leaving it silent. Added now. It will be tested and fixed with the PAT soft delete work rather than here, along with the users join you spotted. That join has the same shape as the org tokens one: a soft-deleted user's tokens still come back carrying their name and email. Worth recording while we are here. |
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Description covers org pats now, thanks. Handling it with the PAT soft delete work makes sense, since the users join needs fixing in the same place.
One non-blocking note on the description, for whoever reads this later. "Frontier does not set deleted_at on organizations yet, so the searches return the same rows as before" holds for the two project searches but not for org users. projects.org_id has a foreign key to organizations(id) with no ON DELETE clause, so an organization with any project row cannot be hard-deleted and that join can never drop a row. policies.resource_id is a bare uuid NOT NULL with no foreign key, because it is polymorphic. So a hard-deleted organization leaves its membership policies behind, and SearchOrganizationUsers on that dead id used to return them. Now it returns nothing.
That is an improvement, not a regression. It just means this PR changes behaviour on today's data too, rather than only guarding a future soft delete.
Merged this with #1952 locally. No conflicts, and both docker suites pass together.
This branch had an error being deployed
3 participants
Follows #1946, #1947, #1949 and #1951, which added this check as they were written. This brings the three searches that merged before we agreed on it into line.
Summary
The org users, org projects and user projects searches never read the organizations table. A soft-deleted organization still returned its rows, while an organization id nobody owns returned nothing. Those two now agree.
Changes
liveon it. The join is on the organizations primary key, so it matches at most one row and cannot duplicate results.o2to match that file.search_live_org_pg_test.go: the same shape is seeded under a live organization and a soft-deleted one, and each search is checked against both.Technical Details
Nothing upstream catches this for these three. They authorize with
IsSuperUser, so the disabled-organization gate inIsAuthorizednever runs, and that gate steps aside for platform superusers anyway. None of their handlers resolve the organization first.The organization service users search is deliberately untouched. Its handler resolves the organization through a lookup that reads live rows only, so a deleted organization is already not found before the query runs.
The organization personal access tokens search has the same gap and is also untouched. It is not one of the searches this series raised PRs for, and its
usersjoin needs the same filter, so it is handled with the PAT soft delete work rather than piecemeal here.The organization tokens search is also untouched. It has no soft-delete filters on any of its three tables, so it gets the whole treatment as its own change rather than the organization half here.
The seeded user and organizations carry avatars because two of these queries cannot scan a null one. That is pre-existing and noted in the test.
Frontier does not set
deleted_aton organizations yet, so the searches return the same rows as before.Test Plan
go test ./internal/store/postgres/passesmainfor all three searchesgolangci-lint run ./internal/store/postgres/...reports no issuesmainon the same database, with only the binary swapped.SearchOrganizationUsersSearchOrganizationProjectsSearchUserProjectsSQL Safety
?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L.//nolint:forbidigoor// #nosec G20xannotations.