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) 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',