feat: add refresh token handling http client #2033
Conversation
TheOneWithTheBraid
force-pushed
the
braid/refresh-token-http-client
branch
from
65e065c to
c7a965c
Compare
TheOneWithTheBraid
had a problem deploying
to
review
GitHub Actions
Failure
TheOneWithTheBraid
force-pushed
the
braid/refresh-token-http-client
branch
from
c7a965c to
84480e8
Compare
|
Is there any update on review of this PR ? |
- implement a `MatrixRefreshTokenClient` to await token refresh before dispatching http requests - improve documentation about soft logout logic - fix dartdoc locations about soft logout Fixes: famedly#2027 Signed-off-by: The one with the braid <info@braid.business>
TheOneWithTheBraid
force-pushed
the
braid/refresh-token-http-client
branch
from
84480e8 to
356f831
Compare
| // we are actually initialized | ||
| client.onSync.value != null && | ||
| // the request is to the homeserver rather than e.g. IDP | ||
| request.url.host == client.homeserver?.host && |
There was a problem hiding this comment.
Can we also add this check: request.url.pathSegments.first == '_matrix'. Someone coud place for example a synapse module at: /_custom/api/v1/...
| final headers = request.headers; | ||
| // hours wasted : unknown :facepalm: | ||
| headers.removeWhere((k, _) => k.toLowerCase() == 'authorization'); | ||
| headers['Authorization'] = 'Bearer ${client.bearerToken!}'; |
There was a problem hiding this comment.
this now always adds the Authorization header right? Even if we don't have a token or don't want to set it for some reason.
There was a problem hiding this comment.
Not sure if I'm happy with doing it on httpclient level. I really get why we want to do it this way we also check it unnecessarily often.
Doing this check on sync had the reason that we do it around every 30 seconds periodically. If we do it for every single request we would have this check done quite a lot, like multiple times per second.
With refresh tokens the access token has a lifetime of 5 minutes by default. Actually we know when it will be no longer valid so we could refresh it 1 minute before the lifetime ends. In the sync handler we do exactly this. If this is not working, we should investigate the reason for it
Sorry for once again coming up with a random PR ... Can I kindly ask you to check whether this implementation matches your expectations about handling soft-logout in more advanced use cases ?
This will especially be relevant for end users once #2024 might be shipped to client developers.
MatrixRefreshTokenClientto await token refresh before dispatching http requestsFixes: #2027