[PM-38923] feat: Add v2 UpdateOrganizationUser command with role-escalation validation#7919
[PM-38923] feat: Add v2 UpdateOrganizationUser command with role-escalation validation#7919jrmccannon wants to merge 49 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7919 +/- ##
===========================================
- Coverage 62.45% 14.80% -47.66%
===========================================
Files 2293 1394 -899
Lines 99797 60496 -39301
Branches 9007 4826 -4181
===========================================
- Hits 62330 8957 -53373
- Misses 35295 51388 +16093
+ Partials 2172 151 -2021 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
41676f0 to
51a91f8
Compare
…lation validation Refactor the OrganizationUsersController.Put flow into a v2 command + validator (returning CommandResult / ValidationResult<T>), gated behind the pm-28365-change-member-email-no-mp feature flag. The validator ports v1's request validation and adds the escalating-role guard by delegating to the shared IOrganizationUserValidationService.CanManage rules (PM-38927), while preserving v1's specific error messages. The controller supplies the acting user's membership (role + permissions) as a plain field so Core stays free of ICurrentContext.
… UpdateOrganizationUser Pass the API-loaded collections into the v2 validation request so the validator validates existence without re-querying. The validator now rejects default user collections with a new CannotAssignDefaultCollection error instead of silently filtering them, and the controller excludes current-access defaults from the preserved read-only set.
…ator tests. Made sure self hosted couldn't increase seat count on SM.
Fold UpdateOrganizationUserValidationRequest into UpdateOrganizationUserRequest so the validator consumes the command request directly. Rename request properties for clarity (OrganizationUserToUpdate, New* for requested changes, ReferencedCollections) and group them logically, and trim verbose comments across the request, command, and validator.
…ationUser Port the v1 grant-subset guard into the v2 validator so a Custom member with only ManageUsers cannot grant a target member custom permissions the actor does not themselves hold. Owners, admins, providers (no membership), and system users remain exempt. Add the CustomUsersCanOnlyGrantOwnPermissions error and validator coverage, and trim redundant comments across the v2 update files.
c06e936 to
0cd8623
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…ser validator Assert the specific denial error type and add cases where the target's current role and the requested role are Owner, exercising both branches of the OnlyOwnersCanManageOwners vs CustomUsersCannotManageAdminsOrOwners mapping.
# Conflicts: # src/Api/AdminConsole/Controllers/OrganizationUsersController.cs
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the refactor of Code Review Details
Note: The existing |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…or related methods.
| // When admins are not allowed access to all collections, a user editing themselves cannot add | ||
| // themselves to collections they don't already have access to. | ||
| if (IsAddingSelfToCollection(request)) | ||
| { | ||
| return Invalid(request, new CannotAddSelfToCollection()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Claims are a good indication of authz logic but it's not exclusively claims based. Collections (and ciphers) use a resource-based authorization model (variously called relationship-based or fine-grained) where authorization depends on the relationship between objects. To me this is authorization because it is enforcing an access restriction based off the user's relationships; that org property effectively toggles between 2 different authorization schemes. It's all very convoluted due to the amount of configurability.
Another rule of thumb I think about: if this action were happening via the public api, would this check be needed? If not, that suggests that it's tied to the concept of a user and their permissions which is authorization.
This is all negotiable, but even if I'm wrong here I think we should defer it and consider together with the rest of the logic that lives at the controller layer, rather than moving it in here at this stage.
EDIT: this also lets you drop CurrentCollectionsIds from the request object, which helps with CurrentCollectionsIds/ReferencedCollections/NewCollections confusion.
| private async Task<bool> IsValidFreeOrganizationAdminAsync(OrganizationUser organizationUser, OrganizationUserType newType, Organization organization) | ||
| { | ||
| if (organization.PlanType != PlanType.Free) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (!organizationUser.UserId.HasValue) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (newType is not (OrganizationUserType.Admin or OrganizationUserType.Owner)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var adminCount = await organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(organizationUser.UserId!.Value); | ||
| var isCurrentAdminOrOwner = organizationUser.Type is OrganizationUserType.Admin or OrganizationUserType.Owner; | ||
|
|
||
| if (isCurrentAdminOrOwner && adminCount <= 1) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (!isCurrentAdminOrOwner && adminCount == 0) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Doing a text search of part of this error mesage ("one free organization") shows that it's reused in a number of flows:
- accept
- confirm
- update
- new org
Could it be moved to the validation service in a way that is general enough to be reusable?
| // Default user collections ("My Items") are never managed through this flow. Exclude any current-access | ||
| // defaults from the preserved set so they are not re-saved; posted defaults are rejected downstream. | ||
| var defaultCollectionIds = postedCollections | ||
| .Concat(currentCollections) | ||
| .Where(c => c.Type == CollectionType.DefaultUserCollection) | ||
| .Select(c => c.Id) | ||
| .ToHashSet(); |
There was a problem hiding this comment.
I have very low confidence in this existing code so I don't want to modify it here if we can help it. I think this will break, because an admin never has write access to a My Items collection, yet it'll be excluded from readonlyCollectionAccess which needs to be added back in to the set, which means the user will lose access to it. But regardless; if there is no documented defect here let's leave it.
Moving it into a private method makes sense (for re-use between v1 and v2 flows) but the guts of it shouldn't change.
| await organizationUserRepository.ReplaceAsync(organizationUser, request.NewCollections?.ToList() ?? []); | ||
|
|
||
| if (request.NewGroups != null) | ||
| { | ||
| await organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, request.NewGroups, | ||
| timeProvider.GetUtcNow().UtcDateTime); | ||
| } |
There was a problem hiding this comment.
Update: after looking at the controller again, this is matching the api interface and the legacy authz logic. None of that's ideal but as discussed not worth fixing here. So I think this can stay as is. (context)
| /// <param name="NewGroups">The updated group access; null leaves groups unchanged.</param> | ||
| /// <param name="PerformedByOrganizationUser">The actor's own membership; null when the actor is not an organization member (e.g. a provider).</param> | ||
| /// <param name="DefaultUserCollectionName">Default collection name used when applicable</param> | ||
| public record UpdateOrganizationUserRequest( |
There was a problem hiding this comment.
The authz logic at the controller continues to do us dirty here and makes it difficult to decouple ourselves from the legacy implementation. But I think you've improved this and it's OK. (context)
| OrganizationUser OrganizationUserToUpdate, | ||
| Organization Organization, | ||
| HashSet<Guid> CurrentCollectionsIds, | ||
| ICollection<Collection> ReferencedCollections, |
There was a problem hiding this comment.
One remaining concern I have is ReferencedCollections, which is raw POST data which is not authorized. Passing the unauthorized request through like this seems like a bit of a footgun. Maybe as a rule, if an object is missing then you're not authorized to act on it, so it fails at the controller layer; then the command is only receiving an authorized request (which is the current intention)? Open to discussion.
There was a problem hiding this comment.
Review completed. Great job, thank you for taking the lead on the service layer, I think it works. I also think passing the same request object from controller -> command -> validator is an improvement. We can discuss more when we catch up next week, I'm interested in what you think.
| await sutProvider.GetDependency<IOrganizationUserRepository>() | ||
| .Received(1) | ||
| .ReplaceAsync(organizationUser, Arg.Any<IEnumerable<CollectionAccessSelection>>()); | ||
| await sutProvider.GetDependency<IOrganizationUserRepository>() | ||
| .Received(1) | ||
| .UpdateGroupsAsync(organizationUser.Id, Arg.Any<IEnumerable<Guid>>(), Arg.Any<DateTime>()); |
There was a problem hiding this comment.
Can we be more specific than Arg.Any? (and throughout, I think Claude tends towards this 'cause it's lazy)
|
|
||
| [Theory] | ||
| [BitAutoData] | ||
| public async Task UpdateUserAsync_AppliesRequestedChangesToTheDatabaseCopy( |
There was a problem hiding this comment.
The test case doesn't match the assertions - nothing here mentions the database. A more accurate description would be "returns an updated organization user". That could be covered in your basic happy path test above though.
| [Theory] | ||
| [BitAutoData] | ||
| public async Task UpdateUserAsync_WhenEnablingSecretsManager_ChecksRequiredSeats( | ||
| SutProvider<UpdateOrganizationUserCommand> sutProvider, | ||
| Organization organization, | ||
| [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) | ||
| { | ||
| organizationUser.AccessSecretsManager = false; | ||
| var request = Setup(sutProvider, organization, organizationUser, targetAccessSecretsManager: true); | ||
|
|
||
| sutProvider.GetDependency<ICountNewSmSeatsRequiredQuery>() | ||
| .CountNewSmSeatsRequiredAsync(organization.Id, 1) | ||
| .Returns(0); | ||
|
|
||
| var result = await sutProvider.Sut.UpdateUserAsync(request); | ||
|
|
||
| Assert.True(result.IsSuccess); | ||
|
|
||
| await sutProvider.GetDependency<ICountNewSmSeatsRequiredQuery>() | ||
| .Received(1) | ||
| .CountNewSmSeatsRequiredAsync(organization.Id, 1); | ||
| await sutProvider.GetDependency<IUpdateSecretsManagerSubscriptionCommand>() | ||
| .DidNotReceive() | ||
| .UpdateSubscriptionAsync(Arg.Any<SecretsManagerSubscriptionUpdate>()); | ||
| } | ||
|
|
||
| [Theory] | ||
| [BitAutoData] | ||
| public async Task UpdateUserAsync_WhenSecretsManagerAccessUnchanged_DoesNotCheckSeats( | ||
| SutProvider<UpdateOrganizationUserCommand> sutProvider, | ||
| Organization organization, | ||
| [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) | ||
| { | ||
| organizationUser.AccessSecretsManager = false; | ||
| var request = Setup(sutProvider, organization, organizationUser, targetAccessSecretsManager: false); | ||
|
|
||
| var result = await sutProvider.Sut.UpdateUserAsync(request); | ||
|
|
||
| Assert.True(result.IsSuccess); | ||
|
|
||
| await sutProvider.GetDependency<ICountNewSmSeatsRequiredQuery>() | ||
| .DidNotReceive() | ||
| .CountNewSmSeatsRequiredAsync(Arg.Any<Guid>(), Arg.Any<int>()); | ||
| } |
There was a problem hiding this comment.
Whether it checks seats or not is an internal implementation detail and doesn't tell you much about correctness. Maybe it checks it but does nothing with the result. What is the external behavior we want to verify? "Scales secrets manager subscription when required" / "Does not scale secrets manager subscription when not required".
|
|
||
| [Theory] | ||
| [BitAutoData] | ||
| public async Task Put_WhenFeatureFlagEnabled_ExcludesDefaultCollectionsFromPreservedAccess( |
There was a problem hiding this comment.
Asserts incorrect behavior - as mentioned above, this will result in the My Items collection access being removed for the target user.
There was a problem hiding this comment.
Please add some api integration tests. I think they will be more useful here given the amount of state and mocking involved.
Move the PUT tests into a dedicated OrganizationUserControllerPutTests file, run each under the flag off (v1) and on (v2), and branch assertions where v1 and v2 differ.
3 participants
🎟️ Tracking
PM-38923 (epic PM-28365 — "Manual single-email change for members without passwords")
📔 Objective
Refactors
OrganizationUsersController.Putand the v1UpdateOrganizationUserCommandinto a v2 command + validator (returningCommandResult/ValidationResult<T>), mirroring the RevokeUser v2 pattern. The v2 path is gated behind thepm-28365-change-member-email-no-mpfeature flag and stays off by default.Key points:
IOrganizationUserValidationService.CanManagerules from [PM-38927] - Extract Organziation User Role Validation #7876, checking both the member's current role and the requested role so an existing higher-ranked member can't be modified from below. v1's specific error messages are preserved by mapping the denial (OnlyOwnersCanManageOwners/CustomUsersCannotManageAdminsOrOwners).ICurrentContext/ClaimsPrincipal; the controller passes the acting user's membership (role + permissions) and theIActingUseras plain inputs. Collection resource authorization (ModifyUserAccess) stays in the controller.Putcollection-auth unit tests to mock the bulkModifyUserAccessauthorization call.Test coverage
Putcollection-auth tests (42/42 in the two controller test classes).📸 Screenshots