[PM-36678] Fix custom users modifying admins for restore and revoke.#7601
[PM-36678] Fix custom users modifying admins for restore and revoke.#7601JimmyVo16 wants to merge 1 commit into
Conversation
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Validators; | ||
|
|
||
| public class CustomUserActingOnAdminValidator(ICurrentContext currentContext) : ICustomUserActingOnAdminValidator |
There was a problem hiding this comment.
Hey @sven-bitwarden , this is a new pattern I’m trying out. Happy to get any feedback.
We’re obviously doing the same check in a lot of places, so I was thinking we could abstract it into a single validator.
Benefits:
- Keeps the logic DRY
- Handles both single-user and bulk-user org cases without requiring multiple async calls.
| // Probes with any admin in the batch. The rule's answer is uniform across every admin | ||
| // in the same organization, so one cached lookup covers the whole batch. | ||
| private async Task<bool> CustomUserCannotRevokeAdminAsync(IEnumerable<OrganizationUser> users) | ||
| { |
There was a problem hiding this comment.
I still need to iterate on this a bit.
| @@ -0,0 +1,8 @@ | |||
| namespace Bit.Core.AdminConsole.Enums; | |||
|
|
|||
| public enum OrganizationUserActionType | |||
There was a problem hiding this comment.
The goal of this was to allow the commands to just pass in the action, while keeping the error messages in the validator. This is mainly to reduce logic in the commands. Also, most of the messages are the same, so centralizing them is a good idea.
I thought about passing in the class type, but that would narrow the calling code to only those types. An enum seems like the easiest solution, but I’m open to ideas.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7601 +/- ##
==========================================
- Coverage 59.73% 59.72% -0.01%
==========================================
Files 2109 2110 +1
Lines 92752 92797 +45
Branches 8246 8252 +6
==========================================
+ Hits 55403 55425 +22
- Misses 35384 35403 +19
- Partials 1965 1969 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
A couple of specific comments below, but more generally:
I do think that we need to develop a pattern of having smaller, more composable validators for shared logic. However, in this case, this is an authorization check that I think needs to live in the api layer. It is effectively an extension of RBAC: you must have this role, but also that role must be equal or greater to the user you're acting on.
Therefore I think this should be handled by the authorization layer, potentially generalizing the RecoveryAccountAuthorizationHandler which implements this logic.
You could imagine, for example, an attribute that handles this for you in a reusable way:
[HttpPut("restore")]
[Authorize<ManageUsersRequirement>]
[Authorize<EqualOrGreaterRoleRequirement>] // additional requirement
public async Task<ListResponseModel<OrganizationUserBulkResponseModel>> BulkRestoreAsync(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model)Or - and I suspect this is the case - it may be too difficult or inefficient to do it from an attribute, in which case you can do it manually. This also means you have the orgUser objects to pass into the command later:
var organizationUsers = await _organizationUserRepository.GetManyAsync(model.Ids);
var authorized =
await _authorizationService.AuthorizeAsync(User, organizationUsers, new EqualOrGreaterRoleRequirement());
// check authorized result, then pass orgUsers into command instead of just idsI definitely agree that we need a reusable solution though.
Let me know what you think.
There was a problem hiding this comment.
This is too narrow - the general pattern here is not "custom users cannot act on admins" (specific), it's "users with lower privileges cannot act on users with higher privileges" (more general).
Here is some example logic from account recovery which fully expresses the rule:
| var isCustom = await currentContext.OrganizationCustom(organizationId); | ||
| _organizationCustomLookup[organizationId] = isCustom; |
There was a problem hiding this comment.
Although this is async, currentContext caches its results locally so you don't need a second layer of caching here. (I also generally think that this is an antipattern so would be pretty hesitant to extend this kind of behavior)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-36678
📔 Objective
📸 Screenshots