Merge pull request #6977 from logto-io/yemq-fix-console-saml-app-attr…

…ibute-mapping fix(console,core): fix SAML app attribute mapping issues
logto-io · Jan 23, 2025 · 3bc701e · 3bc701e
1 parent ed55916
commit 3bc701e
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 16 deletions.
diff --git a/...s/console/src/pages/ApplicationDetails/SamlApplicationDetailsContent/AttributeMapping.tsx b/...s/console/src/pages/ApplicationDetails/SamlApplicationDetailsContent/AttributeMapping.tsx
@@ -1,6 +1,6 @@
 import { type UserClaim, completeUserClaims } from '@logto/core-kit';
 import { type SamlApplicationResponse, samlAttributeMappingKeys } from '@logto/schemas';
-import { useMemo } from 'react';
+import { conditionalArray } from '@silverhand/essentials';
 import { useForm, Controller } from 'react-hook-form';
 import { toast } from 'react-hot-toast';
 import { useTranslation } from 'react-i18next';
@@ -102,12 +102,9 @@ function AttributeMapping({ data, mutateApplication }: Props) {
     })
   );
 
-  const existingKeys = useMemo(() => formValues.map(([key]) => key).filter(Boolean), [formValues]);
-
-  const availableKeys = useMemo(
-    () => completeUserClaims.filter((claim) => !existingKeys.includes(claim)),
-    [existingKeys]
-  );
+  // Not using `useMemo` to avoid the reappearance of the available keys when the form values change.
+  const existingKeys = new Set(formValues.map(([key]) => key).filter(Boolean));
+  const availableKeys = completeUserClaims.filter((claim) => !existingKeys.has(claim));
 
   return (
     <DetailsForm
@@ -153,14 +150,14 @@ function AttributeMapping({ data, mutateApplication }: Props) {
                           <Select
                             isSearchEnabled
                             value={value}
-                            options={[
-                              ...availableKeys.map((claim) => ({
+                            options={conditionalArray(
+                              availableKeys.map((claim) => ({
                                 title: camelCaseToSentenceCase(claim),
                                 value: claim,
                               })),
-                              // If this is not specified, the component will fail to render the current value. The current value has been excluded in `availableKeys`.
-                              { value, title: camelCaseToSentenceCase(value) },
-                            ]}
+                              // If this is not specified, the component will fail to render the current value. The current value has been excluded in `availableKeys`. But we should not show if the current value is empty.
+                              value.trim() && [{ title: camelCaseToSentenceCase(value), value }]
+                            )}
                             onChange={(value) => {
                               onChange(value);
                             }}

diff --git a/packages/core/src/libraries/saml-application/saml-applications.ts b/packages/core/src/libraries/saml-application/saml-applications.ts
@@ -76,11 +76,11 @@ export const createSamlApplicationsLibrary = (queries: Queries) => {
     patchApplicationObject: PatchSamlApplication
   ): Promise<SamlApplicationResponse> => {
     const { name, description, customData, ...config } = patchApplicationObject;
-    const originalApplication = await findApplicationById(id);
     const applicationData = removeUndefinedKeys(
       pick(patchApplicationObject, 'name', 'description', 'customData')
     );
 
+    const originalApplication = await findApplicationById(id);
     assertThat(
       originalApplication.type === ApplicationType.SAML,
       new RequestError({
@@ -89,17 +89,20 @@ export const createSamlApplicationsLibrary = (queries: Queries) => {
       })
     );
 
+    // Can not put this in a single Promise.all with `findApplicationById()` we want to API to throw SAML app only error before throwing other errors.
+    const originalAppConfig = await findSamlApplicationConfigByApplicationId(id);
+
     const [updatedApplication, upToDateSamlConfig] = await Promise.all([
       Object.keys(applicationData).length > 0
         ? updateApplicationById(id, applicationData)
         : originalApplication,
       Object.keys(config).length > 0
         ? updateSamlApplicationConfig({
-            set: config,
+            set: { ...originalAppConfig, ...config },
             where: { applicationId: id },
-            jsonbMode: 'merge',
+            jsonbMode: 'replace',
           })
-        : findSamlApplicationConfigByApplicationId(id),
+        : originalAppConfig,
     ]);
 
     return ensembleSamlApplication({

diff --git a/packages/integration-tests/src/tests/api/application/saml-application.test.ts b/packages/integration-tests/src/tests/api/application/saml-application.test.ts
@@ -142,6 +142,12 @@ describe('SAML application', () => {
         },
       },
     },
+    {
+      name: 'Update with empty attribute mapping',
+      config: {
+        attributeMapping: {},
+      },
+    },
   ])('should update SAML application - %#', async ({ name, config }) => {
     const formattedName = name ? `updated-${name}` : undefined;
     const initConfig = {