Conversation
|
Updates to Preview Branch (eng-1426-bug-in-permission-functions) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
23b9348 to
10e2212
Compare
mdroidian
left a comment
There was a problem hiding this comment.
Just make sure to look at: https://github.com/DiscourseGraphs/discourse-graph/pull/778/changes#r2807642747
Also, ignore the linting CI, I'm unsure why it is failing, I will look into adding better reporting.
| INSERT INTO public."SpaceAccess" as sa (space_id, account_uid, permissions) | ||
| VALUES (space_id_, user_uid, least(my_permissions_in_space(space_id_), COALESCE(permissions_, 'editor'))) |
There was a problem hiding this comment.
🔴 LEAST(NULL, ...) returns NULL causing NOT NULL constraint violation on SpaceAccess INSERT
When upsert_account_in_space inserts a new SpaceAccess row for a user who has a dg_account (i.e. user_uid IS NOT NULL), the permissions value is computed as least(my_permissions_in_space(space_id_), COALESCE(permissions_, 'editor')). If the calling user (identified by auth.uid() from the JWT) has no SpaceAccess entry for the space, my_permissions_in_space returns NULL (since max() over an empty set is NULL). In PostgreSQL, LEAST(NULL, 'editor') evaluates to NULL, which violates the NOT NULL constraint on SpaceAccess.permissions (packages/database/supabase/schemas/account.sql:108).
Root Cause and Impact
my_permissions_in_space at packages/database/supabase/schemas/account.sql:228-237 returns:
SELECT max(permissions) FROM public."SpaceAccess"
JOIN public.my_user_accounts() ON (account_uid = my_user_accounts)
WHERE space_id=space_id_;When no rows match, max() returns NULL.
The INSERT at line 352-353 then computes:
VALUES (space_id_, user_uid, least(my_permissions_in_space(space_id_), COALESCE(permissions_, 'editor')))LEAST(NULL, 'editor') = NULL → fails with NOT NULL constraint.
The same issue occurs on the ON CONFLICT DO UPDATE path at line 356-357 when permissions_ is not NULL:
ELSE least(my_permissions_in_space(space_id_), permissions_)In the current main flows, callers authenticate as the anonymous space user (who has 'editor' permissions), so this doesn't manifest. However, if any caller invokes these functions with a JWT for a user who is not yet in the space, or with no JWT (service role), the function will crash instead of producing a meaningful error.
The fix should wrap the LEAST call with a COALESCE, e.g.: COALESCE(my_permissions_in_space(space_id_), permissions_) or add an explicit NULL check and raise a proper error.
Prompt for agents
In packages/database/supabase/schemas/account.sql, in the upsert_account_in_space function (lines 346-358), the LEAST() calls with my_permissions_in_space() can return NULL when the calling user has no SpaceAccess entry. This violates the NOT NULL constraint on SpaceAccess.permissions. Fix both the INSERT VALUES clause (line 352-353) and the ON CONFLICT DO UPDATE clause (line 356-357). Either: (a) add a COALESCE around my_permissions_in_space so that NULL falls back to the requested permissions, e.g. COALESCE(my_permissions_in_space(space_id_), COALESCE(permissions_, 'editor')), or (b) add an explicit check before the INSERT that raises a proper error if my_permissions_in_space returns NULL. Apply the same fix in the migration file packages/database/supabase/migrations/20260213155959_upsert_user_permissions.sql at lines 41 and 45.
Was this helpful? React with 👍 or 👎 to provide feedback.
https://linear.app/discourse-graphs/issue/ENG-1426/bug-in-permission-functions
Pointed out by Devin in the group-test branch: there was still an issue with using the obsolete editor field of the SpaceAccess structure in that code.
Surprisingly the bug did not make the function fail, probably because the value is rarely set.
Corrected, and generalized the use of permissions as an input parameter to create_account_in_space.
Also, make sure that one cannot give permissions higher than one's own this way;
but also do not lower the permissions if no change was requested.
https://www.loom.com/share/a7089d0681c84d659c1220a70dbb0c0e