Skip to content
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

Passwordless | Reset password with passcodes handle case for non-existent users #2915

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

coldlink
Copy link
Member

@coldlink coldlink commented Sep 24, 2024

What does this change?

In #2852 we implemented passcodes for reset password for some "ACTIVE" users. Namely the ones with both the "password" and "email" authenticator, and users with just the "email" authenticator.

In #2889 we implemented passcode for reset password for the remaining active users, i.e the ones with only the "password" authenticator.

And in #2902 we implemented passcode for reset password for existing users in the non-ACTIVE state.

There is one more group of users that could go through the reset password process, and these are users who currently don't have a Guardian account.

To prevent username enumeration, the behaviour in the UI has to be the same as for all the other user states. Specifically we have to show the /reset-password/email-sent page, with a passcode input box.

Since these are non-existent users, we cannot send an email to them for obvious reasons. So in this PR we've taken the same approach that is used for the existing legacy reset password pages, i.e we just show the email sent page and in our additional information box we include our noAccountInfo messaging. This messaging says that if you don't receive an email within two minutes, then you likely don't have an account, and includes a link to create one.

The cypress test I've added in this PR covers this behaviour.


We also modify the way we set the EncryptedState cookie in this PR. This is because this cookie can get quite large, and in some cases could exceed the maximum size of what was allowed in the Cookie header (only noticed on DEV and CODE).

So we remove undefined keys and values, to reduce the size slightly.

The bigger change was finding out that the Okta IDX API stateHandle is 817 characters in length! But it turns out we don't need to persist the entire stateHandle in the Encrypted State.
In fact all we need is the first 46 characters (before the ~). This is actually the same bit that is required for the stateToken parameter in the /login/token/redirect endpoint used after authentication, see for more context

`/login/token/redirect?stateToken=${response.stateHandle.split('~')[0]}`;

and
stateToken?: string; // state handle from Okta IDX /introspect response but only everything before the first tilde (`stateHandle.split('~')[0]`), used to redirect user to login redirect endpoint to set global session (`/login/token/redirect?stateToken=${stateToken}`)

for more context.
The IDX API seems to work with only that bit (tested with Hoppscotch). So we don't actually need to persist the other 774 characters, and having the cypress tests pass provide us with confidence that this change works.

Tested

  • CODE

Future options

While this PR doesn't send an email to users, we could work with Data Privacy and other teams to figure out if we can send users an email, for example letting users know that they don't currently have an account, or potentially even creating an account for users if they go through the reset password flow.

@coldlink coldlink marked this pull request as ready for review September 24, 2024 10:29
@coldlink coldlink requested a review from a team as a code owner September 24, 2024 10:29
…undefined` values

The `EncryptedState` cookie gets quite large due to the nature of encryption and addition verification tags and signing.

In order to reduce the size of the cookie we set we have done two things.

1. For the Okta IDX API, it turns out we only need the first part of the `stateHandle` to persist. The API continues to work with just this bit.
  - The `stateHandle` string is 817 characters long, however only the first part before the `~` is needed, which is just 46 characters in length.
  - This is actually the same bit that is required for the `stateToken` parameter in the `/login/token/redirect` endpoint used after authentication
    - see: https://github.com/guardian/gateway/blob/34311cdf8524f247a11841fbf7bdf9568f5829ea/src/server/lib/okta/idx/shared/idxFetch.ts#L128
2. We remove `undefined` keys and values, this means we dont have to persist the key and undefined value in the encrypted JSON, thus saving space.
@coldlink coldlink requested review from guardian-ci and removed request for guardian-ci September 24, 2024 13:02
@coldlink coldlink requested review from a team and removed request for a team September 25, 2024 07:54
Copy link
Contributor

@akinsola-guardian akinsola-guardian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking us through this. LGTM

@coldlink coldlink merged commit d01ecae into main Sep 25, 2024
20 checks passed
@coldlink coldlink deleted the mm/reset-password-no-account branch September 25, 2024 15:56
@coldlink coldlink added the passwordless PRs/Issues related to passwordless/passcode functionality label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
passwordless PRs/Issues related to passwordless/passcode functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants