From 70f2a9c4610fc70d310828d477151aeb9c0b2e0c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 11 Jun 2024 11:54:30 -0400 Subject: [PATCH] Fixes issue with expiryTime of OIDC cookie that causes refreshToken workflow to be skipped (#1990) * Bug fix Signed-off-by: Alankarsharma * Update cookie expiry as well Signed-off-by: Alankarsharma * Lint issue fix Signed-off-by: Alankarsharma * fixed test case Signed-off-by: Alankarsharma * Add test for refresh token workflow in OIDC Signed-off-by: Craig Perkins * Fix assertion Signed-off-by: Craig Perkins * Update getKeepAliveExpiry logic for OIDC Signed-off-by: Craig Perkins * Add check to ensure mockClient.post is called Signed-off-by: Craig Perkins --------- Signed-off-by: Alankarsharma Signed-off-by: Craig Perkins Co-authored-by: Alankarsharma Co-authored-by: Darshit Chanpura --- server/auth/types/openid/openid_auth.test.ts | 77 +++++++++++++++++++- server/auth/types/openid/openid_auth.ts | 7 +- server/auth/types/openid/routes.ts | 3 +- 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/server/auth/types/openid/openid_auth.test.ts b/server/auth/types/openid/openid_auth.test.ts index c8cc839e7..58ed56547 100644 --- a/server/auth/types/openid/openid_auth.test.ts +++ b/server/auth/types/openid/openid_auth.test.ts @@ -37,6 +37,12 @@ interface Logger { fatal(message: string): void; } +const mockClient = { post: jest.fn() }; + +jest.mock('@hapi/wreck', () => ({ + defaults: jest.fn(() => mockClient), +})); + describe('test OpenId authHeaderValue', () => { let router: IRouter; let core: CoreSetup; @@ -208,7 +214,7 @@ describe('test OpenId authHeaderValue', () => { expect(wreckHttpsOptions.passphrase).toBeUndefined(); }); - test('Ensure expiryTime is being used to test validity of cookie', async () => { + test('Ensure accessToken expiryTime is being used to test validity of cookie', async () => { const realDateNow = Date.now.bind(global.Date); const dateNowStub = jest.fn(() => 0); global.Date.now = dateNowStub; @@ -229,22 +235,84 @@ describe('test OpenId authHeaderValue', () => { const testCookie: SecuritySessionCookie = { credentials: { authHeaderValue: 'Bearer eyToken', - expiry_time: -1, + expiryTime: 200, }, - expiryTime: 2000, + expiryTime: 10000, username: 'admin', authType: 'openid', }; + // Credentials are valid because 0 < 200 expect(await openIdAuthentication.isValidCookie(testCookie, {})).toBe(true); global.Date.now = realDateNow; }); + test('Ensure refreshToken workflow is called if current time is after access token expiry, but before session expiry', async () => { + const realDateNow = Date.now.bind(global.Date); + const dateNowStub = jest.fn(() => 300); + global.Date.now = dateNowStub; + const oidcConfig: unknown = { + openid: { + header: 'authorization', + scope: [], + extra_storage: { + cookie_prefix: 'testcookie', + additional_cookies: 0, + }, + }, + }; + + const openIdAuthentication = new OpenIdAuthentication( + oidcConfig as SecurityPluginConfigType, + sessionStorageFactory, + router, + esClient, + core, + logger + ); + const testCookie: SecuritySessionCookie = { + credentials: { + authHeaderValue: 'Bearer eyToken', + expiryTime: 200, + refresh_token: 'refreshToken', + }, + expiryTime: 10000, + username: 'admin', + authType: 'openid', + }; + + const mockRequestPayload = JSON.stringify({ + grant_type: 'refresh_token', + client_id: 'clientId', + client_secret: 'clientSecret', + refresh_token: 'refreshToken', + }); + const mockResponsePayload = JSON.stringify({ + id_token: '.eyJleHAiOiIwLjUifQ.', // Translates to {"exp":"0.5"} + access_token: 'accessToken', + refresh_token: 'refreshToken', + }); + mockClient.post.mockResolvedValue({ + res: { statusCode: 200 }, + payload: mockResponsePayload, + }); + + expect(await openIdAuthentication.isValidCookie(testCookie, {})).toBe(true); + expect(mockClient.post).toBeCalledTimes(1); + global.Date.now = realDateNow; + }); + test('getKeepAliveExpiry', () => { + const realDateNow = Date.now.bind(global.Date); + const dateNowStub = jest.fn(() => 300); + global.Date.now = dateNowStub; const oidcConfig: unknown = { openid: { scope: [], }, + session: { + ttl: 3600, + }, }; const openIdAuthentication = new OpenIdAuthentication( @@ -262,6 +330,7 @@ describe('test OpenId authHeaderValue', () => { expiryTime: 1000, }; - expect(openIdAuthentication.getKeepAliveExpiry(testCookie, {})).toBe(1000); + expect(openIdAuthentication.getKeepAliveExpiry(testCookie, {})).toBe(3900); + global.Date.now = realDateNow; }); }); diff --git a/server/auth/types/openid/openid_auth.ts b/server/auth/types/openid/openid_auth.ts index b67e174c8..3292b68e1 100644 --- a/server/auth/types/openid/openid_auth.ts +++ b/server/auth/types/openid/openid_auth.ts @@ -250,12 +250,11 @@ export class OpenIdAuthentication extends AuthenticationType { }; } - // OIDC expiry time is set by the IDP and refreshed via refreshTokens getKeepAliveExpiry( cookie: SecuritySessionCookie, request: OpenSearchDashboardsRequest ): number { - return cookie.expiryTime!; + return Date.now() + this.config.session.ttl; } // TODO: Add token expiration check here @@ -272,7 +271,7 @@ export class OpenIdAuthentication extends AuthenticationType { return false; } - if (cookie.expiryTime > Date.now()) { + if (cookie.credentials.expiryTime > Date.now()) { return true; } @@ -296,8 +295,8 @@ export class OpenIdAuthentication extends AuthenticationType { cookie.credentials = { authHeaderValueExtra: true, refresh_token: refreshTokenResponse.refreshToken, + expiryTime: getExpirationDate(refreshTokenResponse), }; - cookie.expiryTime = getExpirationDate(refreshTokenResponse); setExtraAuthStorage( request, diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index 51ac1a85d..8634f09e2 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -195,9 +195,10 @@ export class OpenIdAuthRoutes { username: user.username, credentials: { authHeaderValueExtra: true, + expiryTime: getExpirationDate(tokenResponse), }, authType: AuthType.OPEN_ID, - expiryTime: getExpirationDate(tokenResponse), + expiryTime: Date.now() + this.config.session.ttl, }; if (this.config.openid?.refresh_tokens && tokenResponse.refreshToken) { Object.assign(sessionStorage.credentials, {