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

Polish the password recovery flow and other small design tweaks #3780

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Jan 7, 2025

Fixes #3032

This can be reviewed commit by commit:

  • Render an earlier loading screen fallback shows a spinner earlier
  • Fix the cross signing reset cancel and error screens as there were some layout issues
  • Typo in the consent screen
  • Make the rate limiter available to the GraphQL API handlers so that we can rate limit the email recovery resend
  • Polish the password recovery page, which includes:
    • showing an error message if the recovery link is expired, with a button to resend the email
    • showing an error message if the recovery link has already been used
    • including an invisible username field in the form, so that password managers can save the new password
  • Better collapsible sections, in sync with the latest Figma designs
  • Adjust layout spacings, to make them more in line with the designs

Copy link

cloudflare-workers-and-pages bot commented Jan 7, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: e708212
Status: ✅  Deploy successful!
Preview URL: https://d9367cff.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-design-review-jan-2.matrix-authentication-service-docs.pages.dev

View logs

This includes:

 - show an error message if the recovery link is expired, with a button
   to resend the email
 - show an error message if the recovery link has already been used
 - include an invisible username field in the form, so that password
   managers can save the new password
@sandhose sandhose force-pushed the quenting/design-review-jan-25 branch from ab7be10 to 0c88b88 Compare January 7, 2025 16:20
@sandhose sandhose force-pushed the quenting/design-review-jan-25 branch from cac9d67 to 20aa7ce Compare January 8, 2025 14:31
@sandhose sandhose requested a review from reivilibre January 8, 2025 14:41
@sandhose sandhose marked this pull request as ready for review January 8, 2025 14:41
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

just one (probably small) thing

Comment on lines +331 to +336
/// The input for the `resendRecoveryEmail` mutation.
#[derive(InputObject)]
pub struct ResendRecoveryEmailInput {
/// The recovery ticket to use.
ticket: String,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the intended flow for this.

I assumed the recovery ticket is the code that arrives in the e-mail (embedded in a URL, probably) and that lets you recover the account.

So why would you use a recovery ticket to resend the recovery e-mail?

If the ticket is more public than that, I don't think I am happy with the idea that you can use a ticket to request the username of the user...

edit: from the client code I think I gleam that it's for expired tickets. If so, can you state that more clearly in the documentation for the resend operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, I've added a comment in 8a69c9a

@sandhose sandhose requested a review from reivilibre January 9, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include an invisible username field in password change forms
2 participants