ENG-1231 Migrate SpaceAccess to use user ids, not platformAccount ids. #660
Conversation
|
Updates to Preview Branch (eng-1231-migrate-spaceaccess-to-use-user-ids-not-platformaccount-ids) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughMigrates space-access from platform-account IDs to auth-user IDs: adds a LocalAccess table, rewrites SpaceAccess to use account_uid (auth.users.id), updates DB functions/views/policies (many to auth.uid()), removes legacy my_account(), and updates application upserts to use account_uid with corresponding migration SQL and test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppFn as create-space (supabase function)
participant Auth as auth.users
participant DB as Public DB (SpaceAccess, LocalAccess, PlatformAccount)
rect rgba(200,230,255,0.4)
Client->>AppFn: request create space (anonymousUser.id)
AppFn->>Auth: use anonymousUser.id (account_uid)
AppFn->>DB: upsert SpaceAccess {account_uid, space_id, editor}
DB-->>DB: ON CONFLICT (account_uid,space_id) apply update
AppFn->>DB: insert LocalAccess {account_id, space_id} DO NOTHING on conflict
DB-->>Client: return space & account identifiers
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 0
🧹 Nitpick comments (3)
packages/database/supabase/schemas/account.sql (3)
94-96: Consider restrictinganonprivileges onLocalAccess.Currently
GRANT ALL ON TABLE public."LocalAccess" TO anongives anonymous users full table access (before RLS). While RLS will still apply, theanonrole typically shouldn't needINSERT/UPDATE/DELETEprivileges. Consider restricting toSELECTonly for defense in depth.This matches the pattern used for
PlatformAccount(line 43) whereanonis revoked.🔎 Suggested change
-GRANT ALL ON TABLE public."LocalAccess" TO anon; +REVOKE ALL ON TABLE public."LocalAccess" FROM anon;
128-130: Same privilege concern forSpaceAccess.Consider restricting
anonprivileges here as well, matching the pattern suggested forLocalAccess.🔎 Suggested change
-GRANT ALL ON TABLE public."SpaceAccess" TO anon; +REVOKE ALL ON TABLE public."SpaceAccess" FROM anon;
169-171: Minor: Aliassais misleading forLocalAccessinsert.The alias
sa(typically suggestingSpaceAccess) is used for theLocalAccessinsert. Consider renaming tolafor clarity.🔎 Suggested change
- INSERT INTO public."LocalAccess" as sa (space_id, account_id) values (space_id_, account_id_) + INSERT INTO public."LocalAccess" as la (space_id, account_id) values (space_id_, account_id_) ON CONFLICT (space_id, account_id) DO NOTHING;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/src/dbTypes.tspackages/database/supabase/functions/create-space/index.tspackages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.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.tspackages/database/supabase/functions/create-space/index.ts
🧠 Learnings (8)
📚 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
📚 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-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/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.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/migrations/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.sql
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
packages/database/supabase/functions/create-space/index.ts
📚 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/account.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 PostgreSQL queries within the discourse-graph project, adding `LIMIT 1` to `auth.uid()` lookups provides positive performance impact and should be retained as an optimization technique.
Applied to files:
packages/database/supabase/schemas/account.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.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 (10)
packages/database/src/dbTypes.ts (1)
1-1: Auto-generated types correctly reflect schema migration.The generated types properly capture the new
LocalAccesstable and theSpaceAccessmigration fromaccount_id(number) toaccount_uid(string/UUID). The relationship definitions are consistent with the schema changes.packages/database/supabase/functions/create-space/index.ts (1)
165-181: SpaceAccess upsert correctly migrated to useaccount_uid.The upsert now uses
anonymousUser.id(UUID fromauth.users) foraccount_uid, and theonConflictkey correctly matches the new composite primary key(account_uid, space_id). The idempotent pattern is preserved.packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql (5)
55-65: Data migration logic is sound.The migration correctly:
- Transfers access records to
SpaceAccessonly for accounts with a linked auth user (dg_account IS NOT NULL)- Removes the
editorcolumn fromLocalAccess(now only inSpaceAccess)- Cleans up anonymous platform accounts from
LocalAccesssince they now live inSpaceAccess
189-192: Verify LocalAccess insert policy for new platform accounts.The
local_access_insert_policyrequiresunowned_account_in_shared_space(account_id) OR is_my_account(account_id). For a brand-new platform account (not yet inLocalAccess),unowned_account_in_shared_spacewould returnfalsesince it requires an existingLocalAccessrow.This appears intentional since
upsert_account_in_spaceusesSECURITY DEFINERto bypass RLS when creating accounts. Please confirm this is the expected behavior—direct inserts are only allowed for accounts already tracked in the system.
164-180: View correctly exposes accounts in shared spaces.The
my_accountsview properly joinsLocalAccessandSpaceAccessto show platform accounts that exist in spaces the current user has access to via theiraccount_uid.
67-122: Security utility functions are well-designed.All functions correctly use
STABLE SECURITY DEFINERwithSET search_path = ''to prevent search path injection attacks. The logic properly enforces the new access model:
is_my_account: Validates ownership viadg_accountmy_space_ids/in_space: CheckSpaceAccessbyaccount_uidaccount_in_shared_space/unowned_account_in_shared_space: Use theLocalAccess↔SpaceAccessjoin pattern
124-162: Upsert function correctly handles claimed vs. unclaimed accounts.The logic appropriately differentiates:
- Claimed accounts (
dg_account IS NOT NULL): Creates bothSpaceAccess(auth user access) andLocalAccess(platform account tracking)- Unclaimed accounts: Creates only
LocalAccessThis aligns with the access model where
SpaceAccessgates user-level permissions andLocalAccesstracks platform account presence.packages/database/supabase/schemas/account.sql (3)
314-330: View definition is consistent with the new access model.The
my_accountsview correctly uses theLocalAccess↔SpaceAccessjoin to expose platform accounts visible to the authenticated user.
366-380: LocalAccess RLS policies are properly defined.Policies correctly enforce:
SELECT: User must haveSpaceAccessto the spaceDELETE/INSERT/UPDATE: User must own the account or it must be an unowned account in a shared spaceNote: Direct inserts for brand-new accounts are blocked (by design) since
upsert_account_in_spaceusesSECURITY DEFINERto bypass RLS.
389-399: AgentIdentifier policies correctly differentiate read vs. write access.The
SELECTpolicy usesaccount_in_shared_space(allows viewing identifiers for any account in a shared space), while mutation policies requireunowned_account_in_shared_space OR is_my_account. This correctly prevents modifying another user's claimed account identifiers while allowing visibility for collaboration.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/database/features/getContext.feature (1)
13-13: "Scenario Outline" without Examples table.Both scenarios are labeled
Scenario Outlinebut lack the requiredExamples:table. In Gherkin,Scenario Outlineis used for parameterized tests with an Examples table. If no parameterization is intended, these should be plainScenarioinstead.🔎 Proposed fix
- Scenario Outline: Calling the getContext steps + Scenario: Calling the getContext steps- Scenario Outline: Calling getContext again + Scenario: Calling getContext againAlso applies to: 26-26
🧹 Nitpick comments (4)
packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql (2)
144-149: Type mismatch:user_uuidisVARCHARbutaccount_uidisUUID.The variable
user_uuidis declared asVARCHAR(line 135/144) but is used to insert intoaccount_uidwhich is of typeUUID. While PostgreSQL will implicitly cast, it's cleaner to declareuser_uuidasUUIDfor type consistency. However, since this is a migration file and per learnings should not be modified after application, this can be addressed in the schema file instead.
67-74: Consider usingEXISTSinstead ofCOUNTfor efficiency.
COUNT(id) > 0scans all matching rows to count them, whileEXISTSshort-circuits after finding the first match. Based on learnings, performance optimizations likeLIMIT 1onauth.uid()lookups are valued in this project.🔎 Proposed optimization
CREATE OR REPLACE FUNCTION public.is_my_account(account_id BIGINT) RETURNS boolean STABLE SECURITY DEFINER SET search_path = '' LANGUAGE sql AS $$ - SELECT COUNT(id) > 0 FROM public."PlatformAccount" - WHERE id = account_id AND dg_account = auth.uid(); + SELECT EXISTS ( + SELECT 1 FROM public."PlatformAccount" + WHERE id = account_id AND dg_account = auth.uid() + LIMIT 1 + ); $$;However, since this is a migration file and per learnings should not be modified after application, this optimization should be applied in the schema file instead.
packages/database/supabase/schemas/account.sql (2)
150-151: Type mismatch:user_uuiddeclared asVARCHARbut used asUUID.The variable
user_uuidis declared asVARCHAR(line 151) butdg_accountisUUIDand it's inserted intoaccount_uidwhich is alsoUUID. While PostgreSQL will implicitly cast, declaring asUUIDis cleaner and provides type safety.🔎 Proposed fix
DECLARE platform_ public."Platform"; account_id_ BIGINT; - user_uuid VARCHAR; + user_uuid UUID; BEGINAlso applies to: 160-166
220-227: ConsiderEXISTSoverCOUNTfor better performance.Based on learnings from this project,
LIMIT 1optimizations onauth.uid()lookups provide positive performance impact. Similarly,EXISTSshort-circuits after finding the first match, whileCOUNTmust scan all matching rows.🔎 Proposed optimization
CREATE OR REPLACE FUNCTION public.is_my_account(account_id BIGINT) RETURNS boolean STABLE SECURITY DEFINER SET search_path = '' LANGUAGE sql AS $$ - SELECT COUNT(id) > 0 FROM public."PlatformAccount" - WHERE id = account_id AND dg_account = auth.uid(); + SELECT EXISTS ( + SELECT 1 FROM public."PlatformAccount" + WHERE id = account_id AND dg_account = auth.uid() + LIMIT 1 + ); $$;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/database/features/getContext.featurepackages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.sql
🧰 Additional context used
🧠 Learnings (6)
📓 Common 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."
📚 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/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.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/20251231203152_migrate_space_access_to_user.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/migrations/20251231203152_migrate_space_access_to_user.sqlpackages/database/supabase/schemas/account.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 PostgreSQL queries within the discourse-graph project, adding `LIMIT 1` to `auth.uid()` lookups provides positive performance impact and should be retained as an optimization technique.
Applied to files:
packages/database/supabase/schemas/account.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.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 (6)
packages/database/features/getContext.feature (1)
17-24: Test expectations correctly updated for the new access model.The updated assertions properly reflect the migration: one
SpaceAccessentry for the authenticated user (viaaccount_uid) and oneLocalAccessentry tracking the platform account's presence in the space. This aligns with the removal of anonymous user entries fromSpaceAccessin the migration.packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql (2)
55-59: Data migration correctly populates SpaceAccess from existing records.The migration properly joins
LocalAccesswithPlatformAccountto transfereditorstatus to the newSpaceAccesstable, filtering only accounts that have a linkeddg_account(authenticated user). This ensures only claimed accounts get access entries.
1-6: Clean migration structure for schema transition.The migration properly handles the schema transition by:
- Revoking permissions and dropping policies before restructuring
- Renaming the old table and constraints
- Creating the new table with proper foreign keys
- Migrating data appropriately
- Dropping the legacy
my_account()function at the endThis is a well-organized approach for this type of schema migration.
Also applies to: 203-203
packages/database/supabase/schemas/account.sql (3)
67-95: Well-structured LocalAccess table definition.The
LocalAccesstable correctly establishes the relationship between platform accounts and spaces with:
- Proper composite primary key
(account_id, space_id)- Cascading foreign keys to both
PlatformAccountandSpace- Appropriate grants for
authenticatedandservice_role
356-362: SpaceAccess policies correctly enforce user-owned access.The RLS policies appropriately restrict modifications to only the user's own records via
account_uid = auth.uid(), while allowing SELECT to anyone in the same space viapublic.in_space(space_id).
364-378: LocalAccess RLS policies properly integrate with new access model.The policies correctly use the helper functions:
in_space(space_id)for SELECT visibilityunowned_account_in_shared_space(account_id) OR is_my_account(account_id)for modificationsThis allows users to modify their own records or unowned accounts in shared spaces, which aligns with the described access control requirements.
https://linear.app/discourse-graphs/issue/ENG-1231/migrate-spaceaccess-to-use-user-ids-not-platformaccount-ids
Currently, SpaceAccess uses PlatformAccounts. This was a mistake, as access is granted to users (cross-accounts) not PlatformAccounts.
OTH, we still need to know which PlatformAccounts have access to a given space, even before the corresponding user is created; for this reason I have created a separate LocalAccess table.
Eventually (separate PR) when the user account is created from a platformAccount, we will create the SpaceAccess from the LocalAccess entries for that PlatformAccount. (Separate issue for now.)
Note: db diff uses migra, which gets confused about views, so the db diff is not empty after migration, but it's clearly an artefact. This is something to keep an eye on. pg_schema_diff is the newer tool that supabase is pushing, but it's less complete for now.
Loom: https://www.loom.com/share/d410830db7bc413e96ae38774da998d0
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.