From ba356d017c1a941ebeaf36c6eddbf0443e7ada50 Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Mon, 28 Sep 2026 13:52:49 -0700 Subject: [PATCH 1/2] feat(web): allow promoting pending members to owner Role changes were rejected unless the member was active, both in the membership service and in the members table action menu. Pending members (added but never signed in) can now be promoted or demoted; suspended members still cannot. The last-owner guards count only active owners, so they now apply only when the target itself is an active owner. Otherwise demoting or removing a pending owner while you are the sole active owner would be refused. Co-Authored-By: Claude Fable 5.1 --- .../settings/members/membersTableActions.tsx | 15 +++--- .../web/src/features/membership/errors.ts | 6 +-- .../membership/membership.service.test.ts | 49 +++++++++++++++++-- .../features/membership/membership.service.ts | 27 +++++++--- packages/web/src/lib/errorCodes.ts | 2 +- 5 files changed, 76 insertions(+), 23 deletions(-) diff --git a/packages/web/src/app/(app)/settings/members/membersTableActions.tsx b/packages/web/src/app/(app)/settings/members/membersTableActions.tsx index 6d119015f..85d26477b 100644 --- a/packages/web/src/app/(app)/settings/members/membersTableActions.tsx +++ b/packages/web/src/app/(app)/settings/members/membersTableActions.tsx @@ -89,11 +89,15 @@ const getDialogCopy = (action: MemberAction, row: TableRowData) => { const name = getDisplayName(row); switch (action) { - case "promote": + case "promote": { + const isPending = row.kind === "member" && row.suspendedAt == null && row.lastActiveAt == null; return { title: "Promote to Owner", - description: `Are you sure you want to promote ${name} to owner? They will have full administrative access.`, + description: isPending + ? `Are you sure you want to promote ${name} to owner? They will have full administrative access once they sign in.` + : `Are you sure you want to promote ${name} to owner? They will have full administrative access.`, }; + } case "demote": return { title: "Demote to Member", @@ -156,8 +160,7 @@ export const MembersTableActions = ({ const isCurrentUser = row.kind === "member" && row.id === currentUserId; const isSuspended = row.kind === "member" && row.suspendedAt != null; const isActiveMember = row.kind === "member" && row.suspendedAt == null && row.lastActiveAt != null; - const isLastActiveOwner = row.kind === "member" - && !isSuspended + const isLastActiveOwner = isActiveMember && row.role === OrgRole.OWNER && activeOwnerCount <= 1; const scimDisabledTitle = scimEnabled ? "SCIM provisioning is enabled" : undefined; @@ -308,7 +311,7 @@ export const MembersTableActions = ({ )} {row.kind === "member" && ( <> - {isActiveMember && row.role === OrgRole.MEMBER && ( + {!isSuspended && row.role === OrgRole.MEMBER && ( )} - {isActiveMember && row.role === OrgRole.OWNER && ( + {!isSuspended && row.role === OrgRole.OWNER && ( ({ message: "Cannot demote the last owner. Promote another member to owner first.", }); -export const memberNotActiveError = (): ServiceError => ({ +export const memberSuspendedError = (): ServiceError => ({ statusCode: StatusCodes.BAD_REQUEST, - errorCode: ErrorCode.MEMBER_NOT_ACTIVE, - message: "Only active members can be promoted or demoted.", + errorCode: ErrorCode.MEMBER_SUSPENDED, + message: "Suspended members cannot be promoted or demoted. Reactivate the member first.", }); // When SCIM is enabled the IdP is the source of truth for membership, so paths diff --git a/packages/web/src/features/membership/membership.service.test.ts b/packages/web/src/features/membership/membership.service.test.ts index dbe48db93..30eade54f 100644 --- a/packages/web/src/features/membership/membership.service.test.ts +++ b/packages/web/src/features/membership/membership.service.test.ts @@ -242,7 +242,7 @@ describe('removeMember', () => { }); test('blocks removing the last active owner', async () => { - prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, suspendedAt: null })); + prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, suspendedAt: null, lastActiveAt: ACTIVE_AT })); prisma.userToOrg.count.mockResolvedValue(1); const result = await removeMember(ORG_ID, USER_ID, { actor: ACTOR }); @@ -252,8 +252,19 @@ describe('removeMember', () => { expect(prisma.userToOrg.delete).not.toHaveBeenCalled(); }); + test('allows removing a pending owner without applying the last-owner guard', async () => { + prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, suspendedAt: null, lastActiveAt: null })); + prisma.userToOrg.count.mockResolvedValue(1); + + const result = await removeMember(ORG_ID, USER_ID, { actor: ACTOR }); + + expect(result).toBeNull(); + expect(prisma.userToOrg.count).not.toHaveBeenCalled(); + expect(prisma.userToOrg.delete).toHaveBeenCalled(); + }); + test('allows removing an owner when others remain', async () => { - prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, suspendedAt: null })); + prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, suspendedAt: null, lastActiveAt: ACTIVE_AT })); prisma.userToOrg.count.mockResolvedValue(2); const result = await removeMember(ORG_ID, USER_ID, { actor: ACTOR }); @@ -306,13 +317,41 @@ describe('setMemberRole', () => { expect(prisma.userToOrg.update).not.toHaveBeenCalled(); }); - test('blocks promoting a pending member', async () => { + test('promotes a pending member to owner and audits it', async () => { prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.MEMBER, lastActiveAt: null })); const result = await setMemberRole(ORG_ID, USER_ID, OrgRole.OWNER, { actor: ACTOR }); + expect(result).toBeNull(); + expect(prisma.userToOrg.update).toHaveBeenCalledWith( + expect.objectContaining({ data: { role: OrgRole.OWNER } }), + ); + expect(mocks.createAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'org.member_promoted_to_owner' })); + }); + + test('demotes a pending owner without applying the last-owner guard', async () => { + prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ role: OrgRole.OWNER, lastActiveAt: null })); + prisma.userToOrg.count.mockResolvedValue(1); + + const result = await setMemberRole(ORG_ID, USER_ID, OrgRole.MEMBER, { actor: ACTOR }); + + expect(result).toBeNull(); + expect(prisma.userToOrg.count).not.toHaveBeenCalled(); + expect(prisma.userToOrg.update).toHaveBeenCalledWith( + expect.objectContaining({ data: { role: OrgRole.MEMBER } }), + ); + }); + + test('blocks promoting a suspended member', async () => { + prisma.userToOrg.findUnique.mockResolvedValue(makeMembership({ + role: OrgRole.MEMBER, + suspendedAt: SUSPENDED_AT, + })); + + const result = await setMemberRole(ORG_ID, USER_ID, OrgRole.OWNER, { actor: ACTOR }); + expect(isServiceError(result)).toBe(true); - expect((result as ServiceError).errorCode).toBe(ErrorCode.MEMBER_NOT_ACTIVE); + expect((result as ServiceError).errorCode).toBe(ErrorCode.MEMBER_SUSPENDED); expect(prisma.userToOrg.update).not.toHaveBeenCalled(); }); @@ -326,7 +365,7 @@ describe('setMemberRole', () => { const result = await setMemberRole(ORG_ID, USER_ID, OrgRole.MEMBER, { actor: ACTOR }); expect(isServiceError(result)).toBe(true); - expect((result as ServiceError).errorCode).toBe(ErrorCode.MEMBER_NOT_ACTIVE); + expect((result as ServiceError).errorCode).toBe(ErrorCode.MEMBER_SUSPENDED); expect(prisma.userToOrg.update).not.toHaveBeenCalled(); }); diff --git a/packages/web/src/features/membership/membership.service.ts b/packages/web/src/features/membership/membership.service.ts index 5004a6122..4a04baa46 100644 --- a/packages/web/src/features/membership/membership.service.ts +++ b/packages/web/src/features/membership/membership.service.ts @@ -8,7 +8,7 @@ import { notFound, type ServiceError } from "@/lib/serviceError"; import { isServiceError } from "@/lib/utils"; import { __unsafePrisma as prisma } from "@/prisma"; import { OrgRole, Prisma, type UserToOrg } from "@sourcebot/db"; -import { lastOwnerDemoteError, lastOwnerError, memberNotActiveError, seatLimitReached } from "./errors"; +import { lastOwnerDemoteError, lastOwnerError, memberSuspendedError, seatLimitReached } from "./errors"; export interface EnsureActiveMemberOptions { actor: AuditActor; @@ -160,7 +160,7 @@ export const removeMember = async ( return notFound("Member not found in this organization"); } - if (target.role === OrgRole.OWNER && target.suspendedAt === null) { + if (isActiveOwner(target)) { if ((await countActiveOwners(tx, orgId)) <= 1) { return lastOwnerError(reason); } @@ -194,9 +194,11 @@ export interface SetMemberRoleOptions { } /** - * Changes a member's role (no-op when unchanged). No session/token revocation: - * role is resolved from the DB on every request, so a change takes effect on the - * member's next request. Seats are unaffected, so no lighthouse sync. + * Changes a member's role (no-op when unchanged). Pending members can be + * promoted or demoted so an owner can be lined up before they first sign in; + * suspended members cannot. No session/token revocation: role is resolved from + * the DB on every request, so a change takes effect on the member's next + * request. Seats are unaffected, so no lighthouse sync. */ export const setMemberRole = async ( orgId: number, @@ -220,12 +222,12 @@ export const setMemberRole = async ( return null; } - if (target.suspendedAt !== null || target.lastActiveAt === null) { - return memberNotActiveError(); + if (target.suspendedAt !== null) { + return memberSuspendedError(); } const isDemotionFromOwner = target.role === OrgRole.OWNER && role !== OrgRole.OWNER; - if (isDemotionFromOwner && target.suspendedAt === null) { + if (isDemotionFromOwner && isActiveOwner(target)) { if ((await countActiveOwners(tx, orgId)) <= 1) { return lastOwnerDemoteError(); } @@ -362,6 +364,15 @@ export const setMembershipSuspended = async ( } }; +// Pending owners have never signed in and are excluded from `countActiveOwners`, +// so the last-owner guards only apply when the target itself is an active owner. +// Otherwise removing or demoting a pending owner would be blocked by the actor's +// own seat being the only active one counted. +const isActiveOwner = (membership: UserToOrg): boolean => + membership.role === OrgRole.OWNER + && membership.suspendedAt === null + && membership.lastActiveAt !== null; + const countActiveOwners = (tx: Prisma.TransactionClient, orgId: number): Promise => tx.userToOrg.count({ where: { diff --git a/packages/web/src/lib/errorCodes.ts b/packages/web/src/lib/errorCodes.ts index 766842b9f..b55053d3f 100644 --- a/packages/web/src/lib/errorCodes.ts +++ b/packages/web/src/lib/errorCodes.ts @@ -37,7 +37,7 @@ export enum ErrorCode { INVALID_GIT_REF = 'INVALID_GIT_REF', LAST_OWNER_CANNOT_BE_DEMOTED = 'LAST_OWNER_CANNOT_BE_DEMOTED', LAST_OWNER_CANNOT_BE_REMOVED = 'LAST_OWNER_CANNOT_BE_REMOVED', - MEMBER_NOT_ACTIVE = 'MEMBER_NOT_ACTIVE', + MEMBER_SUSPENDED = 'MEMBER_SUSPENDED', API_KEY_USAGE_DISABLED = 'API_KEY_USAGE_DISABLED', OAUTH_INSUFFICIENT_SCOPE = 'OAUTH_INSUFFICIENT_SCOPE', MCP_SERVER_ALREADY_EXISTS = 'MCP_SERVER_ALREADY_EXISTS', From ad4576301205a07b7f55535d6f4c89041410ed3e Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Mon, 28 Sep 2026 13:53:01 -0700 Subject: [PATCH 2/2] chore: add changelog entry for #1694 Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45f4d08c7..c1f683d90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Added login wall for code search for Ask GitHub. [#1680](https://github.com/sourcebot-dev/sourcebot/pull/1680) +- [EE] Added support for promoting pending members to owner from the members table. [#1694](https://github.com/sourcebot-dev/sourcebot/pull/1694) ### Removed - Removed the Ask Sourcebot first-visit tutorial banner. [#1675](https://github.com/sourcebot-dev/sourcebot/pull/1675)