MNTOR-4586/recent authentication before deletion #6276
Conversation
rhelmer
force-pushed
the
MNTOR-4586/recent-authentication-before-deletion
branch
from
369b31a to
8222695
Compare
rhelmer
force-pushed
the
MNTOR-4586/recent-authentication-before-deletion
branch
from
8222695 to
acfc2cf
Compare
Vinnl
left a comment
Vinnl
left a comment
There was a problem hiding this comment.
I guess this works, but UX-wise this leaves quite a burden on the user, where they have to know to manually sign out and back in again. Isn't it possible to provide them with a link that calls signOut, then redirects to the settings page, for which they'll be asked to log in again first? And then only show that link if their last signing was more than, say, 55 minutes ago?
| settings-delete-account-recent-auth-required = For your safety, please sign in again (within the last hour) before deleting your account. | ||
| settings-delete-account-error-generic = Something went wrong while trying to delete your account. Try again. |
There was a problem hiding this comment.
Wordsmithing isn't my expertise, but "within the last hour" is confusing to me for something the user still has to do.
| settings-delete-account-recent-auth-required = For your safety, please sign in again (within the last hour) before deleting your account. | |
| settings-delete-account-error-generic = Something went wrong while trying to delete your account. Try again. | |
| settings-delete-account-recent-auth-required = For your safety, please sign in again before deleting your account. | |
| settings-delete-account-error-generic = Something went wrong while trying to delete your account. Please try again. |
There was a problem hiding this comment.
Does it mean "log in again within the next hour", or you "must have done the log in within the last hour"?
There was a problem hiding this comment.
I don't think that the timing is particularly helpful to the user here. Why not just say, "For your safety, please sign in again before deleting your account." ?
| } | ||
| } | ||
| if (token.authenticatedAt) { | ||
| session.authenticatedAt = new Date(token.authenticatedAt).toISOString(); |
There was a problem hiding this comment.
Isn't it safer to just keep the UNIX timestamp here? I don't think we need to preserve the timezone for this use case, so it would be good to be able to be able to avoid date string parsing.
| /** Session data available after deserialising the JWT */ | ||
| interface Session { | ||
| error?: "RefreshAccessTokenError"; | ||
| authenticatedAt?: ISO8601DateString; |
There was a problem hiding this comment.
As above, wouldn't it make sense to use a UNIX timestamp here as well?
| <p | ||
| role="status" | ||
| style={{ | ||
| marginTop: "0.5rem", |
There was a problem hiding this comment.
Probably doesn't matter for such a small and rarely visible surface, but we tend to avoid margins in CSS because they hurt the composability of individual elements. Instead, we try to be consistent in having layout "flow down", e.g. having a flexbox parent container with a gap.
Again, probably not a biggie for this specific code, but good to know.
References:
Jira: MNTOR-4586
Description
Require recent authentication before deleting account via Settings menu.
How to test
There is a unit test, if you want to test manually you can modify
RECENT_AUTH_WINDOW_MSinsrc/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/actions.tsto something very short (e.g. 1 ms) and you should see a message "For your safety, please sign in again (within the last hour) before deleting your account" when trying to delete your account.Checklist (Definition of Done)