diff --git a/.server-changes/team-role-picker-only-assignable-roles.md b/.server-changes/team-role-picker-only-assignable-roles.md new file mode 100644 index 00000000000..8afe456bc43 --- /dev/null +++ b/.server-changes/team-role-picker-only-assignable-roles.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: improvement +--- + +The Team page's role dropdown no longer lists roles above your own, which were rejected when you picked them. Every other role you could pick before is still there, and roles that need a plan upgrade still appear with a link to upgrade. diff --git a/apps/webapp/app/presenters/TeamPresenter.server.ts b/apps/webapp/app/presenters/TeamPresenter.server.ts index f2e5da61a87..43888af4703 100644 --- a/apps/webapp/app/presenters/TeamPresenter.server.ts +++ b/apps/webapp/app/presenters/TeamPresenter.server.ts @@ -1,6 +1,7 @@ import { getTeamMembersAndInvites } from "~/models/member.server"; import { rbac } from "~/services/rbac.server"; import { getCurrentPlan, getLimit, getPlans } from "~/services/platform.v3.server"; +import { offerableRoleIds as computeOfferableRoleIds } from "~/utils/inviteRoleLadder"; import { BasePresenter } from "./v3/basePresenter.server"; export class TeamPresenter extends BasePresenter { @@ -14,25 +15,53 @@ export class TeamPresenter extends BasePresenter { return; } - const [baseLimit, currentPlan, plans, roles, assignableRoleIds, memberRoleMap] = - await Promise.all([ - getLimit(organizationId, "teamMembers", 100_000_000), - getCurrentPlan(organizationId), - getPlans(), - // RBAC role catalogue (system roles + any org-defined custom - // roles). The default fallback returns []; an installed plugin - // may return the seeded system roles plus any custom roles. - rbac.allRoles(organizationId), - // Plan-gated subset — the Teams page disables dropdown options not - // in this set. Server-side enforcement is independent (setUserRole - // rejects a plan-gated assignment regardless of UI state). - rbac.getAssignableRoleIds(organizationId), - // Per-member current role in a single round-trip. - rbac.getUserRoles( - result.members.map((m) => m.user.id), - organizationId - ), - ]); + const [ + baseLimit, + currentPlan, + plans, + roles, + assignableRoleIds, + memberRoleMap, + viewerRole, + systemRoles, + ] = await Promise.all([ + getLimit(organizationId, "teamMembers", 100_000_000), + getCurrentPlan(organizationId), + getPlans(), + // RBAC role catalogue (system roles + any org-defined custom + // roles). The default fallback returns []; an installed plugin + // may return the seeded system roles plus any custom roles. + rbac.allRoles(organizationId), + // Plan-gated subset — the Teams page disables dropdown options not + // in this set. Server-side enforcement is independent (setUserRole + // rejects a plan-gated assignment regardless of UI state). + rbac.getAssignableRoleIds(organizationId), + // Per-member current role in a single round-trip. + rbac.getUserRoles( + result.members.map((m) => m.user.id), + organizationId + ), + // The viewer's own role, plus the system-role ladder it sits on — + // together these say which roles sit above this viewer. + rbac.getUserRole({ userId, organizationId }), + rbac.systemRoles(organizationId), + ]); + + // Roles to offer this viewer in the picker: the catalogue minus whatever + // the system-role ladder puts strictly above their own role, which is the + // only part picking would always be rejected for. Roles the ladder can't + // place — org-defined custom roles, and every role when the viewer's own + // role is itself custom or missing — stay in, so custom roles keep + // behaving as they always have rather than vanishing from the picker. + // + // Deliberately NOT intersected with `assignableRoleIds` — the two answer + // different questions and the Team page renders them differently. A role + // above the viewer's level is left out of the picker altogether, while a + // role that is merely plan-locked still needs to appear with an upgrade + // link. Merging them would offer a viewer "Owner (upgrade)", inviting + // them to pay for something their own role still would not let them + // assign. + const offerableRoleIds = computeOfferableRoleIds(roles, systemRoles, viewerRole?.id ?? null); const memberRoles = result.members.map((m) => ({ userId: m.user.id, @@ -60,6 +89,7 @@ export class TeamPresenter extends BasePresenter { planSeatLimit, roles, assignableRoleIds, + offerableRoleIds, memberRoles, }; } diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx index 59e833d06e3..0b74f48f162 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx @@ -328,6 +328,7 @@ export default function Page() { planSeatLimit, roles, assignableRoleIds, + offerableRoleIds, memberRoles, canManageMembers, canManageBilling, @@ -499,6 +500,7 @@ export default function Page() { currentRoleId={memberRoleByUserId.get(member.user.id) ?? null} roles={roles} assignableRoleIds={assignableRoleIds} + offerableRoleIds={offerableRoleIds} canManageMembers={canManageMembers} />
@@ -688,29 +690,46 @@ function LeaveRemoveButton({ } // Inline role picker — submits a `_formType=set-role` form via fetcher -// so the change persists without a full page reload. Disabled options -// (and the picker itself) reflect plan gating + manage:members; the -// server's setUserRole enforces both checks again as the source of -// truth, so this is a UI-affordance layer only. +// so the change persists without a full page reload. The picker itself, +// the roles it lists and which of those are selectable all reflect +// manage:members, the viewer's own role and plan gating; the server +// validates the submitted role independently, so this is a +// UI-affordance layer only. +// +// Two different sets narrow the list, and they must stay separate: +// offerableRoleIds — the catalogue minus the roles that sit above the +// viewer on the system-role ladder. Those are left out +// of the list entirely; custom roles, which aren't on +// the ladder, are always in it. +// assignableRoleIds — roles the org's plan allows. A role that is +// offerable but not plan-assignable still shows, +// as "Name (upgrade)" linking to billing. function RolePicker({ memberUserId, currentRoleId, roles, assignableRoleIds, + offerableRoleIds, canManageMembers, }: { memberUserId: string; currentRoleId: string | null; roles: Role[]; assignableRoleIds: string[]; + offerableRoleIds: string[]; canManageMembers: boolean; }) { const organization = useOrganization(); const fetcher = useFetcher<{ ok: boolean; error?: string } | { ok: true }>(); const assignable = new Set(assignableRoleIds); - // With no RBAC plugin installed, the loader returns no roles — + const offerable = new Set(offerableRoleIds); + // The member's current role stays in the list even when it sits above the + // viewer, so the controlled `value` below still resolves to a row and the + // dropdown shows the role the member actually holds. + const visibleRoles = roles.filter((r) => offerable.has(r.id) || r.id === currentRoleId); + // With no RBAC plugin installed the loader returns no roles at all — // render nothing rather than an empty dropdown. - if (roles.length === 0) return null; + if (visibleRoles.length === 0) return null; const isSubmitting = fetcher.state === "submitting"; const error = @@ -723,13 +742,16 @@ function RolePicker({ // kept the old role; without `value` the UI would show the // attempted change). value={currentRoleId ?? ""} - items={roles} + items={visibleRoles} variant="tertiary/small" disabled={!canManageMembers || isSubmitting} dropdownIcon - text={(v) => roles.find((r) => r.id === v)?.name ?? "No role"} + text={(v) => visibleRoles.find((r) => r.id === v)?.name ?? "No role"} setValue={(next) => { if (typeof next !== "string" || next === (currentRoleId ?? "")) return; + // The member's current role is listed even when it sits above the + // viewer, so re-check before submitting. + if (!offerable.has(next)) return; // Upgrade-link rows have a value too (Ariakit needs one to // make the row interactive — without it the Link inside // doesn't even register the click), but they shouldn't diff --git a/apps/webapp/app/utils/inviteRoleLadder.ts b/apps/webapp/app/utils/inviteRoleLadder.ts index 5cf321f157d..30f6f0ee55c 100644 --- a/apps/webapp/app/utils/inviteRoleLadder.ts +++ b/apps/webapp/app/utils/inviteRoleLadder.ts @@ -1,7 +1,13 @@ -// An inviter can only assign a role at or below their own. The systemRoles -// array is in canonical order (highest authority first), so array index drives -// the ladder. Custom roles aren't in the table and are refused. Dependency-free -// so the rule can be unit-tested directly. +// The system roles form a ladder: the systemRoles array is in canonical order +// (highest authority first), so array index gives each role a level. Roles the +// org defined itself are not in that array and have no level at all. +// +// Two callers read the ladder, and they treat a role with no level +// differently. The invite flow (`isAtOrBelow`) requires a level on both sides +// and refuses anything else. The Team page's role picker (`offerableRoleIds`) +// only removes what the ladder positively places above the viewer, so custom +// roles stay offerable. Dependency-free so both rules can be unit-tested +// directly. export type LadderRole = { id: string }; @@ -31,3 +37,51 @@ export function isAtOrBelow( if (inviter === undefined || invited === undefined) return false; return invited <= inviter; } + +/** + * Whether the ladder places `candidateRoleId` strictly above `viewerRoleId`. + * Only ever true when both roles have a level: a custom role on either side + * is not comparable, so it is never "above", and nothing is above a viewer + * whose own role has no level. + */ +function isAbove( + roles: ReadonlyArray, + viewerRoleId: string | null, + candidateRoleId: string +): boolean { + if (!viewerRoleId) return false; + const level = buildRoleLevel(roles); + const viewer = level[viewerRoleId]; + const candidate = level[candidateRoleId]; + if (viewer === undefined || candidate === undefined) return false; + return candidate > viewer; +} + +/** + * The subset of `roles` to offer a user holding `viewerRoleId` in a role + * picker. The narrowing is subtractive: a role is dropped only where the + * ladder puts it strictly above the viewer — the case where picking it would + * always be rejected. Everything the ladder can't place stays offerable: + * + * - org-defined custom roles, which have no level, are always offered; + * - a viewer whose own role has no level (a custom role, or no role at all) + * is offered the whole catalogue, since narrowing to nothing would take + * away their ability to manage members entirely — a worse outcome than + * offering a role the server may go on to refuse. + * + * Knows nothing about plan gating: a role the org's plan does not allow is + * still returned, so a caller that wants to surface it as an upgrade + * affordance can. Callers with no upgrade affordance intersect with their + * plan-assignable set themselves. + * + * `systemRoles` is null when no RBAC plugin is installed — with no ladder + * there is nothing to narrow by, so `roles` comes back as-is. + */ +export function offerableRoleIds( + roles: ReadonlyArray, + systemRoles: ReadonlyArray | null, + viewerRoleId: string | null +): string[] { + if (!systemRoles) return roles.map((r) => r.id); + return roles.filter((r) => !isAbove(systemRoles, viewerRoleId, r.id)).map((r) => r.id); +} diff --git a/apps/webapp/test/inviteRoleLadder.test.ts b/apps/webapp/test/inviteRoleLadder.test.ts index 4c8a01c019d..eebe2b71e02 100644 --- a/apps/webapp/test/inviteRoleLadder.test.ts +++ b/apps/webapp/test/inviteRoleLadder.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { isAtOrBelow } from "../app/utils/inviteRoleLadder.js"; +import { isAtOrBelow, offerableRoleIds } from "../app/utils/inviteRoleLadder.js"; // systemRoles in canonical order: highest authority first. const roles = [{ id: "owner" }, { id: "admin" }, { id: "member" }]; @@ -31,3 +31,71 @@ describe("isAtOrBelow", () => { expect(isAtOrBelow(roles, "custom-role-id", "member")).toBe(false); }); }); + +// Property under test: the picker set is the catalogue minus the roles the +// ladder puts strictly above the viewer. Nothing else is removed — roles with +// no ladder position (org-defined custom roles) stay offerable, and so does +// the whole catalogue when the viewer's own role has no position either. +// Plan gating is a separate concern the caller layers on, so a plan-locked +// role must still come back here — the Team page renders it as an upgrade link. +describe("offerableRoleIds", () => { + const catalogue = [{ id: "owner" }, { id: "admin" }, { id: "member" }, { id: "custom-1" }]; + + it("offers the viewer's own level and below", () => { + expect(offerableRoleIds(catalogue, roles, "admin")).toEqual(["admin", "member", "custom-1"]); + expect(offerableRoleIds(catalogue, roles, "member")).toEqual(["member", "custom-1"]); + }); + + it("leaves roles above the viewer out entirely", () => { + expect(offerableRoleIds(catalogue, roles, "admin")).not.toContain("owner"); + expect(offerableRoleIds(catalogue, roles, "member")).not.toContain("owner"); + expect(offerableRoleIds(catalogue, roles, "member")).not.toContain("admin"); + }); + + it("offers the whole catalogue to a viewer at the top of the ladder", () => { + expect(offerableRoleIds(catalogue, roles, "owner")).toEqual([ + "owner", + "admin", + "member", + "custom-1", + ]); + }); + + it("does not filter on plan gating — a plan-locked role is still offerable", () => { + // `owner` may be unavailable on the org's plan; that is the caller's + // concern, and it still needs the id back to render the upgrade row. + expect(offerableRoleIds(catalogue, roles, "owner")).toContain("owner"); + }); + + it("keeps custom roles, which the ladder can't place above anyone", () => { + expect(offerableRoleIds(catalogue, roles, "owner")).toContain("custom-1"); + expect(offerableRoleIds(catalogue, roles, "admin")).toContain("custom-1"); + expect(offerableRoleIds(catalogue, roles, "member")).toContain("custom-1"); + }); + + it("does not narrow at all for a viewer holding a custom role", () => { + // The ladder can't say what is above a role it doesn't list, so leave the + // picker as it was rather than emptying it and stranding the viewer. + expect(offerableRoleIds(catalogue, roles, "custom-1")).toEqual([ + "owner", + "admin", + "member", + "custom-1", + ]); + }); + + it("does not narrow at all for a roleless viewer or with no ladder", () => { + expect(offerableRoleIds(catalogue, roles, null)).toEqual([ + "owner", + "admin", + "member", + "custom-1", + ]); + expect(offerableRoleIds(catalogue, null, "owner")).toEqual([ + "owner", + "admin", + "member", + "custom-1", + ]); + }); +});