Skip to content

Commit 0d5f19d

Browse files
committed
fix(sso): self-heal half-provisioned JIT membership role
ensureOrgMember's create + rbac.setUserRole + compensating delete are not transactional (the RBAC plugin writes on its own connection). The common single-failure case already recovers — a failed setUserRole deletes the row, so the next login retries cleanly. But if the compensating delete ALSO fails, the placeholder MEMBER row is orphaned and the findFirst no-op short-circuits every future login, stranding the user on the wrong role. When an existing membership is found and a JIT role is requested, complete the assignment if (and only if) the RBAC layer shows no role assigned. Gated on 'no role assigned' so it can never demote a deliberately-set role; best-effort so it never throws or rolls back a valid pre-existing membership. Addresses PR #3911 review.
1 parent 44a124f commit 0d5f19d

1 file changed

Lines changed: 45 additions & 0 deletions

File tree

apps/webapp/app/models/orgMember.server.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,35 @@ export type EnsureOrgMemberParams = {
1414

1515
export type EnsureOrgMemberResult = { created: boolean; orgMemberId: string };
1616

17+
// Completes a JIT role assignment for an ALREADY-existing membership whose
18+
// RBAC role never got applied. This is a no-op when a role is already
19+
// assigned, so it can never demote a deliberately-set role — it only fills
20+
// in the gap left by an interrupted provision (see `ensureOrgMember`). Always
21+
// best-effort: a valid membership already exists, so a failure here is logged
22+
// and swallowed rather than thrown.
23+
async function healMissingRoleAssignment(params: {
24+
userId: string;
25+
organizationId: string;
26+
roleId: string;
27+
source: EnsureOrgMemberParams["source"];
28+
}): Promise<void> {
29+
const { userId, organizationId, roleId, source } = params;
30+
31+
const currentRole = await rbac.getUserRole({ userId, organizationId });
32+
if (currentRole !== null) return;
33+
34+
const result = await rbac.setUserRole({ userId, organizationId, roleId });
35+
if (!result.ok) {
36+
logger.warn("ensureOrgMember.setUserRole failed while healing unassigned membership", {
37+
source,
38+
userId,
39+
organizationId,
40+
roleId,
41+
error: result.error,
42+
});
43+
}
44+
}
45+
1746
// Idempotent OrgMember upsert. If the (userId, organizationId) row
1847
// already exists this is a no-op (returns `{ created: false }`); we do
1948
// NOT touch the existing role to avoid demoting a user that JIT happens
@@ -33,6 +62,22 @@ export async function ensureOrgMember(
3362
select: { id: true },
3463
});
3564
if (existing) {
65+
// Existing membership is normally a pure no-op: we don't re-touch the
66+
// role, since a user JIT fires for again may have been deliberately
67+
// promoted and must not be demoted back to the JIT default.
68+
//
69+
// The one exception is self-healing a half-provisioned row. The create +
70+
// setUserRole + compensating delete below are not transactional (the RBAC
71+
// plugin writes on its own connection, so a single DB transaction isn't
72+
// possible). If setUserRole failed AND that compensating delete also
73+
// failed, the placeholder MEMBER row is orphaned — and this findFirst
74+
// would short-circuit every future login, stranding the user on the
75+
// placeholder role forever. So when a JIT role is requested but the RBAC
76+
// layer shows no role assigned, complete the assignment now. It's gated on
77+
// "no role assigned", so it can never demote a real one.
78+
if (roleId !== null) {
79+
await healMissingRoleAssignment({ userId, organizationId, roleId, source });
80+
}
3681
return { created: false, orgMemberId: existing.id };
3782
}
3883

0 commit comments

Comments
 (0)