Passcodes | Release sign in with passcodes to all users #3035
Conversation
coldlink
added
the
passwordless
coldlink
force-pushed
the
mm/passcode-signin-100
branch
from
eef0e8d to
8077e93
Compare
coldlink
force-pushed
the
mm/passcode-signin-100
branch
2 times, most recently
from
3524abe to
de09457
Compare
|
|
||
| try { | ||
| // only attempt to sign in with a passcode if the user currently has the query parameter set | ||
| // this should be removed when we're ready to enable this for all users | ||
| // only attempt to sign in with a passcode if we've enabled passcodes for all users |
There was a problem hiding this comment.
is this comment correct? it seems like this variable means that the user has not forced password sign in with the parameter, right ?
There was a problem hiding this comment.
I've updated the comments on this page to make it clearer about whats going on!
| @@ -417,6 +415,8 @@ export const oktaIdxApiSignInController = async ({ | |||
| } | |||
| } | |||
|
|
|||
| // Otherwise use password sign in | |||
There was a problem hiding this comment.
if the usePasswordSignIn flag is not set but there's no passcode in the request body then it would attempt password sign in ? is this the desired behaviour ?
There was a problem hiding this comment.
That's right. It only attempts to use passcodes if it sees the passcode in the request params, which is just a hidden flag saying to use the passcode flow rather than the password flow
| res.locals.queryParams.usePasscodeSignIn || | ||
| res.locals.abTestAPI.isUserInVariant('PasscodeSignInTest', 'variant'); | ||
| // we're using passwords if the `usePasswordSignIn` query parameter is set | ||
| const usePasscodeSignInFlag = !res.locals.queryParams.usePasswordSignIn; |
There was a problem hiding this comment.
keeping the usePasscodeSignInFlag might be confusing now that it means that the usePasswordSignIn was NOT set
There was a problem hiding this comment.
I've added comments and updated the name to make it clearer about what's going on!
coldlink
force-pushed
the
mm/passcode-signin-100
branch
from
de09457 to
08a4dac
Compare
| // get the email and password from the request body | ||
| // get the email and password from the request body if using passwords | ||
| // or the "passcode" parameter is a hidden input, which is to determine if the | ||
| // user is signing in with a passcode and not an actual passcode value |
There was a problem hiding this comment.
// or the "passcode" parameter is a hidden input, which is to determine if the
// user is signing in with a passcode and not an actual passcode value
I don't really understand this last sentence.. what is the difference between passcode and an actual passcode value?
There was a problem hiding this comment.
It's not a "code" like "123456", but a key/value set to passcode coming from:
gateway/src/client/pages/SignIn.tsx
Line 210 in f28db8f
| @@ -4,7 +4,7 @@ describe('Delete my account flow in Okta', () => { | |||
| cy.visit( | |||
| `/signin?returnUrl=${encodeURIComponent( | |||
| `https://${Cypress.env('BASE_URI')}/welcome/review`, | |||
| )}`, | |||
| )}&usePasswordSignIn=true`, | |||
There was a problem hiding this comment.
does this mean we are not testing the default sign in method now ?
There was a problem hiding this comment.
We are! It's just in other test files that have already been added. Adding usePasswordSignIn to tests just means we don't have to update the authentication specific tests, e.g. for delete my account.
What does this change?
Releases passcodes for sign in to all of the audience following on from #3034
We also remove the AB test and the
usePasscodeSignInquery parameter which would've been needed in order to view the sign in with passcode page.We add a
usePasswordSignInquery parameter flag, which preserves the previous behaviour of sign in with a password directly on the initial sign in page. This is useful as it means tests can still continue to run without having to be integrated with passcodes for sign in.Tested