ENG-1834 Managing group admin rights of other members - #1469
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const response = await client | ||
| .from("group_membership") | ||
| .update({ admin }) | ||
| .eq("member_id", memberId) | ||
| .eq("group_id", groupId) | ||
| .select(); |
There was a problem hiding this comment.
🔴 Demotions can orphan a group
A stale page or concurrent requests let setGroupAdmin demote the final administrator. The update policy checks authority but preserves no administrator. The group then loses invitations and admin management.
Learn more
The last-administrator check exists only in the rendered rows. It uses a count fetched before the user acts, while setGroupAdmin performs an unconditional authorized update. The update policy verifies that the caller is currently an administrator but permits self-demotion. Separate requests can therefore pass the UI guard and remove every administrator.
Example: Alice and Bob are administrators. Both load the page while numAdmins is 2. Bob is demoted first, then Alice uses her still-visible self-demotion control. Both updates succeed, leaving zero administrators.
Recommended fix: Enforce the invariant in the database through one atomic RPC or trigger used by every demotion and membership deletion path. Serialize mutations for each group_id, recount administrators inside that transaction, and reject any mutation that would reduce the count to zero.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Real but self-removal is very rare, and concurrent self-removal is extremely unlikely.
There was a problem hiding this comment.
(And the real solution is a check in the database, which I think is out of scope.)
| pseudoAccounts.map((pseudoAccount) => { | ||
| const memberId = pseudoAccount.dg_account; |
There was a problem hiding this comment.
🟡 Person members lack admin controls
When a person account joins, buildMemberRows omits it because my_pseudo_accounts exposes only anonymous accounts. Administrators get no toggle to grant or revoke that member's rights.
Learn more
Group invitations insert the authenticated user's ID directly into group_membership, so both anonymous space accounts and person accounts can become members. The page derives controls only from my_pseudo_accounts, whose SQL view requires pa.agent_type = 'anonymous'. A person membership can contribute to numAdmins, but it never produces a visible row or control.
Example: Priya signs in with a person account and accepts a member invitation. Her membership exists with admin = false, but the group page lists no row for Priya. An existing administrator cannot promote her.
Recommended fix: Build this screen from all group_membership rows, then attach optional space or profile display data. Add coverage for promoting and demoting a person member with no anonymous pseudo-account row.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
We do not have person accounts yet.
26041f5 to
3452ae6
Compare
3452ae6 to
29144eb
Compare
29144eb to
6594384
Compare
This branch was successfully deployed
2 participants
https://entire.io/gh/DiscourseGraphs/discourse-graph/trails/4
Reviewer brief
The admin control shows as a column of checkboxes only for admin users. Non-admin view is as before.
Verification
Loom and unit tests
Loom video
https://www.loom.com/share/7b04f37e714b42729d0a11e7d82c8654
Scope check
$scope-checkagainst ENG-1834 and the final diff.Done When:buildMemberRowstightens the removal rule so a sole admin can no longer remove their own membership (the previous inline condition allowed it, contradicting its own comment).canSetAdmin); leaving the removal path open would let the sole admin empty the group of admins by another route.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.The reviewer noticed there is a way to remove all admins from a group (making it irrecoverable) with a race condition. I think it's too rare to bother, but there should be a database check at some point.
Other considerations:
The last-admin guard is client-side only. buildMemberRows withholds both the admin checkbox and the Remove button from the last admin, so the UI cannot leave a group with zero admins. Nothing enforces this below that: RLS accepts a self-demotion or self-removal by the last admin, and two admins demoting each other concurrently both succeed. A zero-admin group cannot be repaired without the service role, because is_group_admin is then false for everyone and group_membership_insert_policy only allows inserts into a group that does not yet exist. The fix would be a BEFORE UPDATE / BEFORE DELETE trigger on group_membership. Judged out of scope here: the risk is low at current group sizes, and it is a packages/database change rather than a UI one. Worth its own ticket if groups get larger or more admins per group become normal.
This also tightened an existing rule. The removal condition previously read (isAdmin && (numAdmins > 1 || !isMe)) || isMe, where the trailing clause meant the last admin could always remove themselves and the numAdmins > 1 guard never applied. Self-removal is now scoped to non-admins.
Admins are counted over group_membership, but person members are still not listed. The count drives the guard, so it has to cover everyone who can hold admin rights. Counting it over my_pseudo_accounts would have missed person members, since that view ends WHERE pa.agent_type = 'anonymous', and the viewer's own isAdmin already came from group_membership. Two halves of one permission decision over two populations is the kind of thing that fails silently. Displaying person members is a separate change: the view cannot return them, and it raises questions this ticket does not answer, such as what fills the Space column for a member with no space. The unit test "frees the guard for an admin who holds no listed space row" pins the behavior person accounts will need, so the guard is already correct when they land.
https://linear.app/discourse-graphs/issue/ENG-1834/managing-group-admin-rights-of-other-members