ENG-3013: Add verify email flow for admin users#8180
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (92.43%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8180 +/- ##
==========================================
+ Coverage 85.08% 85.10% +0.02%
==========================================
Files 670 671 +1
Lines 43564 43683 +119
Branches 5111 5124 +13
==========================================
+ Hits 37065 37175 +110
- Misses 5394 5399 +5
- Partials 1105 1109 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/code-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — ENG-3013: Email verification flow for admin users
Overall this is a well-structured, well-tested PR. The implementation closely mirrors the existing password-reset flow, the backend tests cover all the meaningful edge cases (expired token, wrong token, no record, dispatch failure), and the Cypress suite covers the full banner lifecycle including snooze TTL and email-change invalidation. A few issues worth addressing, ranging from a security gap to polish items.
Security
Disabled user can complete verification and obtain a session (user_service.py:442)
verify_email_with_token does not check user.disabled before calling perform_login. A user disabled after requesting verification can still consume the token and receive a valid session. request_email_verification already guards against this on the request side — the token-consumption side should mirror it. See inline comment for suggested fix.
Correctness
Non-atomic create_or_replace (fides_user_email_verification.py:49)
The delete → create pattern is inherited from FidesUserPasswordReset and the risk is low in practice, but if create raises after delete has committed, the user loses their record and must request again with no indication of what happened. An upsert would eliminate this window; at minimum, a DB savepoint around the two operations would make rollback reliable.
Token record not deleted on failed token_valid() check (user_service.py:468)
A wrong token leaves the record in place, allowing repeated guessing attempts within the rate-limit window. The TTL and rate limiter together are a reasonable mitigation, and the existing password-reset path has the same behavior. Worth a conscious follow-up decision.
Minor / polish
-
is_expired()timezone assumption (fides_user_email_verification.py:82):self.created_atis aDateTime(timezone=True)column so SQLAlchemy should return tz-aware objects from Postgres, but if the value is set tz-naively in a test or migration, the comparison withdatetime.now(timezone.utc)will raiseTypeError. The pattern is identical inFidesUserPasswordResetso it works in practice, but explicit tzinfo handling would be safer. -
eslint-disablefor hooks deps (verify-email.tsx:54): The comment explaining whyverifyEmailWithTokenis excluded is accurate, but auseRef-based "has-fired" guard would let you keep the deps array complete without suppressing the lint rule. See inline comment for a pattern. -
EmailVerificationBannerloading state (EmailVerificationBanner.tsx:60): NoisLoadingguard on the email-invite-status query. Behavior is already correct (rendersnullwhile undefined), but an explicitisLoadingcheck would make the intent clearer and prevent a visible banner pop-in on slow connections. -
_app.tsxOR chain (_app.tsx:62): Now at 4 entries; the comment already flags the refactor need. Good candidate for a follow-up ticket. -
Email template plaintext fallback (
email_verification.html:9): The verification URL only exists in an anchorhref. Adding a plaintext copy-paste URL below the link improves deliverability in plain-text clients and strict corporate email filters.
🔬 Codegraph: connected (50455 nodes)
💡 Write /code-review in a comment to re-run this review.
| Raises: | ||
| FidesError: If the token is invalid, expired, or the user is not found. | ||
| """ | ||
| user = FidesUser.get_by(self.db, field="username", value=username) |
There was a problem hiding this comment.
src/fides/service/user/user_service.py:442
verify_email_with_token does not check whether the user is disabled before issuing a session token. If a user is disabled after they request a verification email (but before they click the link), the token would still complete successfully and grant them an active session via perform_login.
request_email_verification already guards against disabled users (line 374), but the consuming side doesn't mirror that check.
Suggested fix — add immediately after the user lookup succeeds:
if user.disabled:
raise FidesError("Invalid or expired verification token.")Using the same opaque error message prevents disclosing that the account is disabled to an unauthenticated caller.
Note:
reset_password_with_tokenhas the same gap; worth patching there in a follow-up if not here.
| matching_verification.delete(self.db) | ||
| raise FidesError("Invalid or expired verification token.") | ||
|
|
||
| if not matching_verification.token_valid(token): |
There was a problem hiding this comment.
src/fides/service/user/user_service.py:468
When token_valid() returns False, the verification record is left intact. Within the rate-limit window, a caller can make repeated guesses against the same record without consuming it. The TTL provides a natural expiry, but explicitly deleting the record on a failed validation attempt would be a tighter control (at the cost of forcing the user to request a fresh token on any typo/link corruption).
This is consistent with how reset_password_with_token behaves, so happy to treat it as a follow-up, but worth a conscious decision here given that the token is only 36 bytes of UUID entropy and the rate limiter is the only active countermeasure.
| cls, db: Session, *, user_id: str, token: str | ||
| ) -> FidesUserEmailVerification: | ||
| """Create a verification record, replacing any existing one for this user.""" | ||
| existing = cls.get_by(db, field="user_id", value=user_id) |
There was a problem hiding this comment.
src/fides/api/models/fides_user_email_verification.py:49
create_or_replace performs a delete followed by a separate create. If super().create() raises after existing.delete(db) has already committed (e.g., a DB constraint violation or connection error), the user is left with no verification record and would need to request a new token — even though they just clicked the CTA.
This is the same pattern used in FidesUserPasswordReset.create_or_replace, so the risk is accepted elsewhere. A safer alternative would be an upsert (e.g., INSERT … ON CONFLICT (user_id) DO UPDATE SET …), which would make the operation atomic, but that's a broader refactor. At minimum, consider wrapping this in a savepoint/subtransaction so the delete is rolled back if the create fails.
| return True | ||
|
|
||
| ttl_minutes = CONFIG.security.email_verification_token_ttl_minutes | ||
| expiration_datetime = self.created_at + timedelta(minutes=ttl_minutes) |
There was a problem hiding this comment.
src/fides/api/models/fides_user_email_verification.py:82
self.created_at is a DateTime(timezone=True) column, so SQLAlchemy should return a tz-aware object from PostgreSQL. However, if created_at happens to be tz-naive (e.g., set directly in tests without a timezone argument), comparing against datetime.now(timezone.utc) (tz-aware) will raise a TypeError at runtime.
The same pattern exists in FidesUserPasswordReset.is_expired, so this likely works in practice, but it's worth being explicit:
expiration_datetime = self.created_at.replace(tzinfo=timezone.utc) if self.created_at.tzinfo is None else self.created_at
expiration_datetime = expiration_datetime + timedelta(minutes=ttl_minutes)Or simply ensure all test fixtures set created_at with timezone.utc.
| }).unwrap(); | ||
| if (cancelled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
clients/admin-ui/src/pages/verify-email.tsx:54
The eslint-disable-next-line react-hooks/exhaustive-deps suppresses a real warning: verifyEmailWithToken is excluded from the dependency array. The comment justification ("RTK Query hook tuples are stable in practice") is accurate, but suppressing the rule means any future change to this effect won't get a lint warning.
A common alternative that avoids the suppression is to hold a ref to whether the request has already fired:
const hasFiredRef = useRef(false);
useEffect(() => {
if (!router.isReady || hasFiredRef.current) return undefined;
// ...
hasFiredRef.current = true;
// fire request
}, [router.isReady, router.query.username, router.query.token, verifyEmailWithToken]);This keeps the deps array complete and prevents any double-fire without relying on implementation details of RTK Query.
| const { data: emailInviteStatus } = useGetEmailInviteStatusQuery(undefined, { | ||
| skip: !user || Boolean(user.email_verified_at), | ||
| }); | ||
| const [requestEmailVerification, { isLoading: isRequesting }] = |
There was a problem hiding this comment.
clients/admin-ui/src/features/common/EmailVerificationBanner.tsx:60
The useGetEmailInviteStatusQuery call has no loading state guard. When emailInviteStatus is undefined (query in-flight), !emailInviteStatus?.enabled evaluates to true and the component renders null, which is the right behavior. But there's a subtle UX edge: if the query is slow (e.g., cold start), an eligible user will see the banner appear with a short delay after the page loads.
This is a minor polish issue — adding an explicit isLoading check and rendering null during load would make the intent clearer, though the behavior is already correct:
const { data: emailInviteStatus, isLoading: emailInviteStatusLoading } = useGetEmailInviteStatusQuery(...);
// ...
if (emailInviteStatusLoading || !emailInviteStatus?.enabled) return null;| // Only the login page is accessible while logged out. If there is | ||
| // a use case for more unprotected routes, Next has a guide for | ||
| // per-page layouts: | ||
| Component === ForgotPassword || |
There was a problem hiding this comment.
clients/admin-ui/src/pages/_app.tsx:62
The existing comment already flags this OR chain as a candidate for refactoring. With VerifyEmail now added as the 4th entry, and the pattern likely to grow (any future unprotected route must be added here manually), this feels like the right moment to establish a per-page opt-out mechanism — e.g., a getLayout static property or a noAuth export on the page component — rather than accumulating more entries. That said, this could be a follow-up tracked as tech debt rather than blocking this PR.
| </head> | ||
| <body> | ||
| <main> | ||
| <p>Please <a href="{{admin_ui_url}}/verify-email?token={{verification_token}}&username={{username | urlencode}}">verify your Fides account email address</a> to enable account recovery features such as self-service password reset.</p> |
There was a problem hiding this comment.
src/fides/api/email_templates/templates/email_verification.html:9
The verification URL is only present as the href of an anchor tag. If an email client strips HTML or the user's client renders plain text, there's no visible URL to copy/paste. The password_reset.html template has the same limitation, so this is consistent, but it's worth noting as a potential deliverability/UX issue — some strict corporate email filters also rewrite or block HTML links.
Consider adding a fallback plaintext URL below the link (as many well-known verification emails do):
<p>Or copy and paste this URL into your browser:<br>
{{admin_ui_url}}/verify-email?token={{verification_token}}&username={{username | urlencode}}</p>
nreyes-dev
added
the
do not merge
1 participant
Ticket ENG-3013
Description Of Changes
Follow-up to ENG-2931 (self-service password reset). That PR introduced
email_verified_atonFidesUserand started setting it during invite acceptance, but pre-existing admin users — i.e. anyone who didn't accept an invite — still have itNULLand so can't use forgot-password. This adds the self-serve verification path for that cohort: an unverified user sees a banner CTA that emails them a tokenized link, and the/verify-emailpage consumes the token, setsemail_verified_at, and auto-logs them in. Gated on email messaging being configured (otherwise the flow can't complete) and on the user not being SSO-only in Plus.Code Changes
FidesUserEmailVerificationmodel + alembic migration (835de27d8c76) for hashed, single-use, TTL-bound verification tokens.email_verification_token_ttl_minutessetting inSecuritySettings(default 30, range 15–60).MessagingActionType.EMAIL_VERIFICATION,EmailVerificationBodyParamsschema,email_verification.htmltemplate, and dispatch-service branch.UserService.request_email_verificationandUserService.verify_email_with_token.POST /user/request-email-verification(authenticated) andPOST /user/verify-email-with-token(unauthenticated, returnsUserLoginResponse) endpoints.email_verification.requested/.completed/.token_expiredaudit event types.EmailVerificationBannerrendered globally from_app.tsx, gated onmessaging/email-invite/status, hidden for verified users and for SSO-only users (password_login_enabled === false). Three visual states: unverified-with-email, no-email-yet, and "sent" confirmation. 7-day localStorage snooze keyed onuserId:email_addressso an email change re-shows the banner./verify-emailNext.js page added to the unprotected-routes branch in_app.tsx.requestEmailVerificationandverifyEmailWithTokenRTK Query mutations inauth.slice.ts.Tagnext to the email field inUserForm, with the verification date in a tooltip on the verified state. Hidden when not verified AND messaging is disabled (no actionable path).#email_addressdeep-link handling inUserForm: scrolls the email field into view, focuses it, and adds a brief blue ring — so the banner's "Add email" CTA lands users directly on the field.fides_user_email_verificationtable in.fides/db_dataset.yml, sofides_db_scanstays green.tests/ops/api/v1/endpoints/test_email_verification.py(14 tests coveringrequest_email_verificationandverify_email_with_tokenacross unauthenticated rejection, already-verified / no-email / disabled-user / messaging-unconfigured skips, token creation + replacement, dispatch failure auditing, valid/invalid/missing/wrong/expired token paths) and Cypress specs incypress/e2e/email-verification.cy.ts.Steps to Confirm
email_addressset,email_verified_at = NULL,disabled = False./user-management, etc.).fides_user_email_verificationand anemail_verification.requestedrow exists inevent_audit.FidesUserEmailVerification.create_or_replace(db, user_id=<id>, token="TEST"), then visit/verify-email?username=<user>&token=TEST. Expect auto-login + redirect to/. In the DB:email_verified_atis now set, the verification row is gone, and anemail_verification.completedaudit event exists.created_ataged past the TTL → same error plus anemail_verification.token_expiredaudit row.email_verified_at, dismiss the banner with ×. Reload — it stays hidden. Set the matchingfides:email-verification-banner-snooze:*localStorage value to a timestamp >7 days ago, reload — banner returns.email_address→ banner switches to "Add an email address" copy. Click Add email → lands on/user-management/profile/<id>#email_addresswith the email field scrolled into view, focused, and briefly ringed in blue.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works