Skip to content

fix(experience): enforce terms agreement when sign-in flow turns into registration#8835

Merged
xiaoyijun merged 1 commit into
masterfrom
yijun-fix-terms-agreement-on-signin-to-register
May 21, 2026
Merged

fix(experience): enforce terms agreement when sign-in flow turns into registration#8835
xiaoyijun merged 1 commit into
masterfrom
yijun-fix-terms-agreement-on-signin-to-register

Conversation

@xiaoyijun
Copy link
Copy Markdown
Contributor

Summary

When the sign-in experience agreement policy is set to ManualRegistrationOnly ("Require checkbox agreement on registration only"), the verification-code sign-in flow had a gap: signing in with an unregistered email/phone, then confirming the "this account doesn't exist, create a new account?" prompt, created the account without ever showing the terms agreement.

Root cause: agreement is enforced purely on the frontend, and this "sign-in turns into registration" path (use-sign-in-flow-code-verification.ts) submitted the registration directly from the confirm modal's onConfirm, never calling termsValidation(). The dedicated registration form and the social/SSO registration callbacks already validate terms; only this path was missing it.

Changes

  • use-sign-in-flow-code-verification.ts: switch the "create a new account?" confirmation from the callback-based modal to the promise-based usePromiseConfirmModal, and on confirm run termsValidation() before registering. termsValidation() is a no-op for Automatic, when terms are not configured, or when the user already agreed (e.g. Manual, where agreement is collected up front on the sign-in form), so no double prompt occurs.
  • ConfirmModalProvider: defensively clear callbackRef when a promise-based modal opens, so a stale callback from a previous callback-based modal can't be invoked when the promise modal is confirmed/cancelled. This also makes it safe to open a promise modal from within a callback modal's handler.

Behavior by policy on this path: Automatic → no prompt (unchanged); ManualRegistrationOnlynow prompts before account creation; Manual → already agreed on the sign-in form, no change.

Testing

unit tests — added a case in VerificationCode/index.test.tsx asserting that under ManualRegistrationOnly, the terms modal is shown and registration only happens after the user agrees. Verified it fails without the fix and passes with it.

Checklist

  • .changeset
  • unit tests
  • integration tests
  • necessary TSDoc comments

Copilot AI review requested due to automatic review settings May 20, 2026 05:50
@xiaoyijun xiaoyijun requested a review from a team May 20, 2026 05:50
@xiaoyijun xiaoyijun requested a review from gao-sun as a code owner May 20, 2026 05:50
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 20, 2026

COMPARE TO master

Total Size Diff 📈 +5.62 KB

Diff by File
Name Diff
.changeset/enforce-terms-on-signin-to-register.md 📈 +550 Bytes
packages/experience/src/Providers/ConfirmModalProvider/index.tsx 📈 +392 Bytes
packages/experience/src/containers/VerificationCode/index.test.tsx 📈 +4.13 KB
packages/experience/src/containers/VerificationCode/use-sign-in-flow-code-verification.ts 📈 +588 Bytes

Comment thread packages/experience/src/containers/VerificationCode/index.test.tsx Fixed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR closes a gap in the Experience verification-code sign-in flow where an unregistered identifier could be converted into a registration without enforcing the configured terms agreement (specifically ManualRegistrationOnly). It does so by ensuring the “sign-in turns into registration” path runs the same terms validation as other registration entry points.

Changes:

  • Update the verification-code sign-in → register confirmation to use the promise-based confirm modal and run termsValidation() before registration submission.
  • Harden ConfirmModalProvider by clearing stale callback refs when opening a promise-based modal.
  • Add a unit test covering ManualRegistrationOnly to ensure terms are prompted before account creation, plus a changeset entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/experience/src/Providers/ConfirmModalProvider/index.tsx Clears stale callback refs when opening a promise-based modal to prevent accidental invocation.
packages/experience/src/containers/VerificationCode/use-sign-in-flow-code-verification.ts Ensures terms agreement is validated before registering when sign-in flow becomes registration.
packages/experience/src/containers/VerificationCode/index.test.tsx Adds a unit test for the sign-in→register path under ManualRegistrationOnly.
.changeset/enforce-terms-on-signin-to-register.md Documents the patch release change for @logto/experience.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/experience/src/containers/VerificationCode/index.test.tsx Outdated
Comment thread packages/experience/src/containers/VerificationCode/index.test.tsx Outdated
@xiaoyijun xiaoyijun force-pushed the yijun-fix-terms-agreement-on-signin-to-register branch from efed02e to b139cab Compare May 20, 2026 06:22
@xiaoyijun xiaoyijun requested a review from Copilot May 20, 2026 06:22
@github-actions github-actions Bot added size/m and removed size/m labels May 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@xiaoyijun xiaoyijun merged commit 346816a into master May 21, 2026
44 of 45 checks passed
@xiaoyijun xiaoyijun deleted the yijun-fix-terms-agreement-on-signin-to-register branch May 21, 2026 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants