From 2d5ef59bf370f0a72451b715c25a69673dcf03fb Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Mon, 30 Jun 2025 12:18:06 -0300 Subject: [PATCH] PR review changes from main IdP department PR (#30418) Addressing comments on main [PR](https://github.com/fleetdm/fleet/pull/30375) for #29609. --- ee/server/integrationtest/scim/scim_test.go | 118 ++++++++++++++++++++ ee/server/integrationtest/scim/suite.go | 3 +- ee/server/scim/users.go | 5 + server/datastore/mysql/scim.go | 10 +- server/service/integrationtest/suite.go | 17 ++- 5 files changed, 140 insertions(+), 13 deletions(-) diff --git a/ee/server/integrationtest/scim/scim_test.go b/ee/server/integrationtest/scim/scim_test.go index 8b26f465d3..9acd61a9d6 100644 --- a/ee/server/integrationtest/scim/scim_test.go +++ b/ee/server/integrationtest/scim/scim_test.go @@ -975,11 +975,48 @@ func testCreateUser(t *testing.T, s *Suite) { // Verify externalId is present and correct assert.Equal(t, "external-system-123456", createResp6["externalId"]) + // Test creating a user with department + userWithDepartment := map[string]interface{}{ + "schemas": []string{ + "urn:ietf:params:scim:schemas:core:2.0:User", + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User", + }, + "userName": "user-with-department@example.com", + "name": map[string]interface{}{ + "givenName": "Foo", + "familyName": "Bar", + }, + "emails": []map[string]interface{}{ + { + "value": "foobar@example.com", + "type": "work", + "primary": true, + }, + }, + "active": true, + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": map[string]interface{}{ + "department": "Engineering", + }, + } + + var createResp7 map[string]interface{} + s.DoJSON(t, "POST", scimPath("/Users"), userWithDepartment, http.StatusCreated, &createResp7) + assert.Equal(t, "user-with-department@example.com", createResp7["userName"]) + userID7 := createResp7["id"].(string) + + // Verify department is present and correct + m_, ok := createResp7["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] + require.True(t, ok) + m, ok := m_.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "Engineering", m["department"]) + // Make sure these users can be deleted. s.Do(t, "DELETE", scimPath("/Users/"+userID3), nil, http.StatusNoContent) s.Do(t, "DELETE", scimPath("/Users/"+userID4), nil, http.StatusNoContent) s.Do(t, "DELETE", scimPath("/Users/"+userID5), nil, http.StatusNoContent) s.Do(t, "DELETE", scimPath("/Users/"+userID6), nil, http.StatusNoContent) + s.Do(t, "DELETE", scimPath("/Users/"+userID7), nil, http.StatusNoContent) } func testUpdateUser(t *testing.T, s *Suite) { @@ -1082,6 +1119,26 @@ func testUpdateUser(t *testing.T, s *Suite) { // Verify the update was successful. assert.Equal(t, "FiRsT-uSeR@eXaMpLe.CoM", updateResp2["userName"]) + // Test 5: Try to update first user's department (should succeed) + schemas_, ok := updatePayload["schemas"] + require.True(t, ok) + schemas, ok := schemas_.([]string) + require.True(t, ok) + updatePayload["schemas"] = append(schemas, "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User") + updatePayload["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] = map[string]interface{}{ + "department": "Engineering", + } + + var updateResp3 map[string]interface{} + s.DoJSON(t, "PUT", scimPath("/Users/"+firstUserID), updatePayload, http.StatusOK, &updateResp3) + + // Verify the update was successful. + m_, ok := updateResp3["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] + require.True(t, ok) + m, ok := m_.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "Engineering", m["department"]) + // Delete the users we created. s.Do(t, "DELETE", scimPath("/Users/"+firstUserID), nil, http.StatusNoContent) s.Do(t, "DELETE", scimPath("/Users/"+secondUserID), nil, http.StatusNoContent) @@ -2995,6 +3052,67 @@ func testPatchUserAttributes(t *testing.T, s *Suite) { assert.Equal(t, "Updates", name["familyName"], "familyName should be updated") }) + t.Run("Patch department", func(t *testing.T) { + patchDepartmentPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department": "QA", + }, + }, + }, + } + + var patchDepartmentResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), patchDepartmentPayload, http.StatusOK, &patchDepartmentResp) + // Verify department is present and correct + m_, ok := patchDepartmentResp["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] + require.True(t, ok) + m, ok := m_.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "QA", m["department"]) + + // Now remove department using path. + patchDepartmentPayload2 := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department", + }, + }, + } + + var patchDepartmentResp2 map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), patchDepartmentPayload2, http.StatusOK, &patchDepartmentResp2) + // Verify department is not present (if there are no extension attributes, then the whole map is not there). + _, ok = patchDepartmentResp2["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] + require.False(t, ok) + + // Now re-add department using path. + patchDepartmentPayload3 := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department", + "value": "Engineering", + }, + }, + } + + var patchDepartmentResp3 map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), patchDepartmentPayload3, http.StatusOK, &patchDepartmentResp3) + // Verify department is present and correct + m_, ok = patchDepartmentResp3["urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"] + require.True(t, ok) + m, ok = m_.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "Engineering", m["department"]) + }) + // /////////////////////////////////////////////// // Tests for patching with explicit operation path diff --git a/ee/server/integrationtest/scim/suite.go b/ee/server/integrationtest/scim/suite.go index 8d42ad056c..5f16313b6b 100644 --- a/ee/server/integrationtest/scim/suite.go +++ b/ee/server/integrationtest/scim/suite.go @@ -19,14 +19,13 @@ func SetUpSuite(t *testing.T, uniqueTestName string) *Suite { license := &fleet.LicenseInfo{ Tier: fleet.TierPremium, } - ds, redisPool, fleetCfg, fleetSvc, ctx := integrationtest.SetUpMySQLAndRedisAndService(t, uniqueTestName, &service.TestServerOpts{ + ds, fleetCfg, fleetSvc, ctx := integrationtest.SetUpMySQLAndService(t, uniqueTestName, &service.TestServerOpts{ License: license, }) logger := log.NewLogfmtLogger(os.Stdout) users, server := service.RunServerForTestsWithServiceWithDS(t, ctx, ds, fleetSvc, &service.TestServerOpts{ License: license, FleetConfig: &fleetCfg, - Pool: redisPool, Logger: logger, EnableSCIM: true, }) diff --git a/ee/server/scim/users.go b/ee/server/scim/users.go index 99b36e562f..a295e7d7c2 100644 --- a/ee/server/scim/users.go +++ b/ee/server/scim/users.go @@ -574,6 +574,11 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO if err != nil { return scim.Resource{}, err } + case op.Path.AttributePath.String() == extensionEnterpriseUserAttributes+":"+departmentAttr: + err = u.patchDepartment(op.Op, op.Value, user) + if err != nil { + return scim.Resource{}, err + } default: level.Info(u.logger).Log("msg", "unsupported patch path", "path", op.Path) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) diff --git a/server/datastore/mysql/scim.go b/server/datastore/mysql/scim.go index cbb3a0dd31..8ea4bd1c9e 100644 --- a/server/datastore/mysql/scim.go +++ b/server/datastore/mysql/scim.go @@ -12,6 +12,7 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" "github.com/go-kit/log" "github.com/go-kit/log/level" + "github.com/google/go-cmp/cmp" "github.com/jmoiron/sqlx" ) @@ -314,14 +315,9 @@ func (ds *Datastore) ReplaceScimUser(ctx context.Context, user *fleet.ScimUser) if rowsAffected == 0 { return notFound("scim user").WithID(user.ID) } + usernameChanged := old.UserName != user.UserName - derefStrPtr := func(s *string) string { - if s == nil { - return "" - } - return *s - } - departmentChanged := derefStrPtr(old.Department) != derefStrPtr(user.Department) + departmentChanged := !cmp.Equal(old.Department, user.Department) // Only update emails if they've changed if emailsNeedUpdate { diff --git a/server/service/integrationtest/suite.go b/server/service/integrationtest/suite.go index f4807e0075..b084eb1b79 100644 --- a/server/service/integrationtest/suite.go +++ b/server/service/integrationtest/suite.go @@ -48,9 +48,11 @@ func SetUpServerURL(t *testing.T, ds *mysql.Datastore, server *httptest.Server) require.NoError(t, err) } -func SetUpMySQLAndRedisAndService(t *testing.T, uniqueTestName string, opts ...*service.TestServerOpts) (*mysql.Datastore, fleet.RedisPool, +func SetUpMySQLAndService(t *testing.T, uniqueTestName string, opts ...*service.TestServerOpts) ( + *mysql.Datastore, config.FleetConfig, - fleet.Service, context.Context) { + fleet.Service, context.Context, +) { ds := mysql.CreateMySQLDS(t) test.AddAllHostsLabel(t, ds) @@ -62,10 +64,17 @@ func SetUpMySQLAndRedisAndService(t *testing.T, uniqueTestName string, opts ...* err = ds.SaveAppConfig(testContext(), appConf) require.NoError(t, err) - redisPool := redistest.SetupRedis(t, uniqueTestName, false, false, false) - fleetCfg := config.TestConfig() fleetSvc, ctx := service.NewTestService(t, ds, fleetCfg, opts...) + return ds, fleetCfg, fleetSvc, ctx +} + +func SetUpMySQLAndRedisAndService(t *testing.T, uniqueTestName string, opts ...*service.TestServerOpts) (*mysql.Datastore, fleet.RedisPool, + config.FleetConfig, + fleet.Service, context.Context, +) { + redisPool := redistest.SetupRedis(t, uniqueTestName, false, false, false) + ds, fleetCfg, fleetSvc, ctx := SetUpMySQLAndService(t, uniqueTestName, opts...) return ds, redisPool, fleetCfg, fleetSvc, ctx }