From 50dff4e0326b3b474ea30cea6d12c358b21a26fe Mon Sep 17 00:00:00 2001 From: Brandon Treston Date: Thu, 11 Dec 2025 10:30:05 -0500 Subject: [PATCH] [PM-28422] Client batching for member actions (#17805) * add member action batching, feature flag, and implement batching for reinvite * clean up, fix tests, remove redundant tests * cleanup * clean up tests * bump cloud limit to 8k --- .../common/people-table-data-source.ts | 2 +- .../members/members.component.ts | 2 +- .../member-actions.service.spec.ts | 343 +++++++++++++++--- .../member-actions/member-actions.service.ts | 89 ++++- 4 files changed, 378 insertions(+), 58 deletions(-) diff --git a/apps/web/src/app/admin-console/common/people-table-data-source.ts b/apps/web/src/app/admin-console/common/people-table-data-source.ts index 0228edb1e8c..9ac370d8c0d 100644 --- a/apps/web/src/app/admin-console/common/people-table-data-source.ts +++ b/apps/web/src/app/admin-console/common/people-table-data-source.ts @@ -24,7 +24,7 @@ export const MaxCheckedCount = 500; * Maximum for bulk reinvite operations when the IncreaseBulkReinviteLimitForCloud * feature flag is enabled on cloud environments. */ -export const CloudBulkReinviteLimit = 4000; +export const CloudBulkReinviteLimit = 8000; /** * Returns true if the user matches the status, or where the status is `null`, if the user is active (not revoked). diff --git a/apps/web/src/app/admin-console/organizations/members/members.component.ts b/apps/web/src/app/admin-console/organizations/members/members.component.ts index ac25278a636..51a2a6dafc0 100644 --- a/apps/web/src/app/admin-console/organizations/members/members.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/members.component.ts @@ -443,7 +443,7 @@ export class MembersComponent extends BaseMembersComponent try { const result = await this.memberActionsService.bulkReinvite( organization, - filteredUsers.map((user) => user.id), + filteredUsers.map((user) => user.id as UserId), ); if (!result.successful) { diff --git a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts index e856ab7afd1..80a330b0db1 100644 --- a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts +++ b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts @@ -4,6 +4,7 @@ import { of } from "rxjs"; import { OrganizationUserApiService, OrganizationUserBulkResponse, + OrganizationUserService, } from "@bitwarden/admin-console/common"; import { OrganizationUserType, @@ -22,9 +23,8 @@ import { newGuid } from "@bitwarden/guid"; import { KeyService } from "@bitwarden/key-management"; import { OrganizationUserView } from "../../../core/views/organization-user.view"; -import { OrganizationUserService } from "../organization-user/organization-user.service"; -import { MemberActionsService } from "./member-actions.service"; +import { REQUESTS_PER_BATCH, MemberActionsService } from "./member-actions.service"; describe("MemberActionsService", () => { let service: MemberActionsService; @@ -308,41 +308,308 @@ describe("MemberActionsService", () => { }); describe("bulkReinvite", () => { - const userIds = [newGuid(), newGuid(), newGuid()]; + const userIds = [newGuid() as UserId, newGuid() as UserId, newGuid() as UserId]; - it("should successfully reinvite multiple users", async () => { - const mockResponse = { - data: userIds.map((id) => ({ - id, - error: null, - })), - continuationToken: null, - } as ListResponse; - organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse); - - const result = await service.bulkReinvite(mockOrganization, userIds); - - expect(result).toEqual({ - successful: mockResponse, - failed: [], + describe("when feature flag is false", () => { + beforeEach(() => { + configService.getFeatureFlag$.mockReturnValue(of(false)); + }); + + it("should successfully reinvite multiple users", async () => { + const mockResponse = new ListResponse( + { + data: userIds.map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse); + + const result = await service.bulkReinvite(mockOrganization, userIds); + + expect(result.failed).toEqual([]); + expect(result.successful).toBeDefined(); + expect(result.successful).toEqual(mockResponse); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith( + organizationId, + userIds, + ); + }); + + it("should handle bulk reinvite errors", async () => { + const errorMessage = "Bulk reinvite failed"; + organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue( + new Error(errorMessage), + ); + + const result = await service.bulkReinvite(mockOrganization, userIds); + + expect(result.successful).toBeUndefined(); + expect(result.failed).toHaveLength(3); + expect(result.failed[0]).toEqual({ id: userIds[0], error: errorMessage }); }); - expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith( - organizationId, - userIds, - ); }); - it("should handle bulk reinvite errors", async () => { - const errorMessage = "Bulk reinvite failed"; - organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue( - new Error(errorMessage), - ); + describe("when feature flag is true (batching behavior)", () => { + beforeEach(() => { + configService.getFeatureFlag$.mockReturnValue(of(true)); + }); + it("should process users in a single batch when count equals REQUESTS_PER_BATCH", async () => { + const userIdsBatch = Array.from({ length: REQUESTS_PER_BATCH }, () => newGuid() as UserId); + const mockResponse = new ListResponse( + { + data: userIdsBatch.map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); - const result = await service.bulkReinvite(mockOrganization, userIds); + organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse); - expect(result.successful).toBeUndefined(); - expect(result.failed).toHaveLength(3); - expect(result.failed[0]).toEqual({ id: userIds[0], error: errorMessage }); + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(result.successful).toBeDefined(); + expect(result.successful?.response).toHaveLength(REQUESTS_PER_BATCH); + expect(result.failed).toHaveLength(0); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes( + 1, + ); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith( + organizationId, + userIdsBatch, + ); + }); + + it("should process users in multiple batches when count exceeds REQUESTS_PER_BATCH", async () => { + const totalUsers = REQUESTS_PER_BATCH + 100; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + + const mockResponse1 = new ListResponse( + { + data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + const mockResponse2 = new ListResponse( + { + data: userIdsBatch.slice(REQUESTS_PER_BATCH).map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + organizationUserApiService.postManyOrganizationUserReinvite + .mockResolvedValueOnce(mockResponse1) + .mockResolvedValueOnce(mockResponse2); + + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(result.successful).toBeDefined(); + expect(result.successful?.response).toHaveLength(totalUsers); + expect(result.failed).toHaveLength(0); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes( + 2, + ); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenNthCalledWith( + 1, + organizationId, + userIdsBatch.slice(0, REQUESTS_PER_BATCH), + ); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenNthCalledWith( + 2, + organizationId, + userIdsBatch.slice(REQUESTS_PER_BATCH), + ); + }); + + it("should aggregate results across multiple successful batches", async () => { + const totalUsers = REQUESTS_PER_BATCH + 50; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + + const mockResponse1 = new ListResponse( + { + data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + const mockResponse2 = new ListResponse( + { + data: userIdsBatch.slice(REQUESTS_PER_BATCH).map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + organizationUserApiService.postManyOrganizationUserReinvite + .mockResolvedValueOnce(mockResponse1) + .mockResolvedValueOnce(mockResponse2); + + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(result.successful).toBeDefined(); + expect(result.successful?.response).toHaveLength(totalUsers); + expect(result.successful?.response.slice(0, REQUESTS_PER_BATCH)).toEqual( + mockResponse1.data, + ); + expect(result.successful?.response.slice(REQUESTS_PER_BATCH)).toEqual(mockResponse2.data); + expect(result.failed).toHaveLength(0); + }); + + it("should handle mixed individual errors across multiple batches", async () => { + const totalUsers = REQUESTS_PER_BATCH + 4; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + + const mockResponse1 = new ListResponse( + { + data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id, index) => ({ + id, + error: index % 10 === 0 ? "Rate limit exceeded" : null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + const mockResponse2 = new ListResponse( + { + data: [ + { id: userIdsBatch[REQUESTS_PER_BATCH], error: null }, + { id: userIdsBatch[REQUESTS_PER_BATCH + 1], error: "Invalid email" }, + { id: userIdsBatch[REQUESTS_PER_BATCH + 2], error: null }, + { id: userIdsBatch[REQUESTS_PER_BATCH + 3], error: "User suspended" }, + ], + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + organizationUserApiService.postManyOrganizationUserReinvite + .mockResolvedValueOnce(mockResponse1) + .mockResolvedValueOnce(mockResponse2); + + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + // Count expected failures: every 10th index (0, 10, 20, ..., 490) in first batch + 2 explicit in second batch + // Indices 0 to REQUESTS_PER_BATCH-1 where index % 10 === 0: that's floor((BATCH_SIZE-1)/10) + 1 values + const expectedFailuresInBatch1 = Math.floor((REQUESTS_PER_BATCH - 1) / 10) + 1; + const expectedFailuresInBatch2 = 2; + const expectedTotalFailures = expectedFailuresInBatch1 + expectedFailuresInBatch2; + const expectedSuccesses = totalUsers - expectedTotalFailures; + + expect(result.successful).toBeDefined(); + expect(result.successful?.response).toHaveLength(expectedSuccesses); + expect(result.failed).toHaveLength(expectedTotalFailures); + expect(result.failed.some((f) => f.error === "Rate limit exceeded")).toBe(true); + expect(result.failed.some((f) => f.error === "Invalid email")).toBe(true); + expect(result.failed.some((f) => f.error === "User suspended")).toBe(true); + }); + + it("should aggregate all failures when all batches fail", async () => { + const totalUsers = REQUESTS_PER_BATCH + 100; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + const errorMessage = "All batches failed"; + + organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue( + new Error(errorMessage), + ); + + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(result.successful).toBeUndefined(); + expect(result.failed).toHaveLength(totalUsers); + expect(result.failed.every((f) => f.error === errorMessage)).toBe(true); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes( + 2, + ); + }); + + it("should handle empty data in batch response", async () => { + const totalUsers = REQUESTS_PER_BATCH + 50; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + + const mockResponse1 = new ListResponse( + { + data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + const mockResponse2 = new ListResponse( + { + data: [], + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + + organizationUserApiService.postManyOrganizationUserReinvite + .mockResolvedValueOnce(mockResponse1) + .mockResolvedValueOnce(mockResponse2); + + const result = await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(result.successful).toBeDefined(); + expect(result.successful?.response).toHaveLength(REQUESTS_PER_BATCH); + expect(result.failed).toHaveLength(0); + }); + + it("should process batches sequentially in order", async () => { + const totalUsers = REQUESTS_PER_BATCH * 2; + const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId); + const callOrder: number[] = []; + + organizationUserApiService.postManyOrganizationUserReinvite.mockImplementation( + async (orgId, ids) => { + const batchIndex = ids.includes(userIdsBatch[0]) ? 1 : 2; + callOrder.push(batchIndex); + + return new ListResponse( + { + data: ids.map((id) => ({ + id, + error: null, + })), + continuationToken: null, + }, + OrganizationUserBulkResponse, + ); + }, + ); + + await service.bulkReinvite(mockOrganization, userIdsBatch); + + expect(callOrder).toEqual([1, 2]); + expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes( + 2, + ); + }); }); }); @@ -427,14 +694,6 @@ describe("MemberActionsService", () => { expect(result).toBe(false); }); - it("should not allow reset password when organization lacks public and private keys", () => { - const org = { ...mockOrganization, hasPublicAndPrivateKeys: false } as Organization; - - const result = service.allowResetPassword(mockOrgUser, org, resetPasswordEnabled); - - expect(result).toBe(false); - }); - it("should not allow reset password when user is not enrolled in reset password", () => { const user = { ...mockOrgUser, resetPasswordEnrolled: false } as OrganizationUserView; @@ -443,12 +702,6 @@ describe("MemberActionsService", () => { expect(result).toBe(false); }); - it("should not allow reset password when reset password is disabled", () => { - const result = service.allowResetPassword(mockOrgUser, mockOrganization, false); - - expect(result).toBe(false); - }); - it("should not allow reset password when user status is not confirmed", () => { const user = { ...mockOrgUser, diff --git a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts index 5e19e26954e..f3774e3cb25 100644 --- a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts +++ b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts @@ -20,9 +20,12 @@ import { EncryptService } from "@bitwarden/common/key-management/crypto/abstract import { ListResponse } from "@bitwarden/common/models/response/list.response"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { KeyService } from "@bitwarden/key-management"; +import { UserId } from "@bitwarden/user-core"; import { OrganizationUserView } from "../../../core/views/organization-user.view"; +export const REQUESTS_PER_BATCH = 500; + export interface MemberActionResult { success: boolean; error?: string; @@ -162,20 +165,36 @@ export class MemberActionsService { } } - async bulkReinvite(organization: Organization, userIds: string[]): Promise { - try { - const result = await this.organizationUserApiService.postManyOrganizationUserReinvite( - organization.id, - userIds, - ); - return { successful: result, failed: [] }; - } catch (error) { - return { - failed: userIds.map((id) => ({ id, error: (error as Error).message ?? String(error) })), - }; + async bulkReinvite(organization: Organization, userIds: UserId[]): Promise { + const increaseBulkReinviteLimitForCloud = await firstValueFrom( + this.configService.getFeatureFlag$(FeatureFlag.IncreaseBulkReinviteLimitForCloud), + ); + if (increaseBulkReinviteLimitForCloud) { + return await this.vNextBulkReinvite(organization, userIds); + } else { + try { + const result = await this.organizationUserApiService.postManyOrganizationUserReinvite( + organization.id, + userIds, + ); + return { successful: result, failed: [] }; + } catch (error) { + return { + failed: userIds.map((id) => ({ id, error: (error as Error).message ?? String(error) })), + }; + } } } + async vNextBulkReinvite( + organization: Organization, + userIds: UserId[], + ): Promise { + return this.processBatchedOperation(userIds, REQUESTS_PER_BATCH, (batch) => + this.organizationUserApiService.postManyOrganizationUserReinvite(organization.id, batch), + ); + } + allowResetPassword( orgUser: OrganizationUserView, organization: Organization, @@ -207,4 +226,52 @@ export class MemberActionsService { orgUser.status === OrganizationUserStatusType.Confirmed ); } + + /** + * Processes user IDs in sequential batches and aggregates results. + * @param userIds - Array of user IDs to process + * @param batchSize - Number of IDs to process per batch + * @param processBatch - Async function that processes a single batch and returns the result + * @returns Aggregated bulk action result + */ + private async processBatchedOperation( + userIds: UserId[], + batchSize: number, + processBatch: (batch: string[]) => Promise>, + ): Promise { + const allSuccessful: OrganizationUserBulkResponse[] = []; + const allFailed: { id: string; error: string }[] = []; + + for (let i = 0; i < userIds.length; i += batchSize) { + const batch = userIds.slice(i, i + batchSize); + + try { + const result = await processBatch(batch); + + if (result?.data) { + for (const response of result.data) { + if (response.error) { + allFailed.push({ id: response.id, error: response.error }); + } else { + allSuccessful.push(response); + } + } + } + } catch (error) { + allFailed.push( + ...batch.map((id) => ({ id, error: (error as Error).message ?? String(error) })), + ); + } + } + + const successful = + allSuccessful.length > 0 + ? new ListResponse(allSuccessful, OrganizationUserBulkResponse) + : undefined; + + return { + successful, + failed: allFailed, + }; + } }