[PM-18718] Refactor Bulk Revoke Users #6601
Conversation
…m-user-command # Conflicts: # src/Api/AdminConsole/Controllers/OrganizationUsersController.cs # src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs # test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs
…. Added ApplyAsync method to execute multiple functions against CommandResult without an error stopping the workflow for side-effects.
…thods for chaining ValidationResult together. Added logic for user type.
…ntent, however in CI, its slow enough for it to return 400 due to the user already being confirmed via validation.
…mccannon/ac/pm-18718-refactor-revoke-bulk-users
…sers # Conflicts: # src/Api/AdminConsole/Controllers/OrganizationUsersController.cs
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6601 +/- ##
==========================================
- Coverage 56.83% 53.14% -3.70%
==========================================
Files 1909 1913 +4
Lines 84810 84966 +156
Branches 7633 7644 +11
==========================================
- Hits 48199 45151 -3048
- Misses 34785 38061 +3276
+ Partials 1826 1754 -72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…sers # Conflicts: # src/Core/Constants.cs
…for an owner to remove the last owner.
|
Claude finished @jrmccannon's task —— View job Code Review CompleteSummary of Changes Since Last ReviewThe author has addressed the critical "last owner" validation issue that was previously missing. The validator now properly checks The author also responded to two previous comments:
Critical Issues FoundNone - the previously identified critical issue has been resolved. Suggested ImprovementsFinding 1: Missing integration test for "last owner" scenarioWhile unit tests cover the "last owner" validation (RevokeOrganizationUsersValidatorTests.cs:256-285), there's no integration test verifying this behavior end-to-end. This is especially important since it's a critical business rule that was initially missing. Recommendation: Suggested test implementation[Fact]
public async Task BulkRevoke_LastConfirmedOwner_ReturnsError()
{
// Arrange: Create org with exactly one confirmed owner
var (soleOwnerEmail, soleOwner) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(
_factory, _organization.Id, OrganizationUserType.Owner);
await _loginHelper.LoginAsync(soleOwnerEmail);
var request = new OrganizationUserBulkRequestModel
{
Ids = [soleOwner.Id]
};
// Act: Attempt to revoke the last owner
var httpResponse = await _client.PutAsJsonAsync($"organizations/{_organization.Id}/users/revoke", request);
var content = await httpResponse.Content.ReadFromJsonAsync<ListResponseModel<OrganizationUserBulkResponseModel>>();
// Assert: Should return error
Assert.Equal(HttpStatusCode.OK, httpResponse.StatusCode);
Assert.NotNull(content);
Assert.Single(content.Data);
Assert.Contains(content.Data, r => r.Id == soleOwner.Id &&
r.Error == "Organization must have at least one confirmed owner.");
// Verify user was NOT revoked
var actualUser = await _factory.GetService<IOrganizationUserRepository>().GetByIdAsync(soleOwner.Id);
Assert.NotNull(actualUser);
Assert.Equal(OrganizationUserStatusType.Confirmed, actualUser.Status);
}Finding 2: Validator checks last owner per-user instead of batch-wideThe current implementation checks if there's a remaining owner for EACH user being revoked (lines 16-18, 30-31 in RevokeOrganizationUsersValidator.cs). This works correctly because However, the logic could be more efficient: Current behavior: If revoking 3 owners where at least one will remain, the query runs once but the check on line 30 evaluates individually for each user. More efficient approach: Check once before the loop if the batch would leave zero owners, then apply that result to all owner-type users. This is a minor optimization and the current implementation is functionally correct. Finding 3: RevokeManyByIdAsync lacks database transaction documentationThe Recommendation: /// <summary>
/// Revokes multiple organization users by their IDs in a single database operation.
/// </summary>
/// <param name="organizationUserIds">The IDs of users to revoke.</param>
/// <remarks>
/// This operation executes atomically via stored procedure.
/// If any error occurs, no users will be revoked and an exception will be thrown.
/// </remarks>
public async Task RevokeManyByIdAsync(IEnumerable<Guid> organizationUserIds)Good Practices Observed
Action Items
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public record UserAlreadyRevoked() : BadRequestError("Already revoked."); | ||
| public record CannotRevokeYourself() : BadRequestError("You cannot revoke yourself."); | ||
| public record OnlyOwnersCanRevokeOwners() : BadRequestError("Only owners can revoke other owners."); | ||
| public record MustHaveConfirmedOwner() : BadRequestError("Organization must have at least one confirmed owner."); |
There was a problem hiding this comment.
💭 The MustHaveConfirmedOwner error is defined but never used in the codebase. The v1 implementation checked for this condition with HasConfirmedOwnersExceptAsync, but the v2 implementation removed this check entirely.
Why this matters:
This could allow revoking the last owner of an organization, which would leave the organization in an unmanageable state. The v1 implementation prevented this at lines 62-66 and 84-87.
Recommendation:
Either implement the check in the validator and use this error, or remove the unused error definition. Based on the v1 implementation and business logic, the check should be implemented.
|
|
||
| public class RevokeOrganizationUsersValidator : IRevokeOrganizationUserValidator | ||
| { | ||
| public ICollection<ValidationResult<OrganizationUser>> Validate(RevokeOrganizationUsersValidationRequest request) => |
There was a problem hiding this comment.
❌ Critical: Missing "last owner" validation
The v1 implementation (RevokeUser/v1/RevokeOrganizationUserCommand.cs:62-66, 84-87) prevented revoking the last confirmed owner with HasConfirmedOwnersExceptAsync. This validation is completely absent in v2.
Security Impact:
- Organizations can be left with zero confirmed owners
- No one would be able to manage the organization
- This is a breaking change from v1 behavior
Required Fix:
Add validation to ensure the organization will have at least one confirmed owner after the revocation:
// After line 9, inject IHasConfirmedOwnersExceptQuery
// In Validate method, before returning results:
var userIdsBeingRevoked = request.OrganizationUsersToRevoke.Select(u => u.Id);
if (!await hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(
request.OrganizationId, userIdsBeingRevoked, includeProvider: true))
{
// Return error for all users being revoked
}
| return; | ||
| } | ||
|
|
||
| await organizationUserRepository.RevokeManyByIdAsync(validUsers.Select(u => u.Id)); |
There was a problem hiding this comment.
The v1 implementation called RevokeAsync individually for each user (line 117 in v1), but v2 uses RevokeManyByIdAsync for all valid users at once. While this is generally more efficient, it creates a problem:
Issue:
If the database operation fails midway through a batch update, some users may be revoked while others aren't, but all would be marked as success in the results since validation passed.
The v1 approach:
- Revoked users one at a time in a try-catch
- Could partially succeed and return mixed results
- Each user's success/failure was independent
Recommendation:
Consider whether the all-or-nothing batch approach is appropriate, or if partial success handling is needed. If partial failures are possible, wrap this in transaction handling or add error handling that updates the results accordingly.
There was a problem hiding this comment.
It would not be marked as a success because the repository would throw the exception and the transaction would be rolled back. This would be an "all-or-nothing" batch and the user would get a 500 error.
| )); | ||
| } | ||
|
|
||
| private async Task<RevokeOrganizationUsersValidationRequest> CreateValidationRequestsAsync( |
There was a problem hiding this comment.
🎨 Method can be made static or simplified
The CreateValidationRequestsAsync method only uses injected dependencies to fetch data and doesn't use any instance state. Consider:
- Making the repository calls directly in
RevokeUsersAsyncto reduce indirection - Or making this method static if you prefer the separation
This would improve code clarity and reduce unnecessary method complexity.
| { | ||
| OrganizationId = orgId, | ||
| OrganizationUserIdsToRevoke = model.Ids.ToArray(), | ||
| PerformedBy = new StandardUser(currentUserId.Value, await _currentContext.OrganizationOwner(orgId)) |
There was a problem hiding this comment.
The v2 validator checks IsOrganizationOwnerOrProvider (line 19 in RevokeOrganizationUsersValidator.cs), but when creating the StandardUser here, we only pass the owner status from OrganizationOwner(orgId), not provider status.
The issue:
StandardUser constructor likely only sets the owner flag, but the validator expects IsOrganizationOwnerOrProvider to include both owner AND provider status. This could break provider revocation scenarios.
Integration test coverage:
The test BulkRevoke_ProviderRevokesOwner_ReturnsOk (OrganizationUserControllerBulkRevokeTests.cs:319) tests this scenario and should catch this, but verify the StandardUser implementation includes provider logic.
Recommendation:
Verify that StandardUser and IActingUser.IsOrganizationOwnerOrProvider properly handle provider permissions, or explicitly check provider status here.
...onsole/OrganizationFeatures/OrganizationUsers/RevokeUser/v2/RevokeOrganizationUserCommand.cs
Show resolved
Hide resolved
| // Assert | ||
| Assert.Single(results); | ||
| Assert.True(results.First().IsValid); | ||
| } |
There was a problem hiding this comment.
The validator tests are comprehensive for the rules that ARE implemented, but there's no test for preventing the revocation of the last confirmed owner.
Missing test cases:
- Attempting to revoke the only confirmed owner (should fail)
- Attempting to revoke all confirmed owners in a batch (should fail)
- Attempting to revoke all but one owner, where the remaining owner is not confirmed (should fail)
- Revoking some owners while leaving at least one confirmed owner (should succeed)
These tests should be added once the "last owner" validation is implemented in the validator (see my comment on RevokeOrganizationUsersValidator.cs:10).
| var httpResponse = await _client.PutAsJsonAsync($"organizations/{_organization.Id}/users/revoke", request); | ||
|
|
||
| Assert.Equal(HttpStatusCode.OK, httpResponse.StatusCode); | ||
| } |
There was a problem hiding this comment.
The integration tests cover many scenarios but don't test the critical case of attempting to revoke the last confirmed owner. This is a business rule that was enforced in v1 but appears to be missing in v2.
Recommended additional test:
[Fact]
public async Task BulkRevoke_LastConfirmedOwner_ReturnsError()
{
// Arrange: Create org with exactly one confirmed owner
// Act: Attempt to revoke that owner
// Assert: Should return error "Organization must have at least one confirmed owner."
}This test would currently pass if the feature flag is disabled (using v1), but would likely fail with v2 enabled, confirming the missing validation.
🎟️ Tracking
PM-18718
📔 Objective
This is an optimized refactor of the bulk revoke users action. It utilizes the v2 Command Result and Validation Result patterns.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes