Forgot password flow #2866
Forgot password flow #2866
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
a792639
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5e908fd.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://quenting-forgot-password.matrix-authentication-service-docs.pages.dev |
sandhose
force-pushed
the
quenting/forgot-password
branch
12 times, most recently
from
8631ed9 to
b32c534
Compare
| // XXX: is that the right thing to do? | ||
| return Ok(( | ||
| cookie_jar, | ||
| url_builder.redirect(&mas_router::AccountRecoveryStart), | ||
| ) | ||
| .into_response()); |
There was a problem hiding this comment.
maybe we should say the link 'expired' or something. I guess.
There was a problem hiding this comment.
I mean it's really that you got to the wrong place, as lookup_session will give you even consumed/expired sessions. A 404 would be better, but we don't have a nice way to display that page.
I think we should track in a separate issue to have nice 404 pages for when you go to pages with an unknown ID, because we have many cases like that
There was a problem hiding this comment.
well this kind of raises another Q to me: Do we really want to keep old expired sessions around in the DB forever?
I can't say I see much reason to keep them for a very long time, because it's just going to be ever-growing cruft.
There was a problem hiding this comment.
Generally we don't cleanup most things in the database. I rather soft-delete stuff and keep things for now, and introduce maintenance/cleanup tasks later once we have a nice way of doing this?
There was a problem hiding this comment.
cynical take: that's how you get Synapse ;-)
but fine
| .with_language(&locale), | ||
| )?; | ||
| return Ok((cookie_jar, Html(rendered)).into_response()); | ||
| } |
There was a problem hiding this comment.
all the logic above this seems to be in common with the get route; wonder if can we pull this out into a separate function without it being too awkward?
There was a problem hiding this comment.
yeah that's a thing I haven't really figured out how to do nicely, that wouldn't be the only place having this issue :(
There was a problem hiding this comment.
any insight why this is hard? what happens if you try it?
There was a problem hiding this comment.
Such a function would have to return either
- an error
- a response
- the user + the repository because passing it as a
&mutwon't work because of Send/Sync shenanigans…
Which means a custom enum for the return type of such a method? or at least a Result<Either<Response, (User, BoxRepository)>, FancyError>, which isn't exactly pretty?
sandhose
force-pushed
the
quenting/forgot-password
branch
from
c401045 to
a792639
Compare
Fixes #13
This implements the password recovery flow as designed here.
The feature is disabled by default.
A few things to do still to exactly match the designs:
<table>maybe?)Open questions:
setPasswordAPI to accept a recovery 'ticket', and make it easier to have the same form validation logicI made it so that it's reviewable commit by commit