[PM-38796] Create link confirmation endpoint#7907
Conversation
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to create default collection for user confirmed via invite link."); | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7907 +/- ##
==========================================
+ Coverage 61.34% 61.40% +0.05%
==========================================
Files 2244 2248 +4
Lines 98682 98960 +278
Branches 8915 8939 +24
==========================================
+ Hits 60537 60763 +226
- Misses 36008 36050 +42
- Partials 2137 2147 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JimmyVo16
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the new invite-link confirmation endpoint: the Code Review Details
Previously raised threads on this PR still apply (not re-posted): missing audit-log event on confirm, doc comment claiming emergency-access removal the implementation does not perform, leftover TODO in the validator, and the question about the omitted device push-sync.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <summary> | ||
| /// Creates the user's default collection when the Organization Data Ownership policy applies. Failures | ||
| /// are logged but not surfaced: the user is already confirmed and the collection can be recreated. | ||
| /// </summary> | ||
| private async Task CreateDefaultCollectionAsync(Organization organization, OrganizationUser organizationUser, string defaultUserCollectionName) | ||
| { | ||
| try | ||
| { | ||
| if (!await ShouldCreateDefaultCollectionAsync(organization, organizationUser, defaultUserCollectionName)) |
There was a problem hiding this comment.
Details and fix
ConfirmAsync moves a user to Confirmed (a security-sensitive membership change) but logs no event. Both comparable flows do:
ConfirmOrganizationUserCommandlogsEventType.OrganizationUser_Confirmed.AcceptOrganizationInviteLinkCommandlogsEventType.OrganizationUser_InviteLinkAccepted.
Without an event, organization admins have no audit trail of link-confirmed memberships, which is relevant to Bitwarden's SOC 2 / HIPAA obligations. Consider injecting IEventService and logging OrganizationUser_Confirmed for organizationUser after the membership is persisted.
| /// Confirms a user into an organization through an invite link. Runs the shared read-only precheck | ||
| /// (<see cref="IConfirmOrganizationInviteLinkValidator"/>), creates the membership when the user is not | ||
| /// yet a member, confirms it with the supplied organization key, and performs the policy-driven side | ||
| /// effects: creating a default collection for Organization Data Ownership, enrolling the user in account | ||
| /// recovery when required, and removing the user's emergency access. |
There was a problem hiding this comment.
Details and fix
This summary (and the matching remarks in ConfirmOrganizationInviteLinkCommand.cs and ConfirmOrganizationInviteLinkRequest.cs) states the command performs "removing the user's emergency access." ConfirmAsync never calls IDeleteEmergencyAccessCommand — the dependency is not even injected.
The sibling AcceptOrganizationInviteLinkCommand calls deleteEmergencyAccessCommand.DeleteAllByUserIdAsync(user.Id) when the automatic-user-confirmation policy is enabled, and ConfirmOrganizationUserCommand does the same. Since confirmation is the higher-privilege step, this looks like a missing side effect rather than only a stale comment.
Either implement the emergency-access removal (gated on the automatic-user-confirmation policy, mirroring the accept flow) or, if intentionally omitted, remove the claim from all three doc comments so maintainers aren't misled.
| { | ||
| return singleOrgError; | ||
|
|
||
| // TODO: Jimmy reassess this |
There was a problem hiding this comment.
🎨 SUGGESTED: Remove the // TODO: Jimmy reassess this before merge — it's a personal-name TODO left in production code with no actionable context.
| /// <summary> | ||
| /// Confirms an existing membership (a pending email invitation or an accepted membership) by linking | ||
| /// it to the user, releasing the org key, and moving it straight to <see cref="OrganizationUserStatusType.Confirmed"/>. | ||
| /// Persisting via <c>ReplaceAsync</c> bumps the user's account revision date so their other devices sync. | ||
| /// </summary> | ||
| private async Task<CommandResult<OrganizationUser>> ConfirmExistingMembershipAsync( | ||
| OrganizationUser existingOrganizationUser, User user, string orgUserKey) | ||
| { | ||
| existingOrganizationUser.Status = OrganizationUserStatusType.Confirmed; | ||
| existingOrganizationUser.UserId = user.Id; | ||
| existingOrganizationUser.Email = null; | ||
| existingOrganizationUser.Key = orgUserKey; | ||
|
|
||
| await organizationUserRepository.ReplaceAsync(existingOrganizationUser); | ||
|
|
||
| return existingOrganizationUser; | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: Is the omission of a device push-sync intentional here?
Details
The other confirm-to-Confirmed flows push an org-keys sync so the user's already-signed-in devices pick up the new org key immediately:
ConfirmOrganizationUserCommand→PushSyncOrgKeysAsync(plus device-registration cleanup)AutomaticallyConfirmOrganizationUserCommand→PushSyncOrganizationKeysAsync
This command releases the org key (Key = orgUserKey) and moves the membership straight to Confirmed, but injects no IPushNotificationService. The doc comment notes the ReplaceAsync revision-date bump handles other-device sync, but that relies on the next poll rather than an immediate push, and CreateConfirmedMembershipAsync (new membership via CreateAsync) has no equivalent note. Was relying solely on the revision-date bump — with no PushSyncOrgKeysAsync — a deliberate choice for this endpoint?
|
|
||
| // Act | ||
| var response = await joinerClient.PostAsJsonAsync( | ||
| "/organizations/users/invite-link/link-confirm", BuildConfirmRequest(code)); |
There was a problem hiding this comment.
Details and fix
The controller declares the endpoint as:
[HttpPost("/organizations/users/invite-link/confirm")]
public async Task<IResult> ConfirmInviteLink(...)but all three requests in this file POST to /organizations/users/invite-link/link-confirm (lines 80, 104, 118). There is no link-confirm route anywhere in src, so these requests never reach ConfirmInviteLink.
ConfirmInviteLink_WithValidRequest_ReturnsOkAndConfirmsMembership asserts HttpStatusCode.OK and will fail (the missing route returns 404/405). ConfirmInviteLink_WithUnknownCode_ReturnsNotFound passes for the wrong reason — 404 comes from the missing route, not from InviteLinkNotFound.
Change the three request paths to /organizations/users/invite-link/confirm to match the controller.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38796
https://bitwarden.atlassian.net/browse/PM-40048
📔 Objective
📸 Screenshots