Fixes issue with expiryTime of OIDC cookie that causes refreshToken w…

…orkflow to be skipped (#1990) * Bug fix Signed-off-by: Alankarsharma <alankar.sharma005@gmail.com> * Update cookie expiry as well Signed-off-by: Alankarsharma <alankar.sharma005@gmail.com> * Lint issue fix Signed-off-by: Alankarsharma <alankar.sharma005@gmail.com> * fixed test case Signed-off-by: Alankarsharma <alankar.sharma005@gmail.com> * Add test for refresh token workflow in OIDC Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix assertion Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update getKeepAliveExpiry logic for OIDC Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add check to ensure mockClient.post is called Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Alankarsharma <alankar.sharma005@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Alankarsharma <alankar.sharma005@gmail.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com>
opensearch-project · Jun 11, 2024 · 70f2a9c · 70f2a9c
1 parent 489ae54
commit 70f2a9c
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 9 deletions.
diff --git 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
@@ -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<unknown, unknown, unknown, any>
   ): 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
@@ -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, {