From e5e17d59ac6d28ccb238883cfa18bac68487248f Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Wed, 22 Apr 2026 17:54:10 +0200 Subject: [PATCH] Handle SCIM user email rename via external ID fallback When a user's email is renamed in the identity provider (e.g. Google Workspace), the external ID stays the same but the email changes. The SCIM CreateUser now falls back to external ID lookup when no profile is found by identity, and reassociates the existing profile to the new identity instead of failing with a 409 uniqueness error. Also removes user emails from bridge sync error messages to avoid logging PII, using external IDs instead. Signed-off-by: Sacha Al Himdani --- e2e/console/scim_test.go | 275 +++++++++++++++++++++++++++++ pkg/coredata/membership.go | 8 +- pkg/coredata/membership_profile.go | 83 +++++++++ pkg/iam/scim/bridge/bridge.go | 8 +- pkg/iam/scim/service.go | 159 ++++++++++------- 5 files changed, 462 insertions(+), 71 deletions(-) create mode 100644 e2e/console/scim_test.go diff --git a/e2e/console/scim_test.go b/e2e/console/scim_test.go new file mode 100644 index 000000000..9b3f7059f --- /dev/null +++ b/e2e/console/scim_test.go @@ -0,0 +1,275 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package console_test + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.probo.inc/probo/e2e/internal/factory" + "go.probo.inc/probo/e2e/internal/testutil" +) + +type scimClient struct { + t testing.TB + client *http.Client + token string + endpoint string +} + +func newSCIMClient(t testing.TB, owner *testutil.Client) *scimClient { + t.Helper() + + const query = ` + mutation($input: CreateSCIMConfigurationInput!) { + createSCIMConfiguration(input: $input) { + scimConfiguration { id } + token + } + } + ` + + var result struct { + CreateSCIMConfiguration struct { + ScimConfiguration struct { + ID string `json:"id"` + } `json:"scimConfiguration"` + Token string `json:"token"` + } `json:"createSCIMConfiguration"` + } + + err := owner.ExecuteConnect(query, map[string]any{ + "input": map[string]any{ + "organizationId": owner.GetOrganizationID().String(), + }, + }, &result) + require.NoError(t, err, "GraphQL request failed") + + require.NotEmpty(t, result.CreateSCIMConfiguration.Token) + + return &scimClient{ + t: t, + client: &http.Client{}, + token: result.CreateSCIMConfiguration.Token, + endpoint: testutil.GetBaseURL() + "/api/connect/v1/scim/2.0", + } +} + +func (sc *scimClient) createUser(userName, fullName, externalID string, active bool) (string, int) { + sc.t.Helper() + + payload := map[string]any{ + "schemas": []string{"urn:ietf:params:scim:schemas:core:2.0:User"}, + "userName": userName, + "active": active, + "externalId": externalID, + "name": map[string]any{ + "givenName": "Test", + "familyName": "User", + }, + "displayName": fullName, + "emails": []map[string]any{ + {"value": userName, "primary": true}, + }, + } + + return sc.doRequest("POST", "/Users", payload) +} + +func (sc *scimClient) listUsers() (string, int) { + sc.t.Helper() + return sc.doRequest("GET", "/Users", nil) +} + +func (sc *scimClient) getUser(id string) (string, int) { + sc.t.Helper() + return sc.doRequest("GET", "/Users/"+id, nil) +} + +func (sc *scimClient) deleteUser(id string) (string, int) { + sc.t.Helper() + return sc.doRequest("DELETE", "/Users/"+id, nil) +} + +func (sc *scimClient) doRequest(method, path string, payload any) (string, int) { + sc.t.Helper() + + var body io.Reader + if payload != nil { + data, err := json.Marshal(payload) + require.NoError(sc.t, err) + body = bytes.NewReader(data) + } + + req, err := http.NewRequest(method, sc.endpoint+path, body) + require.NoError(sc.t, err) + + req.Header.Set("Authorization", "Bearer "+sc.token) + if payload != nil { + req.Header.Set("Content-Type", "application/scim+json") + } + + resp, err := sc.client.Do(req) + require.NoError(sc.t, err) + defer func() { _ = resp.Body.Close() }() + + respBody, err := io.ReadAll(resp.Body) + require.NoError(sc.t, err) + + return string(respBody), resp.StatusCode +} + +func TestSCIM_CreateUser(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + sc := newSCIMClient(t, owner) + + t.Run("create a new user", func(t *testing.T) { + t.Parallel() + + email := factory.SafeEmail() + body, status := sc.createUser(email, "New User", "ext-create-1", true) + + assert.Equal(t, http.StatusCreated, status, body) + + var resource map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &resource)) + assert.Equal(t, email, resource["userName"]) + assert.NotEmpty(t, resource["id"]) + }) + + t.Run("duplicate user returns 409", func(t *testing.T) { + t.Parallel() + + email := factory.SafeEmail() + _, status := sc.createUser(email, "Dup User", "ext-dup-1", true) + require.Equal(t, http.StatusCreated, status) + + _, status = sc.createUser(email, "Dup User", "ext-dup-1", true) + assert.Equal(t, http.StatusConflict, status) + }) +} + +func TestSCIM_ListUsers(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + sc := newSCIMClient(t, owner) + + email := factory.SafeEmail() + _, status := sc.createUser(email, "List User", "ext-list-1", true) + require.Equal(t, http.StatusCreated, status) + + body, status := sc.listUsers() + require.Equal(t, http.StatusOK, status, body) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &response)) + + resources := response["Resources"].([]any) + assert.GreaterOrEqual(t, len(resources), 1) +} + +func TestSCIM_ExternalIDFallback(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + sc := newSCIMClient(t, owner) + + t.Run("email rename reuses profile via external ID", func(t *testing.T) { + t.Parallel() + + externalID := "google-" + factory.SafeName("") + oldEmail := factory.SafeEmail() + newEmail := factory.SafeEmail() + + // Create user with old email + body, status := sc.createUser(oldEmail, "Rename User", externalID, true) + require.Equal(t, http.StatusCreated, status, body) + + var created map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &created)) + originalID := created["id"].(string) + + // Create user with new email but same external ID (simulates email rename) + body, status = sc.createUser(newEmail, "Rename User", externalID, true) + require.Equal(t, http.StatusCreated, status, body) + + var updated map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &updated)) + + // Should reuse the same profile (same ID) + assert.Equal(t, originalID, updated["id"].(string), "profile ID should be preserved after email rename") + assert.Equal(t, newEmail, updated["userName"], "email should be updated") + + // Verify via GET that the profile is consistent + body, status = sc.getUser(originalID) + require.Equal(t, http.StatusOK, status, body) + + var fetched map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &fetched)) + assert.Equal(t, newEmail, fetched["userName"]) + }) + + t.Run("different external ID creates new profile", func(t *testing.T) { + t.Parallel() + + email := factory.SafeEmail() + + _, status := sc.createUser(email, "User A", "ext-a-"+factory.SafeName(""), true) + require.Equal(t, http.StatusCreated, status) + + // Same email, different external ID — should fail (email already taken) + _, status = sc.createUser(email, "User B", "ext-b-"+factory.SafeName(""), true) + assert.Equal(t, http.StatusConflict, status) + }) +} + +func TestSCIM_DeleteUser(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + sc := newSCIMClient(t, owner) + + email := factory.SafeEmail() + body, status := sc.createUser(email, "Delete User", "ext-del-1", true) + require.Equal(t, http.StatusCreated, status, body) + + var created map[string]any + require.NoError(t, json.Unmarshal([]byte(body), &created)) + userID := created["id"].(string) + + _, status = sc.deleteUser(userID) + assert.Equal(t, http.StatusNoContent, status) + + _, status = sc.getUser(userID) + assert.Equal(t, http.StatusNotFound, status) +} + +func TestSCIM_Unauthorized(t *testing.T) { + t.Parallel() + + client := &http.Client{} + req, err := http.NewRequest("GET", testutil.GetBaseURL()+"/api/connect/v1/scim/2.0/Users", nil) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) +} diff --git a/pkg/coredata/membership.go b/pkg/coredata/membership.go index 36c0216fe..51bc488db 100644 --- a/pkg/coredata/membership.go +++ b/pkg/coredata/membership.go @@ -286,6 +286,7 @@ func (m *Membership) Update(ctx context.Context, conn pg.Tx, scope Scoper) error UPDATE iam_memberships SET + identity_id = @identity_id, role = @role, updated_at = @updated_at WHERE @@ -296,9 +297,10 @@ WHERE query = fmt.Sprintf(query, scope.SQLFragment()) args := pgx.StrictNamedArgs{ - "id": m.ID, - "role": m.Role, - "updated_at": m.UpdatedAt, + "id": m.ID, + "identity_id": m.IdentityID, + "role": m.Role, + "updated_at": m.UpdatedAt, } maps.Copy(args, scope.SQLArguments()) diff --git a/pkg/coredata/membership_profile.go b/pkg/coredata/membership_profile.go index a0ad61bf2..972e32a15 100644 --- a/pkg/coredata/membership_profile.go +++ b/pkg/coredata/membership_profile.go @@ -262,6 +262,87 @@ LIMIT 1; return nil } +func (p *MembershipProfile) LoadByExternalIDAndOrganizationID( + ctx context.Context, + conn pg.Querier, + scope Scoper, + externalID string, + organizationID gid.GID, +) error { + q := ` +SELECT + p.id, + p.identity_id, + p.organization_id, + i.email_address, + p.source, + p.state, + p.full_name, + p.kind, + p.additional_email_addresses, + p.position, + p.contract_start_date, + p.contract_end_date, + '' AS organization_name, + p.user_name, + p.external_id, + p.nickname, + p.locale, + p.timezone, + p.profile_url, + p.preferred_language, + p.given_name, + p.family_name, + p.formatted_name, + p.middle_name, + p.honorific_prefix, + p.honorific_suffix, + p.employee_number, + p.department, + p.cost_center, + p.enterprise_organization, + p.division, + p.manager_value, + p.created_at, + p.updated_at +FROM + iam_membership_profiles p +INNER JOIN identities i + ON i.id = p.identity_id +WHERE + p.%s + AND p.external_id = @external_id + AND p.organization_id = @organization_id +LIMIT 1; +` + + q = fmt.Sprintf(q, scope.SQLFragment()) + + args := pgx.StrictNamedArgs{ + "external_id": externalID, + "organization_id": organizationID, + } + maps.Copy(args, scope.SQLArguments()) + + rows, err := conn.Query(ctx, q, args) + if err != nil { + return fmt.Errorf("cannot query profile: %w", err) + } + + profile, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[MembershipProfile]) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrResourceNotFound + } + + return fmt.Errorf("cannot collect profile: %w", err) + } + + *p = profile + + return nil +} + func (p *MembershipProfiles) LoadByIDs( ctx context.Context, conn pg.Querier, @@ -1149,6 +1230,7 @@ func (p *MembershipProfile) Update( UPDATE iam_membership_profiles SET + identity_id = @identity_id, source = @source, state = @state, full_name = @full_name, @@ -1186,6 +1268,7 @@ WHERE args := pgx.StrictNamedArgs{ "id": p.ID, + "identity_id": p.IdentityID, "source": p.Source, "state": p.State, "full_name": p.FullName, diff --git a/pkg/iam/scim/bridge/bridge.go b/pkg/iam/scim/bridge/bridge.go index 3bdd27c9f..2eebd6887 100644 --- a/pkg/iam/scim/bridge/bridge.go +++ b/pkg/iam/scim/bridge/bridge.go @@ -83,7 +83,7 @@ func (s *Bridge) Run(ctx context.Context) (created, updated, deleted, deactivate existingSCIM, exists := scimUsersByEmail[email] if !exists { if err := s.scimClient.CreateUser(ctx, &pu); err != nil { - errs = append(errs, fmt.Errorf("cannot create user %q %q: %w", pu.ExternalID, pu.UserName, err)) + errs = append(errs, fmt.Errorf("cannot create user %q: %w", pu.ExternalID, err)) continue } created++ @@ -104,7 +104,7 @@ func (s *Bridge) Run(ctx context.Context) (created, updated, deleted, deactivate if needsUpdate { if err := s.scimClient.UpdateUser(ctx, existingSCIM.ID, &pu); err != nil { - errs = append(errs, fmt.Errorf("cannot update user %q: %w", pu.UserName, err)) + errs = append(errs, fmt.Errorf("cannot update user %q: %w", pu.ExternalID, err)) continue } updated++ @@ -121,7 +121,7 @@ func (s *Bridge) Run(ctx context.Context) (created, updated, deleted, deactivate if s.isExcluded(email) { if err := s.scimClient.DeleteUser(ctx, scimUser.ID); err != nil { - errs = append(errs, fmt.Errorf("cannot delete user %q: %w", email, err)) + errs = append(errs, fmt.Errorf("cannot delete user %q: %w", scimUser.ExternalID, err)) continue } deleted++ @@ -133,7 +133,7 @@ func (s *Bridge) Run(ctx context.Context) (created, updated, deleted, deactivate } if err := s.scimClient.DeactivateUser(ctx, scimUser.ID); err != nil { - errs = append(errs, fmt.Errorf("cannot deactivate user %q: %w", email, err)) + errs = append(errs, fmt.Errorf("cannot deactivate user %q: %w", scimUser.ExternalID, err)) continue } deactivated++ diff --git a/pkg/iam/scim/service.go b/pkg/iam/scim/service.go index 45bd7f959..3c398bce9 100644 --- a/pkg/iam/scim/service.go +++ b/pkg/iam/scim/service.go @@ -177,6 +177,7 @@ func (s *Service) CreateUser( eventType := coredata.WebhookEventTypeUserUpdated profile = &coredata.MembershipProfile{} + if err := profile.LoadByIdentityIDAndOrganizationID( ctx, tx, @@ -184,42 +185,63 @@ func (s *Service) CreateUser( identity.ID, config.OrganizationID, ); err != nil { - if errors.Is(err, coredata.ErrResourceNotFound) { + if !errors.Is(err, coredata.ErrResourceNotFound) { + return fmt.Errorf("cannot load profile: %w", err) + } + + // Profile not found by identity. Try by external ID to + // handle email renames in identity providers (e.g. Google + // Workspace) where the external ID stays the same but the + // email changes. If found, update it to point to the new + // identity. + if externalIdPtr != nil { + if err := profile.LoadByExternalIDAndOrganizationID( + ctx, + tx, + scope, + *externalIdPtr, + config.OrganizationID, + ); err == nil { + // Migrate the existing membership to the new identity + // so the user's role is preserved. + oldIdentityID := profile.IdentityID + existingMembership := &coredata.Membership{} + if err := existingMembership.LoadByIdentityIDAndOrganizationID( + ctx, + tx, + scope, + oldIdentityID, + config.OrganizationID, + ); err == nil { + existingMembership.IdentityID = identity.ID + existingMembership.UpdatedAt = now + if err := existingMembership.Update(ctx, tx, scope); err != nil { + return fmt.Errorf("cannot update membership identity: %w", err) + } + } else if !errors.Is(err, coredata.ErrResourceNotFound) { + return fmt.Errorf("cannot load membership for identity migration: %w", err) + } + + profile.IdentityID = identity.ID + profile.EmailAddress = emailAddr + applyUserAttributes(profile, attrs, externalIdPtr, profileState, now) + if err := profile.Update(ctx, tx, scope); err != nil { + return fmt.Errorf("cannot update profile: %w", err) + } + } else if !errors.Is(err, coredata.ErrResourceNotFound) { + return fmt.Errorf("cannot load profile by external id: %w", err) + } + } + + if profile.ID == (gid.GID{}) { profile = &coredata.MembershipProfile{ - ID: gid.New(config.OrganizationID.TenantID(), coredata.MembershipProfileEntityType), - IdentityID: identity.ID, - OrganizationID: config.OrganizationID, - EmailAddress: emailAddr, - Source: coredata.ProfileSourceSCIM, - State: profileState, - FullName: attrs.FullName, - Position: &attrs.Title, - UserName: &attrs.UserName, - ExternalID: externalIdPtr, - Nickname: ref.RefOrNil(attrs.Nickname), - Locale: ref.RefOrNil(attrs.Locale), - Timezone: ref.RefOrNil(attrs.Timezone), - ProfileUrl: ref.RefOrNil(attrs.ProfileUrl), - PreferredLanguage: ref.RefOrNil(attrs.PreferredLanguage), - GivenName: ref.RefOrNil(attrs.GivenName), - FamilyName: ref.RefOrNil(attrs.FamilyName), - FormattedName: ref.RefOrNil(attrs.FormattedName), - MiddleName: ref.RefOrNil(attrs.MiddleName), - HonorificPrefix: ref.RefOrNil(attrs.HonorificPrefix), - HonorificSuffix: ref.RefOrNil(attrs.HonorificSuffix), - EmployeeNumber: ref.RefOrNil(attrs.EmployeeNumber), - Department: ref.RefOrNil(attrs.Department), - CostCenter: ref.RefOrNil(attrs.CostCenter), - EnterpriseOrganization: ref.RefOrNil(attrs.EnterpriseOrganization), - Division: ref.RefOrNil(attrs.Division), - ManagerValue: ref.RefOrNil(attrs.ManagerValue), - CreatedAt: now, - UpdatedAt: now, - } - if attrs.UserType != "" { - kind := attrs.UserType - profile.Kind = &kind + ID: gid.New(config.OrganizationID.TenantID(), coredata.MembershipProfileEntityType), + IdentityID: identity.ID, + OrganizationID: config.OrganizationID, + EmailAddress: emailAddr, + CreatedAt: now, } + applyUserAttributes(profile, attrs, externalIdPtr, profileState, now) err = profile.Insert(ctx, tx) if err != nil { @@ -229,8 +251,6 @@ func (s *Service) CreateUser( return fmt.Errorf("cannot insert profile: %w", err) } eventType = coredata.WebhookEventTypeUserCreated - } else { - return fmt.Errorf("cannot load profile: %w", err) } } else { if profile.Source == coredata.ProfileSourceSCIM { @@ -249,34 +269,7 @@ func (s *Service) CreateUser( } } - profile.Source = coredata.ProfileSourceSCIM - profile.State = profileState - profile.FullName = attrs.FullName - profile.Position = &attrs.Title - profile.UserName = &attrs.UserName - profile.ExternalID = externalIdPtr - profile.Nickname = ref.RefOrNil(attrs.Nickname) - profile.Locale = ref.RefOrNil(attrs.Locale) - profile.Timezone = ref.RefOrNil(attrs.Timezone) - profile.ProfileUrl = ref.RefOrNil(attrs.ProfileUrl) - profile.PreferredLanguage = ref.RefOrNil(attrs.PreferredLanguage) - profile.GivenName = ref.RefOrNil(attrs.GivenName) - profile.FamilyName = ref.RefOrNil(attrs.FamilyName) - profile.FormattedName = ref.RefOrNil(attrs.FormattedName) - profile.MiddleName = ref.RefOrNil(attrs.MiddleName) - profile.HonorificPrefix = ref.RefOrNil(attrs.HonorificPrefix) - profile.HonorificSuffix = ref.RefOrNil(attrs.HonorificSuffix) - profile.EmployeeNumber = ref.RefOrNil(attrs.EmployeeNumber) - profile.Department = ref.RefOrNil(attrs.Department) - profile.CostCenter = ref.RefOrNil(attrs.CostCenter) - profile.EnterpriseOrganization = ref.RefOrNil(attrs.EnterpriseOrganization) - profile.Division = ref.RefOrNil(attrs.Division) - profile.ManagerValue = ref.RefOrNil(attrs.ManagerValue) - profile.UpdatedAt = now - if attrs.UserType != "" { - kind := attrs.UserType - profile.Kind = &kind - } + applyUserAttributes(profile, attrs, externalIdPtr, profileState, now) if err := profile.Update(ctx, tx, scope); err != nil { return fmt.Errorf("cannot update profile: %w", err) } @@ -742,6 +735,44 @@ func (s *Service) updateUser( return profile, nil } +func applyUserAttributes( + profile *coredata.MembershipProfile, + attrs scimUserAttributes, + externalID *string, + state coredata.ProfileState, + now time.Time, +) { + profile.Source = coredata.ProfileSourceSCIM + profile.State = state + profile.FullName = attrs.FullName + profile.Position = &attrs.Title + profile.UserName = &attrs.UserName + profile.ExternalID = externalID + profile.Nickname = ref.RefOrNil(attrs.Nickname) + profile.Locale = ref.RefOrNil(attrs.Locale) + profile.Timezone = ref.RefOrNil(attrs.Timezone) + profile.ProfileUrl = ref.RefOrNil(attrs.ProfileUrl) + profile.PreferredLanguage = ref.RefOrNil(attrs.PreferredLanguage) + profile.GivenName = ref.RefOrNil(attrs.GivenName) + profile.FamilyName = ref.RefOrNil(attrs.FamilyName) + profile.FormattedName = ref.RefOrNil(attrs.FormattedName) + profile.MiddleName = ref.RefOrNil(attrs.MiddleName) + profile.HonorificPrefix = ref.RefOrNil(attrs.HonorificPrefix) + profile.HonorificSuffix = ref.RefOrNil(attrs.HonorificSuffix) + profile.EmployeeNumber = ref.RefOrNil(attrs.EmployeeNumber) + profile.Department = ref.RefOrNil(attrs.Department) + profile.CostCenter = ref.RefOrNil(attrs.CostCenter) + profile.EnterpriseOrganization = ref.RefOrNil(attrs.EnterpriseOrganization) + profile.Division = ref.RefOrNil(attrs.Division) + profile.ManagerValue = ref.RefOrNil(attrs.ManagerValue) + profile.UpdatedAt = now + + if attrs.UserType != "" { + kind := attrs.UserType + profile.Kind = &kind + } +} + func (s *Service) DeleteUser( ctx context.Context, config *coredata.SCIMConfiguration,