Organization admin roles, space managers, and shareable link branding#1823
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| repo.membership(user.id, orgId).pipe( | ||
| Effect.map((v) => | ||
| v.pipe( | ||
| Option.filter((v) => v.role === "owner" || v.role === "admin"), |
There was a problem hiding this comment.
If there’s any chance organization_members.role has mixed casing in existing data (similar to spaces), it’d be safer to normalize here so admins don’t lose access unexpectedly.
| Option.filter((v) => v.role === "owner" || v.role === "admin"), | |
| Option.filter((v) => { | |
| const role = String(v.role).toLowerCase(); | |
| return role === "owner" || role === "admin"; | |
| }), |
| Option.filter((v) => { | ||
| const role = String(v.role); | ||
| return role === "admin" || role === "Admin"; | ||
| }), |
There was a problem hiding this comment.
This can be simplified and made a bit more future-proof (handles ADMIN/Admin/etc.) by normalizing once.
| Option.filter((v) => { | |
| const role = String(v.role); | |
| return role === "admin" || role === "Admin"; | |
| }), | |
| Option.filter((v) => String(v.role).toLowerCase() === "admin"), |
| .notNull() | ||
| .default("member") | ||
| .$type<"member" | "Admin">(), | ||
| .$type<"admin" | "member">(), |
There was a problem hiding this comment.
Since space_members.role was previously typed as "Admin" | "member" and is now "admin" | "member", it might be worth adding a DB migration to normalize existing rows (e.g. UPDATE space_members SET role = 'admin' WHERE role = 'Admin') to avoid runtime mismatches from legacy data.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| ALTER TABLE `organizations` ADD `shareableLinkIconUrl` varchar(1024); No newline at end of file | |||
There was a problem hiding this comment.
"Admin" space-member roles
The schema type for spaceMembers.role was changed from "member" | "Admin" to "admin" | "member", but the migration only adds the new shareableLinkIconUrl column — it does not UPDATE space_members SET role = 'admin' WHERE role = 'Admin'. Existing rows keep the legacy capitalised value. The code defensively handles this (normalizeSpaceRole explicitly maps "Admin" → "admin", and SpacesPolicy.isAdmin checks for both), but the inconsistency in persisted data grows over time and requires these workarounds to stay indefinitely. Adding a UPDATE statement to this migration (or a separate one) would let those legacy guards be removed cleanly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/database/migrations/0025_demonic_mother_askani.sql
Line: 1
Comment:
**Missing data migration for legacy `"Admin"` space-member roles**
The schema type for `spaceMembers.role` was changed from `"member" | "Admin"` to `"admin" | "member"`, but the migration only adds the new `shareableLinkIconUrl` column — it does not `UPDATE space_members SET role = 'admin' WHERE role = 'Admin'`. Existing rows keep the legacy capitalised value. The code defensively handles this (`normalizeSpaceRole` explicitly maps `"Admin" → "admin"`, and `SpacesPolicy.isAdmin` checks for both), but the inconsistency in persisted data grows over time and requires these workarounds to stay indefinitely. Adding a `UPDATE` statement to this migration (or a separate one) would let those legacy guards be removed cleanly.
How can I resolve this? If you propose a fix, please make it concise.| targetRole: OrganizationRole | null | undefined; | ||
| }) { | ||
| if (!canManageOrganizationMembers(actorRole)) return false; | ||
| if (isOrganizationOwnerTarget({ targetUserId, ownerId, targetRole })) { | ||
| return false; | ||
| } | ||
| if (actorUserId && targetUserId && actorUserId === targetUserId) return false; | ||
| return true; | ||
| } | ||
|
|
||
| export function canManageSpace({ | ||
| organizationRole, | ||
| spaceRole, | ||
| }: { | ||
| organizationRole: OrganizationRole | null | undefined; | ||
| spaceRole: SpaceRole | null | undefined; | ||
| }) { | ||
| return ( | ||
| organizationRole === "owner" || | ||
| organizationRole === "admin" || | ||
| spaceRole === "admin" | ||
| ); | ||
| } | ||
|
|
||
| export function canChangeSpaceMemberRole({ | ||
| canManage, | ||
| targetUserId, | ||
| createdById, | ||
| nextRole, | ||
| }: { | ||
| canManage: boolean; |
There was a problem hiding this comment.
canRemoveOrganizationMember and canChangeOrganizationMemberRole block changes to the owner and to the actor themselves, but do not prevent one admin from removing or demoting a peer admin. As written, Admin A can demote Admin B to "member" (or remove them entirely), and Admin B can do the same to Admin A simultaneously. This creates a race condition between peers and means the owner is the only stable privileged principal.
If the intent is a flat admin tier where peers can fully manage each other this is working as designed, but it is worth an explicit comment, and most multi-admin systems add a rank check — only actors whose role outranks the target can modify that target — to prevent this class of conflict.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/permissions/roles.ts
Line: 155-185
Comment:
**Admin-on-admin removal and role demotion is permitted**
`canRemoveOrganizationMember` and `canChangeOrganizationMemberRole` block changes to the owner and to the actor themselves, but do not prevent one admin from removing or demoting a peer admin. As written, Admin A can demote Admin B to `"member"` (or remove them entirely), and Admin B can do the same to Admin A simultaneously. This creates a race condition between peers and means the owner is the only stable privileged principal.
If the intent is a flat admin tier where peers can fully manage each other this is working as designed, but it is worth an explicit comment, and most multi-admin systems add a rank check — only actors whose role outranks the target can modify that target — to prevent this class of conflict.
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai pleasw re-review the pr |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| @@ -147,11 +184,8 @@ export async function getDashboardData(user: typeof userSelectProps) { | |||
| and( | |||
| eq(spaces.organizationId, activeOrganizationId), | |||
| or( | |||
There was a problem hiding this comment.
Space visibility still looks creator/public/member-only here. If org admin users are intended to manage spaces they aren’t explicitly added to, they still won’t see private spaces from this query (so currentUserCanManage can’t help). Consider broadening the or(...) when currentOrganizationRole is admin|owner, or skipping the visibility filter entirely for org managers.
| throw new Error("Settings are required"); | ||
| } | ||
|
|
||
| if (!user.activeOrganizationId) { |
There was a problem hiding this comment.
Minor nit: the org fetch a few lines below is effectively a PK lookup; adding .limit(1) there would keep it consistent with most other selects in the codebase (and avoid scanning if a unique constraint ever changes).
|
|
||
| yield* imageUploads.applyUpdate({ | ||
| payload: Option.some({ | ||
| contentType: file.type, |
There was a problem hiding this comment.
File.type is client-controlled; since you already validate via allowedImageTypes, it seems safer to normalize what you persist/send.
| contentType: file.type, | |
| contentType: file.type.toLowerCase(), |
| @@ -0,0 +1 @@ | |||
| ALTER TABLE `organizations` ADD `shareableLinkIconUrl` varchar(1024); No newline at end of file | |||
There was a problem hiding this comment.
Given the space-member role normalization to lowercase elsewhere in this PR, it might be worth adding a follow-up migration to normalize existing space_members.role values (Admin -> admin). The code is defensive today, but a one-time cleanup would prevent legacy values from sticking around indefinitely.
| await db() | ||
| .update(spaceMembers) | ||
| .set({ role: "admin" }) | ||
| .where(sql`LOWER(${spaceMembers.role}) = 'admin'`); |
There was a problem hiding this comment.
LOWER(role) = 'admin' matches already-normalized admin rows too, so this will keep rewriting all admin rows on every deploy. Adding a <> 'admin' guard would keep the backfill to legacy casing only.
| .where(sql`LOWER(${spaceMembers.role}) = 'admin'`); | |
| .where( | |
| sql`${spaceMembers.role} <> 'admin' AND LOWER(${spaceMembers.role}) = 'admin'`, | |
| ); |
| await requireOrganizationSettingsManager(user.id, spaceId); | ||
| } else { | ||
| const access = await getSpaceAccess(user.id, spaceId); | ||
| if (!access?.canManage) { |
There was a problem hiding this comment.
Worth double-checking canManage is the intended gate here. Since the query below already restricts to videos.ownerId = user.id, allowing regular space members to add their own videos to a space should be safe if that’s a desired workflow.
| await requireOrganizationSettingsManager(user.id, spaceId); | ||
| } else { | ||
| const access = await getSpaceAccess(user.id, spaceId); | ||
| if (!access?.canManage) { |
There was a problem hiding this comment.
Same note as add-videos.ts: this changes removal to manager-only. Since you only remove videos the user owns, it might be worth confirming the stricter permission is intentional (vs allowing any space member to remove their own from the space).
| if (spaceOrOrg.variant === "space") | ||
| yield* spacesPolicy.isMember(spaceOrOrg.space.id); | ||
| else yield* orgsPolicy.isOwner(spaceOrOrg.organization.id); | ||
| yield* canManageSpaceOrOrg(folder.spaceId); |
There was a problem hiding this comment.
This tightens folder edit (and the new canCreateIn) from “any space member” to “space admin or org admin/owner”. If regular members are expected to help organize folders, this is a behavior change that might surprise folks; if it’s intentional, all good, but it may be worth sanity-checking call sites/UI expectations.
1 participant
adminrole (alongsideownerandmember) with shared settings/member management where appropriate, while keeping billing and org deletion owner-only where it already was scoped.admin(including Loom import and web-domain schema) and updates the desktop API contract for the new org role.TextTrackCueListdoes not exposeitem().Greptile Summary
This PR introduces an org
adminrole (betweenownerandmember), wires rank-based permission helpers across server actions and Effect policies for orgs/spaces/folders, normalises space-member roles to lowercaseadmin, and adds Pro shareable-link branding (custom icon, org-icon preference, Cap logo toggle) with upload/remove server actions and a new share-page branding component.roles.ts,authorization.ts,space-authorization.ts,update-member-role.ts,remove-member.ts): centralisedcan*predicates with outranks-based guards are consistently applied across all org and space management actions;OrganisationsRepoand both backend policies are updated to resolve effective roles correctly.shareable-link-icon.ts,ShareableLinkIcon.tsx, share page): Pro-gated upload/remove/preference actions backed by the existingImageUploadseffect service;getSharePageBrandingcorrectly short-circuits for non-Pro orgs.space_member_role_backfill.ts,migrate.ts): backfill script normalises legacy'Admin'to'admin'inspace_membersand is now hooked intomigrateDb();FoldersPolicy.canEditand the newcanCreateInnow require admin/owner access, a tighter gate than the previous membership-only check.Confidence Score: 5/5
Safe to merge; all auth changes are additive or tightening, with no regressions in core ownership/billing guards.
The new permission layer is well-structured and consistently applied across all org and space management actions. The three flagged items are scoped to intentional-but-undocumented behaviour changes rather than broken access controls or data loss paths.
packages/database/migrations/space_member_role_backfill.ts (backfill runs on every deploy and touches already-correct rows), packages/web-backend/src/Folders/FoldersPolicy.ts and apps/web/actions/spaces/add-videos.ts (tighter-than-before gates for regular space members worth confirming as intentional).
Important Files Changed
Comments Outside Diff (2)
apps/web/app/(org)/dashboard/dashboard-data.ts, line 183-195 (link)The space visibility query requires the user to have created the space, have the space be public, or already be a
space_membersrow. An org admin who was never explicitly added to a private space will not see it in this query —currentUserCanManagewill never betruefor those spaces because the rows are simply absent from the result set.The PR description states admins should be able to manage spaces consistently. If that includes discovering and managing private spaces without being added first, the
ORclause needs an additional branch. If the intended behaviour is that admins can only manage spaces they can already see, a code comment clarifying this design decision would help future readers.Prompt To Fix With AI
apps/web/actions/organization/settings.ts, line 60-65 (link).limit(1)on what is effectively a primary-key lookup. Semantically harmless sinceidis unique, but every other similar select in the codebase uses.limit(1).Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(web): cover peer admin rules in org..." | Re-trigger Greptile