fix(core,schemas): use database index to prevent custom domain conflict (#5342)

packages/core/src/middleware/koa-slonik-error-handler.test.ts

@@ -99,7 +99,7 @@ describe('koaSlonikErrorHandler middleware', () => {
     );
   });
 
-  it('UniqueIntegrityConstraintViolationError for protected application', async () => {
+  it('UniqueIntegrityConstraintViolationError for protected application host', async () => {
     const error = new UniqueIntegrityConstraintViolationError(
       new Error(' '),
       'applications__protected_app_metadata_host'
@@ -115,4 +115,21 @@ describe('koaSlonikErrorHandler middleware', () => {
       })
     );
   });
+
+  it('UniqueIntegrityConstraintViolationError for protected application custom domain', async () => {
+    const error = new UniqueIntegrityConstraintViolationError(
+      new Error(' '),
+      'applications__protected_app_metadata_custom_domain'
+    );
+    next.mockImplementationOnce(() => {
+      throw error;
+    });
+
+    await expect(koaSlonikErrorHandler()(ctx, next)).rejects.toMatchError(
+      new RequestError({
+        code: 'domain.hostname_already_exists',
+        status: 422,
+      })
+    );
+  });
 });

packages/core/src/middleware/koa-slonik-error-handler.ts

@@ -11,6 +11,7 @@ import {
 import RequestError from '#src/errors/RequestError/index.js';
 import { DeletionError, InsertionError, UpdateError } from '#src/errors/SlonikError/index.js';
 
+/* eslint-disable complexity */
 export default function koaSlonikErrorHandler<StateT, ContextT>(): Middleware<StateT, ContextT> {
   return async (ctx, next) => {
     try {
@@ -27,14 +28,20 @@ export default function koaSlonikErrorHandler<StateT, ContextT>(): Middleware<St
         });
       }
 
-      if (
-        error instanceof UniqueIntegrityConstraintViolationError &&
-        error.constraint === 'applications__protected_app_metadata_host'
-      ) {
-        throw new RequestError({
-          code: 'application.protected_application_subdomain_exists',
-          status: 422,
-        });
+      if (error instanceof UniqueIntegrityConstraintViolationError) {
+        if (error.constraint === 'applications__protected_app_metadata_host') {
+          throw new RequestError({
+            code: 'application.protected_application_subdomain_exists',
+            status: 422,
+          });
+        }
+
+        if (error.constraint === 'applications__protected_app_metadata_custom_domain') {
+          throw new RequestError({
+            code: 'domain.hostname_already_exists',
+            status: 422,
+          });
+        }
       }
 
       if (error instanceof CheckIntegrityConstraintViolationError) {
@@ -76,3 +83,4 @@ export default function koaSlonikErrorHandler<StateT, ContextT>(): Middleware<St
     }
   };
 }
+/* eslint-enable complexity */

packages/core/src/queries/application.test.ts

@@ -1,4 +1,4 @@
-import { ApplicationType, Applications } from '@logto/schemas';
+import { Applications } from '@logto/schemas';
 import { convertToIdentifiers, convertToPrimitiveOrSql, excludeAutoSetFields } from '@logto/shared';
 import { createMockPool, createMockQueryResult, sql } from 'slonik';
 import { snakeCase } from 'snake-case';
@@ -22,7 +22,6 @@ const { createApplicationQueries } = await import('./application.js');
 const {
   findTotalNumberOfApplications,
   findApplicationById,
-  findApplicationByProtectedAppCustomDomain,
   insertApplication,
   updateApplicationById,
   deleteApplicationById,
@@ -67,28 +66,6 @@ describe('application query', () => {
     await findApplicationById(id);
   });
 
-  it('findApplicationByProtectedAppCustomDomain', async () => {
-    const domain = 'my.blog.com';
-    const rowData = { domain };
-
-    const expectSql = sql`
-      select ${sql.join(Object.values(fields), sql`, `)}
-      from ${table}
-      where ${fields.protectedAppMetadata} ? 'customDomains' 
-      and ${fields.protectedAppMetadata}->'customDomains' @> $1::jsonb
-      and ${fields.type} = $2
-    `;
-
-    mockQuery.mockImplementationOnce(async (sql, values) => {
-      expectSqlAssert(sql, expectSql.sql);
-      expect(values).toEqual([JSON.stringify([domain]), ApplicationType.Protected]);
-
-      return createMockQueryResult([rowData]);
-    });
-
-    await findApplicationByProtectedAppCustomDomain(domain);
-  });
-
   it('insertApplication', async () => {
     const keys = excludeAutoSetFields(Applications.fieldKeys);
 

packages/core/src/queries/application.ts

@@ -130,19 +130,6 @@ export const createApplicationQueries = (pool: CommonQueryMethods) => {
 
   const findApplicationById = buildFindEntityByIdWithPool(pool)(Applications);
 
-  /**
-   * Find an protected application by its custom domain.
-   * the domain is stored in the `customDomains` field of the `protectedAppMetadata` field.
-   */
-  const findApplicationByProtectedAppCustomDomain = async (domain: string) =>
-    pool.maybeOne<Application>(sql`
-      select ${sql.join(Object.values(fields), sql`, `)}
-      from ${table}
-      where ${fields.protectedAppMetadata} ? 'customDomains'
-      and ${fields.protectedAppMetadata}->'customDomains' @> ${sql.jsonb([domain])}
-      and ${fields.type} = ${ApplicationType.Protected}
-    `);
-
   const insertApplication = buildInsertIntoWithPool(pool)(Applications, {
     returning: true,
   });
@@ -244,7 +231,6 @@ export const createApplicationQueries = (pool: CommonQueryMethods) => {
     findApplications,
     findTotalNumberOfApplications,
     findApplicationById,
-    findApplicationByProtectedAppCustomDomain,
     insertApplication,
     updateApplication,
     updateApplicationById,

packages/core/src/routes/applications/application-protected-app-metadata.test.ts

@@ -1,6 +1,5 @@
-import { type Application, DomainStatus } from '@logto/schemas';
+import { DomainStatus } from '@logto/schemas';
 import { pickDefault } from '@logto/shared/esm';
-import { type Nullable } from '@silverhand/essentials';
 
 import { mockCloudflareData, mockProtectedApplication } from '#src/__mocks__/index.js';
 import { mockIdGenerators } from '#src/test-utils/nanoid.js';
@@ -13,9 +12,6 @@ const protectedAppSignInCallbackUrl = 'sign-in-callback';
 
 const updateApplicationById = jest.fn();
 const findApplicationById = jest.fn(async () => mockProtectedApplication);
-const findApplicationByProtectedAppCustomDomain = jest.fn(
-  async (): Promise<Nullable<Application>> => null
-);
 
 const mockDomainResponse = {
   domain: mockDomain,
@@ -50,7 +46,6 @@ const tenantContext = new MockTenant(
     applications: {
       findApplicationById,
       updateApplicationById,
-      findApplicationByProtectedAppCustomDomain,
     },
   },
   undefined,
@@ -133,16 +128,6 @@ describe('application protected app metadata routes', () => {
         });
       expect(response.status).toEqual(400);
     });
-
-    it('throw when the domain is already in use', async () => {
-      findApplicationByProtectedAppCustomDomain.mockResolvedValueOnce(mockProtectedApplication);
-      const response = await requester
-        .post(`/applications/asdf/protected-app-metadata/custom-domains`)
-        .send({
-          domain: mockDomain,
-        });
-      expect(response.status).toEqual(422);
-    });
   });
 
   describe('DELETE /applications/:applicationId/protected-app-metadata/custom-domains/:domain', () => {

packages/core/src/routes/applications/application-protected-app-metadata.ts

@@ -13,11 +13,7 @@ export default function applicationProtectedAppMetadataRoutes<T extends AuthedRo
     router,
     {
       queries: {
-        applications: {
-          findApplicationById,
-          updateApplicationById,
-          findApplicationByProtectedAppCustomDomain,
-        },
+        applications: { findApplicationById, updateApplicationById },
       },
       libraries: {
         applications: { validateProtectedApplicationById },
@@ -78,19 +74,13 @@ export default function applicationProtectedAppMetadataRoutes<T extends AuthedRo
       const { protectedAppMetadata, oidcClientMetadata } = await findApplicationById(id);
       assertThat(protectedAppMetadata, 'application.protected_app_not_configured');
 
+      // Only allow one domain, be careful when changing this, the unique index on the database
+      // is based on this assumption
       assertThat(
         !protectedAppMetadata.customDomains || protectedAppMetadata.customDomains.length === 0,
         'domain.limit_to_one_domain'
       );
 
-      assertThat(
-        !(await findApplicationByProtectedAppCustomDomain(domain)),
-        new RequestError({
-          code: 'domain.hostname_already_exists',
-          status: 422,
-        })
-      );
-
       const customDomain = await addDomainToRemote(domain);
       await updateApplicationById(id, {
         protectedAppMetadata: { ...protectedAppMetadata, customDomains: [customDomain] },

packages/schemas/alterations/next-1706585206-protected-app-custom-domain-unique.ts

@@ -0,0 +1,21 @@
+import { sql } from 'slonik';
+
+import type { AlterationScript } from '../lib/types/alteration.js';
+
+const alteration: AlterationScript = {
+  up: async (pool) => {
+    await pool.query(sql`
+      create unique index applications__protected_app_metadata_custom_domain
+      on applications (
+        (protected_app_metadata->'customDomains'->0->>'domain')
+      );
+    `);
+  },
+  down: async (pool) => {
+    await pool.query(sql`
+      drop index applications__protected_app_metadata_custom_domain;
+    `);
+  },
+};
+
+export default alteration;

packages/schemas/tables/applications.sql

@@ -28,3 +28,8 @@ create unique index applications__protected_app_metadata_host
   on applications (
     (protected_app_metadata->>'host')
   );
+
+create unique index applications__protected_app_metadata_custom_domain
+  on applications (
+    (protected_app_metadata->'customDomains'->0->>'domain')
+  );