[PM-41481] Fix policy enforcement when accepting invite link - #8146
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR extracts invite-link accept eligibility into Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var isProviderForOrganization = (await providerOrganizationRepository.GetManyByUserAsync(user.Id)) | ||
| .Any(po => po.OrganizationId == organization.Id); |
There was a problem hiding this comment.
IProviderOrganizationRepository.GetManyByUserAsync does not answer "is this user a provider user for this org", so the exemption never fires here.
Details and fix
ProviderOrganizationProviderDetails_ReadByUserId is:
FROM [dbo].[ProviderOrganizationView] PO
INNER JOIN [dbo].[OrganizationUser] OU ON PO.OrganizationId = OU.OrganizationId
INNER JOIN [dbo].[Provider] P ON PO.ProviderId = P.Id
WHERE OU.UserId = @UserIdIt never touches ProviderUser. It returns provider-managed organizations that the user is already an OrganizationUser of. (ProviderOrganizationReadByUserIdQuery in EF is the same join.) CurrentContext.GetOrganizationProviderDetails uses it in exactly that sense.
That is a different predicate from PolicyDetails.IsProvider, which PolicyDetails_ReadByUserId computes as:
EXISTS (SELECT 1 FROM [dbo].[ProviderUserView] PU
INNER JOIN [dbo].[ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId]
WHERE PU.[UserId] = OU.[UserId] AND PO.[OrganizationId] = P.[OrganizationId])Consequence on this path: the joiner reaching ValidatePoliciesAsync has either no OrganizationUser row for the target org (brand-new), or an Invited/Staged row with UserId = null — Accepted/Confirmed/Revoked are already rejected by ValidateExistingMembershipStatus. The OU.UserId = @UserId join therefore matches nothing for the target org, so isProviderForOrganization is always false and the Single-Org / 2FA provider exemption is dead. Conversely, if a user-linked row ever did reach here, it would exempt any member of a provider-managed org, provider user or not.
The unit tests stub IProviderOrganizationRepository directly, so ValidateAsync_ProviderForOrganization_* pass without exercising this.
Using the ProviderUser-rooted query matches PolicyDetails.IsProvider:
var isProviderForOrganization = (await providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id))
.Any(o => o.OrganizationId == organization.Id);(ProviderUserProviderOrganizationDetailsView joins ProviderUser → ProviderOrganization → Organization. IProviderOrganizationRepository then becomes an unused dependency.)
| // Automatic User Confirmation - this organization | ||
| if (request.AutoConfirmPolicyEnabled) | ||
| { | ||
| // Autoconfirm enforcement: any provider user cannot be a member of an autoconfirm org. | ||
| var isProviderUserForAnyOrganization = (await providerUserRepository.GetManyByUserAsync(user.Id)).Count != 0; | ||
| if (isProviderUserForAnyOrganization) | ||
| { | ||
| return new ProviderUsersCannotAcceptInviteLink(); | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: This narrows the provider-user block from "always" to "only when Auto-Confirm is on" — intended, and should the confirm path follow?
Details
AcceptOrganizationInviteLinkCommand previously rejected provider users unconditionally:
// Provider users cannot accept invite links
if ((await providerUserRepository.GetManyByUserAsync(user.Id)).Count != 0)
{
return new ProviderUsersCannotAcceptInviteLink();
}After this PR a provider user can accept an invite link into any organization that does not have Auto-Confirm enabled. The reasoning is sound if the original block was only ever an Auto-Confirm concern, but the PR description scopes this change to policy-enforcement gaps, so the relaxation is easy to miss on review.
ConfirmOrganizationInviteLinkValidator (line 75) still carries the unconditional block via ConfirmProviderUsersCannotAcceptInviteLink, so accept and confirm now disagree on whether a provider user may join through a link.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8146 +/- ##
==========================================
+ Coverage 62.98% 63.01% +0.03%
==========================================
Files 2312 2314 +2
Lines 100300 100377 +77
Branches 9021 9033 +12
==========================================
+ Hits 63174 63256 +82
+ Misses 34944 34938 -6
- Partials 2182 2183 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| private async Task<Error?> ValidateFreeOrganizationAdminLimitAsync(AcceptInviteLinkMembershipValidationRequest request) | ||
| { | ||
| if (request.ExistingMembership?.Type is OrganizationUserType.Owner or OrganizationUserType.Admin && | ||
| request.Organization.PlanType == PlanType.Free && | ||
| await organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(request.User.Id) > 0) | ||
| { | ||
| return new OnlyOneFreeOrganizationAdminAllowed(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
This should be moved to the OrganizationUserValidationService as an overload on ValidateFreeOrgAdminLimitAsync
| { | ||
| var user = request.User; | ||
| var organization = request.Organization; | ||
| var allOrganizationMemberships = await organizationUserRepository.GetManyByUserAsync(user.Id); |
There was a problem hiding this comment.
This method GetManyByUserAsync(user.Id) will miss invited users since they'll have an email but not a UserId. The stored procedures that PolicyRequirementQuery uses check for both UserId and email addresses.
There was a problem hiding this comment.
This isn't used to pull policies, this is used to check if the user is part of any other organizations if Single Org or Autoconfirm apply. That restriction doesn't apply to invited users, so that's OK. It mirrors the existing check here: https://github.com/bitwarden/server/blob/main/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs#L161
JimmyVo16
left a comment
JimmyVo16
left a comment
There was a problem hiding this comment.
I went ahead and approved this in case my comment turns out to be a non-issue, since we have a pretty big time zone gap.
|
@eliykat I tested this against my work in bitwarden/clients#21574, and it does fix the issue w/ admin account recovery enrollment not working. Before: PM-39706.-.Open.Org.Invite.Link.-.Non.SSO.Org.with.Admin.Acct.Recovery.Policy.-.New.User.Registration.+.Acceptance.Works.+.User.isn.t.enrolled.bug.movAfter: PM-39706.-.Open.Org.Invite.Link.-.Non.SSO.Org.with.Admin.Acct.Recovery.Policy.-.New.User.Registration.+.Acceptance.Works.+.User.is.enrolled.in.admin.account.recovery.mov
|
thank you for the quick turn around on the fix!
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41481
📔 Objective
Target organization policy checks were not being enforced properly via the accept invite link flow for new users. This is because
PolicyRequirementQueryexpects anInvitedorganizationUser to exist for pre-acceptance checks, assuming that a direct invite has been sent. However, the invite link flow handles 3 separate states:PolicyRequirementQueryto query/joinPolicyRequirementQuerysprocsPolicyRequirementQueryThis affected:
The solution here is not DRY, but is relatively contained and is intended to unblock the feature asap.
Rather than try to separate out these 3 possible states, reimplement the policy checks that we need:
PolicyRequirementQueryfor checking other organizations' policiesThis breaks the
PolicyRequirementQueryabstraction, however in my view this shows that it's too closely tied to database state and we need to revisit this (milestone 3). This is a temporary workaround in the meantime.Options considered and discarded:
PolicyDetailsintoPolicyRequirementQuery- unclear what the status should be or how the differentIPolicyRequirementimplementations would handle this - too unpredictable.📸 Screenshots