Merge pull request #6310 from logto-io/gao-generate-secret-on-creation

refactor: generate application secret on creation

Author: gao-sun

packages/console/src/pages/ApplicationDetails/ApplicationDetailsContent/EndpointsAndCredentials.tsx

@@ -1,10 +1,10 @@
 import {
   type ApplicationSecret,
-  ApplicationType,
   DomainStatus,
   type Application,
   type SnakeCaseOidcConfig,
   internalPrefix,
+  hasSecrets,
 } from '@logto/schemas';
 import { appendPath } from '@silverhand/essentials';
 import { useCallback, useContext, useMemo, useState } from 'react';
@@ -57,11 +57,7 @@ function EndpointsAndCredentials({
   const [showCreateSecretModal, setShowCreateSecretModal] = useState(false);
   const secrets = useSWR<ApplicationSecretRow[], RequestError>(`api/applications/${id}/secrets`);
   const api = useApi();
-  const shouldShowAppSecrets = [
-    ApplicationType.Traditional,
-    ApplicationType.MachineToMachine,
-    ApplicationType.Protected,
-  ].includes(type);
+  const shouldShowAppSecrets = hasSecrets(type);
 
   const toggleShowMoreEndpoints = useCallback(() => {
     setShowMoreEndpoints((previous) => !previous);

packages/core/src/__mocks__/index.ts

@@ -17,6 +17,7 @@ import {
   LogtoOidcConfigKey,
   DomainStatus,
   LogtoJwtTokenKey,
+  internalPrefix,
 } from '@logto/schemas';
 
 import { protectedAppSignInCallbackUrl } from '#src/constants/index.js';
@@ -33,7 +34,7 @@ export * from './protected-app.js';
 export const mockApplication: Application = {
   tenantId: 'fake_tenant',
   id: 'foo',
-  secret: mockId,
+  secret: internalPrefix + mockId,
   name: 'foo',
   type: ApplicationType.SPA,
   description: null,

packages/core/src/routes/applications/application.test.ts

@@ -53,6 +53,7 @@ const tenantContext = new MockTenant(
       ),
       updateApplicationById,
     },
+    applicationSecrets: { insert: jest.fn() },
   },
   undefined,
   {

packages/core/src/routes/applications/application.ts

@@ -5,6 +5,7 @@ import {
   InternalRole,
   ApplicationType,
   Applications,
+  hasSecrets,
 } from '@logto/schemas';
 import { generateStandardId, generateStandardSecret } from '@logto/shared';
 import { conditional } from '@silverhand/essentials';
@@ -40,28 +41,12 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
   ...[
     router,
     {
-      queries: { applications, applicationsRoles, roles },
+      queries,
       id: tenantId,
       libraries: { quota, protectedApps },
     },
   ]: RouterInitArgs<T>
 ) {
-  const {
-    deleteApplicationById,
-    findApplicationById,
-    insertApplication,
-    updateApplicationById,
-    countApplications,
-    findApplications,
-  } = applications;
-
-  const {
-    findApplicationsRolesByApplicationId,
-    insertApplicationsRoles,
-    deleteApplicationRole,
-    findApplicationsRolesByRoleId,
-  } = applicationsRoles;
-
   router.get(
     '/applications',
     koaPagination({ isOptional: true }),
@@ -107,15 +92,15 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
       const search = parseSearchParamsForSearch(searchParams);
 
       const excludeApplicationsRoles = excludeRoleId
-        ? await findApplicationsRolesByRoleId(excludeRoleId)
+        ? await queries.applicationsRoles.findApplicationsRolesByRoleId(excludeRoleId)
         : [];
 
       const excludeApplicationIds = excludeApplicationsRoles.map(
         ({ applicationId }) => applicationId
       );
 
       if (paginationDisabled) {
-        ctx.body = await findApplications({
+        ctx.body = await queries.applications.findApplications({
           search,
           excludeApplicationIds,
           excludeOrganizationId,
@@ -127,14 +112,14 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
       }
 
       const [{ count }, applications] = await Promise.all([
-        countApplications({
+        queries.applications.countApplications({
           search,
           excludeApplicationIds,
           excludeOrganizationId,
           isThirdParty,
           types,
         }),
-        findApplications(
+        queries.applications.findApplications(
           {
             search,
             excludeApplicationIds,
@@ -164,36 +149,29 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
     async (ctx, next) => {
       const { oidcClientMetadata, protectedAppMetadata, ...rest } = ctx.guard.body;
 
-      // When creating a m2m app, should check both m2m limit and application limit.
-      if (rest.type === ApplicationType.MachineToMachine) {
-        await quota.guardKey('machineToMachineLimit');
-      }
-
-      // Guard third party application limit
-      if (rest.isThirdParty) {
-        await quota.guardKey('thirdPartyApplicationsLimit');
-      }
-
-      await quota.guardKey('applicationsLimit');
+      await Promise.all([
+        rest.type === ApplicationType.MachineToMachine && quota.guardKey('machineToMachineLimit'),
+        rest.isThirdParty && quota.guardKey('thirdPartyApplicationsLimit'),
+        quota.guardKey('applicationsLimit'),
+      ]);
 
       assertThat(
         rest.type !== ApplicationType.Protected || protectedAppMetadata,
         'application.protected_app_metadata_is_required'
       );
 
-      // Third party applications must be traditional type
       if (rest.isThirdParty) {
         assertThat(
           rest.type === ApplicationType.Traditional,
           'application.invalid_third_party_application_type'
         );
       }
 
-      const application = await insertApplication({
+      const getSecret = () =>
+        EnvSet.values.isDevFeaturesEnabled ? generateInternalSecret() : generateStandardSecret();
+      const application = await queries.applications.insertApplication({
         id: generateStandardId(),
-        secret: EnvSet.values.isDevFeaturesEnabled
-          ? generateStandardSecret()
-          : generateInternalSecret(),
+        secret: getSecret(),
         oidcClientMetadata: buildOidcClientMetadata(oidcClientMetadata),
         ...conditional(
           rest.type === ApplicationType.Protected &&
@@ -203,18 +181,25 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
         ...rest,
       });
 
+      if (EnvSet.values.isDevFeaturesEnabled && hasSecrets(application.type)) {
+        await queries.applicationSecrets.insert({
+          name: 'Default secret',
+          applicationId: application.id,
+          value: generateStandardSecret(),
+        });
+      }
+
       if (application.type === ApplicationType.Protected) {
         try {
           await protectedApps.syncAppConfigsToRemote(application.id);
         } catch (error: unknown) {
           // Delete the application if failed to sync to remote
-          await deleteApplicationById(application.id);
+          await queries.applications.deleteApplicationById(application.id);
           throw error;
         }
       }
 
       ctx.body = application;
-
       return next();
     }
   );
@@ -238,8 +223,9 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
         return next();
       }
 
-      const application = await findApplicationById(id);
-      const applicationsRoles = await findApplicationsRolesByApplicationId(id);
+      const application = await queries.applications.findApplicationById(id);
+      const applicationsRoles =
+        await queries.applicationsRoles.findApplicationsRolesByApplicationId(id);
 
       ctx.body = {
         ...application,
@@ -276,8 +262,8 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
       // This role is NOT intended for user assignment.
       if (isAdmin !== undefined) {
         const [applicationsRoles, internalAdminRole] = await Promise.all([
-          findApplicationsRolesByApplicationId(id),
-          roles.findRoleByRoleName(InternalRole.Admin),
+          queries.applicationsRoles.findApplicationsRolesByApplicationId(id),
+          queries.roles.findRoleByRoleName(InternalRole.Admin),
         ]);
         const usedToBeAdmin = includesInternalAdminRole(applicationsRoles);
 
@@ -291,17 +277,17 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
         );
 
         if (isAdmin && !usedToBeAdmin) {
-          await insertApplicationsRoles([
+          await queries.applicationsRoles.insertApplicationsRoles([
             { id: generateStandardId(), applicationId: id, roleId: internalAdminRole.id },
           ]);
         } else if (!isAdmin && usedToBeAdmin) {
-          await deleteApplicationRole(id, internalAdminRole.id);
+          await queries.applicationsRoles.deleteApplicationRole(id, internalAdminRole.id);
         }
       }
 
       if (protectedAppMetadata) {
         const { type, protectedAppMetadata: originProtectedAppMetadata } =
-          await findApplicationById(id);
+          await queries.applications.findApplicationById(id);
         assertThat(type === ApplicationType.Protected, 'application.protected_application_only');
         assertThat(
           originProtectedAppMetadata,
@@ -310,7 +296,7 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
             status: 422,
           })
         );
-        await updateApplicationById(id, {
+        await queries.applications.updateApplicationById(id, {
           protectedAppMetadata: {
             ...originProtectedAppMetadata,
             ...protectedAppMetadata,
@@ -320,16 +306,16 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
           await protectedApps.syncAppConfigsToRemote(id);
         } catch (error: unknown) {
           // Revert changes on sync failure
-          await updateApplicationById(id, {
+          await queries.applications.updateApplicationById(id, {
             protectedAppMetadata: originProtectedAppMetadata,
           });
           throw error;
         }
       }
 
       ctx.body = await (Object.keys(rest).length > 0
-        ? updateApplicationById(id, rest)
-        : findApplicationById(id));
+        ? queries.applications.updateApplicationById(id, rest)
+        : queries.applications.findApplicationById(id));
 
       return next();
     }
@@ -344,7 +330,7 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
     }),
     async (ctx, next) => {
       const { id } = ctx.guard.params;
-      const { type, protectedAppMetadata } = await findApplicationById(id);
+      const { type, protectedAppMetadata } = await queries.applications.findApplicationById(id);
       if (type === ApplicationType.Protected && protectedAppMetadata) {
         assertThat(
           !protectedAppMetadata.customDomains || protectedAppMetadata.customDomains.length === 0,
@@ -354,7 +340,7 @@ export default function applicationRoutes<T extends ManagementApiRouter>(
         await protectedApps.deleteRemoteAppConfigs(protectedAppMetadata.host);
       }
       // Note: will need delete cascade when application is joint with other tables
-      await deleteApplicationById(id);
+      await queries.applications.deleteApplicationById(id);
       ctx.status = 204;
 
       return next();

packages/integration-tests/src/tests/api/application/application.secrets.test.ts

@@ -1,4 +1,4 @@
-import { ApplicationType, type Application } from '@logto/schemas';
+import { ApplicationType, hasSecrets, internalPrefix, type Application } from '@logto/schemas';
 import { cond, noop } from '@silverhand/essentials';
 import { HTTPError } from 'ky';
 
@@ -11,6 +11,8 @@ import {
 } from '#src/api/application.js';
 import { devFeatureTest, randomString } from '#src/utils.js';
 
+const defaultSecretName = 'Default secret';
+
 devFeatureTest.describe('application secrets', () => {
   const applications: Application[] = [];
   const createApplication = async (...args: Parameters<typeof createApplicationApi>) => {
@@ -39,19 +41,29 @@ devFeatureTest.describe('application secrets', () => {
           }
         )
       );
+      expect(application.secret).toMatch(new RegExp(`^${internalPrefix}`));
+
+      // Check the default secret
+      const secrets = await getApplicationSecrets(application.id);
+      if (hasSecrets(type)) {
+        expect(secrets).toHaveLength(1);
+        expect(secrets[0]).toEqual(
+          expect.objectContaining({
+            applicationId: application.id,
+            name: defaultSecretName,
+          })
+        );
+      } else {
+        expect(secrets).toHaveLength(0);
+      }
+
       const secretName = randomString();
       const secretPromise = createApplicationSecret({
         applicationId: application.id,
         name: secretName,
       });
 
-      if (
-        [
-          ApplicationType.MachineToMachine,
-          ApplicationType.Protected,
-          ApplicationType.Traditional,
-        ].includes(type)
-      ) {
+      if (hasSecrets(type)) {
         expect(await secretPromise).toEqual(
           expect.objectContaining({ applicationId: application.id, name: secretName })
         );
@@ -137,8 +149,12 @@ devFeatureTest.describe('application secrets', () => {
     expect(await getApplicationSecrets(application.id)).toEqual(
       expect.arrayContaining([secret1, secret2])
     );
-    await deleteApplicationSecret(application.id, secretName1);
-    await deleteApplicationSecret(application.id, secretName2);
+
+    await Promise.all([
+      deleteApplicationSecret(application.id, secretName1),
+      deleteApplicationSecret(application.id, secretName2),
+      deleteApplicationSecret(application.id, defaultSecretName),
+    ]);
     expect(await getApplicationSecrets(application.id)).toEqual([]);
   });
 });

packages/schemas/src/utils/application.ts

@@ -0,0 +1,9 @@
+import { ApplicationType } from '../db-entries/custom-types.js';
+
+/** If the application type has (or can have) secrets. */
+export const hasSecrets = (type: ApplicationType) =>
+  [
+    ApplicationType.MachineToMachine,
+    ApplicationType.Protected,
+    ApplicationType.Traditional,
+  ].includes(type);

packages/schemas/src/utils/index.ts

@@ -1,3 +1,4 @@
+export * from './application.js';
 export * from './role.js';
 export * from './management-api.js';
 export * from './domain.js';