Improve token refresh experience#10007
Conversation
Issue: ecamp#8950 Signed-off-by: BacLuc <lucius.bachmann@clubpage.ch>
…in another tab This also supports oauth and keeps the state the user was in. Also do not route to the login page as long as we don't know if we need to authenticate or not. Signed-off-by: BacLuc <lucius.bachmann@clubpage.ch>
c1a30c7 to
c6c0cd3
Compare
carlobeltrame
left a comment
carlobeltrame
left a comment
There was a problem hiding this comment.
I tried to read through this and make sense of it, but it's too complex to wrap my head around in a reasonable amount of time. Do you @BacLuc @pmattmann understand all of this and see justification for every line? For my part, I am not comfortable with having AI-generated code in this order of complexity for something as essential as the login state of the application. In line with our at-least-one-approval policy, I think at least two members of the core team should really understand what's going on here before merging. IMO, documentation and unit tests are also essential missing parts here.
Feel free to dismiss my review as soon as two members of the core team understand the details of what's going on here and feel comfortable maintaining it.
| let refreshTokenPromise = null | ||
| let reAuthPromise = null | ||
| let reAuthResolve = null | ||
| let reAuthReject = null |
There was a problem hiding this comment.
Top-level variables like this make it very hard to write unit tests for this file, and this complex logic definitely deserves unit testing, not just an e2e test which may or may not be marked as immature and mostly ignored in the future.
Also, consider extracting the reAuth logic into a separate file.
| function waitForReAuth() { | ||
| if (!reAuthPromise) { | ||
| reAuthPromise = new Promise((resolve, reject) => { | ||
| reAuthResolve = resolve | ||
| reAuthReject = reject | ||
| }) | ||
| } | ||
| let refreshedSuccessfully = false | ||
| if (!isLoggedIn()) { | ||
| return reAuthPromise | ||
| } |
There was a problem hiding this comment.
There is no waiting in here, is the naming correct?
| export function resolveReAuth() { | ||
| reAuthResolve?.() | ||
| reAuthPromise = null | ||
| reAuthResolve = null | ||
| reAuthReject = null | ||
| store.commit('setAuthRequired', false) | ||
| } | ||
|
|
||
| export function rejectReAuth() { | ||
| reAuthReject?.(new Error('Re-authentication cancelled')) | ||
| reAuthPromise = null | ||
| reAuthResolve = null | ||
| reAuthReject = null | ||
| store.commit('setAuthRequired', false) | ||
| } |
There was a problem hiding this comment.
This all feels a lot like an anti-pattern... There are so many moving parts in this file: The module-level let variables, the flags in the store, the flags in local storage, some custom fields on the request object, the singleton pattern, ...
I think we should first define the target architecture of this feature.
| if ( | ||
| status !== 401 || | ||
| !request || | ||
| request._authRetried || | ||
| isRefreshRequest(request) || | ||
| hasLoggedOutFromLocalStorage() | ||
| ) { |
There was a problem hiding this comment.
This is very hard to understand. Can we use a local variable for this?
| if ( | |
| status !== 401 || | |
| !request || | |
| request._authRetried || | |
| isRefreshRequest(request) || | |
| hasLoggedOutFromLocalStorage() | |
| ) { | |
| const needsAuthRetry = (status === 401 && request && !request._authRetried && !isRefreshRequest(request) && !hasLoggedOutFromLocalStorage()) | |
| if (!needsAuthRetry) { |
| const axiosInstance = axios.create({ | ||
| axiosInstance = axios.create({ |
There was a problem hiding this comment.
Changing this const to a let seems unnecessary, since it's never reassigned..? (the setupAxiosAuthInterceptor cannot reassign it, it can only mutate the argument)
| }) | ||
| } | ||
|
|
||
| function refreshAuthTokenSingleton() { |
There was a problem hiding this comment.
In javascript, a singleton would usually be realized as a separate module (js file) exporting the necessary functions and objects. See our plugins for examples, e.g. i18n or slugify or passwordStrength
Thanks for the feedback. As soon as we know which features we want we can start split up the thing, improve the code for the single steps and get them in devel one by one. (as long is there is feedback on the way). |
|
I have not tested it, just tried to read the code so far. The improvements you list in the PR description sound reasonable to me. Not sure whether there is more to be done, I think we both agree that we should collect the use cases first, and I don't yet have them collected. Once we have the use cases and user flows, we can think about an architecture to support them, and then we can implement. I support the agile approach, doing everything in small steps. But reading this PR, I do not believe that this is the easiest and cleanest way to acheive these features. It does not matter to me if this PR has everything working feature-wise, because in order to acccept a PR, both the functionality and the code must be good enough. So taking a step back and designing a clean architecture before implementing the agile steps might be appropriate here, is what I am saying. |
3 participants
refresh_token: retry requests to the api after refreshing the auth token
Issue: #8950
frontend: if the token refresh doesn't work, allow the user to login in another tab
This also supports oauth and keeps the state the user was in.
Also do not route to the login page as long as we don't know if
we need to authenticate or not.