From 3a4298c1794ad61c4fa3d3d186aa4235b8742424 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 19 Jun 2025 11:42:44 +0200 Subject: [PATCH] fix(scim): add type attribute to ScimEmail (#9690) # Which Problems Are Solved - SCIM PATCH operations for users from Entra ID for the `emails` attribute fails due to missing `type` subattribute # How the Problems Are Solved - Adds the `type` attribute to the `ScimUser` struct and sets the default value to `"work"` in the `mapWriteModelToScimUser()` method. # Additional Changes # Additional Context The SCIM handlers for POST and PUT ignore multiple emails and only uses the primary email for a given user, or falls back to the first email if none are marked as primary. PATCH operations however, will attempt to resolve the provided filter in `operations[].path`. Some services, such as Entra ID, only support patching emails by filtering for `emails[type eq "(work|home|other)"].value`, which fails with Zitadel as the ScimUser struct (and thus the generated schema) doesn't include the `type` field. This commit adds the `type` field to work around this issue, while still preserving compatibility with filters such as `emails[primary eq true].value`. - https://discord.com/channels/927474939156643850/927866013545025566/1356556668527448191 --------- Co-authored-by: Christer Edvartsen Co-authored-by: Thomas Siegfried Krampl --- ...vice_provider_config_expected_schemas.json | 11 +++++ ..._provider_config_expected_user_schema.json | 11 +++++ ..._replace_test_minimal_with_email_type.json | 17 ++++++++ .../integration_test/users_create_test.go | 1 + .../scim/integration_test/users_get_test.go | 1 + .../integration_test/users_replace_test.go | 40 ++++++++++++++++++- .../integration_test/users_update_test.go | 9 +++++ internal/api/scim/metadata/metadata.go | 3 ++ internal/api/scim/resources/user.go | 1 + internal/api/scim/resources/user_mapping.go | 4 ++ internal/api/scim/resources/user_metadata.go | 5 ++- .../api/scim/resources/user_patch_test.go | 34 ++++++++++++++++ 12 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 internal/api/scim/integration_test/testdata/users_replace_test_minimal_with_email_type.json diff --git a/internal/api/scim/integration_test/testdata/service_provider_config_expected_schemas.json b/internal/api/scim/integration_test/testdata/service_provider_config_expected_schemas.json index bc87b8e2e1..2751c85a79 100644 --- a/internal/api/scim/integration_test/testdata/service_provider_config_expected_schemas.json +++ b/internal/api/scim/integration_test/testdata/service_provider_config_expected_schemas.json @@ -233,6 +233,17 @@ "mutability": "readWrite", "returned": "always", "uniqueness": "none" + }, + { + "name": "type", + "description": "For details see RFC7643", + "type": "string", + "multiValued": false, + "required": false, + "caseExact": true, + "mutability": "readWrite", + "returned": "always", + "uniqueness": "none" } ], "multiValued": true, diff --git a/internal/api/scim/integration_test/testdata/service_provider_config_expected_user_schema.json b/internal/api/scim/integration_test/testdata/service_provider_config_expected_user_schema.json index a199fe1465..35d0e356b3 100644 --- a/internal/api/scim/integration_test/testdata/service_provider_config_expected_user_schema.json +++ b/internal/api/scim/integration_test/testdata/service_provider_config_expected_user_schema.json @@ -225,6 +225,17 @@ "mutability": "readWrite", "returned": "always", "uniqueness": "none" + }, + { + "name": "type", + "description": "For details see RFC7643", + "type": "string", + "multiValued": false, + "required": false, + "caseExact": true, + "mutability": "readWrite", + "returned": "always", + "uniqueness": "none" } ], "multiValued": true, diff --git a/internal/api/scim/integration_test/testdata/users_replace_test_minimal_with_email_type.json b/internal/api/scim/integration_test/testdata/users_replace_test_minimal_with_email_type.json new file mode 100644 index 0000000000..b7e8d87590 --- /dev/null +++ b/internal/api/scim/integration_test/testdata/users_replace_test_minimal_with_email_type.json @@ -0,0 +1,17 @@ +{ + "schemas": [ + "urn:ietf:params:scim:schemas:core:2.0:User" + ], + "userName": "acmeUser1-minimal-replaced", + "name": { + "familyName": "Ross-replaced", + "givenName": "Bethany-replaced" + }, + "emails": [ + { + "value": "user1-minimal-replaced@example.com", + "primary": true, + "type": "work" + } + ] +} \ No newline at end of file diff --git a/internal/api/scim/integration_test/users_create_test.go b/internal/api/scim/integration_test/users_create_test.go index 8b6986666c..35d5297878 100644 --- a/internal/api/scim/integration_test/users_create_test.go +++ b/internal/api/scim/integration_test/users_create_test.go @@ -391,6 +391,7 @@ func TestCreateUser_metadata(t *testing.T) { test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:locale", "en-US") test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:ims", `[{"value":"someaimhandle","type":"aim"},{"value":"twitterhandle","type":"X"}]`) test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:roles", `[{"value":"my-role-1","display":"Rolle 1","type":"main-role","primary":true},{"value":"my-role-2","display":"Rolle 2","type":"secondary-role"}]`) + test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:emails", `[{"value":"bjensen@example.com","primary":true,"type":"work"},{"value":"babs@jensen.org","primary":false,"type":"home"}]`) }, retryDuration, tick) } diff --git a/internal/api/scim/integration_test/users_get_test.go b/internal/api/scim/integration_test/users_get_test.go index 0106e47ca2..8a1bab6c93 100644 --- a/internal/api/scim/integration_test/users_get_test.go +++ b/internal/api/scim/integration_test/users_get_test.go @@ -115,6 +115,7 @@ func TestGetUser(t *testing.T) { { Value: "bjensen@example.com", Primary: true, + Type: "work", }, }, PhoneNumbers: []*resources.ScimPhoneNumber{ diff --git a/internal/api/scim/integration_test/users_replace_test.go b/internal/api/scim/integration_test/users_replace_test.go index 770ed06959..1c99592b01 100644 --- a/internal/api/scim/integration_test/users_replace_test.go +++ b/internal/api/scim/integration_test/users_replace_test.go @@ -27,6 +27,9 @@ var ( //go:embed testdata/users_replace_test_minimal_with_external_id.json minimalUserWithExternalIDJson []byte + //go:embed testdata/users_replace_test_minimal_with_email_type.json + minimalUserWithEmailTypeReplaceJson []byte + //go:embed testdata/users_replace_test_minimal.json minimalUserReplaceJson []byte @@ -303,7 +306,42 @@ func TestReplaceUser_removeOldMetadata(t *testing.T) { Id: createdUser.ID, }) require.NoError(tt, err) - require.Equal(tt, 0, len(md.Result)) + require.Equal(tt, 1, len(md.Result)) + + mdMap := make(map[string]string) + for i := range md.Result { + mdMap[md.Result[i].Key] = string(md.Result[i].Value) + } + + test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:emails", "[{\"value\":\"user1@example.com\",\"primary\":true}]") + }, retryDuration, tick) + + _, err = Instance.Client.UserV2.DeleteUser(CTX, &user.DeleteUserRequest{UserId: createdUser.ID}) + require.NoError(t, err) +} + +func TestReplaceUser_emailType(t *testing.T) { + // ensure old metadata is removed correctly + createdUser, err := Instance.Client.SCIM.Users.Create(CTX, Instance.DefaultOrg.Id, fullUserJson) + require.NoError(t, err) + + _, err = Instance.Client.SCIM.Users.Replace(CTX, Instance.DefaultOrg.Id, createdUser.ID, minimalUserWithEmailTypeReplaceJson) + require.NoError(t, err) + + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(CTX, time.Minute) + require.EventuallyWithT(t, func(tt *assert.CollectT) { + md, err := Instance.Client.Mgmt.ListUserMetadata(CTX, &management.ListUserMetadataRequest{ + Id: createdUser.ID, + }) + require.NoError(tt, err) + require.Equal(tt, 1, len(md.Result)) + + mdMap := make(map[string]string) + for i := range md.Result { + mdMap[md.Result[i].Key] = string(md.Result[i].Value) + } + + test.AssertMapContains(tt, mdMap, "urn:zitadel:scim:emails", "[{\"value\":\"user1-minimal-replaced@example.com\",\"primary\":true,\"type\":\"work\"}]") }, retryDuration, tick) _, err = Instance.Client.UserV2.DeleteUser(CTX, &user.DeleteUserRequest{UserId: createdUser.ID}) diff --git a/internal/api/scim/integration_test/users_update_test.go b/internal/api/scim/integration_test/users_update_test.go index 2fe129291a..77e55bac60 100644 --- a/internal/api/scim/integration_test/users_update_test.go +++ b/internal/api/scim/integration_test/users_update_test.go @@ -137,9 +137,18 @@ func TestUpdateUser(t *testing.T) { NickName: "", ProfileUrl: test.Must(schemas.ParseHTTPURL("http://login.example.com/bjensen")), Emails: []*resources.ScimEmail{ + { + Value: "bjensen@example.com", + Type: "work", + }, + { + Value: "babs@jensen.org", + Type: "home", + }, { Value: "babs@example.com", Primary: true, + Type: "home", }, }, Addresses: []*resources.ScimAddress{ diff --git a/internal/api/scim/metadata/metadata.go b/internal/api/scim/metadata/metadata.go index 66a0a2483c..28e42290d1 100644 --- a/internal/api/scim/metadata/metadata.go +++ b/internal/api/scim/metadata/metadata.go @@ -30,6 +30,7 @@ const ( KeyAddresses Key = KeyPrefix + "addresses" KeyEntitlements Key = KeyPrefix + "entitlements" KeyRoles Key = KeyPrefix + "roles" + KeyEmails Key = KeyPrefix + "emails" ) var ( @@ -47,6 +48,7 @@ var ( KeyAddresses, KeyEntitlements, KeyRoles, + KeyEmails, } AttributePathToMetadataKeys = map[string][]Key{ @@ -64,6 +66,7 @@ var ( "addresses": {KeyAddresses}, "entitlements": {KeyEntitlements}, "roles": {KeyRoles}, + "emails": {KeyEmails}, } ) diff --git a/internal/api/scim/resources/user.go b/internal/api/scim/resources/user.go index 13baed5d51..6506ae35c7 100644 --- a/internal/api/scim/resources/user.go +++ b/internal/api/scim/resources/user.go @@ -90,6 +90,7 @@ type ScimIms struct { type ScimEmail struct { Value string `json:"value" scim:"required"` Primary bool `json:"primary"` + Type string `json:"type,omitempty"` } type ScimPhoneNumber struct { diff --git a/internal/api/scim/resources/user_mapping.go b/internal/api/scim/resources/user_mapping.go index 171af87238..260e50846a 100644 --- a/internal/api/scim/resources/user_mapping.go +++ b/internal/api/scim/resources/user_mapping.go @@ -382,6 +382,10 @@ func (h *UsersHandler) mapAndValidateMetadata(ctx context.Context, user *ScimUse if err := extractJsonMetadata(ctx, md, metadata.KeyRoles, &user.Roles); err != nil { logging.OnError(err).Warn("Could not deserialize scim roles metadata") } + + if err := extractJsonMetadata(ctx, md, metadata.KeyEmails, &user.Emails); err != nil { + logging.OnError(err).Warn("Could not deserialize scim emails metadata") + } } func (h *UsersHandler) buildResourceForQuery(ctx context.Context, user *query.User) *schemas.Resource { diff --git a/internal/api/scim/resources/user_metadata.go b/internal/api/scim/resources/user_metadata.go index 69d85e40e5..3e018507fe 100644 --- a/internal/api/scim/resources/user_metadata.go +++ b/internal/api/scim/resources/user_metadata.go @@ -129,7 +129,8 @@ func getValueForMetadataKey(user *ScimUser, key metadata.Key) ([]byte, error) { metadata.KeyAddresses, metadata.KeyEntitlements, metadata.KeyIms, - metadata.KeyPhotos: + metadata.KeyPhotos, + metadata.KeyEmails: val, err := json.Marshal(value) if err != nil { return nil, err @@ -223,6 +224,8 @@ func getRawValueForMetadataKey(user *ScimUser, key metadata.Key) interface{} { return user.Locale case metadata.KeyTimezone: return user.Timezone + case metadata.KeyEmails: + return user.Emails case metadata.KeyProvisioningDomain: break } diff --git a/internal/api/scim/resources/user_patch_test.go b/internal/api/scim/resources/user_patch_test.go index ff3fc720bf..0c8aadc388 100644 --- a/internal/api/scim/resources/user_patch_test.go +++ b/internal/api/scim/resources/user_patch_test.go @@ -685,6 +685,39 @@ func TestOperationCollection_Apply(t *testing.T) { }, wantErr: true, }, + { + name: "replace filter complex subattribute multiple emails primary value", + op: &patch.Operation{ + Operation: patch.OperationTypeReplace, + Path: test.Must(filter.ParsePath(`emails[primary eq true].value`)), + Value: json.RawMessage(`"jeanie.rebecca.pendleton@example.com"`), + }, + want: &ScimUser{ + Emails: []*ScimEmail{ + { + Value: "jeanie.rebecca.pendleton@example.com", + Primary: true, + }, + }, + }, + }, + { + name: "replace filter complex subattribute multiple emails type value", + op: &patch.Operation{ + Operation: patch.OperationTypeReplace, + Path: test.Must(filter.ParsePath(`emails[type eq "work"].value`)), + Value: json.RawMessage(`"jeanie.rebecca.pendleton@example.com"`), + }, + want: &ScimUser{ + Emails: []*ScimEmail{ + { + Value: "jeanie.rebecca.pendleton@example.com", + Primary: true, + Type: "work", + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -711,6 +744,7 @@ func TestOperationCollection_Apply(t *testing.T) { { Value: "jeanie.pendleton@example.com", Primary: true, + Type: "work", }, }, PhoneNumbers: []*ScimPhoneNumber{