ENG-1179 Group ACL table (For F1B) #662
Conversation
|
Updates to Preview Branch (eng-1179-group-acl-table-for-f1b) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
changed the base branch from
main
to
eng-1232-create-group-table-and-use-it-in-space-access-functions
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds per-item RBAC: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB as Database (RLS)
participant Func as Access Functions
participant AccessTbl as ContentAccess/ConceptAccess
participant Auth as auth.users
Client->>DB: SELECT content/concept row
activate DB
DB->>DB: Evaluate public.in_space(space_id)
alt in_space == true
DB-->>Client: RETURN row
else
DB->>Func: public.can_view_specific_<content|concept>(id)
activate Func
Func->>Auth: resolve current user account UIDs
Auth-->>Func: account UIDs
Func->>AccessTbl: lookup entry (account_uid, item_id)
AccessTbl-->>Func: found? (true/false)
Func-->>DB: boolean
deactivate Func
alt true
DB-->>Client: RETURN row
else false
DB-->>Client: DENY / no rows
end
end
deactivate DB
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 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.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/src/dbTypes.tspackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
packages/database/src/dbTypes.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
📚 Learning: 2025-10-08T18:55:18.029Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
Applied to files:
packages/database/supabase/schemas/content.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/src/dbTypes.ts
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/schemas/content.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-06-07T02:56:57.093Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-07T02:56:57.093Z
Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
Applied to files:
packages/database/supabase/schemas/content.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/schemas/content.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, when a table is renamed using ALTER TABLE ... RENAME TO, the renamed table retains all of its properties including Row Level Security (RLS) settings. Therefore, if RLS was enabled on the original table, it remains enabled on the renamed table without needing to re-execute ALTER TABLE ... ENABLE ROW LEVEL SECURITY.
Applied to files:
packages/database/supabase/schemas/content.sqlpackages/database/supabase/schemas/concept.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/src/dbTypes.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
packages/database/src/dbTypes.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (6)
packages/database/src/dbTypes.ts (1)
250-279: Generated types correctly reflect the new schema.The auto-generated
ConceptAccessandContentAccesstable types, along with thecan_view_specific_conceptandcan_view_specific_contentfunction declarations, properly mirror the SQL schema changes. The relationship definitions correctly reference both base tables and security views.Also applies to: 457-493, 1390-1391
packages/database/supabase/schemas/content.sql (2)
210-220: Function implementation is correct.The
SECURITY DEFINERwith emptysearch_pathis the appropriate security pattern. TheLIMIT 1provides good optimization for existence checks.Minor note: The parameter
idshadows the column namecontent_idcomparison works correctly becauseidrefers to the parameter andcontent_idis the column, but using a prefixed parameter name (e.g.,p_idortarget_id) would improve clarity.
238-242: View and RLS policy updates are well-structured.The
my_contentsview correctly mirrors the RLS policy logic, and splitting into granular policies (SELECT/DELETE/INSERT/UPDATE) follows PostgreSQL best practices. The SELECT policy appropriately allows either space membership or per-item access, while write operations remain restricted to space members.Based on learnings, this view-based approach provides substantial performance gains over direct table access.
Also applies to: 645-652
packages/database/supabase/schemas/concept.sql (1)
152-162: Function, view, and policy changes are correct.The
can_view_specific_conceptfunction follows the same secure pattern as its content counterpart. Themy_conceptsview update and granular RLS policy split are properly implemented.Also applies to: 181-185, 445-452
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql (2)
35-72: Migration correctly implements the access control infrastructure.The functions, views, and policy updates are properly structured. Using
DROP POLICY IF EXISTSbefore creating new policies ensures idempotency. The view definitions correctly implement the dual access path (space membership OR per-item access).Also applies to: 109-148
31-33: Migration aligns with schema definitions but inherits the missing RLS issue.The migration correctly creates both access tables and updates policies. However, the same concern applies:
ContentAccessandConceptAccesstables should have RLS enabled to prevent unauthorized manipulation of access entries.Based on learnings, the schema files in
supabase/schemas/are the source of truth - the RLS fix should be applied there, and this migration would then reflect those changes.Also applies to: 105-107
⛔ Skipped due to learnings
Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 165 File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45 Timestamp: 2025-05-20T03:11:07.917Z Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 165 File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45 Timestamp: 2025-05-20T03:11:07.917Z Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 165 File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21 Timestamp: 2025-05-20T03:04:21.602Z Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 198 File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337 Timestamp: 2025-06-09T16:57:14.681Z Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/database/supabase/schemas/concept.sql (1)
118-150: Missing RLS policies on ConceptAccess table.The ConceptAccess table has grants but no Row Level Security enabled. This is inconsistent with ContentAccess (which has RLS in content.sql lines 664-674) and leaves the table unprotected.
🔎 Proposed fix to enable RLS and add policies
Add after line 150:
+ALTER TABLE public."ConceptAccess" ENABLE ROW LEVEL SECURITY; + +DROP POLICY IF EXISTS concept_access_policy ON public."ConceptAccess"; +DROP POLICY IF EXISTS concept_access_select_policy ON public."ConceptAccess"; +CREATE POLICY concept_access_select_policy ON public."ConceptAccess" FOR SELECT USING (public.concept_in_space(concept_id) OR public.can_access_account(account_uid)); +DROP POLICY IF EXISTS concept_access_delete_policy ON public."ConceptAccess"; +CREATE POLICY concept_access_delete_policy ON public."ConceptAccess" FOR DELETE USING (public.concept_in_editable_space(concept_id) OR public.can_access_account(account_uid)); +DROP POLICY IF EXISTS concept_access_insert_policy ON public."ConceptAccess"; +CREATE POLICY concept_access_insert_policy ON public."ConceptAccess" FOR INSERT WITH CHECK (public.concept_in_editable_space(concept_id)); +DROP POLICY IF EXISTS concept_access_update_policy ON public."ConceptAccess"; +CREATE POLICY concept_access_update_policy ON public."ConceptAccess" FOR UPDATE WITH CHECK (public.concept_in_editable_space(concept_id));
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/database/src/dbTypes.tspackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/database/src/dbTypes.ts
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/schemas/account.sql:0-0
Timestamp: 2025-06-09T16:55:48.941Z
Learning: Security work including RLS policies and permission restrictions is currently postponed in the discourse-graph project as communicated by maparent.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/schemas/account.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-07-14T05:51:46.322Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-14T05:51:46.322Z
Learning: The concept_in_space function in the discourse-graph project is slower than using a more direct approach with in_space(space_id) directly in RLS policies, which is why a more direct approach is sometimes preferred for performance reasons.
Applied to files:
packages/database/supabase/schemas/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2025-10-08T18:55:18.029Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/schemas/concept.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, when a table is renamed using ALTER TABLE ... RENAME TO, the renamed table retains all of its properties including Row Level Security (RLS) settings. Therefore, if RLS was enabled on the original table, it remains enabled on the renamed table without needing to re-execute ALTER TABLE ... ENABLE ROW LEVEL SECURITY.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-06-09T16:55:48.941Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/schemas/account.sql:0-0
Timestamp: 2025-06-09T16:55:48.941Z
Learning: Security work including RLS policies and permission restrictions is currently postponed in the discourse-graph project as communicated by maparent.
Applied to files:
packages/database/supabase/schemas/content.sql
📚 Learning: 2025-06-07T02:56:57.093Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-07T02:56:57.093Z
Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
Applied to files:
packages/database/supabase/schemas/content.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (12)
packages/database/supabase/schemas/account.sql (3)
266-278: LGTM! Well-structured security utility.The function correctly checks both direct account ownership and group membership for access control.
342-352: LGTM! Efficient editor space aggregation.The function correctly aggregates space IDs where the user has editor privileges, with proper handling of empty results.
355-365: LGTM! Correct editor access check.The function properly verifies editor privileges for a specific space using an efficient EXISTS query.
packages/database/supabase/schemas/concept.sql (4)
152-162: LGTM! Correct per-concept visibility check.The function properly queries ConceptAccess to determine if the user has specific access to a concept.
181-185: LGTM! Properly extends concept visibility.The view correctly combines space-based access with per-item access grants.
441-449: LGTM! Correct editor access helper.The function properly delegates to editor_in_space after resolving the concept's space.
454-462: LGTM! Well-structured granular policies.The policies correctly separate read access (space + per-item) from write access (space only).
packages/database/supabase/schemas/content.sql (5)
168-200: LGTM! Well-structured access control table.The ContentAccess table definition is correct with proper foreign keys, indexes, and grants.
210-220: LGTM! Correct per-content visibility check.The function properly queries ContentAccess to determine user-specific access.
238-242: LGTM! Properly extends content visibility.The view correctly combines space-based and per-item access for content retrieval.
627-635: LGTM! Correct editor access helper.The function properly resolves the content's space before checking editor privileges.
654-662: LGTM! Consistent granular policies.The Content policies correctly mirror the Concept pattern with read/write separation.
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
Outdated
Show resolved
Hide resolved
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/database/supabase/schemas/content.sql (1)
180-180: Fix comment wording for clarity.The comment reads "The content in which the content is located", which is confusing. It should describe that this column identifies the content item for which access is being granted.
🔎 Suggested wording
-COMMENT ON COLUMN public."ContentAccess".content_id IS 'The content in which the content is located'; +COMMENT ON COLUMN public."ContentAccess".content_id IS 'The content item for which access is granted';packages/database/supabase/schemas/concept.sql (1)
130-130: Fix comment wording for clarity.Similar to ContentAccess, this comment has confusing wording: "The concept in which the concept is located".
🔎 Suggested wording
-COMMENT ON COLUMN public."ConceptAccess".concept_id IS 'The concept in which the concept is located'; +COMMENT ON COLUMN public."ConceptAccess".concept_id IS 'The concept for which access is granted';packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql (1)
70-70: Note: Comment wording issue in migration.The comment has unclear wording: "The content in which the content is located". However, per project learnings, migration files should not be modified after creation.
Consider updating the corresponding comment in the schema file
packages/database/supabase/schemas/content.sql(line 180) to ensure future schema states have clear documentation.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/schemas/account.sql:0-0
Timestamp: 2025-06-09T16:55:48.941Z
Learning: Security work including RLS policies and permission restrictions is currently postponed in the discourse-graph project as communicated by maparent.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
📚 Learning: 2025-10-08T18:55:18.029Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In the discourse-graph database schema, security views like `my_concepts`, `my_contents`, and `my_documents` were introduced to significantly improve query performance. Direct table access had dismal performance, while the view-based approach provides substantial performance gains.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, when a table is renamed using ALTER TABLE ... RENAME TO, the renamed table retains all of its properties including Row Level Security (RLS) settings. Therefore, if RLS was enabled on the original table, it remains enabled on the renamed table without needing to re-execute ALTER TABLE ... ENABLE ROW LEVEL SECURITY.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2025-07-14T05:51:46.322Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-14T05:51:46.322Z
Learning: The concept_in_space function in the discourse-graph project is slower than using a more direct approach with in_space(space_id) directly in RLS policies, which is why a more direct approach is sometimes preferred for performance reasons.
Applied to files:
packages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sqlpackages/database/supabase/schemas/content.sql
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: When reviewing SQL functions in the discourse-graph codebase, be mindful that suggesting additional explicit error handling may introduce unnecessary database queries. Functions that use appropriate ON CONFLICT clauses may already handle errors sufficiently without needing extra validation queries.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-08-21T23:47:06.754Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-21T23:47:06.754Z
Learning: Supabase edge functions in the discourse-graph codebase cannot reuse code from elsewhere in the codebase due to runtime environment isolation constraints. Code duplication in edge functions at packages/database/supabase/functions/ is necessary and expected.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, database functions that use STRICT clauses and ON CONFLICT statements already have appropriate error handling. Suggesting additional explicit error handling may be unnecessary and could introduce performance overhead through extra database queries.
Applied to files:
packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql
📚 Learning: 2025-06-09T16:55:48.941Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/schemas/account.sql:0-0
Timestamp: 2025-06-09T16:55:48.941Z
Learning: Security work including RLS policies and permission restrictions is currently postponed in the discourse-graph project as communicated by maparent.
Applied to files:
packages/database/supabase/schemas/content.sql
📚 Learning: 2025-06-07T02:56:57.093Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-07T02:56:57.093Z
Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
Applied to files:
packages/database/supabase/schemas/content.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (15)
packages/database/supabase/schemas/content.sql (5)
210-220: Well-designed access check function.The
can_view_specific_contentfunction correctly usesSECURITY DEFINERto check ContentAccess entries, and theLIMIT 1optimization is appropriate for an existence check.
238-242: Correct integration of per-item access control.The updated view properly combines space membership with specific content access grants, maintaining the performance benefits of the view-based approach noted in project learnings.
627-635: Essential helper function for access control.The
content_in_editable_spacefunction correctly resolves the content's space_id before checking editor privileges, addressing the critical issue from previous reviews.
655-662: Well-structured granular RLS policies for Content.The policy split provides appropriate access control: SELECT allows both space membership and individual grants, while modifications require space membership. This is a reasonable security model.
664-674: ContentAccess policies implement appropriate security model.The policies correctly enforce that:
- Space editors control who gets access (INSERT/UPDATE)
- Users can revoke their own access (DELETE with
can_access_account)- Users can view their own grants (SELECT with
can_access_account)All referenced functions and dependencies are properly defined:
can_access_account()checks both direct account ownership and group membership viapublic.group_membershipcontent_in_space()andcontent_in_editable_space()exist and correctly delegate to the appropriate space access checks- The policies now correctly use
content_in_editable_space(content_id)to prevent non-editors from inserting or updating access recordspackages/database/supabase/schemas/concept.sql (5)
152-162: Consistent implementation with content access pattern.The
can_view_specific_conceptfunction correctly mirrors the content access implementation, maintaining consistency across the RBAC system.
181-185: Properly extends concept visibility with per-item grants.The view update correctly implements the same dual access model as
my_contents, maintaining consistency across the system.
441-449: Correct helper function for concept editor checks.The
concept_in_editable_spacefunction properly implements the editor privilege check, consistent with the content implementation.
455-462: Appropriate RLS policies for Concept table.The granular policies correctly implement the security model: SELECT allows individual grants, while modifications require space membership. This mirrors the Content policies, maintaining consistency.
464-474: ConceptAccess policies correctly mirror ContentAccess design.The policies implement the same security model as ContentAccess, with correct use of
concept_in_editable_space. This consistency is beneficial for maintaining and understanding the access control system.packages/database/supabase/migrations/20260102140646_content_and_concept_access_tables.sql (5)
26-36: Core editor privilege check implemented correctly.The
editor_in_spacefunction properly checks editor status via SpaceAccess table, serving as the foundation for the content and concept editable space checks.
38-56: Helper functions correctly resolve space context for editor checks.Both
content_in_editable_spaceandconcept_in_editable_spaceproperly look up the space_id before delegating toeditor_in_space, fixing the critical issue flagged in previous reviews.
131-141: ContentAccess RLS policies correctly implemented.The policies now use
content_in_editable_space(content_id)for INSERT/UPDATE/DELETE editor checks, resolving the critical issue from previous reviews whereeditor_in_space(content_id)was incorrectly used with a content ID instead of a space ID.
219-229: ConceptAccess RLS policies correctly mirror ContentAccess.The policies correctly use
concept_in_editable_space(concept_id)for editor checks, maintaining consistency with the ContentAccess implementation and resolving the analogous critical issue.
1-11: No action needed. Thegroup_membershiptable is properly created in migration20260101183250_group_access.sql, which runs before this migration. The table schema (member_id and group_id columns) matches the function's expectations.
maparent
force-pushed
the
eng-1232-create-group-table-and-use-it-in-space-access-functions
branch
from
8170222 to
fc1bfa1
Compare
maparent
force-pushed
the
eng-1179-group-acl-table-for-f1b
branch
from
c87b3f8 to
467227e
Compare
https://linear.app/discourse-graphs/issue/ENG-1179/group-acl-table-for-f1b
This introduces separate tables for Concept and Content access. Also a few security utility functions.
https://www.loom.com/share/3f9f40dd9a6d493b9042701190883285
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.