-
Notifications
You must be signed in to change notification settings - Fork 212
Fix potential race condition bug in pwless login #2758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ea3b3df
dc12c4a
fc5e79e
d48e1af
c1c3804
0e89a9d
80b0bfd
86d6229
eea3677
5d705a3
6af0bca
91c722e
9f6a29c
58bbcb3
8dec3de
b0a004f
774ab91
952ef3e
1233881
874797a
db741e2
9180224
847f979
bddbfe4
443a51a
07d4e75
eaa944a
c9626da
168c9f2
c5c5756
f329e64
2ec021a
c60168e
1fd92cd
251b983
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,20 +12,19 @@ import {Button, Divider, Stack, Text} from '@salesforce/retail-react-app/app/com | |
| import LoginFields from '@salesforce/retail-react-app/app/components/forms/login-fields' | ||
| import StandardLogin from '@salesforce/retail-react-app/app/components/standard-login' | ||
| import SocialLogin from '@salesforce/retail-react-app/app/components/social-login' | ||
| import {LOGIN_TYPES} from '@salesforce/retail-react-app/app/constants' | ||
|
|
||
| const noop = () => {} | ||
| const PasswordlessLogin = ({ | ||
| form, | ||
| handleForgotPasswordClick, | ||
| handlePasswordlessLoginClick, | ||
| isSocialEnabled = false, | ||
| idps = [], | ||
| setLoginType | ||
| setLoginType = noop | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't need this prop in the logic but we can't remove it now since it will be a breaking change. |
||
| }) => { | ||
| const [showPasswordView, setShowPasswordView] = useState(false) | ||
|
|
||
| const handlePasswordButton = async (e) => { | ||
| setLoginType(LOGIN_TYPES.PASSWORD) | ||
| const isValid = await form.trigger() | ||
| // Manually trigger the browser native form validations | ||
| const domForm = e.target.closest('form') | ||
|
|
@@ -48,8 +47,8 @@ const PasswordlessLogin = ({ | |
| /> | ||
| <Button | ||
| type="submit" | ||
| onClick={() => { | ||
| handlePasswordlessLoginClick() | ||
| onClick={(e) => { | ||
| handlePasswordlessLoginClick(e) | ||
| form.clearErrors('global') | ||
| }} | ||
| isLoading={form.formState.isSubmitting} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,6 @@ import { | |
| API_ERROR_MESSAGE, | ||
| CREATE_ACCOUNT_FIRST_ERROR_MESSAGE, | ||
| FEATURE_UNAVAILABLE_ERROR_MESSAGE, | ||
| LOGIN_TYPES, | ||
| PASSWORDLESS_ERROR_MESSAGES, | ||
| USER_NOT_FOUND_ERROR | ||
| } from '@salesforce/retail-react-app/app/constants' | ||
|
|
@@ -88,8 +87,6 @@ export const AuthModal = ({ | |
| const register = useAuthHelper(AuthHelpers.Register) | ||
| const appOrigin = useAppOrigin() | ||
|
|
||
| const [loginType, setLoginType] = useState(LOGIN_TYPES.PASSWORD) | ||
| const [passwordlessLoginEmail, setPasswordlessLoginEmail] = useState(initialEmail) | ||
| const {getPasswordResetToken} = usePasswordReset() | ||
| const authorizePasswordlessLogin = useAuthHelper(AuthHelpers.AuthorizePasswordless) | ||
| const passwordlessConfigCallback = getConfig().app.login?.passwordless?.callbackURI | ||
|
|
@@ -103,66 +100,67 @@ export const AuthModal = ({ | |
| ) | ||
| const mergeBasket = useShopperBasketsMutation('mergeBasket') | ||
|
|
||
| const submitForm = async (data) => { | ||
| const handlePasswordlessLogin = async (email) => { | ||
| try { | ||
| const redirectPath = window.location.pathname + (window.location.search || '') | ||
| await authorizePasswordlessLogin.mutateAsync({ | ||
| userid: email, | ||
| callbackURI: `${callbackURL}?redirectUrl=${redirectPath}` | ||
| }) | ||
| setCurrentView(EMAIL_VIEW) | ||
| } catch (error) { | ||
| const message = USER_NOT_FOUND_ERROR.test(error.message) | ||
| ? formatMessage(CREATE_ACCOUNT_FIRST_ERROR_MESSAGE) | ||
| : PASSWORDLESS_ERROR_MESSAGES.some((msg) => msg.test(error.message)) | ||
| ? formatMessage(FEATURE_UNAVAILABLE_ERROR_MESSAGE) | ||
| : formatMessage(API_ERROR_MESSAGE) | ||
| form.setError('global', {type: 'manual', message}) | ||
| } | ||
| } | ||
|
|
||
| const submitForm = async (data, isPasswordless = false) => { | ||
| form.clearErrors() | ||
|
|
||
| const onLoginSuccess = () => { | ||
| navigate('/account') | ||
| } | ||
|
|
||
| const handlePasswordlessLogin = async (email) => { | ||
| try { | ||
| const redirectPath = window.location.pathname + window.location.search | ||
| await authorizePasswordlessLogin.mutateAsync({ | ||
| userid: email, | ||
| callbackURI: `${callbackURL}?redirectUrl=${redirectPath}` | ||
| }) | ||
| setCurrentView(EMAIL_VIEW) | ||
| } catch (error) { | ||
| const message = USER_NOT_FOUND_ERROR.test(error.message) | ||
| ? formatMessage(CREATE_ACCOUNT_FIRST_ERROR_MESSAGE) | ||
| : PASSWORDLESS_ERROR_MESSAGES.some((msg) => msg.test(error.message)) | ||
| ? formatMessage(FEATURE_UNAVAILABLE_ERROR_MESSAGE) | ||
| : formatMessage(API_ERROR_MESSAGE) | ||
| form.setError('global', {type: 'manual', message}) | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| login: async (data) => { | ||
| if (loginType === LOGIN_TYPES.PASSWORD) { | ||
| try { | ||
| await login.mutateAsync({ | ||
| username: data.email, | ||
| password: data.password | ||
| if (isPasswordless) { | ||
| const email = data.email | ||
| await handlePasswordlessLogin(email) | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| await login.mutateAsync({ | ||
| username: data.email, | ||
| password: data.password | ||
| }) | ||
| const hasBasketItem = baskets?.baskets?.[0]?.productItems?.length > 0 | ||
| // we only want to merge basket when the user is logged in as a recurring user | ||
| // only recurring users trigger the login mutation, new user triggers register mutation | ||
| // this logic needs to stay in this block because this is the only place that tells if a user is a recurring user | ||
| // if you change logic here, also change it in login page | ||
| const shouldMergeBasket = hasBasketItem && prevAuthType === 'guest' | ||
| if (shouldMergeBasket) { | ||
| mergeBasket.mutate({ | ||
| headers: { | ||
| // This is not required since the request has no body | ||
| // but CommerceAPI throws a '419 - Unsupported Media Type' error if this header is removed. | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| parameters: { | ||
| createDestinationBasket: true | ||
| } | ||
| }) | ||
| const hasBasketItem = baskets?.baskets?.[0]?.productItems?.length > 0 | ||
| // we only want to merge basket when the user is logged in as a recurring user | ||
| // only recurring users trigger the login mutation, new user triggers register mutation | ||
| // this logic needs to stay in this block because this is the only place that tells if a user is a recurring user | ||
| // if you change logic here, also change it in login page | ||
| const shouldMergeBasket = hasBasketItem && prevAuthType === 'guest' | ||
| if (shouldMergeBasket) { | ||
| mergeBasket.mutate({ | ||
| headers: { | ||
| // This is not required since the request has no body | ||
| // but CommerceAPI throws a '419 - Unsupported Media Type' error if this header is removed. | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| parameters: { | ||
| createDestinationBasket: true | ||
| } | ||
| }) | ||
| } | ||
| } catch (error) { | ||
| const message = /Unauthorized/i.test(error.message) | ||
| ? formatMessage(LOGIN_ERROR) | ||
| : formatMessage(API_ERROR_MESSAGE) | ||
| form.setError('global', {type: 'manual', message}) | ||
| } | ||
| } else if (loginType === LOGIN_TYPES.PASSWORDLESS) { | ||
| setPasswordlessLoginEmail(data.email) | ||
| await handlePasswordlessLogin(data.email) | ||
| } catch (error) { | ||
| const message = /Unauthorized/i.test(error.message) | ||
| ? formatMessage(LOGIN_ERROR) | ||
| : formatMessage(API_ERROR_MESSAGE) | ||
| form.setError('global', {type: 'manual', message}) | ||
| } | ||
| }, | ||
| register: async (data) => { | ||
|
|
@@ -198,15 +196,15 @@ export const AuthModal = ({ | |
| } | ||
| }, | ||
| email: async () => { | ||
| await handlePasswordlessLogin(passwordlessLoginEmail) | ||
| const email = form.getValues().email || initialEmail | ||
| await handlePasswordlessLogin(email) | ||
| } | ||
| }[currentView](data) | ||
| } | ||
|
|
||
| // Reset form and local state when opening the modal | ||
| useEffect(() => { | ||
| if (isOpen) { | ||
| setLoginType(LOGIN_TYPES.PASSWORD) | ||
| setCurrentView(initialView) | ||
| form.reset() | ||
| } | ||
|
|
@@ -223,15 +221,14 @@ export const AuthModal = ({ | |
| fieldsRef?.[initialField]?.ref.focus() | ||
| }, [form.control?.fieldsRef?.current]) | ||
|
|
||
| // Clear form state when changing views | ||
| useEffect(() => { | ||
| form.reset() | ||
| // we don't want to reset the form on email view | ||
| // because we want to pass the email to PasswordlessEmailConfirmation | ||
| if (currentView !== EMAIL_VIEW) { | ||
| form.reset() | ||
| } | ||
| }, [currentView]) | ||
|
|
||
| useEffect(() => { | ||
| setPasswordlessLoginEmail(initialEmail) | ||
| }, [initialEmail]) | ||
|
|
||
| useEffect(() => { | ||
| // Lets determine if the user has either logged in, or registed. | ||
| const loggingIn = currentView === LOGIN_VIEW | ||
|
|
@@ -302,16 +299,20 @@ export const AuthModal = ({ | |
| {!form.formState.isSubmitSuccessful && currentView === LOGIN_VIEW && ( | ||
| <LoginForm | ||
| form={form} | ||
| submitForm={submitForm} | ||
| submitForm={(data) => { | ||
| const shouldUsePasswordless = | ||
alexvuong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| isPasswordlessEnabled && !data.password | ||
alexvuong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return submitForm(data, shouldUsePasswordless) | ||
| }} | ||
| clickCreateAccount={() => setCurrentView(REGISTER_VIEW)} | ||
| handlePasswordlessLoginClick={() => | ||
| setLoginType(LOGIN_TYPES.PASSWORDLESS) | ||
| } | ||
| //TODO: potentially remove this prop in the next major release since | ||
| // we don't need to use this props anymore | ||
| handlePasswordlessLoginClick={noop} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't need this prop in the logic but we can't remove it now since it will be a breaking change. |
||
| handleForgotPasswordClick={() => setCurrentView(PASSWORD_VIEW)} | ||
| isPasswordlessEnabled={isPasswordlessEnabled} | ||
| isSocialEnabled={isSocialEnabled} | ||
| idps={idps} | ||
| setLoginType={setLoginType} | ||
| setLoginType={noop} | ||
| /> | ||
| )} | ||
| {!form.formState.isSubmitSuccessful && currentView === REGISTER_VIEW && ( | ||
|
|
@@ -332,7 +333,7 @@ export const AuthModal = ({ | |
| <PasswordlessEmailConfirmation | ||
| form={form} | ||
| submitForm={submitForm} | ||
| email={passwordlessLoginEmail} | ||
| email={form.getValues().email || initialEmail} | ||
| /> | ||
| )} | ||
| </ModalBody> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete this constant from
constants.jsif we are not using it anymore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it since removing it will be a breaking change