ENG-1260 Permission denied when attempting to sync megacog nodes w/ database for embeddings #680
Conversation
…as for update using works.
|
Updates to Preview Branch (ENG-1260-permission-denied-sync) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request updates Row-Level Security (RLS) UPDATE policies across multiple database tables, transitioning from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Fix all issues with AI agents
In
@packages/database/supabase/migrations/20260109035002_update_policy_using.sql:
- Around line 1-16: Add the missing DROP and CREATE for content_update_policy:
drop the existing policy content_update_policy on public."Content" and recreate
it as CREATE POLICY content_update_policy ON public."Content" FOR UPDATE USING
(public.in_space(space_id)); ensure it mirrors the other policy changes (uses
USING instead of WITH CHECK) and place these statements alongside the other
DROP/CREATE pairs in the migration so the Content table's UPDATE policy is
updated consistently.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/supabase/migrations/20260109035002_update_policy_using.sqlpackages/database/supabase/schemas/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/schemas/content.sql
🧰 Additional context used
🧠 Learnings (8)
📓 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: 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/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: 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).
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:13.774Z
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.
📚 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/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260109035002_update_policy_using.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/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260109035002_update_policy_using.sql
📚 Learning: 2026-01-01T18:13:13.774Z
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:13.774Z
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/account.sqlpackages/database/supabase/schemas/concept.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/schemas/account.sqlpackages/database/supabase/schemas/concept.sqlpackages/database/supabase/migrations/20260109035002_update_policy_using.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.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.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/20260109035002_update_policy_using.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). (3)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (7)
packages/database/supabase/schemas/account.sql (3)
453-454: LGTM - Policy switch from WITH CHECK to USING.The change to use
USINGinstead ofWITH CHECKfor UPDATE policies is appropriate here. Since the condition checks account ownership (dg_account = auth.uid()or unowned shared account access), this predicate is stable across the update operation and works correctly withUSINGsemantics.
473-474: Consistent policy change for SpaceAccess.The switch to
USING (account_uid = auth.uid())is correct. The account_uid ownership check is appropriate for both row visibility and update validation.
489-490: Remaining UPDATE policies correctly updated.The LocalAccess, AgentIdentifier, and group_membership UPDATE policies consistently follow the same pattern, switching to
USINGwith their respective access-control predicates. These changes align with the migration file and resolve the reported permission-denied failures.Also applies to: 508-509, 522-523
packages/database/supabase/schemas/content.sql (2)
662-663: Content UPDATE policy correctly updated.The
content_update_policynow usesUSING (public.in_space(space_id))which is appropriate for controlling update access based on space membership. This aligns with the migration and resolves the reported issue.
674-675: ContentAccess UPDATE policy correctly updated.The
content_access_update_policyusesUSING (public.content_in_editable_space(content_id))which appropriately restricts updates to users with editor access to the content's space. This is consistent with the INSERT policy which also requires editable space access.packages/database/supabase/schemas/concept.sql (2)
461-462: Concept UPDATE policy correctly updated.The
concept_update_policychange toUSING (public.in_space(space_id))is consistent with thecontent_update_policypattern and the migration file.
473-474: ConceptAccess UPDATE policy correctly updated.The
concept_access_update_policyusesUSING (public.concept_in_editable_space(concept_id))which mirrors the ContentAccess policy pattern. This ensures consistent access control semantics across the codebase.
https://linear.app/discourse-graphs/issue/ENG-1260/permission-denied-when-attempting-to-sync-megacog-nodes-w-database-for
The upsert_content fails in the case of content update. It probably never worked, this has nothing to do with the latest changes; we were mostly testing with inserts.
It turns out this is not an issue if the update policy is defined with "using" instead of "with check". The latter syntax applies only after the change; leaving the before case unspecified, hence restrictive. "using" used alone applies both before and after the update.
https://www.loom.com/share/00ac64c512004161905909ed5a45cfdd
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.