Merge pull request #6978 from logto-io/yemq-fix-saml-app-attributes

fix: fix SAML app attribute mapping not working issue

packages/console/src/pages/ApplicationDetails/SamlApplicationDetailsContent/AttributeMapping.tsx

@@ -10,7 +10,6 @@ import CirclePlus from '@/assets/icons/circle-plus.svg?react';
 import DetailsForm from '@/components/DetailsForm';
 import FormCard from '@/components/FormCard';
 import Button from '@/ds-components/Button';
-import CopyToClipboard from '@/ds-components/CopyToClipboard';
 import DynamicT from '@/ds-components/DynamicT';
 import IconButton from '@/ds-components/IconButton';
 import Select from '@/ds-components/Select';
@@ -22,7 +21,7 @@ import { trySubmitSafe } from '@/utils/form';
 import styles from './AttributeMapping.module.scss';
 import { camelCaseToSentenceCase } from './utils';
 
-const defaultFormValue: Array<[UserClaim | 'id' | '', string]> = [['id', '']];
+const defaultFormValue: Array<[UserClaim | 'sub' | '', string]> = [['sub', '']];
 
 type Props = {
   readonly data: SamlApplicationResponse;
@@ -31,10 +30,10 @@ type Props = {
 
 /**
  * Type for the attribute mapping form data.
- * Array of tuples containing key (UserClaim or 'id' or empty string) and value pairs
+ * Array of tuples containing key (UserClaim or 'sub' or empty string) and value pairs
  */
 type FormData = {
-  attributeMapping: Array<[key: UserClaim | 'id' | '', value: string]>;
+  attributeMapping: Array<[key: UserClaim | 'sub' | '', value: string]>;
 };
 
 const keyPrefix = 'attributeMapping';
@@ -46,7 +45,7 @@ const getOrderedAttributeMapping = (
     samlAttributeMappingKeys
       .filter((key) => key in attributeMapping)
       // eslint-disable-next-line no-restricted-syntax
-      .map((key) => [key, attributeMapping[key]] as [UserClaim | 'id', string])
+      .map((key) => [key, attributeMapping[key]] as [UserClaim | 'sub', string])
   );
 };
 
@@ -140,51 +139,45 @@ function AttributeMapping({ data, mutateApplication }: Props) {
                 // eslint-disable-next-line react/no-array-index-key
                 <tr key={index} className={styles.row}>
                   <td>
-                    {key === 'id' ? (
-                      <CopyToClipboard displayType="block" variant="border" value={key} />
-                    ) : (
-                      <Controller
-                        control={control}
-                        name={`${keyPrefix}.${index}.0` as const}
-                        render={({ field: { onChange, value } }) => (
-                          <Select
-                            isSearchEnabled
-                            value={value}
-                            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`. But we should not show if the current value is empty.
-                              value.trim() && [{ title: camelCaseToSentenceCase(value), value }]
-                            )}
-                            onChange={(value) => {
-                              onChange(value);
-                            }}
-                          />
-                        )}
-                      />
-                    )}
+                    <Controller
+                      control={control}
+                      name={`${keyPrefix}.${index}.0` as const}
+                      render={({ field: { onChange, value } }) => (
+                        <Select
+                          isSearchEnabled
+                          value={value}
+                          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`. But we should not show if the current value is empty.
+                            value.trim() && [{ title: camelCaseToSentenceCase(value), value }]
+                          )}
+                          onChange={(value) => {
+                            onChange(value);
+                          }}
+                        />
+                      )}
+                    />
                   </td>
                   <td>
                     <TextInput {...register(`${keyPrefix}.${index}.1`)} />
                   </td>
                   <td>
-                    {key !== 'id' && (
-                      <IconButton
-                        onClick={() => {
-                          const currentValues = [
-                            ...formValues.slice(0, index),
-                            ...formValues.slice(index + 1),
-                          ];
-                          setValue('attributeMapping', currentValues, {
-                            shouldDirty: true,
-                          });
-                        }}
-                      >
-                        <CircleMinus />
-                      </IconButton>
-                    )}
+                    <IconButton
+                      onClick={() => {
+                        const currentValues = [
+                          ...formValues.slice(0, index),
+                          ...formValues.slice(index + 1),
+                        ];
+                        setValue('attributeMapping', currentValues, {
+                          shouldDirty: true,
+                        });
+                      }}
+                    >
+                      <CircleMinus />
+                    </IconButton>
                   </td>
                 </tr>
               );

packages/core/src/saml-application/SamlApplication/index.test.ts

@@ -245,7 +245,7 @@ describe('SamlApplication', () => {
         {
           ...mockDetails,
           attributeMapping: {
-            id: 'id',
+            sub: 'sub',
             name: 'name',
           },
         },
@@ -317,19 +317,19 @@ describe('SamlApplication', () => {
       expect(template.attributes).toEqual([
         {
           name: 'userId',
-          valueTag: 'attrUserId',
+          valueTag: 'userId',
           nameFormat: 'urn:oasis:names:tc:SAML:2.0:attrname-format:basic',
           valueXsiType: 'xs:string',
         },
         {
           name: 'emailAddress',
-          valueTag: 'attrEmailAddress',
+          valueTag: 'emailAddress',
           nameFormat: 'urn:oasis:names:tc:SAML:2.0:attrname-format:basic',
           valueXsiType: 'xs:string',
         },
         {
           name: 'displayName',
-          valueTag: 'attrDisplayName',
+          valueTag: 'displayName',
           nameFormat: 'urn:oasis:names:tc:SAML:2.0:attrname-format:basic',
           valueXsiType: 'xs:string',
         },
@@ -389,11 +389,11 @@ describe('SamlApplication', () => {
       const tagValues = samlApp.exposedBuildSamlAttributesTagValues(mockUser);
 
       expect(tagValues).toEqual({
+        attrAvatar: 'null',
         attrUserId: 'user123',
         attrEmailAddress: 'user@example.com',
         attrDisplayName: 'Test User',
       });
-      expect(tagValues).not.toHaveProperty('attrAvatar');
     });
   });
 });

packages/core/src/saml-application/SamlApplication/index.ts

@@ -354,8 +354,8 @@ export class SamlApplication {
     for (const claim of Object.keys(this.config.attributeMapping) as Array<
       keyof SamlAttributeMapping
     >) {
-      // Ignore `id` claim since this will always be included.
-      if (claim === 'id') {
+      // Ignore `sub` claim since this will always be included.
+      if (claim === 'sub') {
         continue;
       }
 
@@ -409,7 +409,6 @@ export class SamlApplication {
         SubjectConfirmationDataNotOnOrAfter: expireAt.toISOString(),
         NameIDFormat,
         NameID,
-        // TODO: should get the request ID from the input parameters, pending https://github.com/logto-io/logto/pull/6881.
         InResponseTo: samlRequestId ?? 'null',
         /**
          * User attributes for SAML response
@@ -440,7 +439,7 @@ export class SamlApplication {
       context: samlLogInResponseTemplate,
       attributes: Object.values(this.config.attributeMapping).map((value) => ({
         name: value,
-        valueTag: generateSamlAttributeTag(value),
+        valueTag: value,
         nameFormat: samlAttributeNameFormatBasic,
         valueXsiType: samlValueXmlnsXsi.string,
       })),
@@ -454,10 +453,15 @@ export class SamlApplication {
       Object.entries(this.config.attributeMapping)
         .map(([key, value]) => {
           // eslint-disable-next-line no-restricted-syntax
-          return [value, userInfo[key as keyof IdTokenProfileStandardClaims]] as [string, unknown];
+          return [value, userInfo[key as keyof IdTokenProfileStandardClaims] ?? null] as [
+            string,
+            unknown,
+          ];
         })
-        .filter(([_, value]) => Boolean(value))
-        .map(([key, value]) => [generateSamlAttributeTag(key), String(value)])
+        .map(([key, value]) => [
+          generateSamlAttributeTag(key),
+          typeof value === 'object' ? JSON.stringify(value) : String(value),
+        ])
     );
   };
 

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

@@ -128,7 +128,7 @@ describe('SAML application', () => {
         acsUrl: null,
         entityId: null,
         attributeMapping: {
-          id: 'sub',
+          sub: 'sub',
           preferred_username: 'username',
           email: 'email_address',
         },
@@ -138,7 +138,7 @@ describe('SAML application', () => {
       name: 'Update with minimal attribute mapping',
       config: {
         attributeMapping: {
-          id: 'sub',
+          sub: 'sub',
         },
       },
     },

packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts

@@ -2,16 +2,17 @@ import { type ToZodObject } from '@logto/connector-kit';
 import { completeUserClaims, type UserClaim } from '@logto/core-kit';
 import { z } from 'zod';
 
-export type SamlAttributeMapping = Partial<Record<UserClaim | 'id', string>>;
+export type SamlAttributeMapping = Partial<Record<UserClaim | 'sub', string>>;
 
-export const samlAttributeMappingKeys = Object.freeze(['id', ...completeUserClaims] satisfies Array<
-  keyof SamlAttributeMapping
->);
+export const samlAttributeMappingKeys = Object.freeze([
+  'sub',
+  ...completeUserClaims,
+] satisfies Array<keyof SamlAttributeMapping>);
 
 export const samlAttributeMappingGuard = z
   .object(
     Object.fromEntries(
-      samlAttributeMappingKeys.map((claim): [UserClaim | 'id', z.ZodString] => [claim, z.string()])
+      samlAttributeMappingKeys.map((claim): [UserClaim | 'sub', z.ZodString] => [claim, z.string()])
     )
   )
   .partial() satisfies z.ZodType<SamlAttributeMapping>;