-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: bypass 2FA when logging with email (VO-380) #1174
Conversation
ae83952
to
8adbbcb
Compare
src/screens/login/LoginScreen.js
Outdated
|
||
if (emailVerifiedCode) { | ||
// We will use it at a later stage when starting the OAuth flow | ||
emailVerifiedCodeRef.current = emailVerifiedCode |
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.
Here I would suggest to put this data into the state
object for two reasons.
First because the login state is already complex so I think it is better to keep it centralised
Second because, as for the other state's entries, we need to ensure it is correctly reset when login is canceled. This is done when calling setState({step: CLOUDERY_STEP})
(i.e. in the handleBackPress
callback or in cancelLogin
method). With your implementation the email verified code is not cleared and will be send to the next login attempt.
Also the email code can be set only in setInstanceData
as MagicLink is already an email based mechanism and the OIDC is not an email based one, so none of them would receive an email code.
So I think that instead of using the useRef you can safely put the code in the setState at line 190 and retrieve it in the state destructuring at line 347.
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.
updated
bc51a5f
to
57397e6
Compare
src/screens/login/LoginScreen.js
Outdated
async ({ instance, fqdn }) => { | ||
/** | ||
* Sets the instance data. | ||
* @param {{ instance: Record<string, unknown>, fqdn: string }} instanceData - The first parameter object with `instance` as a record of string keys to unknown values, and `fqdn` as a string. |
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.
instance is supposed to be a string, did you find a case where it was something else? 🤔
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.
updated
Using a react ref to store the param, as to not disturb the existing flow. cozy-client has been updated to use the new loginFlagship method. Keep in mind this has not been deployed into prod stack-wise at the time of this commit. The feature will not work with only the front-end implementation here.
57397e6
to
6612ee5
Compare
✨ Features