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,