Auth/PM-41503 and PM-41533 - Registration - Add open org invite flow support - #8159
Conversation
Narrower AC-owned query that only verifies an open-org-invite link is real (link exists + code matches, org exists + enabled, invite links feature enabled). Sits alongside GetOrganizationInviteLinkStatusQuery, which does additional seat/SSO work meant for display flows. Registration paths only need the validation, so pulling that into its own query avoids the extra seat-count round-trip on every registration attempt that carries an open-org-invite payload.
…e is provided
Register-start and register-finish now accept an OpenOrgInvite payload
({organizationId, code}). When present and validated via the AC-owned
IValidateOrganizationInviteLinkQuery, the invite's organization is
excluded from the BlockClaimedDomain policy check so a user reaching
registration via that org's invite link can proceed with an email whose
domain the org has claimed. Attacker scenarios (mismatched org) remain
blocked because the exclusion is scoped to the specific invite.
Register-finish gains a dedicated
RegisterUserViaEmailVerificationTokenAndOpenOrgInvite method so future
open-invite obligations (2FA policy handling, etc. — PM-41533) can accrue
there without complicating the vanilla EmailVerification path. A DTO
validation rule rejects OpenOrgInvite alongside any non-EmailVerification
token type.
JaredSnider-Bitwarden
added
t:feature
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the open-org-invite registration flow across both endpoints: Code Review Details
Verified and intentionally not flagged: |
… for open-org-invite registration Extend the open-org-invite registration path so that when the target organization has "Require 2FA" enabled the user is initialized with Email 2FA before the User row is persisted, mirroring the direct-invite path's timing. On success the org is looked up and the org-aware welcome email variant is dispatched.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…AllowedDomains
ValidateOrganizationInviteLinkQuery previously proved only that the
link existed, the code matched, and the org was enabled with
UseInviteLinks. That allowed a bearer of the public {orgId, code} to
receive the claimed-domain-block exclusion even when the link's
AllowedDomains would reject the registering email at accept time.
Extend the validator to also require the email's domain to be in the
link's AllowedDomains (using InviteLinkDomainValidator, matching every
other invite-link consumer), returning the canonical EmailDomainNotAllowed
error when it isn't. Both register-start and register-finish now pass
the caller's email through to the validator, so the domain-block
exclusion is only granted when the link would actually admit that email.
Adds AC-owned unit tests for the new check, an integration test at
register-start proving the gap and its closure, and a mirror test at
register-finish so a future single-caller regression is caught.
…orgId helper The direct-invite helper's body was byte-for-byte identical to SetUserEmail2FaIfOrgPolicyEnabledByOrgIdAsync once the org id was in hand, so any future change to how Email 2FA is seeded would have to be made twice (or the two registration paths would silently drift). The outer helper still owns the org-user lookup and the "return the OrganizationUser to the caller" contract; only the 2FA seeding delegates.
…k feature flag Every other invite-link surface (create/get/update/refresh/delete/status/ policies/validate-email-domain, plus a couple of Organization/OrganizationUsers endpoints) carries [RequireFeature(FeatureFlagKeys.GenerateInviteLink)]. Registration was the only entry point still honoring OpenOrgInvite payloads regardless of the flag, so flipping the flag off would silence the admin/API surfaces while registration kept granting the claimed-domain-block exclusion for links whose management endpoints had already stopped responding. Guards PostRegisterSendVerificationEmail and PostRegisterFinish with an inline check that throws FeatureUnavailableException (→ 404) to match how [RequireFeature] behaves on the sibling surfaces. Every existing integration test that exercises OpenOrgInvite now declaratively turns the flag on, and a new register-start test turns it off and asserts 404.
…xture The new OpenOrgInvite property on RegisterFinishRequestModel was being auto-populated by AutoFixture in every integration test that seeds a user via /accounts/register/finish, routing them into the new open-invite branch and failing token validation with HTTP 400.
…test The new PostRegisterSendEmailVerification_WithOpenOrgInvite_InvalidLink integration test was using the shared factory, which has the feature flag off, so the controller's guard returned 404 instead of the 400 the invite-link validator produces. Use a local factory with the flag enabled, matching the pattern used by the other OpenOrgInvite tests in this file.
Both tests assert FeatureUnavailableException, which maps to 404 (not 400). Renaming to _ThrowsFeatureUnavailable makes the intended status semantics unambiguous and matches the sibling integration test's naming.
| /// feature disabled; <see cref="EmailDomainNotAllowed"/> if the email's domain is not in | ||
| /// the link's AllowedDomains. | ||
| /// </returns> | ||
| Task<CommandResult> ValidateAsync(Guid organizationId, Guid code, string email); |
There was a problem hiding this comment.
♻️ DEBT: This is signature-identical to the pre-existing IValidateOrganizationInviteLinkEmailDomainQuery in the same namespace, with inverted success semantics.
Details and suggestion
Both live in Bit.Core.AdminConsole.OrganizationFeatures.InviteLinks.Interfaces and expose:
Task<CommandResult> ValidateAsync(Guid organizationId, Guid code, string email); // new
Task<CommandResult<bool>> ValidateAsync(Guid organizationId, Guid code, string email); // existingThe failure modes read the same at the call site but mean opposite things. For the new query, a disallowed domain is IsError. For the existing one, a disallowed domain is IsSuccess == true with AsSuccess == false. A future caller that injects the existing interface and writes the if (result.IsError) throw; pattern used in RegisterUserCommand and SendVerificationEmailForRegistrationCommand gets a silently-passing domain check on a security boundary.
Separately, the link-lookup + CodeMatches + org enabled/UseInviteLinks sequence is now the third copy (see GetOrganizationInviteLinkStatusQuery and ValidateOrganizationInviteLinkEmailDomainQuery). If invite-link validity ever gains a term (revocation, expiry, seat gating), the registration path is the one most likely to be missed.
Two low-cost options: give this one a name that conveys the fuller scope (e.g. IValidateOrganizationInviteLinkForRegistrationQuery), and/or have it delegate the domain check to IValidateOrganizationInviteLinkEmailDomainQuery so the AllowedDomains rule has a single owner.
The XML doc already contrasts this query with IGetOrganizationInviteLinkStatusQuery; the EmailDomain sibling is the closer overlap and worth calling out there at minimum.
There was a problem hiding this comment.
@eliykat , I could use some feedback here. I think the ValidateOrganizationInviteLinkEmailDomainQuery is not actually sufficient for our needs as it doesn't perform all the checks that we need re ensuring the link is still valid? So... I think this is fine.
There was a problem hiding this comment.
I wasn't familiar with ValidateOrganizationInviteLinkEmailDomainQuery. It does the same thing as you want here, although I think your implementation is better, because:
- it checks the organization properties (enabled, useInviteLinks)
- it uses the CommandResult in a clearer way.
To address this, please:
- replace the current
ValidateOrganizationInviteLinkEmailDomainQuerywith yours - update the current caller to match the new query interface, but without changing the api contract:
// OrganizationInviteLinksController.ValidateEmailDomain
var result = await validateOrganizationInviteLinkQuery.ValidateAsync(model.OrganizationId, model.Code, model.Email);
// Preserve the existing client contract: report the domain check as an IsAllowed boolean
// rather than surfacing a disallowed domain as an error status.
if (result is { IsError: true, AsError: EmailDomainNotAllowed })
{
return TypedResults.Ok(new OrganizationInviteLinkValidateEmailDomainResponseModel(false));
}
return Handle(result, _ =>
TypedResults.Ok(new OrganizationInviteLinkValidateEmailDomainResponseModel(true)));
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8159 +/- ##
===========================================
+ Coverage 15.10% 63.07% +47.97%
===========================================
Files 1417 2318 +901
Lines 61422 100594 +39172
Branches 4901 9055 +4154
===========================================
+ Hits 9279 63454 +54175
+ Misses 51978 34948 -17030
- Partials 165 2192 +2027 ☔ 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.
Good fix, just some adjustments to comments and aligning it with existing code.
| return new InviteLinkNotFound(); | ||
| } | ||
|
|
||
| var organization = await organizationRepository.GetByIdAsync(inviteLink.OrganizationId); |
There was a problem hiding this comment.
Should use IOrganizationAbilityCacheService.GetOrganizationAbilityAsync instead. (cache vs. db call)
| /// feature disabled; <see cref="EmailDomainNotAllowed"/> if the email's domain is not in | ||
| /// the link's AllowedDomains. | ||
| /// </returns> | ||
| Task<CommandResult> ValidateAsync(Guid organizationId, Guid code, string email); |
There was a problem hiding this comment.
I wasn't familiar with ValidateOrganizationInviteLinkEmailDomainQuery. It does the same thing as you want here, although I think your implementation is better, because:
- it checks the organization properties (enabled, useInviteLinks)
- it uses the CommandResult in a clearer way.
To address this, please:
- replace the current
ValidateOrganizationInviteLinkEmailDomainQuerywith yours - update the current caller to match the new query interface, but without changing the api contract:
// OrganizationInviteLinksController.ValidateEmailDomain
var result = await validateOrganizationInviteLinkQuery.ValidateAsync(model.OrganizationId, model.Code, model.Email);
// Preserve the existing client contract: report the domain check as an IsAllowed boolean
// rather than surfacing a disallowed domain as an error status.
if (result is { IsError: true, AsError: EmailDomainNotAllowed })
{
return TypedResults.Ok(new OrganizationInviteLinkValidateEmailDomainResponseModel(false));
}
return Handle(result, _ =>
TypedResults.Ok(new OrganizationInviteLinkValidateEmailDomainResponseModel(true)));| /// <summary> | ||
| /// Validates that an open organization invite link is usable for the given email — narrower | ||
| /// than <see cref="IGetOrganizationInviteLinkStatusQuery"/>, which additionally computes seat | ||
| /// availability and SSO status for display to a landing user. This validator exists for flows | ||
| /// (e.g., registration) that only need to confirm the link is real, valid, and admits the | ||
| /// caller's email — the last check gates the domain-block bypass that the caller applies on | ||
| /// success, so possession of the {orgId, code} alone must not be sufficient when the link's | ||
| /// AllowedDomains would reject the email at accept time. | ||
| /// </summary> | ||
| /// <param name="organizationId">The organization's ID (from the URL path).</param> | ||
| /// <param name="code">The public invite link code.</param> | ||
| /// <param name="email">The registering user's email; checked against the link's AllowedDomains.</param> | ||
| /// <returns> | ||
| /// Void success if the link is valid and admits the email; <see cref="InviteLinkNotFound"/> | ||
| /// if the link does not exist, the code does not match, or the organization is missing or | ||
| /// disabled; <see cref="InviteLinkNotAvailable"/> if the organization has the invite links | ||
| /// feature disabled; <see cref="EmailDomainNotAllowed"/> if the email's domain is not in | ||
| /// the link's AllowedDomains. | ||
| /// </returns> |
There was a problem hiding this comment.
Claude xmldoc :((( Way too wordy and too much context associated with this specific PR.
| /// <summary> | |
| /// Validates that an open organization invite link is usable for the given email — narrower | |
| /// than <see cref="IGetOrganizationInviteLinkStatusQuery"/>, which additionally computes seat | |
| /// availability and SSO status for display to a landing user. This validator exists for flows | |
| /// (e.g., registration) that only need to confirm the link is real, valid, and admits the | |
| /// caller's email — the last check gates the domain-block bypass that the caller applies on | |
| /// success, so possession of the {orgId, code} alone must not be sufficient when the link's | |
| /// AllowedDomains would reject the email at accept time. | |
| /// </summary> | |
| /// <param name="organizationId">The organization's ID (from the URL path).</param> | |
| /// <param name="code">The public invite link code.</param> | |
| /// <param name="email">The registering user's email; checked against the link's AllowedDomains.</param> | |
| /// <returns> | |
| /// Void success if the link is valid and admits the email; <see cref="InviteLinkNotFound"/> | |
| /// if the link does not exist, the code does not match, or the organization is missing or | |
| /// disabled; <see cref="InviteLinkNotAvailable"/> if the organization has the invite links | |
| /// feature disabled; <see cref="EmailDomainNotAllowed"/> if the email's domain is not in | |
| /// the link's AllowedDomains. | |
| /// </returns> | |
| /// <summary> | |
| /// Validates that an open organization invite link is valid and that the email | |
| /// matches its allowed domains. It does NOT check that the email has been | |
| /// verified - the caller must check this separately if required. | |
| /// </summary> | |
| /// <param name="organizationId">The organization's ID (from the URL path).</param> | |
| /// <param name="code">The public invite link code.</param> | |
| /// <param name="email">The registering user's email; checked against the link's AllowedDomains.</param> | |
| /// <returns> | |
| /// A successful CommandResult if the link is valid and matches the email; a failed | |
| /// CommandResult if validation fails or the invite link is otherwise not available. | |
| /// </returns> |
| // Gate on the same check every other invite-link consumer uses. Possession of a valid | ||
| // {orgId, code} is not sufficient — the link must actually admit this email domain. | ||
| // The caller applies a domain-block-policy exclusion on success, so an insufficient | ||
| // check here would let a bearer of the code bypass a claimed-domain block by | ||
| // registering an email the link would reject at accept time. |
There was a problem hiding this comment.
Unnecessary comment - it shouldn't document its caller. Please delete.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41503
https://bitwarden.atlassian.net/browse/PM-41533
📔 Objective
To add proper registration support for registering with a normal email verification token and an open org invite link in client state. This involves not blocking the domain of the link if it is claimed by that org as well as enforcing 2FA required if the org has that enabled.
📸 Screenshots
Integration tests are sufficient.