[PM-38927] - Extract Organziation User Role Validation#7876
Conversation
jrmccannon
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7876 +/- ##
=======================================
Coverage 61.35% 61.36%
=======================================
Files 2236 2238 +2
Lines 98547 98563 +16
Branches 8911 8914 +3
=======================================
+ Hits 60468 60482 +14
- Misses 35943 35944 +1
- Partials 2136 2137 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
A couple of quick comments, but my main feedback - my thinking was to re-introduce a service layer for smaller shared logic like this. e.g. OrganizationUserValidationService which can be called by multiple validators associated with different flows, and which would provide a bit more flexibility. That flexibility includes multiple params instead of request objects (where reasonable) which allows it to be easily called from different contexts without them having to share or construct the same request object (which adds a lot of boilerplate).
i.e. command -> validator (1:1 relationship with the command) -> domainValidationService (shared logic only, many:many relationship with validators).
Not sure if we discussed this - I might be mixing up my conversations. I will try to document some of my thoughts here this sprint.
The case against using dedicated validator classes for this level is:
- confusion between command-specific validators and reusable validators
- proliferation of classes (and associated boilerplate) for small, reusable bits of logic
- excessive request object mapping for them to be reusable
Let me know what you think and we can discuss further.
| /// <summary> | ||
| /// Validates whether the acting user can manage members at all, independent of any target. Use this where | ||
| /// authorization needs the gate without a specific target. Returns valid, or a <see cref="MissingManagePermissionError"/>. | ||
| /// </summary> | ||
| public static async Task<ValidationResult<OrganizationUserManageMembersRequest>> ValidateCanManageMembersAsync(OrganizationUserManageMembersRequest request) | ||
| { | ||
| var isProvider = !IsAuthorizedByRole(request) && await request.IsProviderUserForOrg(); | ||
|
|
||
| return IsAuthorizedByRole(request) || isProvider | ||
| ? Valid(request) | ||
| : Invalid(request, new MissingManagePermissionError()); | ||
| } |
There was a problem hiding this comment.
We can already do simple RBAC checks well at the controller layer via attributes. I don't think we need to duplicate it here. This would be limited to the escalation concern.
| // The acting user can manage members; confirm the target's role is within their authority. A provider | ||
| // user has Owner-level authority, so treat them as an Owner. The expensive provider lookup already ran | ||
| // inside the gate above, so we reuse the cheap synchronous role check here rather than repeating it. | ||
| var effectiveRole = IsAuthorizedByRole(request) ? request.ActingUserRole : OrganizationUserType.Owner; |
There was a problem hiding this comment.
I think this could be simplified, and it's better to avoid implicit logic such as:
- if you're not an admin then you must be a provider based on the logic of a separate private method
- if you're a provider we're going to say you're an owner
I would just check the role first, then check provider status if required. If we are doing multiple provider lookups there are better ways to handle that - e.g. push it out to the caller to pass a memoized Func or just a concrete boolean value.
| public record OrganizationUserActionRequest( | ||
| OrganizationUserType? ActingUserRole, | ||
| Permissions? ActingUserPermissions, | ||
| Func<Permissions, bool> PermissionPicker, | ||
| Func<Task<bool>> IsProviderUserForOrg, | ||
| OrganizationUserType TargetUserRole) | ||
| : OrganizationUserManageMembersRequest(ActingUserRole, ActingUserPermissions, PermissionPicker, IsProviderUserForOrg); |
There was a problem hiding this comment.
This is really flexible but I think it degrades the interface. Simplified:
OrganizationUser? actingUserOrganizationUser targetUserbool actingUserIsProvider
Then if this is a service method - they can easily be passed as arguments.
That all makes sense and we did have the combo. Went down this path to see how it would look and got a little carried away. I've updated it to match the signature requested and hopefully cleaned up some other things. |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the extraction of organization-user "can manage" authorization logic into a new reusable Code Review DetailsNo findings. Notes verified during review:
|
|
2 participants
🎟️ Tracking
PM-38927
📔 Objective
Consolidates logic for Organization User Aciton validation to single class. This will be consumed when checking if an org user can perform an action or if an org user can perform an action on another org user.