From 5e8bd34af67b589f649cf8f1ec5a7e8b89de9ab8 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Tue, 9 Apr 2024 13:03:58 -0400 Subject: [PATCH] feedback --- README.md | 44 ++++++++++++++++++++++++++++++++---- lib/oidc/dpop.ts | 14 +++++------- lib/oidc/mixin/index.ts | 9 ++++---- lib/oidc/types/api.ts | 4 ++-- test/apps/app/src/testApp.ts | 8 +++---- test/spec/OktaAuth/api.ts | 23 +++++++++++-------- test/spec/oidc/dpop.ts | 8 ++++--- 7 files changed, 75 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 4c399b563..bca3b9994 100644 --- a/README.md +++ b/README.md @@ -429,7 +429,7 @@ DPoP: eyJ0eXAiOiJkcG9wK2p3dCIsIm... ```javascript async function dpopAuthenticatedFetch (url, options) { const { method } = options; - const dpop = await authClient.getDPoPHeaders({ url, method }); + const dpop = await authClient.getDPoPAuthorizationHeaders({ url, method }); // dpop = { Authorization: "DPoP token****", Dpop: "proof****" } const headers = new Headers({...options.headers, ...dpop}); return fetch(url, {...options, headers }); @@ -456,9 +456,9 @@ async function dpopAuthenticatedFetch (url, options) { // resp = HTTP/1.1 401 Unauthorized... if (!resp.ok) { - const nonce = authClient.getDPoPNonceHeader(resp.headers); + const nonce = authClient.parseUseDPoPNonceError(resp.headers); if (nonce) { - const retryDpop = await authClient.getDPoPHeaders({ url, method, nonce }); + const retryDpop = await authClient.getDPoPAuthorizationHeaders({ url, method, nonce }); const retryHeaders = new Headers({...options.headers, ...retryDpop}); return fetch(url, {...options, headers: retryHeaders }); } @@ -481,7 +481,7 @@ useEffect(() => { }, []); ``` -#### Clear Storage (*Recommended*) +#### Clear DPoP Storage (*Recommended*) DPoP requires the generation of a `CryptoKeyPair` which needs to persist in storage. Methods like `signOut()` or `revokeAccessToken()` will clear the key pair, however users don't always explicitly logout. It's good practice to clear storage before login to flush any orphaned key pairs generated from previously requested tokens ```javascript @@ -1003,6 +1003,9 @@ This is accomplished by selecting a single tab to handle the network requests to * [tx.resume](#txresume) * [tx.exists](#txexists) * [transaction.status](#transactionstatus) +* [getDPoPAuthorizationHeaders](#getdpopauthorizationheaders) +* [parseUseDPoPNonceError](#parseusedpopnonceerror) +* [clearDPoPStorage](#cleardpopstorage) * [session](#session) * [session.setCookieAndRedirect](#sessionsetcookieandredirectsessiontoken-redirecturi) * [session.exists](#sessionexists) @@ -1354,6 +1357,39 @@ See [authn API](docs/authn.md#txexists). See [authn API](docs/authn.md#transactionstatus). +### `getDPoPAuthorizationHeaders(params)` + +> :link: web browser only
+> :hourglass: async
+ +Requires [dpop](#dpop) set to `true`. Returns `Authorization` and `Dpop` header values to build a DPoP protected-request. + +Params: `url` and (http) `method` are required. +* `accessToken` is optional, but will be read from `tokenStorage` if not provided +* `nonce` is optional, may be provided via `use_dpop_nonce` pattern from Resource Server ([more info](#handling-use_dpop_nonce)) + +### `parseUseDPoPNonceError(headers)` + +> :link: web browser only
+ +Utility to extract and parse the `WWW-Authenticate` and `DPoP-Nonce` headers from a network response from a DPoP-protected request. Should the response be in the following format, the `nonce` value will be returned. Otherwise returns `null` + +``` +HTTP/1.1 401 Unauthorized +WWW-Authenticate: DPoP error="use_dpop_nonce", \ + error_description="Resource server requires nonce in DPoP proof" +DPoP-Nonce: eyJ7S_zG.eyJH0-Z.HX4w-7v +``` + +### `clearDPoPStorage(clearAll=false)` + +> :link: web browser only
+> :hourglass: async
+ +Clears storage location of `CryptoKeyPair`s generated and used by DPoP. Pass `true` to remove all key pairs as it's possible for orphaned key pairs to exist. If `clearAll` is `false`, the key pair bound to the current `accessToken` in tokenStorage will be removed. + +It's recommended to call this function during user login. [See Example](#clear-dpop-storage-recommended) + ### `session` #### `session.setCookieAndRedirect(sessionToken, redirectUri)` diff --git a/lib/oidc/dpop.ts b/lib/oidc/dpop.ts index 668087718..2c45df945 100644 --- a/lib/oidc/dpop.ts +++ b/lib/oidc/dpop.ts @@ -160,14 +160,12 @@ export async function findKeyPair (pairId?: string): Promise { throw new AuthSdkError(`Unable to locate dpop key pair required for refresh${pairId ? ` (${pairId})` : ''}`); } -// deletes a specifc KP, or the entire db if no id is passed -export async function clearDPoPKeyPair (pairId?: string): Promise { - if (pairId) { - await invokeStoreMethod('delete', pairId); - } - else { - await invokeStoreMethod('clear'); - } +export async function clearDPoPKeyPair (pairId: string): Promise { + await invokeStoreMethod('delete', pairId); +} + +export async function clearAllDPoPKeyPairs (): Promise { + await invokeStoreMethod('clear'); } // generates a crypto (non-extractable) private key pair and writes it to indexeddb, returns key (id) diff --git a/lib/oidc/mixin/index.ts b/lib/oidc/mixin/index.ts index d58f1066b..99c0ed230 100644 --- a/lib/oidc/mixin/index.ts +++ b/lib/oidc/mixin/index.ts @@ -37,6 +37,7 @@ import { getOAuthUrls, isLoginRedirect, hasResponseType } from '../util'; import { generateDPoPProof, clearDPoPKeyPair, + clearAllDPoPKeyPairs, clearDPoPKeyPairAfterRevoke, findKeyPair, isDPoPNonceError @@ -339,7 +340,7 @@ export function mixinOAuth } const dpopPairId = accessToken?.dpopPairId ?? refreshToken?.dpopPairId; - if (this.options.dpop) { + if (this.options.dpop && dpopPairId) { await clearDPoPKeyPair(dpopPairId); } @@ -373,7 +374,7 @@ export function mixinOAuth } } - async getDPoPHeaders (params: DPoPRequest): Promise { + async getDPoPAuthorizationHeaders (params: DPoPRequest): Promise { if (!this.options.dpop) { throw new AuthSdkError('DPoP is not configured for this client instance'); } @@ -397,7 +398,7 @@ export function mixinOAuth async clearDPoPStorage (clearAll=false): Promise { if (clearAll) { - return clearDPoPKeyPair(); + return clearAllDPoPKeyPairs(); } const tokens = await this.tokenManager.getTokens(); @@ -408,7 +409,7 @@ export function mixinOAuth } } - getDPoPNonceHeader (headers: HeadersInit): string | null { + parseUseDPoPNonceError (headers: HeadersInit): string | null { const wwwAuth = WWWAuthError.getWWWAuthenticateHeader(headers); const wwwErr = WWWAuthError.parseHeader(wwwAuth ?? ''); if (isDPoPNonceError(wwwErr)) { diff --git a/lib/oidc/types/api.ts b/lib/oidc/types/api.ts index 009db2bb9..8cb6043ab 100644 --- a/lib/oidc/types/api.ts +++ b/lib/oidc/types/api.ts @@ -181,7 +181,7 @@ export interface OktaAuthOAuthInterface revokeAccessToken(accessToken?: AccessToken): Promise; revokeRefreshToken(refreshToken?: RefreshToken): Promise; - getDPoPHeaders(params: DPoPRequest): Promise; + getDPoPAuthorizationHeaders(params: DPoPRequest): Promise; clearDPoPStorage(clearAll: boolean): Promise; - getDPoPNonceHeader(headers: HeadersInit): string | null; + parseUseDPoPNonceError(headers: HeadersInit): string | null; } diff --git a/test/apps/app/src/testApp.ts b/test/apps/app/src/testApp.ts index 9d7d39943..e8301b7fb 100644 --- a/test/apps/app/src/testApp.ts +++ b/test/apps/app/src/testApp.ts @@ -189,7 +189,7 @@ function bindFunctions(testApp: TestApp, window: Window): void { startService: testApp.startService.bind(testApp), stopService: testApp.stopService.bind(testApp), enrollAuthenticator: testApp.enrollAuthenticator.bind(testApp), - getDPoPHeaders: testApp.getDPoPHeaders.bind(testApp) + getDPoPAuthorizationHeaders: testApp.getDPoPAuthorizationHeaders.bind(testApp) }; Object.keys(boundFunctions).forEach(functionName => { (window as any)[functionName] = makeClickHandler((boundFunctions as any)[functionName]); @@ -739,9 +739,9 @@ class TestApp { }); } - async getDPoPHeaders(): Promise { + async getDPoPAuthorizationHeaders(): Promise { // passes dummy values for url and method - const headers = await this.oktaAuth.getDPoPHeaders({ + const headers = await this.oktaAuth.getDPoPAuthorizationHeaders({ url: 'http://localhost:8080/foo', method: 'POST' }); @@ -983,7 +983,7 @@ class TestApp { Simulate cross-tab token renew
  • - Generate DPoP Proof + Generate DPoP Proof
  • Enroll Authenticator diff --git a/test/spec/OktaAuth/api.ts b/test/spec/OktaAuth/api.ts index 726bd5016..ae7cba1e5 100644 --- a/test/spec/OktaAuth/api.ts +++ b/test/spec/OktaAuth/api.ts @@ -17,6 +17,7 @@ const mocked = { findKeyPair: jest.fn(), // generateDPoPProof: jest.fn(), clearDPoPKeyPair: jest.fn(), + clearAllDPoPKeyPairs: jest.fn(), clearDPoPKeyPairAfterRevoke: jest.fn(), }; jest.mock('../../../lib/oidc/dpop', () => { @@ -634,14 +635,14 @@ describe('OktaAuth (api)', function() { }); - describe('getDPoPHeaders', () => { + describe('getDPoPAuthorizationHeaders', () => { beforeEach(() => { auth = new OktaAuth({ issuer, dpop: true }); }); it('should throw if dpop is not enabled', async () => { auth.options.dpop = false; - await expect(auth.getDPoPHeaders({ + await expect(auth.getDPoPAuthorizationHeaders({ url: 'localhost:8080/test', method: 'GET' })).rejects.toThrow(new AuthSdkError('DPoP is not configured for this client instance')); }); @@ -651,7 +652,7 @@ describe('OktaAuth (api)', function() { jest.spyOn(auth.tokenManager, 'getTokensSync').mockReturnValue({ accessToken }); mocked.findKeyPair.mockImplementation(async () => await generateKeyPair()); - const headers = await auth.getDPoPHeaders({ url: 'localhost:8080/test', method: 'GET' }); + const headers = await auth.getDPoPAuthorizationHeaders({ url: 'localhost:8080/test', method: 'GET' }); expect(headers).toMatchObject({ Authorization: `DPoP ${accessToken.accessToken}`, @@ -660,7 +661,7 @@ describe('OktaAuth (api)', function() { }); it('should throw if no accessToken exists', async () => { - await expect(auth.getDPoPHeaders({ url: 'localhost:8080/test', method: 'GET' })) + await expect(auth.getDPoPAuthorizationHeaders({ url: 'localhost:8080/test', method: 'GET' })) .rejects.toThrow(new AuthSdkError('AccessToken is required to generate a DPoP Proof')); }); @@ -668,7 +669,7 @@ describe('OktaAuth (api)', function() { const accessToken = { accessToken: 'fake' }; mocked.findKeyPair.mockImplementation(async () => await generateKeyPair()); - const headers = await auth.getDPoPHeaders( + const headers = await auth.getDPoPAuthorizationHeaders( { url: 'localhost:8080/test', method: 'GET', accessToken } ); @@ -686,13 +687,14 @@ describe('OktaAuth (api)', function() { it('should clear all DPoP keys', async () => { await auth.clearDPoPStorage(true); - expect(mocked.clearDPoPKeyPair).toHaveBeenCalled(); - expect(mocked.clearDPoPKeyPair.mock.calls[0]).toEqual([]); // called with no args + expect(mocked.clearAllDPoPKeyPairs).toHaveBeenCalled(); + expect(mocked.clearDPoPKeyPair).not.toHaveBeenCalled(); }); it('should no-op', async () => { await auth.clearDPoPStorage(); expect(mocked.clearDPoPKeyPair).not.toHaveBeenCalled(); + expect(mocked.clearAllDPoPKeyPairs).not.toHaveBeenCalled(); }); it('should clear specific key pair', async () => { @@ -700,10 +702,11 @@ describe('OktaAuth (api)', function() { jest.spyOn(auth.tokenManager, 'getTokens').mockReturnValue({ accessToken }); await auth.clearDPoPStorage(); expect(mocked.clearDPoPKeyPair).toHaveBeenCalledWith('foo'); + expect(mocked.clearAllDPoPKeyPairs).not.toHaveBeenCalled(); }); }); - describe('getDPoPNonceHeader', () => { + describe('parseUseDPoPNonceError', () => { it('get header from Headers instance', () => { if (!global.Headers) { // do not fail in node tests @@ -715,7 +718,7 @@ describe('OktaAuth (api)', function() { 'WWW-Authenticate': `DPoP error="use_dpop_nonce", error_description="Resource server requires nonce in DPoP proof"` }); - const nonce = auth.getDPoPNonceHeader(headers); + const nonce = auth.parseUseDPoPNonceError(headers); expect(nonce).toEqual('nonceuponatime'); }); @@ -725,7 +728,7 @@ describe('OktaAuth (api)', function() { 'WWW-Authenticate': `DPoP error="use_dpop_nonce", error_description="Resource server requires nonce in DPoP proof"` }; - const nonce = auth.getDPoPNonceHeader(headers); + const nonce = auth.parseUseDPoPNonceError(headers); expect(nonce).toEqual('nonceuponatime'); }); }); diff --git a/test/spec/oidc/dpop.ts b/test/spec/oidc/dpop.ts index a1a33f070..0f6093c63 100644 --- a/test/spec/oidc/dpop.ts +++ b/test/spec/oidc/dpop.ts @@ -28,7 +28,7 @@ describe('dpop', () => { describe('Key Store', () => { beforeEach(async () => { - await dpop.clearDPoPKeyPair(); + await dpop.clearAllDPoPKeyPairs(); indexedDB.deleteDatabase('OktaAuthJs'); }); @@ -62,13 +62,15 @@ describe('dpop', () => { await expect(dpop.findKeyPair(kp1.keyPairId)).rejects.toThrow(); await expect(dpop.findKeyPair(kp2.keyPairId)).resolves.toBeDefined(); }); + }); - it('should delete all KPs when no keyId is provided', async () => { + describe('clearAllDPoPKeyPairs', () => { + it('should delete all KPs', async () => { const kp1 = await dpop.createDPoPKeyPair(); const kp2 = await dpop.createDPoPKeyPair(); await expect(dpop.findKeyPair(kp1.keyPairId)).resolves.toBeDefined(); await expect(dpop.findKeyPair(kp2.keyPairId)).resolves.toBeDefined(); - await dpop.clearDPoPKeyPair(); + await dpop.clearAllDPoPKeyPairs(); await expect(dpop.findKeyPair(kp1.keyPairId)).rejects.toThrow(); await expect(dpop.findKeyPair(kp2.keyPairId)).rejects.toThrow(); });