Merge pull request #2423 from logto-io/charles-change-session-health-validation-method

refactor(core): change session health validation method for session APIs

packages/core/src/lib/session.ts

@@ -1,10 +1,10 @@
-import { conditional } from '@silverhand/essentials';
 import { getUnixTime } from 'date-fns';
 import type { Context } from 'koa';
 import type { InteractionResults, Provider } from 'oidc-provider';
 
 import RequestError from '@/errors/RequestError';
 import { findUserById, updateUserById } from '@/queries/user';
+import assertThat from '@/utils/assert-that';
 
 export const assignInteractionResults = async (
   ctx: Context,
@@ -17,32 +17,20 @@ export const assignInteractionResults = async (
   // have to do it manually
   // refer to: https://github.com/panva/node-oidc-provider/blob/c243bf6b6663c41ff3e75c09b95fb978eba87381/lib/actions/authorization/interactions.js#L106
   const details = merge ? await provider.interactionDetails(ctx.req, ctx.res) : undefined;
-  const ts = getUnixTime(new Date());
+
-  const mergedResult = {
-    // Merge with current result
-    ...details?.result,
-    ...result,
-  };
   const redirectTo = await provider.interactionResult(
     ctx.req,
     ctx.res,
     {
-      ...mergedResult,
+      // Merge with current result
-      ...conditional(
+      ...details?.result,
-        mergedResult.login && {
+      ...result,
-          login: {
-            ...mergedResult.login,
-            // Update ts(timestamp) if the accountId has been set in result
-            ts: result.login?.accountId ? ts : mergedResult.login.ts,
-          },
-        }
-      ),
     },
     {
       mergeWithLastSubmission: merge,
     }
   );
-  ctx.body = { redirectTo, ts };
+  ctx.body = { redirectTo };
 };
 
 export const checkSessionHealth = async (
@@ -50,23 +38,24 @@ export const checkSessionHealth = async (
   provider: Provider,
   tolerance = 10 * 60 // 10 mins
 ) => {
-  const { result } = await provider.interactionDetails(ctx.req, ctx.res);
+  const { accountId, loginTs } = await provider.Session.get(ctx);
 
-  if (!result?.login?.accountId) {
+  assertThat(
-    throw new RequestError('auth.unauthorized');
+    accountId,
-  }
+    new RequestError({ code: 'auth.unauthorized', id: accountId, status: 401 })
+  );
 
-  if (!result.login.ts || result.login.ts < getUnixTime(new Date()) - tolerance) {
+  if (!loginTs || loginTs < getUnixTime(new Date()) - tolerance) {
-    const { passwordEncrypted, primaryPhone, primaryEmail } = await findUserById(
+    const { passwordEncrypted, primaryPhone, primaryEmail } = await findUserById(accountId);
-      result.login.accountId
-    );
 
     // No authenticated method configured for this user. Pass!
     if (!passwordEncrypted && !primaryPhone && !primaryEmail) {
       return;
     }
-    throw new RequestError('auth.require_re_authentication');
+    throw new RequestError({ code: 'auth.require_re_authentication', status: 422 });
   }
+
+  return accountId;
 };
 
 export const saveUserFirstConsentedAppId = async (userId: string, applicationId: string) => {

packages/core/src/routes/session/continue.test.ts

@@ -89,8 +89,7 @@ describe('session -> continueRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: mockUser.id } }),
-        expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -122,8 +121,7 @@ describe('session -> continueRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: mockUser.id } }),
-        expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -158,8 +156,7 @@ describe('session -> continueRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: mockUser.id } }),
-        expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -191,8 +188,7 @@ describe('session -> continueRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: mockUser.id } }),
-        expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
         expect.anything()
       );
     });

packages/core/src/routes/session/password.test.ts

@@ -111,8 +111,7 @@ describe('session -> password routes', () => {
     expect(interactionResult).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+      expect.objectContaining({ login: { accountId: mockUser.id } }),
-      expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
       expect.anything()
     );
   });
@@ -128,8 +127,7 @@ describe('session -> password routes', () => {
     expect(interactionResult).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+      expect.objectContaining({ login: { accountId: mockUser.id } }),
-      expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
       expect.anything()
     );
   });
@@ -145,8 +143,7 @@ describe('session -> password routes', () => {
     expect(interactionResult).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+      expect.objectContaining({ login: { accountId: mockUser.id } }),
-      expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
       expect.anything()
     );
   });
@@ -175,8 +172,7 @@ describe('session -> password routes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: 'user1' } }),
-        expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }),
         expect.anything()
       );
       jest.useRealTimers();

packages/core/src/routes/session/passwordless.test.ts

@@ -396,8 +396,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: mockUser.id },
-          login: { accountId: mockUser.id, ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -419,8 +418,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: mockUser.id },
-          login: { accountId: mockUser.id, ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -562,8 +560,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: mockUser.id },
-          login: { accountId: mockUser.id, ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -587,8 +584,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: mockUser.id },
-          login: { accountId: mockUser.id, ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -698,8 +694,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: 'user1' },
-          login: { accountId: 'user1', ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -721,8 +716,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: 'user1' },
-          login: { accountId: 'user1', ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -828,8 +822,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: 'user1' },
-          login: { accountId: 'user1', ts: expect.any(Number) },
         }),
         expect.anything()
       );
@@ -851,8 +844,7 @@ describe('session -> passwordlessRoutes', () => {
         expect.anything(),
         expect.anything(),
         expect.objectContaining({
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+          login: { accountId: 'user1' },
-          login: { accountId: 'user1', ts: expect.any(Number) },
         }),
         expect.anything()
       );

packages/core/src/routes/session/social.test.ts

@@ -1,4 +1,3 @@
-/* eslint-disable max-lines */
 import { ConnectorType } from '@logto/connector-kit';
 import type { User } from '@logto/schemas';
 import { SignUpIdentifier } from '@logto/schemas';
@@ -234,8 +233,7 @@ describe('session -> socialRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: mockUser.id } }),
-        expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -318,8 +316,7 @@ describe('session -> socialRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: 'user1' } }),
-        expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -353,8 +350,7 @@ describe('session -> socialRoutes', () => {
       expect(interactionResult).toHaveBeenCalledWith(
         expect.anything(),
         expect.anything(),
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        expect.objectContaining({ login: { accountId: 'user1' } }),
-        expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }),
         expect.anything()
       );
     });
@@ -368,10 +364,7 @@ describe('session -> socialRoutes', () => {
     });
 
     it('throw error if result parsing fails', async () => {
-      interactionDetails.mockResolvedValueOnce({
+      interactionDetails.mockResolvedValueOnce({ result: { login: { accountId: mockUser.id } } });
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-        result: { login: { accountId: mockUser.id, ts: expect.any(Number) } },
-      });
       const response = await sessionRequest
         .post(`${registerRoute}`)
         .send({ connectorId: 'connectorId' });
@@ -443,4 +436,3 @@ describe('session -> socialRoutes', () => {
     });
   });
 });
-/* eslint-enable max-lines */

packages/core/src/routes/session/utils.test.ts

@@ -121,8 +121,7 @@ describe('signInWithPassword()', () => {
     expect(interactionResult).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+      expect.objectContaining({ login: { accountId: mockUser.id } }),
-      expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }),
       expect.anything()
     );
   });