-
Notifications
You must be signed in to change notification settings - Fork 2
ENG-30: Refresh token #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,12 +6,21 @@ import type { AuthorizationServerProvider } from "./AuthorizationServerProvider. | |||||||||||||
| import { ClientProvider } from "./ClientProvider.js" | ||||||||||||||
| import { supportsOfflineAccess } from "./supportsOfflineAccess.js" | ||||||||||||||
|
|
||||||||||||||
| type CacheEntry = { created: number, tokenResult: oauth.TokenEndpointResponse, dpopKey: CryptoKeyPair } | ||||||||||||||
| type CacheEntry = { | ||||||||||||||
| created: number, | ||||||||||||||
| tokenResult: oauth.TokenEndpointResponse, | ||||||||||||||
| dpopKey: CryptoKeyPair, | ||||||||||||||
| client: oauth.Client, | ||||||||||||||
| authorizationServer: oauth.AuthorizationServer, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export class DPoPTokenProvider implements TokenProvider { | ||||||||||||||
| readonly #codeProvider: CodeProvider | ||||||||||||||
| readonly #callbackUri: string | ||||||||||||||
| readonly #cache = new Map<string, CacheEntry> // TODO: Take cache from caller | ||||||||||||||
|
|
||||||||||||||
| // TODO: Take cache from caller | ||||||||||||||
| // TODO: Once cache is externalized, document that it should not be shared between clients (which would lead to impersonation) | ||||||||||||||
| readonly #cache = new Map<string, CacheEntry> | ||||||||||||||
| readonly #asProvider: AuthorizationServerProvider | ||||||||||||||
| readonly #clientProvider: ClientProvider | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -27,20 +36,37 @@ export class DPoPTokenProvider implements TokenProvider { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async upgrade(request: Request): Promise<Request> { | ||||||||||||||
| const {dpopKey, tokenResult: {access_token}} = await this.getCachedToken(request) | ||||||||||||||
|
|
||||||||||||||
| const headers = new Headers(request.headers) | ||||||||||||||
|
|
||||||||||||||
| headers.set("DPoP", await DPoP.generateProof(dpopKey, request.url, request.method, undefined, access_token)) | ||||||||||||||
| headers.set("Authorization", ["DPoP", access_token].join(" ")) | ||||||||||||||
|
|
||||||||||||||
| return new Request(request, {headers}) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private async getCachedToken(request: Request): Promise<CacheEntry> { | ||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic diagram is super helpful, let's reuse it for documentation of the library when relevant. |
||||||||||||||
| // TODO: More robust key via callback to support complex caching scenarios | ||||||||||||||
| let tokenData = this.#cache.get(request.url) | ||||||||||||||
| const cached = this.#cache.get(request.url) | ||||||||||||||
|
|
||||||||||||||
| // TODO: Support actively refreshing the token | ||||||||||||||
| if (tokenData === undefined || isExpired(tokenData)) { | ||||||||||||||
| tokenData = await this.obtainToken(request) | ||||||||||||||
| this.#cache.set(request.url, tokenData) | ||||||||||||||
| if (cached !== undefined && !isExpired(cached)) { | ||||||||||||||
| return cached | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const headers = new Headers(request.headers) | ||||||||||||||
| const refreshed = await this.refreshToken(request) | ||||||||||||||
| if (refreshed !== undefined) { | ||||||||||||||
| this.#cache.set(request.url, refreshed) | ||||||||||||||
| return refreshed | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+58
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a separate request is initiated during this 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. |
||||||||||||||
|
|
||||||||||||||
| headers.set("DPoP", await DPoP.generateProof(tokenData.dpopKey, request.url, request.method, undefined, tokenData.tokenResult.access_token)) | ||||||||||||||
| headers.set("Authorization", ["DPoP", tokenData.tokenResult.access_token].join(" ")) | ||||||||||||||
| return new Request(request, {headers}) | ||||||||||||||
| const fresh = await this.obtainToken(request) | ||||||||||||||
| this.#cache.set(request.url, fresh) | ||||||||||||||
|
|
||||||||||||||
| return fresh | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private async obtainToken(request: Request): Promise<CacheEntry> { | ||||||||||||||
| const authorizationServer = await this.#asProvider.getAuthorizationServer(request) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -106,7 +132,38 @@ export class DPoPTokenProvider implements TokenProvider { | |||||||||||||
|
|
||||||||||||||
| const tokenResult = await oauth.processAuthorizationCodeResponse(authorizationServer, clientRegistration, tokenResponse, {expectedNonce: this.nonceVerificationOverride(authorizationServer.issuer, nonce)}) | ||||||||||||||
|
|
||||||||||||||
| return {created: Date.now(), tokenResult, dpopKey} | ||||||||||||||
| return {created: Date.now(), tokenResult, dpopKey, client: clientRegistration, authorizationServer} | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private async refreshToken(request: Request): Promise<CacheEntry | undefined> { | ||||||||||||||
| const cached = this.#cache.get(request.url) | ||||||||||||||
| if (cached === undefined) { | ||||||||||||||
| return undefined | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+138
to
+142
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just call
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| if (cached.tokenResult.refresh_token === undefined) { | ||||||||||||||
| return undefined | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const dpop = oauth.DPoP({}, cached.dpopKey) | ||||||||||||||
| const options = {DPoP: dpop, signal: request.signal} | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||
|
|
||||||||||||||
| const tokenResponse = await oauth.refreshTokenGrantRequest(cached.authorizationServer, cached.client, this.getClientAuth(cached.authorizationServer.issuer, cached.client), cached.tokenResult.refresh_token, options) | ||||||||||||||
|
|
||||||||||||||
| let tokenResult: oauth.TokenEndpointResponse | ||||||||||||||
| try { | ||||||||||||||
| tokenResult = await oauth.processRefreshTokenResponse(cached.authorizationServer, cached.client, tokenResponse) | ||||||||||||||
| } catch (e) { | ||||||||||||||
| if (e instanceof oauth.ResponseBodyError && e.error === "invalid_grant") { | ||||||||||||||
| console.debug("Access token could not be refreshed") | ||||||||||||||
|
|
||||||||||||||
| return undefined | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| throw e | ||||||||||||||
|
Comment on lines
+162
to
+163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the refresh token is invalid for any reason; we should make sure it is cleared from the cache. |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return {created: Date.now(), tokenResult, dpopKey: cached.dpopKey, client: cached.client, authorizationServer: cached.authorizationServer} | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private getClientAuth(issuer: string, client: oauth.OmitSymbolProperties<oauth.Client>): oauth.ClientAuth { | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refresh tokens are bound to clients, so we must cache the client registration data with the rest.
-- RFC 6749, §6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cac016d