[PM-43035] - Set ActingUser Type for providers

[jrmccannon]

🎟️ Tracking

PM-43035

📔 Objective

An error is being thrown when a provider attempts to update an organization member in an organization they manage. This will add a StandardUser representation for them and clean up the abstraction of validating user actions and updating. Callers will no longer have to construct the IOrganizationUserRole if they have an IActingUser object.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.97%. Comparing base (b89568d) to head (c4d11dd).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8313      +/-   ##
==========================================
- Coverage   64.03%   63.97%   -0.06%     
==========================================
  Files        2473     2472       -1     
  Lines      106017   105812     -205     
  Branches     9613     9598      -15     
==========================================
- Hits        67886    67693     -193     
+ Misses      35771    35769       -2     
+ Partials     2360     2350      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR introduces IGetActingUserForOrganizationQuery to resolve the caller into either a StandardUser or the new ProviderUser acting-user model, and removes the provider lookup from OrganizationUserValidationService so its CanManage/CanManageRoleChange methods become synchronous role-only checks. Provider authority is now expressed by type: UpdateOrganizationUserValidator skips the role-change check for any non-StandardUser, which matches the legacy behavior where CurrentContext.OrganizationOwner returned true for provider users. The nullable-to-default change on StandardUser.OrganizationUserType is deny-safe — the only reader is the role-change validator, and the remaining two-argument constructor call sites now yield the lowest-authority role instead of throwing. Unit tests for the new query, the reworked validation service, and an integration test covering the provider-acting-user path are included.

Code Review Details

• ❓ : Caller who is both an org member and a confirmed provider user resolves as StandardUser and loses provider authority
  • src/Api/AdminConsole/Authorization/GetActingUserForOrganizationQuery.cs:13

[claude]

❓ QUESTION: A caller who is both an org member and a confirmed provider user for that org now resolves as StandardUser and loses provider authority — is that intended?

Details

Membership wins here, and CurrentContext.ProviderIdForOrg also returns null when the caller is a member of the org, so such a caller can never resolve to ProviderUser.

Trace for a User-role member who is also a confirmed ProviderAdmin over the org:

• ManageUsersRequirement still authorizes the request — BasePermissionRequirement falls through to the provider check.
• ValidateRoleChange then runs with the member role only and returns a cannot-manage error, so the update fails.

Before this change, both the v2 service (IsProviderAsync) and the legacy path (CurrentContext.OrganizationOwner, which returns true for any provider user of the org) granted provider authority regardless of membership. If a provider user is never expected to also hold a membership in the managed organization, this is a non-issue.

[eliykat]

This Claude feedback is correct. I suggest:

        var isProvider = await currentContext.ProviderUserForOrgAsync(organziationId);
        var membership = currentContext.GetOrganization(organizationId);

        return new StandardUser(userId, isProvider, membership?.Type, membership?.Permissions);

We are losing the lazy evaluation of provider status, that might be something we discuss as a follow-up.

[eliykat]

I had to back up and look at the problem being solved here. As I understand it, the problem is in UpdateOrganizationUserValidator:

    private async Task<Error?> ValidateRoleChangeAsync(UpdateOrganizationUserRequest request)
    {
        if (request.PerformedBy is not StandardUser standardUser)
        {
            return null;
        }

        var actingUser = new OrganizationUserRole(
            standardUser.OrganizationUserType!.Value,          // null reference exception
            request.OrganizationUserToUpdate.OrganizationId,
            standardUser.Permissions);

   // snip

        return await organizationUserValidationService.CanManageRoleChangeAsync(
            standardUser.UserId!.Value,
            actingUser,
            request.OrganizationUserToUpdate,
            newTargetUser);
    }

So the interesting thing here is that the type system already wants you to consider providers - OrganizationUserType is nullable because not everyone is a member. But, this is a lot of semantic information to represent just by the property being nullable. It's easy to assume the type system is wrong and override it (incorrectly) - so you are representing it with a separate type entirely.

I am on the fence about this:

• now the validator is skipping the role escalation check if the acting user is a provider. That is a leaky abstraction; the caller knows about these cases in which role escalation doesn't apply and then skips calling the method that is meant to encapsulate it.
• it muddies IActingUser, which admittedly is a bit unclear at the moment, but it was originally meant to distinguish between private and public API actors. I also think it's more straightforward to keep all private-api-authenticated-users as a single type so that understanding the type hierarchy is not required to consume it properly.
• a user can be a member OR a provider OR both. StandardUser vs ProviderUser can only represent either/or.

On the other hand, the choices for making this property non-nullable are either:

• separate types like you've done, so it can be omitted entirely
• representing all providers as Owners, which is what we've done to date, but I think it's misleading. There are some actions that an Owner can take that a provider can't, unless they are also an owner.
• have an enum similar to OrganizationUserStatusType, but with a Not a member value - representing what null does today. That is probably my favourite option, but it's boilerplate.

For now, the best I can think of is...

• drop ProviderUser : IActingUser and keep using StandardUser for this case
• pass in a StandardUser as the first argument to CanManageRoleChangeAsync, so that method has the full context for the acting user. That solves the root cause of the bug where you have to map a nullable value to a non-nullable value.
• deprecate IsOrganizationOwnerOrProvider on IActingUser and add StandardUser.IsProvider instead, to remove the overlap with OrganizationUserStatusType, which is confusing imo. Now the type clearly represents the either/or/both possibilities.
• ... and OrganizationUserStatusType is still nullable. That is the weakness here. But we truly do need to represent a lack of role.

The query is good and useful given that you need to pull multiple data sources together to construct the object 👍

None of this is perfect so happy to discuss further.

[jrmccannon]

For now, the best I can think of is...

• drop ProviderUser : IActingUser and keep using StandardUser for this case
• pass in a StandardUser as the first argument to CanManageRoleChangeAsync, so that method has the full context for the acting user. That solves the root cause of the bug where you have to map a nullable value to a non-nullable value.
• deprecate IsOrganizationOwnerOrProvider on IActingUser and add StandardUser.IsProvider instead, to remove the overlap with OrganizationUserStatusType, which is confusing imo. Now the type clearly represents the either/or/both possibilities.
• ... and OrganizationUserStatusType is still nullable. That is the weakness here. But we truly do need to represent a lack of role.

I made most of those changes with a few caveats that I think clean this up.

• I brought the StandardUser check inside the ValidationService. While this does add another spot referencing IActingUser, it cleans up the interface and doesn't enforce callers to know how exactly it should look.
• I changed the provider branch of the ActingUserQuery to just return a StandardUser with isProvider: true.
• I added a private method to take the StandardUser and return the correctly constructed IOrganizationUserRole object with a comment around providers being able to manage all users. It will also return an error if it somehow reaches an odd state where neither a provider nor an org member attempts to validate through. That error will propagate through.

[r-tome]

Looks great to me! Fixed the bug and tidied up the code, nice work.

[r-tome]

💡 We could add a test to assert what should happen when a user is both a member and a provider

[eliykat]

This Claude feedback is correct. I suggest:

        var isProvider = await currentContext.ProviderUserForOrgAsync(organziationId);
        var membership = currentContext.GetOrganization(organizationId);

        return new StandardUser(userId, isProvider, membership?.Type, membership?.Permissions);

We are losing the lazy evaluation of provider status, that might be something we discuss as a follow-up.

[eliykat]

LGTM thank you - some more work to be done here to align these interfaces, but this looks good for the bugfix.