Conversation
01a2960 to
d74f833
Compare
| created: number, | ||
| tokenResult: oauth.TokenEndpointResponse, | ||
| dpopKey: CryptoKeyPair, | ||
| client: oauth.Client, |
There was a problem hiding this comment.
Refresh tokens are bound to clients, so we must cache the client registration data with the rest.
Because refresh tokens are typically long-lasting credentials used to
request additional access tokens, the refresh token is bound to the
client to which it was issued.
-- RFC 6749, §6
There was a problem hiding this comment.
👍
This cache implementation should always be isolated to one client. In browsers it should be origin bound. Otherwise, we are enabling clients to impersonate one another - by using their credentials.
Please add documentation to this effect at the points where this cache is configured.
| return new Request(request, {headers}) | ||
| } | ||
|
|
||
| private async getCachedToken(request: Request): Promise<CacheEntry> { |
There was a problem hiding this comment.
This is the substantial change:
flowchart TD
start(["Get token"])
getCached["Get token from cache"]
found{"Cached token exists?"}
expired{"Cached token expired?"}
refresh[["Refresh token"]]
refreshed{"Refresh succeed?"}
storeRefreshed["Cache refreshed token"]
obtain[["Get new token"]]
storeNew["Cache new token"]
useRefreshed(["Use refreshed token"])
useCached(["Use cached token"])
useNew(["Use new token"])
start --> getCached
getCached --> found
found -- no --> refresh
found -- yes --> expired
expired -- no --> useCached
expired -- yes --> refresh
refresh --> refreshed
refreshed -- yes --> storeRefreshed
storeRefreshed --> useRefreshed
refreshed -- no --> obtain
obtain --> storeNew
storeNew --> useNew
storeNew ~~~ useCached
storeNew ~~~ useRefreshed
There was a problem hiding this comment.
This logic diagram is super helpful, let's reuse it for documentation of the library when relevant.
| private async refreshToken(request: Request): Promise<CacheEntry | undefined> { | ||
| const cached = this.#cache.get(request.url) | ||
| if (cached === undefined) { | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
Why not just call refreshToken directly on the CacheEntry
| private async refreshToken(request: Request): Promise<CacheEntry | undefined> { | |
| const cached = this.#cache.get(request.url) | |
| if (cached === undefined) { | |
| return undefined | |
| } | |
| private async refreshToken(cached: CacheEntry): Promise<CacheEntry | undefined> { |
| return undefined | ||
| } | ||
|
|
||
| const authorizationServer = await this.#asProvider.getAuthorizationServer(request) |
There was a problem hiding this comment.
It is a bad idea to be determining the authorisation server from the request here.
The refresh_token is bound to a specific authorisation server - so we must always refresh against that server.
If it is not already a property of the tokenResult, you should make the authorisation server a property of the CacheEntry and get it from that.
| const refreshed = await this.refreshToken(request) | ||
| if (refreshed !== undefined) { | ||
| this.#cache.set(request.url, refreshed) | ||
| return refreshed | ||
| } |
There was a problem hiding this comment.
If a separate request is initiated during this await it will trigger another refresh using the same refresh token.
This will create a race condition and one of those requests will fail as a refresh token is to only be used against an authorisation server once.
|
|
||
| const authorizationServer = await this.#asProvider.getAuthorizationServer(request) | ||
| const dpop = oauth.DPoP({}, cached.dpopKey) | ||
| const options = {DPoP: dpop, signal: request.signal} |
There was a problem hiding this comment.
The signal of a refresh should not be attached to this specific request. It is possible that this request gets cancelled after the refresh token has been presented to the AS, but before the response has been transmitted to the client.
In this case the client has no valid refresh token; and a full re-authentication flow will need to be completed to upgrade subsequent requests.
The only case where a refresh should be aborted is when the client plans on making no further requests. When we move to having a persistent cache over an in-memory cache - this means the client will never be used again; which is an extreme edge case.
This is also related to https://github.com/solid-contrib/reactive-authentication/pull/42/changes#r4025845055.
|
|
||
| throw e |
There was a problem hiding this comment.
If the refresh token is invalid for any reason; we should make sure it is cleared from the cache.
3 participants
Introduces functionality to refresh expired, cached access tokens when the authorization server supports it.
d74f833 is the substantial change, other commits just prepare the stage.