-
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
Passcodes | Set up passcodes for registration #2639
base: main
Are you sure you want to change the base?
Conversation
42cbf7b
to
ee5a834
Compare
9e69a50
to
add2649
Compare
ee5a834
to
1825c32
Compare
9693763
to
40201c7
Compare
1825c32
to
f6036aa
Compare
40201c7
to
b35c732
Compare
f6036aa
to
c195305
Compare
619b9a9
to
3b6394c
Compare
b56e151
to
983a966
Compare
09b3a5c
to
50de391
Compare
2abba14
to
40406c7
Compare
40406c7
to
f7caeba
Compare
f7caeba
to
ee9e89a
Compare
Some questions/notes (I think I've answered the first two of these for myself by reading the code but I wanted to double check because they're edge cases):
|
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.
I'm very happy (and impressed) with this! I'm not approving it since it needs rebase with main
anyway, and I've left some comments, but generally speaking I think it's very much ready for a CODE deploy behind a flag.
src/server/routes/register.ts
Outdated
requestState: mergeRequestState(state, { | ||
pageData: { | ||
email: readEncryptedStateCookie(req)?.email, | ||
timeUntilTokenExpiry: encryptedState.stateHandleExpiresAt |
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.
You could be pedantic/avoid confusion and call this msUntilTokenExpiry
. Just because some OAuth stuff uses seconds instead.
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.
I did it this way as timeUntilTokenExpiry
was already a thing in Gateway, and wanted to avoid a refactor.
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.
hopefully the new function name makes it clearer
const passwordAuthenticatorId = | ||
challengeAnswerResponse.remediation.value | ||
.flatMap((remediation) => { | ||
if (remediation.name === 'select-authenticator-enroll') { | ||
const parsedRemediation = | ||
selectAuthenticationEnrollSchema.safeParse(remediation); | ||
|
||
if (parsedRemediation.success) { | ||
return parsedRemediation.data.value.flatMap((value) => { | ||
if (value.name === 'authenticator') { | ||
return value.options.flatMap((option) => { | ||
if (option.label === 'Password') { | ||
if ( | ||
option.value.form.value.some( | ||
(v) => v.value === 'password', | ||
) | ||
) { | ||
return [ | ||
option.value.form.value.find( | ||
(v) => v.name === 'id', | ||
)?.value, | ||
]; | ||
} | ||
} | ||
}); | ||
} | ||
}); | ||
} | ||
} | ||
}) | ||
.filter( | ||
(id): id is string => typeof id === 'string' && id.length > 0, | ||
); |
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.
This is terrifying. No suggestions to fix it, it just is. :(
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.
A further thought: is there any way at all that we could leverage zod to handle this complex nested object? It feels like a maintenance nightmare otherwise.
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.
this is leveraging Zod as best I can 😱
it's just annoying at there are so many states to what the IDX API returns, it's not consistent, so we always have to double check everything.
(id): id is string => typeof id === 'string' && id.length > 0, | ||
); | ||
|
||
if (!passwordAuthenticatorId.length) { |
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.
I'm surprised Typescript can promise that passwordAuthenticatorId
is set to anything after all those nested flatMap
s and if
s! Maybe passwordAuthenticatorId?.length
is safer anyway?
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.
if (!passwordAuthenticatorId.length) {
is effectively the same as doing if (passwordAuthenticatorId?.length) {
in this scenario?
<> | ||
{email ? ( | ||
<MainBodyText> | ||
We’ve sent an email to <b>{email}</b> with verification |
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.
This is odd wording ('verification instructions and a verification code') - surely just 'with a verification code' is fine?
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.
I'll speak to @nickhartyyy about this
<MainBodyText> | ||
We’ve sent you an email with verification instructions and a | ||
verification code. |
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.
Similar odd wording here.
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.
and this
} else { | ||
logger.error( | ||
'Failed to set validation flags in Okta as there was no id', | ||
undefined, | ||
{ | ||
request_id, | ||
}, | ||
); | ||
} |
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.
Does anything rely on this? If the email isn't validated successfully, what happens to the short-lived user session next? What happens to any new user sessions? (Basically, are we happy leaving the error only logged?)
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.
one of the cases where it should be theoretically impossible to get to, but helpful if we log an error message
on next sign in they'd be sent the "security email" if they were ever to end up in this scenario
|
…to Encrypted State
…for passcode registration
…` flags when registering with passcodes
… passcode expiry
ee9e89a
to
71c074b
Compare
What does this change?
We've now upgraded to Okta Identity Engine. This unlocks a number of new features which previously weren't feasible. One of these is "passwordless", or the idea of eliminating, or reducing the need for passwords.
We also currently have an issue within our OAuth flows where users don't always end up signed in the same browser context that they registered from. This can occur if a user clicks a reset password/registration verification link in their on another browser or device, or if deeplinking/https interception isn't working properly on the user's device.
One way of fixing both these issues it to use One Time Passcodes (OTPs), where instead of a user clicking a link in an email, they have to type a 6 digit number into an input field which performs the same action of verifying the user. This means that the user always stays within the same browser device/context, while also securely verifying the user.
To begin with we're going to only implement OTPs for registration only, with the user still having to set a password, this is the focus of this PR. After this we will being rolling out passcodes for sign in and password reset too, thus eliminating passwords altogether, or at lease making them optional.
This PR builds on the work previously done in #2625 where the initial work to set up the IDX API was done, and #2637 where most of the APIs for passcodes were set up. The focus of this PR is to wire up the registration functionality to use the passcode API, and set up the frontend to handle this.
This PR however does not enable passcodes directly in
PROD
, theusePasscodesRegistration
query parameter flag must be manually set in order to opt in, and the switch has to be enabled inPROD
, it is currently set tofalse
. This allows us to slowly opt in and first test on the CODE environment, followed by PROD behind the flag, followed by enabling it without the flag.We're currently only enabling passcodes for new users who are registering. Existing users that are attempting to go through the registration flow will fallback to the legacy/current flow without passcodes for now until full passwordless is implemented. The full flowchart can be seen in #2567.
The Journey
The steps to enable passcode registration are as follows:
POST /register
POST /register
registrationPasscodesEnabled
feature switch enabled andusePasscodeRegistration
query parameter is set (this step is temporary before rolling out to all users)interaction_handle
+authState
interaction_handle
to get thestateHandle
authState
with thestateToken
andencryptedRegistrationConsents
select-enroll-profile
remediation property which means registration is allowed using the IDX API/enroll
endpoint to attempt to start the registration process/enroll/new
endpoint to attempt to register the user with emailemail
andstateHandle
through the flow/between pagesPOST /register/code
POST /register/code
code
from the request body and read thestateHandle
from the encrypted state cookiestateHandle
is valid with theintrospect
endpoint/challenge/answer
endpoint with thecode
andstateHandle
i. If valid continue
ii. If invalid, we throw and handle a specific error case for invalid passcode:
api.authn.error.PASSCODE_INVALID
passwordAuthenticatorId
/credentials/enroll
method with the authenticator id andmethodType
set topassword
/welcome/password
page where they'll set a passwordGET /welcome/password
GET /welcome/:token
route, so we update the existing handler, which uses thecheckTokenInOkta
fromcheckPasswordToken.ts
registrationPasscodesEnabled
feature switch enabled andusePasscodeRegistration
query parameter is set (this step is temporary before rolling out to all users)token
parameter is a tokenstateHandle
is valid with theintrospect
endpoint and making sure it has theenroll-authenticator
object so we know it's in the correct statePOST /welcome/password
POST /welcome/:token
route, so we update the existing handler, which uses thechangePasswordInOkta
fromchangePassword.ts
registrationPasscodesEnabled
feature switch enabled andusePasscodeRegistration
query parameter is set (this step is temporary before rolling out to all users)token
parameter is a tokenstateHandle
is valid with theintrospect
endpoint and making sure it has theenroll-authenticator
object so we know it's in the correct statesetPasswordAndRedirect
function, which uses the/challenge/answer
endpoint to set a password. This sets a global okta session, and completes the flow by redirecting the user to the interaction code callback endpoint, but with acode
parameter (not aninteraction_code
) and thestate
.Demo
Screen.Recording.2024-05-22.at.15.59.22.mov
Plan
usePasscodeRegistration=true
While improving/making refinements.
Tested?