[PM-35830] Add ChangeEmailCommand

[ike-kottlowski]

🎟️ Tracking

PM-35830

📔 Objective

Introduces a new ChangeEmailCommand that encapsulates the user email change flow.

• Adds IChangeEmailCommand / ChangeEmailCommand under Bit.Core.Auth.UserFeatures.UserEmail, including claimed-domain enforcement, duplicate-email rejection, Stripe customer email sync with rollback on failure, and post-change push notifications (logout vs. settings sync depending on whether the user has a master password).
• Adds ChangeEmailCommandTests covering claimed-domain enforcement, disabled claiming orgs, duplicate email, same-user email, Stripe sync, Stripe rollback, and push branching.

This PR introduces the command only; switching callers from UserService.ChangeEmailAsync to the new command will follow in a separate PR.

Introduces a Admin Console Query to check if the email domain is allowed to be used.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.47%. Comparing base (d903096) to head (9232b47).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7650      +/-   ##
==========================================
- Coverage   64.86%   60.47%   -4.39%     
==========================================
  Files        2140     2142       +2     
  Lines       94629    94709      +80     
  Branches     8445     8454       +9     
==========================================
- Hits        61378    57275    -4103     
- Misses      31155    35429    +4274     
+ Partials     2096     2005      -91     

☔ View full report in Codecov by Sentry.
📢 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.

[eliykat]

As discussed in Slack, I think that the Admin Console layer can handle this.

Validation such as checking the admin role, making sure the user is claimed, and making sure the new email address is claimed, is all part of the admin/organization domain that we will handle.

The cross-team dependency on Auth is much narrower: we don't own the User domain and we don't know how to update a user's email (in the narrow sense). So that is the main thing we are looking for here.

I think if you remove the claimed domain check in this method, everything else LGTM. Thank you!

[ike-kottlowski]

Spoke with @jrmccannon yesterday and we found the correct approach. This is addressed in this commit: fd534bf

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[eliykat]

Letting @jrmccannon handle this one as he's discussed the approach with you already.

[ike-kottlowski]

@claude please review this PR.

[claude]

Claude finished @ike-kottlowski's task in 2m 56s —— View job

---

Claude PR Review

• Read PR description and project context
• Read all changed files
• Analyze for bugs, security issues, code quality
• Post review feedback

---

Nice, well-tested command. The split between the cross-team IOrganizationDomainAllowEmailChangeQuery (owned by Admin Console / claimed-domain logic) and the ChangeEmailCommand (owned by Auth / the narrow act of updating an email) reads cleanly. The test coverage on both the query and command is thorough (claiming/non-claiming orgs, disabled orgs, multi-org, Stripe rollback, push-branching, invalid-domain parsing). A few things worth a look before this lands and especially before the callers are migrated in the follow-up PR.

🔴 MasterPasswordSalt drifts from Email — likely auth regression for existing callers

ChangeEmailCommand.cs:40-45

UserService.ChangeEmailAsync deliberately sets user.MasterPasswordSalt = newEmail (twice — on success and on rollback) with an explicit comment:

// We need this to backfill the salt for now to keep the email and salt always in sync.

The new command never touches MasterPasswordSalt. The PR says the follow-up will "switch callers from UserService.ChangeEmailAsync to the new command." If any of those callers represent users with a master password, the post-change row will have MasterPasswordSalt != Email, which the existing comment warns against. Tests confirm the omission is intentional (ChangeEmailAsync_NonStripeUserWithMasterPassword_UpdatesFieldsAndPushesLogout does not assert salt behavior at all).

Either:

1. The new command is intentionally passwordless-only (it currently has no such guard — HasMasterPassword() branches in the push, but nothing rejects master-password users). Add a guard + interface comment.
2. The command supports both, and needs to preserve the salt-sync invariant (and roll it back on Stripe failure).

Worth resolving before the follow-up migrates callers — silent salt drift is the kind of thing that surfaces as "cannot log in" much later.

🟡 Stripe-failure rollback path swallows context

ChangeEmailCommand.cs:48-65

The catch is bare (catch { … throw; }) — any exception type triggers rollback, including ones we wouldn't want to swallow into rollback semantics. Two suggestions:

• Inject ILogger<ChangeEmailCommand> and log the Stripe failure before rolling back. RegisterUserCommand already logs analogous events (RegisterUserCommand.cs:402-405). Without it, a flaky Stripe path is invisible in production.
• Catch a typed exception (StripeException or similar). A bare catch will also intercept OperationCanceledException, which then triggers a rollback ReplaceAsync against a possibly-cancelled context.

If the rollback ReplaceAsync itself throws, the original Stripe exception is lost — consider wrapping the rollback write in its own try/catch and logging at minimum.

🟡 Interface docstring contradicts the implementation

IChangeEmailCommand.cs:15-25 vs ChangeEmailCommand.cs:24-32

The interface tells callers they MUST perform organization-domain policy enforcement before invoking the command. The implementation then performs that exact check itself (EnsureNewEmailDomainAllowedByPolicyAsync). Two interpretations:

1. The implementation does it as defense-in-depth — say so in the docstring so callers don't think it's their job.
2. The check belongs in the caller (this aligns with the review feedback from @eliykat about Admin Console handling its own domain). Then the command shouldn't redo it.

Right now the docstring and code disagree, which makes it ambiguous for the follow-up PR's caller migration.

🟢 Same-domain short-circuit comparison is case-sensitive

ChangeEmailCommand.cs:89

if (newDomain == CoreHelpers.GetEmailDomain(user.Email))

CoreHelpers.GetEmailDomain returns the raw domain substring (trimmed only). A user whose stored email is me@Example.com changing to me@example.com won't short-circuit — they'll fall through to the full policy lookup. Functionally correct, but the comment says "noop" and it isn't quite. Compare with StringComparison.OrdinalIgnoreCase.

🟢 GatewayCustomerId null-check changes silent-skip behavior

ChangeEmailCommand.cs:52

UserService.ChangeEmailAsync (line 446) calls UpdateCustomerEmailAddressAsync whenever Gateway == Stripe, which would ArgumentNullException on a null customer ID. The new command silently skips. Probably the intended improvement, but it's a behavior change worth calling out in the PR description so the follow-up migration is reviewed with this in mind. The test ChangeEmailAsync_StripeUserWithoutGatewayCustomerId_SkipsSyncAndCompletes codifies the new behavior.

🟢 Minor doc nit

ChangeEmailCommand.cs:78-82

The comment says the gate "mirrors the gate enforced by RegisterUserCommand." RegisterUserCommand.ValidateEmailDomainNotBlockedAsync only checks the BlockClaimedDomainAccountCreation policy — it does not check whether the registering user is already claimed (because they're new). The new command additionally enforces the claiming-org-verified-domain rule via IsAllowedAsync. The two gates aren't quite mirrors; this is a richer check. Either reword or remove the comparison.

Nice things

• The query separation lets the claimed-domain logic be owned by Admin Console (matches @eliykat's review direction) while keeping the Auth-owned email-update narrow.
• Test naming is excellent — each test reads as a behavior spec.
• The rollback test (…RestoresPreviousEmailAndRevisionDatesThenRethrows) is exactly the test that prevents this code from regressing.
• OrganizationDomainAllowEmailChangeQuery tests cover the cross-product of claimed-vs-not and blocked-vs-not, including the multi-org and mixed-enabled cases.

---

• Branch: auth/pm-35830/change-email-command