refactor(core): refactor the sso-connector config guard logic (#4752)

* refactor(core): refactor the sso-connector config guard logic

refactor the sso-connector config guard logic

* chore(core): update with some more comments

update with some more comments

* refactor(core): adjust the input type of parseConnectorConfig

make the config requred in the parseConnectorConfig's input

* refactor(core): add the isSupportedSsoConnector type assertion (#4754)

* refactor(core): add the isSupportedSsoConnector type assertion

add the isSupportedSsoConnector type assertion

* chore(core): remove the useless comments

remove the useless comments

* refactor(core): reorg the sso utils

reorg the sso utils

* refactor(core): extract sso connector library

extract sso connector library

packages/core/src/__mocks__/sso.ts

@@ -1,9 +1,11 @@
 import { type SsoConnector } from '@logto/schemas';
 
-export const mockSsoConnector: SsoConnector = {
+import { SsoProviderName } from '#src/sso/types/index.js';
+
+export const mockSsoConnector = {
   id: 'mock-sso-connector',
   tenantId: 'mock-tenant',
-  providerName: 'OIDC',
+  providerName: SsoProviderName.OIDC,
   connectorName: 'mock-connector-name',
   config: {},
   domains: [],
@@ -11,4 +13,4 @@ export const mockSsoConnector: SsoConnector = {
   syncProfile: true,
   ssoOnly: true,
   createdAt: Date.now(),
-};
+} satisfies SsoConnector;

packages/core/src/libraries/sso-connector.test.ts

@@ -0,0 +1,63 @@
+import { mockSsoConnector } from '#src/__mocks__/sso.js';
+import RequestError from '#src/errors/RequestError/index.js';
+import { MockQueries } from '#src/test-utils/tenant.js';
+
+import { createSsoConnectorLibrary } from './sso-connector.js';
+
+const { jest } = import.meta;
+
+const findAllSsoConnectors = jest.fn();
+const geConnectorById = jest.fn();
+
+const queries = new MockQueries({
+  ssoConnectors: { findAll: findAllSsoConnectors, findById: geConnectorById },
+});
+
+describe('SsoConnectorLibrary', () => {
+  const ssoConnectorLibrary = createSsoConnectorLibrary(queries);
+
+  it('getSsoConnectors() should filter unsupported sso connectors', async () => {
+    const { getSsoConnectors } = ssoConnectorLibrary;
+
+    findAllSsoConnectors.mockResolvedValueOnce([
+      2,
+      [
+        mockSsoConnector,
+        {
+          ...mockSsoConnector,
+          providerName: 'unsupported',
+        },
+      ],
+    ]);
+
+    const connectors = await getSsoConnectors();
+
+    expect(connectors).toEqual([mockSsoConnector]);
+  });
+
+  it('getSsoConnectorById() should throw 404 if the connector is not supported', async () => {
+    const { getSsoConnectorById } = ssoConnectorLibrary;
+
+    geConnectorById.mockResolvedValueOnce({
+      ...mockSsoConnector,
+      providerName: 'unsupported',
+    });
+
+    await expect(getSsoConnectorById('id')).rejects.toMatchError(
+      new RequestError({
+        code: 'connector.not_found',
+        status: 404,
+      })
+    );
+  });
+
+  it('getSsoConnectorById() should return the connector if it is supported', async () => {
+    const { getSsoConnectorById } = ssoConnectorLibrary;
+
+    geConnectorById.mockResolvedValueOnce(mockSsoConnector);
+
+    const connector = await getSsoConnectorById('id');
+
+    expect(connector).toEqual(mockSsoConnector);
+  });
+});

packages/core/src/libraries/sso-connector.ts

@@ -0,0 +1,38 @@
+import { assert } from '@silverhand/essentials';
+
+import RequestError from '#src/errors/RequestError/index.js';
+import { type SupportedSsoConnector } from '#src/sso/types/index.js';
+import { isSupportedSsoConnector } from '#src/sso/utils.js';
+import type Queries from '#src/tenants/Queries.js';
+
+export const createSsoConnectorLibrary = (queries: Queries) => {
+  const { ssoConnectors } = queries;
+
+  const getSsoConnectors = async () => {
+    const [_, entities] = await ssoConnectors.findAll();
+
+    return entities.filter((connector): connector is SupportedSsoConnector =>
+      isSupportedSsoConnector(connector)
+    );
+  };
+
+  const getSsoConnectorById = async (id: string) => {
+    const connector = await ssoConnectors.findById(id);
+
+    // Return 404 if the connector is not supported
+    assert(
+      isSupportedSsoConnector(connector),
+      new RequestError({
+        code: 'connector.not_found',
+        status: 404,
+      })
+    );
+
+    return connector;
+  };
+
+  return {
+    getSsoConnectors,
+    getSsoConnectorById,
+  };
+};

packages/core/src/routes/sso-connector/index.ts

@@ -1,12 +1,12 @@
 import { SsoConnectors, jsonObjectGuard } from '@logto/schemas';
 import { generateStandardShortId } from '@logto/shared';
-import { conditional } from '@silverhand/essentials';
+import { conditional, assert } from '@silverhand/essentials';
 import { z } from 'zod';
 
 import RequestError from '#src/errors/RequestError/index.js';
 import koaGuard from '#src/middleware/koa-guard.js';
-import { ssoConnectorFactories } from '#src/sso/index.js';
-import { SsoProviderName } from '#src/sso/types/index.js';
+import { ssoConnectorFactories, standardSsoConnectorProviders } from '#src/sso/index.js';
+import { isSupportedSsoProvider, isSupportedSsoConnector } from '#src/sso/utils.js';
 import { tableToPathname } from '#src/utils/SchemaRouter.js';
 
 import { type AuthedRouter, type RouterInitArgs } from '../types.js';
@@ -20,7 +20,6 @@ import {
 } from './type.js';
 import {
   parseFactoryDetail,
-  isSupportedSsoProvider,
   parseConnectorConfig,
   fetchConnectorProviderDetails,
 } from './utils.js';
@@ -29,11 +28,13 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
   const [
     router,
     {
+      libraries: { ssoConnector: ssoConnectorLibrary },
       queries: { ssoConnectors },
     },
   ] = args;
 
   const pathname = `/${tableToPathname(SsoConnectors.table)}`;
+  const { getSsoConnectorById, getSsoConnectors } = ssoConnectorLibrary;
 
   /*
     Get all supported single sign on connector factory details
@@ -53,7 +54,7 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
       const providerConnectors = new Set<ConnectorFactoryDetail>();
 
       for (const factory of factories) {
-        if ([SsoProviderName.OIDC].includes(factory.providerName)) {
+        if (standardSsoConnectorProviders.includes(factory.providerName)) {
           standardConnectors.add(parseFactoryDetail(factory, locale));
         } else {
           providerConnectors.add(parseFactoryDetail(factory, locale));
@@ -81,7 +82,7 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
       const { body } = ctx.guard;
       const { providerName, connectorName, config, ...rest } = body;
 
-      // TODO: @simeng-li new SSO error code
+      // Return 422 if the connector provider is not supported
       if (!isSupportedSsoProvider(providerName)) {
         throw new RequestError({
           code: 'connector.not_found',
@@ -92,9 +93,9 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
 
       /* 
         Validate the connector config if it's provided.
-        Allow partial config DB insert
+        Allow partial config settings on create.
        */
-      const parsedConfig = parseConnectorConfig(providerName, config);
+      const parsedConfig = config && parseConnectorConfig(providerName, config, true);
       const connectorId = generateStandardShortId();
 
       const connector = await ssoConnectors.insert({
@@ -119,16 +120,14 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
       status: [200],
     }),
     async (ctx, next) => {
-      // Query all connectors
-      const [_, entities] = await ssoConnectors.findAll();
+      const connectors = await getSsoConnectors();
 
       // Fetch provider details for each connector
       const connectorsWithProviderDetails = await Promise.all(
-        entities.map(async (connector) => fetchConnectorProviderDetails(connector))
+        connectors.map(async (connector) => fetchConnectorProviderDetails(connector))
       );
 
-      // Filter out unsupported connectors
-      ctx.body = connectorsWithProviderDetails.filter(Boolean);
+      ctx.body = connectorsWithProviderDetails;
 
       return next();
     }
@@ -145,20 +144,11 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
     async (ctx, next) => {
       const { id } = ctx.guard.params;
 
-      // Fetch the connector
-      const connector = await ssoConnectors.findById(id);
+      const connector = await getSsoConnectorById(id);
 
       // Fetch provider details for the connector
       const connectorWithProviderDetails = await fetchConnectorProviderDetails(connector);
 
-      // Return 404 if the connector is not found
-      if (!connectorWithProviderDetails) {
-        throw new RequestError({
-          code: 'connector.not_found',
-          status: 404,
-        });
-      }
-
       ctx.body = connectorWithProviderDetails;
 
       return next();
@@ -175,7 +165,6 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
     async (ctx, next) => {
       const { id } = ctx.guard.params;
 
-      // Delete the connector
       await ssoConnectors.deleteById(id);
       ctx.status = 204;
       return next();
@@ -195,23 +184,12 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
       const { id } = ctx.guard.params;
       const { body } = ctx.guard;
 
-      // Fetch the connector
-      const originalConnector = await ssoConnectors.findById(id);
+      const originalConnector = await getSsoConnectorById(id);
       const { providerName } = originalConnector;
-
-      // Return 422 if the connector provider is not supported
-      if (!isSupportedSsoProvider(providerName)) {
-        throw new RequestError({
-          code: 'connector.not_found',
-          type: providerName,
-          status: 422,
-        });
-      }
-
       const { config, ...rest } = body;
 
       // Validate the connector config if it's provided
-      const parsedConfig = parseConnectorConfig(providerName, config);
+      const parsedConfig = config && parseConnectorConfig(providerName, config);
 
       // Check if there's any valid update
       const hasValidUpdate = parsedConfig ?? Object.keys(rest).length > 0;
@@ -224,7 +202,12 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
           })
         : originalConnector;
 
-      // Fetch provider details for the connector
+      // Make the typescript happy
+      assert(
+        isSupportedSsoConnector(connector),
+        new RequestError({ code: 'connector.not_found', status: 404 })
+      );
+
       const connectorWithProviderDetails = await fetchConnectorProviderDetails(connector);
 
       ctx.body = connectorWithProviderDetails;
@@ -246,29 +229,24 @@ export default function singleSignOnRoutes<T extends AuthedRouter>(...args: Rout
       const { id } = ctx.guard.params;
       const { body } = ctx.guard;
 
-      // Fetch the connector
-      const { providerName, config } = await ssoConnectors.findById(id);
+      const { providerName, config } = await getSsoConnectorById(id);
 
-      // Return 422 if the connector provider is not supported
-      if (!isSupportedSsoProvider(providerName)) {
-        throw new RequestError({
-          code: 'connector.not_found',
-          type: providerName,
-          status: 422,
-        });
-      }
-
-      // Validate the connector config
-      const parsedConfig = parseConnectorConfig(providerName, body);
-
-      // Patch update the connector config
-      const connector = await ssoConnectors.updateById(id, {
-        config: {
-          ...config,
-          ...parsedConfig,
-        },
+      // Merge with existing config and revalidate
+      const parsedConfig = parseConnectorConfig(providerName, {
+        ...config,
+        ...body,
       });
 
+      const connector = await ssoConnectors.updateById(id, {
+        config: parsedConfig,
+      });
+
+      // Make the typescript happy
+      assert(
+        isSupportedSsoConnector(connector),
+        new RequestError({ code: 'connector.not_found', status: 404 })
+      );
+
       // Fetch provider details for the connector
       const connectorWithProviderDetails = await fetchConnectorProviderDetails(connector);
 

packages/core/src/routes/sso-connector/utils.test.ts

@@ -12,19 +12,7 @@ await mockEsmWithActual('#src/sso/OidcConnector/utils.js', () => ({
 }));
 
 const { ssoConnectorFactories } = await import('#src/sso/index.js');
-const { isSupportedSsoProvider, parseFactoryDetail, fetchConnectorProviderDetails } = await import(
-  './utils.js'
-);
-
-describe('isSupportedSsoProvider', () => {
-  it.each(Object.values(SsoProviderName))('should return true for %s', (providerName) => {
-    expect(isSupportedSsoProvider(providerName)).toBe(true);
-  });
-
-  it('should return false for unknown provider', () => {
-    expect(isSupportedSsoProvider('unknown-provider')).toBe(false);
-  });
-});
+const { parseFactoryDetail, fetchConnectorProviderDetails } = await import('./utils.js');
 
 describe('parseFactoryDetail', () => {
   it.each(Object.values(SsoProviderName))('should return correct detail for %s', (providerName) => {
@@ -54,15 +42,11 @@ describe('parseFactoryDetail', () => {
 });
 
 describe('fetchConnectorProviderDetails', () => {
-  it('should return undefined for unsupported provider', async () => {
-    const connector = { ...mockSsoConnector, providerName: 'unknown-provider' };
-    const result = await fetchConnectorProviderDetails(connector);
-
-    expect(result).toBeUndefined();
-  });
-
   it('providerConfig should be undefined if connector config is invalid', async () => {
-    const connector = { ...mockSsoConnector, config: { clientId: 'foo' } };
+    const connector = {
+      ...mockSsoConnector,
+      config: { clientId: 'foo' },
+    };
     const result = await fetchConnectorProviderDetails(connector);
 
     expect(result).toEqual({

packages/core/src/routes/sso-connector/utils.ts

@@ -1,19 +1,16 @@
 import { type I18nPhrases } from '@logto/connector-kit';
-import { type JsonObject, type SsoConnector } from '@logto/schemas';
+import { type JsonObject } from '@logto/schemas';
 import { conditional, trySafe } from '@silverhand/essentials';
 
 import RequestError from '#src/errors/RequestError/index.js';
 import { type SingleSignOnFactory, ssoConnectorFactories } from '#src/sso/index.js';
-import { type SsoProviderName } from '#src/sso/types/index.js';
+import { type SsoProviderName, type SupportedSsoConnector } from '#src/sso/types/index.js';
 
 import { type SsoConnectorWithProviderConfig } from './type.js';
 
 const isKeyOfI18nPhrases = (key: string, phrases: I18nPhrases): key is keyof I18nPhrases =>
   key in phrases;
 
-export const isSupportedSsoProvider = (providerName: string): providerName is SsoProviderName =>
-  providerName in ssoConnectorFactories;
-
 export const parseFactoryDetail = (
   factory: SingleSignOnFactory<SsoProviderName>,
   locale: string
@@ -29,21 +26,25 @@ export const parseFactoryDetail = (
 };
 
 /* 
-  Validate and partially parse the connector config if it's provided.
+  Validate the connector config if it's provided.
+  Throw error if the config is invalid.
+  Partially validate the config if allowPartial is true.
 */
-export const parseConnectorConfig = (providerName: SsoProviderName, config?: JsonObject) => {
-  if (!config) {
-    return;
-  }
-
+export const parseConnectorConfig = (
+  providerName: SsoProviderName,
+  config: JsonObject,
+  allowPartial?: boolean
+) => {
   const factory = ssoConnectorFactories[providerName];
 
-  const result = factory.configGuard.partial().safeParse(config);
+  const result = allowPartial
+    ? factory.configGuard.partial().safeParse(config)
+    : factory.configGuard.safeParse(config);
 
   if (!result.success) {
     throw new RequestError({
       code: 'connector.invalid_config',
-      status: 422,
+      status: 400,
       details: result.error.flatten(),
     });
   }
@@ -51,22 +52,17 @@ export const parseConnectorConfig = (providerName: SsoProviderName, config?: Jso
   return result.data;
 };
 
+/* 
+  Safely fetch and parse the detailed connector config from provider. 
+  Return undefined if failed to fetch or parse the config.
+*/
 export const fetchConnectorProviderDetails = async (
-  connector: SsoConnector
-): Promise<SsoConnectorWithProviderConfig | undefined> => {
+  connector: SupportedSsoConnector
+): Promise<SsoConnectorWithProviderConfig> => {
   const { providerName } = connector;
 
-  // Return undefined if the provider is not supported
-  if (!isSupportedSsoProvider(providerName)) {
-    return undefined;
-  }
-
   const { logo, constructor } = ssoConnectorFactories[providerName];
 
-  /* 
-    Safely fetch and parse the detailed connector config from provider. 
-    Return undefined if failed to fetch or parse the config.
-  */
   const providerConfig = await trySafe(async () => {
     const instance = new constructor(connector);
     return instance.getConfig();

packages/core/src/sso/index.ts

@@ -25,3 +25,5 @@ export const ssoConnectorFactories: {
 } = {
   [SsoProviderName.OIDC]: oidcSsoConnectorFactory,
 };
+
+export const standardSsoConnectorProviders = Object.freeze([SsoProviderName.OIDC]);

packages/core/src/sso/types/index.ts

@@ -15,3 +15,7 @@ export abstract class SingleSignOn {
 export enum SsoProviderName {
   OIDC = 'OIDC',
 }
+
+export type SupportedSsoConnector = Omit<SsoConnector, 'providerName'> & {
+  providerName: SsoProviderName;
+};

packages/core/src/sso/utils.test.ts

@@ -0,0 +1,12 @@
+import { SsoProviderName } from './types/index.js';
+import { isSupportedSsoProvider } from './utils.js';
+
+describe('isSupportedSsoProvider', () => {
+  it.each(Object.values(SsoProviderName))('should return true for %s', (providerName) => {
+    expect(isSupportedSsoProvider(providerName)).toBe(true);
+  });
+
+  it('should return false for unknown provider', () => {
+    expect(isSupportedSsoProvider('unknown-provider')).toBe(false);
+  });
+});

packages/core/src/sso/utils.ts

@@ -0,0 +1,11 @@
+import { type SsoConnector } from '@logto/schemas';
+
+import { ssoConnectorFactories } from './index.js';
+import { type SupportedSsoConnector, type SsoProviderName } from './types/index.js';
+
+export const isSupportedSsoProvider = (providerName: string): providerName is SsoProviderName =>
+  providerName in ssoConnectorFactories;
+
+export const isSupportedSsoConnector = (
+  connector: SsoConnector
+): connector is SupportedSsoConnector => isSupportedSsoProvider(connector.providerName);

packages/core/src/tenants/Libraries.ts

@@ -8,6 +8,7 @@ import { createPhraseLibrary } from '#src/libraries/phrase.js';
 import { createQuotaLibrary } from '#src/libraries/quota.js';
 import { createSignInExperienceLibrary } from '#src/libraries/sign-in-experience/index.js';
 import { createSocialLibrary } from '#src/libraries/social.js';
+import { createSsoConnectorLibrary } from '#src/libraries/sso-connector.js';
 import { createUserLibrary } from '#src/libraries/user.js';
 import { createVerificationStatusLibrary } from '#src/libraries/verification-status.js';
 
@@ -24,6 +25,7 @@ export default class Libraries {
   verificationStatuses = createVerificationStatusLibrary(this.queries);
   domains = createDomainLibrary(this.queries);
   quota = createQuotaLibrary(this.queries, this.cloudConnection, this.connectors);
+  ssoConnector = createSsoConnectorLibrary(this.queries);
 
   constructor(
     public readonly tenantId: string,

packages/integration-tests/src/tests/api/sso-connectors.test.ts

@@ -9,6 +9,9 @@ import {
   patchSsoConnectorById,
   patchSsoConnectorConfigById,
 } from '#src/api/sso-connector.js';
+import { logtoUrl } from '#src/constants.js';
+
+const logtoIssuer = `${logtoUrl}/oidc`;
 
 describe('sso-connector library', () => {
   it('should return sso-connector-factories', async () => {
@@ -214,7 +217,8 @@ describe('patch sso-connector by id', () => {
     await expect(
       patchSsoConnectorById(id, {
         config: {
-          issuer: 23,
+          clientId: 'foo',
+          issuer: logtoIssuer,
         },
       })
     ).rejects.toThrow(HTTPError);
@@ -230,7 +234,9 @@ describe('patch sso-connector by id', () => {
       connectorName: 'integration_test connector updated',
       config: {
         clientId: 'foo',
-        issuer: 'https://test.com',
+        clientSecret: 'bar',
+        issuer: logtoIssuer,
+        scope: 'profile email',
       },
       syncProfile: true,
     });
@@ -240,7 +246,9 @@ describe('patch sso-connector by id', () => {
     expect(connector).toHaveProperty('connectorName', 'integration_test connector updated');
     expect(connector).toHaveProperty('config', {
       clientId: 'foo',
-      issuer: 'https://test.com',
+      clientSecret: 'bar',
+      issuer: logtoIssuer,
+      scope: 'profile email openid', // Should merged with default scope openid
     });
     expect(connector).toHaveProperty('syncProfile', true);
   });
@@ -255,11 +263,14 @@ describe('patch sso-connector config by id', () => {
     const { id } = await createSsoConnector({
       providerName: 'OIDC',
       connectorName: 'integration_test connector',
+      config: {
+        clientSecret: 'bar',
+      },
     });
 
     await expect(
       patchSsoConnectorConfigById(id, {
-        issuer: 23,
+        clientId: 'foo',
       })
     ).rejects.toThrow(HTTPError);
   });
@@ -268,11 +279,14 @@ describe('patch sso-connector config by id', () => {
     const { id } = await createSsoConnector({
       providerName: 'OIDC',
       connectorName: 'integration_test connector',
+      config: {
+        clientId: 'foo',
+      },
     });
 
     const connector = await patchSsoConnectorConfigById(id, {
-      clientId: 'foo',
-      issuer: 'https://test.com',
+      clientSecret: 'bar',
+      issuer: logtoIssuer,
     });
 
     expect(connector).toHaveProperty('id', id);
@@ -280,7 +294,9 @@ describe('patch sso-connector config by id', () => {
     expect(connector).toHaveProperty('connectorName', 'integration_test connector');
     expect(connector).toHaveProperty('config', {
       clientId: 'foo',
-      issuer: 'https://test.com',
+      clientSecret: 'bar',
+      issuer: logtoIssuer,
+      scope: 'openid',
     });
   });
 });