matrix-org / matrix-org/matrix-js-sdk
Unclear tokenRefreshFunction documentation leads to errors and potential deadlocks
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 2.2k
- Forks
- 704
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 40
Description
Hi!
We are currently looking at implementing a `tokenRefreshFunction` for our application's login flow (it simply posts the application's JWT token that is exchanged for an AT by the homeserver), but can't make heads or tails of what is expected of SDK users.
* The SDK automatically calls `tokenRefreshFunction` when a request fails with `M_UNKNOWN_TOKEN`, before retrying. Is calling `MatrixClient.loginRequest` to initialize the client advisable or should we let it authenticate in `startClient` after it receives `M_UNKNOWN_TOKEN` on `/capabilities`? I can't find any relevant documentation. #4632 suggests re-creating the client with the result of `loginRequest` but I fail to see the point compared to letting `tokenRefreshFunction` update the credentials?
* Why does `loginRequest` call `this.http.authedRequest`? `/login` is [an unauthenticated endpoint](https://spec.matrix.org/v1.13/client-server-api/#post_matrixclientv3login). Not only is the `Authorization` header redundant, but `authedRequest` causes a nasty **deadlock** when calling `loginRequest` from `tokenRefreshFunction` which only becomes apparent in certain circumstances:
1. On `M_UNKNOWN_TOKEN`, the SDK calls `this.tokenRefreshPromise ??= this.doTokenRefresh(attempt);`
1. `tokenRefreshFunction` calls `loginRequest`. At this point we haven't yet awaited anything so the `tokenRefreshPromise` is not yet set. `loginRequest` continues unimpeded
1. If it succeeds, everything works(!).
1. However if it fails with `M_LIMIT_EXCEEDED` (perfectly normal behavior)
1. The custom `tokenRefreshFunction` retries `loginRequest` after waiting the appropriate amount of time
1. `loginRequest` calls `this.http.authedRequest`.
1. `this.http.authedRequest` -> `this.tokenRefresher.prepareForRequest` -> `refreshIfNeeded` awaits `tokenRefreshPromise`... this is a deadlock because we are in said promise! (Note that this would have happened on the first call to `loginRequest` if `tokenRefreshFunction` had awaited any promise before then, which allows `this.tokenRefreshPromise` to be set, so this deadlock can also happen without a retry mechanism)
I am unsure if any of this is intended design or not. What seems to work in our case (whether intended or not) is:
* Setting a dummy token in `createClient` to ensure `tokenRefreshFunction` is called on the first authed request
* Implementing `tokenRefreshFunction` with a retry mechanism to take care of rate-limiting or other server-side failures
* Implementing `tokenRefreshFunction` with calls `matrixClient.http.request()` instead of the more intuitive `matrixClient.loginRequest()` to avoid the deadlock explained above
If this is intended design then this should be clearly documented IMO. If not I would appreciate guidance regarding the best practices.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the tokenRefreshFunction flow and the loginRequest, startClient, and http.authedRequest entry points described in the report. Trace tokenRefreshPromise through M_UNKNOWN_TOKEN handling and rate-limit retries. Done means the supported initialization and retry behavior, including login's interaction with authentication refresh, is clarified or corrected and documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, authentication
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 42/100