Organization admin roles, space managers, and shareable link branding#1823
Organization admin roles, space managers, and shareable link branding#1823richiemcilroy wants to merge 28 commits into
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.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
adminorg role sitting betweenownerandmember, wires permission helpers through server actions, Effect policies, and dashboard data, and adds Pro-tier shareable-link branding (custom icon, Cap-logo toggle) alongside a share-page fix forTextTrackCueListiteration.roles.ts,authorization.ts,space-authorization.ts): newgetEffectiveOrganizationRole/getEffectiveSpaceRolehelpers unify role resolution; server actions are updated to callrequireOrganizationSettingsManagerorrequireSpaceManagerinstead of raw owner checks.remove-member,update-member-role,add/remove-videos, and space member mutations all gain proper admin-aware auth;remove-membernow wraps space-membership cleanup in a transaction.shareable-link-icon.tsadds Pro-gated upload/remove/preference actions;page.tsxrendersSharePageBrandingwith a hard Pro gate so non-pro orgs always show the Cap logo regardless of DB settings.Confidence Score: 4/5
The change is broadly safe; the core auth logic is well-tested and the Pro gate on branding is correctly enforced server-side. The main areas to verify before merging are the admin visibility gap for private spaces and the symmetric admin-on-admin permission model.
The permission helpers are well-structured and backed by a new test suite. The main concerns are that org admins cannot discover private spaces they have not been explicitly added to (defeating the stated "admins can manage spaces" goal for undiscovered spaces) and that peer admins can demote or remove each other, which may or may not be the intended design. The missing data-normalisation migration for legacy "Admin" space-member rows leaves defensive code in place indefinitely but does not break functionality. No auth bypasses or data-loss paths were found.
apps/web/app/(org)/dashboard/dashboard-data.ts (space visibility query for org admins) and apps/web/lib/permissions/roles.ts (admin-on-admin policy decision)
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 (1): Last reviewed commit: "test(web): extend role permission and br..." | Re-trigger Greptile