From 361f7a2edc0145528107c71753dee6e4d8c8b4c6 Mon Sep 17 00:00:00 2001 From: Lars Date: Wed, 5 Feb 2025 16:17:20 +0100 Subject: [PATCH] fix: relax parsing of SCIM user 'active' flag to improve compatibility (#9296) # Which Problems Are Solved - Microsoft Entra invokes the user patch endpoint with `"active": "True"` / `"active": "False"` when patching a user. This is a well-known bug in MS Entra (see [here](https://learn.microsoft.com/en-us/entra/identity/app-provisioning/application-provisioning-config-problem-scim-compatibility)), but the bug fix has not landed yet and/or the feature flag does not work. # How the Problems Are Solved - To ensure compatibility with MS Entra, the parsing of the the boolean active flag of the scim user is relaxed and accepts strings in any casing that resolve to `true` or `false` as well as raw boolean values. # Additional Context Part of https://github.com/zitadel/zitadel/issues/8140 --- .../api/scim/integration_test/bulk_test.go | 7 +-- .../testdata/users_update_test_full.json | 8 +++ .../integration_test/users_create_test.go | 5 +- .../scim/integration_test/users_get_test.go | 3 +- .../scim/integration_test/users_list_test.go | 2 +- .../integration_test/users_replace_test.go | 3 +- .../integration_test/users_update_test.go | 3 +- internal/api/scim/resources/user.go | 2 +- internal/api/scim/resources/user_mapping.go | 4 +- .../api/scim/resources/user_patch_test.go | 3 +- internal/api/scim/schemas/bool.go | 41 ++++++++++++ internal/api/scim/schemas/bool_test.go | 63 +++++++++++++++++++ 12 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 internal/api/scim/schemas/bool.go create mode 100644 internal/api/scim/schemas/bool_test.go diff --git a/internal/api/scim/integration_test/bulk_test.go b/internal/api/scim/integration_test/bulk_test.go index b3beb8dc38..2c9f95a262 100644 --- a/internal/api/scim/integration_test/bulk_test.go +++ b/internal/api/scim/integration_test/bulk_test.go @@ -14,7 +14,6 @@ import ( "time" "github.com/brianvoe/gofakeit/v6" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -289,7 +288,7 @@ func TestBulk(t *testing.T) { }, DisplayName: "scim-bulk-created-user-0-given-name scim-bulk-created-user-0-family-name", PreferredLanguage: test.Must(language.Parse("en")), - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), Emails: []*resources.ScimEmail{ { Value: "scim-bulk-created-user-0@example.com", @@ -308,7 +307,7 @@ func TestBulk(t *testing.T) { DisplayName: "scim-bulk-created-user-1-given-name scim-bulk-created-user-1-family-name", NickName: "scim-bulk-created-user-1-nickname-patched", PreferredLanguage: test.Must(language.Parse("en")), - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), Emails: []*resources.ScimEmail{ { Value: "scim-bulk-created-user-1@example.com", @@ -333,7 +332,7 @@ func TestBulk(t *testing.T) { DisplayName: "scim-bulk-created-user-2-given-name scim-bulk-created-user-2-family-name", NickName: "scim-bulk-created-user-2-nickname-patched", PreferredLanguage: test.Must(language.Parse("en")), - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), Emails: []*resources.ScimEmail{ { Value: "scim-bulk-created-user-2@example.com", diff --git a/internal/api/scim/integration_test/testdata/users_update_test_full.json b/internal/api/scim/integration_test/testdata/users_update_test_full.json index 62bacecb05..23403f3e5b 100644 --- a/internal/api/scim/integration_test/testdata/users_update_test_full.json +++ b/internal/api/scim/integration_test/testdata/users_update_test_full.json @@ -134,6 +134,14 @@ "op": "replace", "path": "password", "value": "Password2!" + }, + // replace active state + { + "op": "replace", + "path": "active", + // quoted uppercase bool + // (ensure compatibility with Microsoft Entra) + "value": "True" } ] } \ 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 bebb7fd00e..1202add3d4 100644 --- a/internal/api/scim/integration_test/users_create_test.go +++ b/internal/api/scim/integration_test/users_create_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -154,7 +153,7 @@ var ( PreferredLanguage: language.MustParse("en-US"), Locale: "en-US", Timezone: "America/Los_Angeles", - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), } ) @@ -191,7 +190,7 @@ func TestCreateUser(t *testing.T) { name: "minimal inactive user", body: minimalInactiveUserJson, want: &resources.ScimUser{ - Active: gu.Ptr(false), + Active: schemas.NewRelaxedBool(false), }, }, { diff --git a/internal/api/scim/integration_test/users_get_test.go b/internal/api/scim/integration_test/users_get_test.go index 76e59c373d..7651952b43 100644 --- a/internal/api/scim/integration_test/users_get_test.go +++ b/internal/api/scim/integration_test/users_get_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -112,7 +111,7 @@ func TestGetUser(t *testing.T) { PreferredLanguage: language.Make("en-US"), Locale: "en-US", Timezone: "America/Los_Angeles", - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), Emails: []*resources.ScimEmail{ { Value: "bjensen@example.com", diff --git a/internal/api/scim/integration_test/users_list_test.go b/internal/api/scim/integration_test/users_list_test.go index 3df97fae28..d364fa219d 100644 --- a/internal/api/scim/integration_test/users_list_test.go +++ b/internal/api/scim/integration_test/users_list_test.go @@ -239,7 +239,7 @@ func TestListUser(t *testing.T) { assert.Equal(t, 1, resp.StartIndex) assert.Len(t, resp.Resources, 1) assert.True(t, strings.HasPrefix(resp.Resources[0].UserName, "scim-username-0")) - assert.False(t, *resp.Resources[0].Active) + assert.False(t, resp.Resources[0].Active.Bool()) }, }, { diff --git a/internal/api/scim/integration_test/users_replace_test.go b/internal/api/scim/integration_test/users_replace_test.go index bd4f34cef7..9f88621872 100644 --- a/internal/api/scim/integration_test/users_replace_test.go +++ b/internal/api/scim/integration_test/users_replace_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -167,7 +166,7 @@ func TestReplaceUser(t *testing.T) { PreferredLanguage: language.MustParse("en-CH"), Locale: "en-CH", Timezone: "Europe/Zurich", - Active: gu.Ptr(false), + Active: schemas.NewRelaxedBool(false), }, }, { diff --git a/internal/api/scim/integration_test/users_update_test.go b/internal/api/scim/integration_test/users_update_test.go index 8334fad2e5..2fe129291a 100644 --- a/internal/api/scim/integration_test/users_update_test.go +++ b/internal/api/scim/integration_test/users_update_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -208,7 +207,7 @@ func TestUpdateUser(t *testing.T) { PreferredLanguage: language.MustParse("en-US"), Locale: "en-US", Timezone: "America/Los_Angeles", - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), }, }, } diff --git a/internal/api/scim/resources/user.go b/internal/api/scim/resources/user.go index 8d9ef70592..bc8d864994 100644 --- a/internal/api/scim/resources/user.go +++ b/internal/api/scim/resources/user.go @@ -39,7 +39,7 @@ type ScimUser struct { PreferredLanguage language.Tag `json:"preferredLanguage,omitempty"` Locale string `json:"locale,omitempty"` Timezone string `json:"timezone,omitempty"` - Active *bool `json:"active,omitempty"` + Active *scim_schemas.RelaxedBool `json:"active,omitempty"` Emails []*ScimEmail `json:"emails,omitempty" scim:"required"` PhoneNumbers []*ScimPhoneNumber `json:"phoneNumbers,omitempty"` Password *scim_schemas.WriteOnlyString `json:"password,omitempty"` diff --git a/internal/api/scim/resources/user_mapping.go b/internal/api/scim/resources/user_mapping.go index 79c75ab113..171af87238 100644 --- a/internal/api/scim/resources/user_mapping.go +++ b/internal/api/scim/resources/user_mapping.go @@ -273,7 +273,7 @@ func (h *UsersHandler) mapToScimUser(ctx context.Context, user *query.User, md m FamilyName: user.Human.LastName, GivenName: user.Human.FirstName, }, - Active: gu.Ptr(user.State.IsEnabled()), + Active: schemas.NewRelaxedBool(user.State.IsEnabled()), } if string(user.Human.Email) != "" { @@ -311,7 +311,7 @@ func (h *UsersHandler) mapWriteModelToScimUser(ctx context.Context, user *comman FamilyName: user.LastName, GivenName: user.FirstName, }, - Active: gu.Ptr(user.UserState.IsEnabled()), + Active: schemas.NewRelaxedBool(user.UserState.IsEnabled()), } if string(user.Email) != "" { diff --git a/internal/api/scim/resources/user_patch_test.go b/internal/api/scim/resources/user_patch_test.go index 5ed83b94a9..ff3fc720bf 100644 --- a/internal/api/scim/resources/user_patch_test.go +++ b/internal/api/scim/resources/user_patch_test.go @@ -7,7 +7,6 @@ import ( "strings" "testing" - "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/text/language" @@ -707,7 +706,7 @@ func TestOperationCollection_Apply(t *testing.T) { PreferredLanguage: language.MustParse("en-US"), Locale: "en-US", Timezone: "America/New_York", - Active: gu.Ptr(true), + Active: schemas.NewRelaxedBool(true), Emails: []*ScimEmail{ { Value: "jeanie.pendleton@example.com", diff --git a/internal/api/scim/schemas/bool.go b/internal/api/scim/schemas/bool.go new file mode 100644 index 0000000000..d0f45c5dec --- /dev/null +++ b/internal/api/scim/schemas/bool.go @@ -0,0 +1,41 @@ +package schemas + +import ( + "encoding/json" + "strings" + + "github.com/muhlemmer/gu" + + "github.com/zitadel/zitadel/internal/zerrors" +) + +// RelaxedBool a bool which is more relaxed when it comes to json (un)marshaling. +// This ensures compatibility with some bugged scim providers, +// such as Microsoft Entry, which sends booleans as "True" or "False". +// See also https://learn.microsoft.com/en-us/entra/identity/app-provisioning/application-provisioning-config-problem-scim-compatibility. +type RelaxedBool bool + +func NewRelaxedBool(value bool) *RelaxedBool { + return gu.Ptr(RelaxedBool(value)) +} + +func (b *RelaxedBool) MarshalJSON() ([]byte, error) { + return json.Marshal(bool(*b)) +} + +func (b *RelaxedBool) UnmarshalJSON(bytes []byte) error { + str := strings.ToLower(string(bytes)) + switch { + case str == "true" || str == "\"true\"": + *b = true + case str == "false" || str == "\"false\"": + *b = false + default: + return zerrors.ThrowInvalidArgumentf(nil, "SCIM-BOO1", "bool expected, got %v", str) + } + return nil +} + +func (b *RelaxedBool) Bool() bool { + return bool(*b) +} diff --git a/internal/api/scim/schemas/bool_test.go b/internal/api/scim/schemas/bool_test.go new file mode 100644 index 0000000000..b73572189f --- /dev/null +++ b/internal/api/scim/schemas/bool_test.go @@ -0,0 +1,63 @@ +package schemas + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRelaxedBool_MarshalJSON(t *testing.T) { + tests := []struct { + name string + input bool + expected string + }{ + {name: "true", input: true, expected: "true"}, + {name: "false", expected: "false"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + value := NewRelaxedBool(tt.input) + bytes, err := json.Marshal(value) + require.NoError(t, err) + assert.Equal(t, tt.expected, string(bytes)) + }) + } +} + +func TestRelaxedBool_UnmarshalJSON(t *testing.T) { + tests := []struct { + name string + input string + expected bool + wantErr bool + }{ + {name: "valid true", input: "true", expected: true}, + {name: "valid false", input: "false"}, + {name: "quoted true", input: `"true"`, expected: true}, + {name: "quoted pascal case true", input: `"True"`, expected: true}, + {name: "quoted upper case true", input: `"TRUE"`, expected: true}, + {name: "quoted false", input: `"false"`}, + {name: "quoted pascal case false", input: `"False"`}, + {name: "quoted upper case false", input: `"FALSE"`}, + {name: "invalid value", input: "invalid", wantErr: true}, + {name: "number value", input: "1", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + value := new(RelaxedBool) + err := json.Unmarshal([]byte(tt.input), &value) + + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, value.Bool()) + } + }) + } +}