Repository navigation
Fix SMS auth channel on login and signup - #312
Merged
Merged
Conversation
The auth screens assumed the email channel. `AUTH_CHANNEL=sms` is a supported configuration but was unreachable from the UI: - Login collected only an email. With `channel: 'sms'` the API resolved the user by email, so a user without a phone got a 500 from `sendSms`, and an unknown identifier produced a challenge with no recipient — a confirm screen with a blank target where entering a code did nothing. Login now collects a phone when the channel is SMS and sends whichever identifier matches, so the challenge always carries one back. - Signup required an email and made the phone optional regardless of channel, the opposite of what the signup route validates. The required identifier now follows the channel, plus an email whenever `AUTH_TYPE=password`, since password login only accepts one. - Both request bodies are whitelisted. The routes reject unknown fields, so a new form field would otherwise break the request. `normalizePhone` moves from Signup into `utils/phone` now that both screens use it. Also aligns the code-entry screen with the screens around it, which the above makes reachable for the first time. `BasicLayout` was drawing a different card width, position and logo mid-flow, and owned chrome its consumers disagreed about — Onboard was rendering a second logo inside a nested card. The layout is now a centered shell and each screen supplies its own logo and card via the new `AuthLogo`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AUTH_CHANNEL=smsis a supported configuration that could not be used from the UI. Both auth screens assumed the email channel. Verified end to end in the browser against a local API on both channels.Why it was broken
Login collected only an email, so with
channel: 'sms'the API resolved the user by email and then delivered by SMS. Three outcomes:sendSmsthrows "No phone number specified."createChallengereadsphoneoff the request body, which had none. The confirm screen rendered "code sent to ." and typing a code did nothing — its guard requires an identifier, so no request was ever made.Signup had the inverse problem: it required an email and made the phone optional whatever the channel, while
routes/signup.jsrequires the identifier that matches the channel. Under SMS the form demanded an email the API does not want and allowed submitting without the phone it does, so the failure arrived as a server error in the page-level banner instead of on the field.What changed
AUTH_TYPE=password, since/auth/password/loginonly accepts an email. Without that, apassword+smsconfiguration would create phone-only accounts that can never log in.normalizePhonemoves fromSignup.jsintoutils/phone, since both screens now need it.Layout
Making the code flow reachable exposed that
/confirm-codesat onBasicLayoutwhile/loginand/signupuseSplitAuthLayout— different card width, vertical position, logo and background, all swapping mid-flow.BasicLayoutalso owned chrome its consumers disagreed about: Onboard brought its own logo and card, so it rendered two logos in nested cards.BasicLayoutis now a centered shell and each screen supplies its own logo and card through the newAuthLogo./confirm-codedeliberately keepsBasicLayoutrather than moving to the split layout, so the marketing panel stays off the code step.Reviewer notes
Login.jshunk in Fix signup email, user updates and code login #304. It includes that PR'sauthChannel→channelrename and the droppedpasswordfield, so the two will conflict — merge order matters.ENAMETOOLONGand the unique-check exclusion) are untouched here./otp/sendreturns a challenge whether or not the user exists, but/otp/loginthen answers "User not found." versus "Incorrect code.", so the enumeration protection at the send step is undone at the confirm step on both channels.