diff --git a/changes/28196-SCIM-for-Entra-ID b/changes/28196-SCIM-for-Entra-ID new file mode 100644 index 0000000000..63f5590cef --- /dev/null +++ b/changes/28196-SCIM-for-Entra-ID @@ -0,0 +1 @@ +Added ability to sync end user's IdP information with Microsoft Entra ID using SCIM protocol. diff --git a/docs/Contributing/SCIM-integration.md b/docs/Contributing/SCIM-integration.md index 48c4994307..6f1d14a7b0 100644 --- a/docs/Contributing/SCIM-integration.md +++ b/docs/Contributing/SCIM-integration.md @@ -51,12 +51,20 @@ Run test using [Runscope](https://www.runscope.com/). See [instructions](https:/ ### Testing Entra ID integration -Use [scimvalidator.microsoft.com](https://scimvalidator.microsoft.com/). Only test the attributes that we have implemented. To see our supported attributes, check the schema: +Use [scimvalidator.microsoft.com](https://scimvalidator.microsoft.com/). Only test the attributes that we have implemented. +![SCIM-Entra-ID-Validator-User-attributes.png](assets/SCIM-Entra-ID-Validator-User-attributes.png) +![SCIM-Entra-ID-Validator-Group-attributes.png](assets/SCIM-Entra-ID-Validator-Group-attributes.png) + +To see our supported attributes, check the schema: ``` GET https://localhost:8080/api/latest/fleet/scim/Schemas ``` +Results (2025/05/06) + +![SCIM-Entra-ID-Validator-results.png](assets/SCIM-Entra-ID-Validator-results.png) + ## Authentication We use same authentication as API. HTTP header: `Authorization: Bearer xyz` diff --git a/docs/Contributing/assets/SCIM-Entra-ID-Validator-Group-attributes.png b/docs/Contributing/assets/SCIM-Entra-ID-Validator-Group-attributes.png new file mode 100644 index 0000000000..a0a52a790b Binary files /dev/null and b/docs/Contributing/assets/SCIM-Entra-ID-Validator-Group-attributes.png differ diff --git a/docs/Contributing/assets/SCIM-Entra-ID-Validator-User-attributes.png b/docs/Contributing/assets/SCIM-Entra-ID-Validator-User-attributes.png new file mode 100644 index 0000000000..9684ceda72 Binary files /dev/null and b/docs/Contributing/assets/SCIM-Entra-ID-Validator-User-attributes.png differ diff --git a/docs/Contributing/assets/SCIM-Entra-ID-Validator-results.png b/docs/Contributing/assets/SCIM-Entra-ID-Validator-results.png new file mode 100644 index 0000000000..e97a54295f Binary files /dev/null and b/docs/Contributing/assets/SCIM-Entra-ID-Validator-results.png differ diff --git a/ee/server/integrationtest/scim/scim_test.go b/ee/server/integrationtest/scim/scim_test.go index 37b54b1f7b..76f3e878f7 100644 --- a/ee/server/integrationtest/scim/scim_test.go +++ b/ee/server/integrationtest/scim/scim_test.go @@ -33,6 +33,8 @@ func TestSCIM(t *testing.T) { {"UpdateGroup", testUpdateGroup}, {"PatchUserEmails", testPatchUserEmails}, {"PatchUserAttributes", testPatchUserAttributes}, + {"PatchGroupAttributes", testPatchGroupAttributes}, + {"PatchGroupMembers", testPatchGroupMembers}, {"UsersPagination", testUsersPagination}, {"GroupsPagination", testGroupsPagination}, {"UsersAndGroups", testUsersAndGroups}, @@ -426,7 +428,6 @@ func testGroupsBasicCRUD(t *testing.T, s *Suite) { member := members[0].(map[string]interface{}) assert.Equal(t, userID, member["value"]) assert.Equal(t, "User", member["type"]) - assert.Equal(t, "Users/"+userID, member["$ref"]) // Test getting a group by ID var getResp map[string]interface{} @@ -455,6 +456,49 @@ func testGroupsBasicCRUD(t *testing.T, s *Suite) { assert.True(t, ok, "Resources should be an array") assert.GreaterOrEqual(t, len(resources), 1, "Should have at least 1 group") + // Create a second group with a different display name for filtering tests + secondGroupID, _ := createTestGroup(t, s, "Second Test Group", []string{userID}) + + // Test filtering groups by displayName - first group + var filterResp1 map[string]interface{} + s.DoJSON(t, "GET", scimPath("/Groups"), nil, http.StatusOK, &filterResp1, "filter", `displayName eq "Test Group"`) + assert.EqualValues(t, filterResp1["schemas"], []interface{}{"urn:ietf:params:scim:api:messages:2.0:ListResponse"}) + filterResources1, ok := filterResp1["Resources"].([]interface{}) + assert.True(t, ok, "Resources should be an array") + assert.Equal(t, 1, len(filterResources1), "Should have exactly 1 group matching the filter") + + // Verify it's the correct group + if len(filterResources1) > 0 { + group, ok := filterResources1[0].(map[string]interface{}) + assert.True(t, ok, "Group should be an object") + assert.Equal(t, groupID, group["id"], "Should be the first group") + assert.Equal(t, "Test Group", group["displayName"], "Should have the correct display name") + } + + // Test filtering groups by displayName - second group + var filterResp2 map[string]interface{} + s.DoJSON(t, "GET", scimPath("/Groups"), nil, http.StatusOK, &filterResp2, "filter", `displayName eq "Second Test Group"`) + assert.EqualValues(t, filterResp2["schemas"], []interface{}{"urn:ietf:params:scim:api:messages:2.0:ListResponse"}) + filterResources2, ok := filterResp2["Resources"].([]interface{}) + assert.True(t, ok, "Resources should be an array") + assert.Equal(t, 1, len(filterResources2), "Should have exactly 1 group matching the filter") + + // Verify it's the correct group + if len(filterResources2) > 0 { + group, ok := filterResources2[0].(map[string]interface{}) + assert.True(t, ok, "Group should be an object") + assert.Equal(t, secondGroupID, group["id"], "Should be the second group") + assert.Equal(t, "Second Test Group", group["displayName"], "Should have the correct display name") + } + + // Test filtering groups by non-existent displayName + var filterResp3 map[string]interface{} + s.DoJSON(t, "GET", scimPath("/Groups"), nil, http.StatusOK, &filterResp3, "filter", `displayName eq "Non-Existent Group"`) + assert.EqualValues(t, filterResp3["schemas"], []interface{}{"urn:ietf:params:scim:api:messages:2.0:ListResponse"}) + filterResources3, ok := filterResp3["Resources"].([]interface{}) + assert.True(t, ok, "Resources should be an array") + assert.Empty(t, filterResources3, "Should have no groups matching the filter") + // Test updating a group updateGroupPayload := map[string]interface{}{ "schemas": []string{"urn:ietf:params:scim:schemas:core:2.0:Group"}, @@ -470,39 +514,6 @@ func testGroupsBasicCRUD(t *testing.T, s *Suite) { _, membersExist := updateResp["members"] assert.False(t, membersExist, "Members should not be present or should be empty") - // Test patching a group (updating just the displayName) - patchGroupPayload := map[string]interface{}{ - "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, - "Operations": []map[string]interface{}{ - { - "op": "replace", - "path": "displayName", - "value": "Patched Test Group", - }, - }, - } - - var patchResp map[string]interface{} - s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchGroupPayload, http.StatusOK, &patchResp) - assert.Equal(t, "Patched Test Group", patchResp["displayName"]) - - // Test patching a group without path attribute (updating just the displayName) - patchGroupPayload = map[string]interface{}{ - "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, - "Operations": []map[string]interface{}{ - { - "op": "replace", - "value": map[string]interface{}{ - "displayName": "Patched Again Test Group", - }, - }, - }, - } - - patchResp = nil - s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchGroupPayload, http.StatusOK, &patchResp) - assert.Equal(t, "Patched Again Test Group", patchResp["displayName"]) - // Test deleting a group s.Do(t, "DELETE", scimPath("/Groups/"+groupID), nil, http.StatusNoContent) @@ -615,7 +626,6 @@ func testCreateGroup(t *testing.T, s *Suite) { assert.True(t, ok, "Member should be an object") memberValues = append(memberValues, memberMap["value"].(string)) assert.Equal(t, "User", memberMap["type"]) - assert.Contains(t, memberMap["$ref"], "Users/") } for _, userID := range userIDs { @@ -1801,6 +1811,317 @@ func testPatchUserEmails(t *testing.T, s *Suite) { assert.True(t, primaryFound, "One email should be marked as primary") }) + t.Run("Add a new email with path specified", func(t *testing.T) { + // First, ensure we have a clean starting state with one email + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Now add a new email with path specified + addEmailPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "emails", + "value": []map[string]interface{}{ + { + "value": "added-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + } + + var addEmailResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addEmailPayload, http.StatusOK, &addEmailResp) + + // Verify both emails are present + emails, ok := addEmailResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + assert.Equal(t, 2, len(emails), "Should have 2 emails after adding one") + + // Check that both the original and new emails are present + emailValues := make([]string, 0, 2) + emailTypes := make(map[string]string) + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + emailValue := email["value"].(string) + emailValues = append(emailValues, emailValue) + emailTypes[emailValue] = email["type"].(string) + } + + assert.Contains(t, emailValues, "work-email@example.com", "Original work email should still be present") + assert.Contains(t, emailValues, "added-email@example.com", "Added home email should be present") + assert.Equal(t, "work", emailTypes["work-email@example.com"], "Original email should be of type work") + assert.Equal(t, "home", emailTypes["added-email@example.com"], "Added email should be of type home") + }) + + t.Run("Add a new email without path specified", func(t *testing.T) { + // First, ensure we have a clean starting state with one email + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Now add a new email without path specified (should add to the resource) + addEmailPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "added-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + }, + } + + var addEmailResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addEmailPayload, http.StatusOK, &addEmailResp) + + // Verify both emails are present + emails, ok := addEmailResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + assert.Equal(t, 2, len(emails), "Should have 2 emails after adding one") + + // Check that both the original and new emails are present + emailValues := make([]string, 0, 2) + emailTypes := make(map[string]string) + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + emailValue := email["value"].(string) + emailValues = append(emailValues, emailValue) + emailTypes[emailValue] = email["type"].(string) + } + + assert.Contains(t, emailValues, "work-email@example.com", "Original work email should still be present") + assert.Contains(t, emailValues, "added-email@example.com", "Added home email should be present") + assert.Equal(t, "work", emailTypes["work-email@example.com"], "Original email should be of type work") + assert.Equal(t, "home", emailTypes["added-email@example.com"], "Added email should be of type home") + }) + + t.Run("Remove an email by type filter", func(t *testing.T) { + // First, ensure we have both work and home emails + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + { + "value": "home-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Delete the home email using a type filter + deleteEmailPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": `emails[type eq "home"]`, + }, + }, + } + + var deleteEmailResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), deleteEmailPayload, http.StatusOK, &deleteEmailResp) + + // Verify only work email remains + emails, ok := deleteEmailResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + assert.Equal(t, 1, len(emails), "Should have only 1 email after deleting home email") + + // Check that only the work email remains + email, ok := emails[0].(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + assert.Equal(t, "work-email@example.com", email["value"], "Work email should remain") + assert.Equal(t, "work", email["type"], "Remaining email should be of type work") + assert.Equal(t, true, email["primary"], "Work email should be primary") + }) + + t.Run("Remove all emails", func(t *testing.T) { + // First, ensure we have both work and home emails + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + { + "value": "home-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Delete all emails by removing the entire emails attribute + deleteAllEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "emails", + }, + }, + } + + var deleteAllEmailsResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), deleteAllEmailsPayload, http.StatusOK, &deleteAllEmailsResp) + + // Verify emails attribute is not present or is empty + _, hasEmails := deleteAllEmailsResp["emails"] + assert.False(t, hasEmails, "Emails attribute should be removed after deleting all emails") + }) + + t.Run("Combined add and remove operations in a single request", func(t *testing.T) { + // First, ensure we have both work and home emails + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + { + "value": "home-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Perform combined operations: remove home email and add other email + combinedOpsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": `emails[type eq "home"]`, + }, + { + "op": "add", + "path": "emails", + "value": []map[string]interface{}{ + { + "value": "other-email@example.com", + "type": "other", + "primary": false, + }, + }, + }, + }, + } + + var combinedOpsResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), combinedOpsPayload, http.StatusOK, &combinedOpsResp) + + // Verify we have two emails: work and other (home was removed) + emails, ok := combinedOpsResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + assert.Equal(t, 2, len(emails), "Should have 2 emails after combined operations") + + // Check that the work email remains and other email was added + emailValues := make([]string, 0, 2) + emailTypes := make(map[string]string) + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + emailValue := email["value"].(string) + emailValues = append(emailValues, emailValue) + emailTypes[emailValue] = email["type"].(string) + } + + assert.Contains(t, emailValues, "work-email@example.com", "Work email should remain") + assert.Contains(t, emailValues, "other-email@example.com", "Other email should be added") + assert.NotContains(t, emailValues, "home-email@example.com", "Home email should be removed") + assert.Equal(t, "work", emailTypes["work-email@example.com"], "Work email should be of type work") + assert.Equal(t, "other", emailTypes["other-email@example.com"], "Other email should be of type other") + }) + t.Run("Patch emails by type filter", func(t *testing.T) { // First, ensure we have both work and home emails setupEmailsPayload := map[string]interface{}{ @@ -2058,6 +2379,164 @@ func testPatchUserEmails(t *testing.T, s *Suite) { assert.Equal(t, false, otherEmail["primary"], "Other email primary flag should remain false") }) + t.Run("Add individual email attribute", func(t *testing.T) { + // First, ensure we have a work email with known values + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Add a new email with just the value attribute first, because it is required + addEmailTypePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": `emails[type eq "home"]`, + "value": []map[string]interface{}{ + { + "value": "home-email@example.com", + }, + }, + }, + }, + } + + var addEmailTypeResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addEmailTypePayload, http.StatusOK, &addEmailTypeResp) + + // Now modify the type attribute of the email + addEmailValuePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": `emails[type eq "home"].type`, + "value": "other", + }, + }, + } + + var addEmailValueResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addEmailValuePayload, http.StatusOK, &addEmailValueResp) + + // Finally add the primary attribute to the email + addEmailPrimaryPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": `emails[type eq "other"].primary`, + "value": true, + }, + }, + } + + var addEmailPrimaryResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addEmailPrimaryPayload, http.StatusOK, &addEmailPrimaryResp) + + // Verify both emails are present and home email is now primary + emails, ok := addEmailPrimaryResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + assert.Equal(t, 2, len(emails), "Should have 2 emails") + + var workEmail, otherEmail map[string]interface{} + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + if email["type"] == "work" { + workEmail = email + } else if email["type"] == "other" { + otherEmail = email + } + } + + require.NotNil(t, workEmail, "Work email should exist") + require.NotNil(t, otherEmail, "Other email should exist") + + assert.Equal(t, "work-email@example.com", workEmail["value"], "Work email value should be correct") + assert.Equal(t, "home-email@example.com", otherEmail["value"], "Other email value should be correct") + assert.Equal(t, false, workEmail["primary"], "Work email should no longer be primary") + assert.Equal(t, true, otherEmail["primary"], "Other email should now be primary") + }) + + t.Run("Remove individual email attribute", func(t *testing.T) { + // First, ensure we have both work and home emails with known values + setupEmailsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "emails": []map[string]interface{}{ + { + "value": "work-email@example.com", + "type": "work", + "primary": true, + }, + { + "value": "home-email@example.com", + "type": "home", + "primary": false, + }, + }, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupEmailsPayload, http.StatusOK, &setupResp) + + // Remove the primary attribute from the work email + removePrimaryPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": `emails[type eq "work"].primary`, + }, + }, + } + + var removePrimaryResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), removePrimaryPayload, http.StatusOK, &removePrimaryResp) + + // Verify the primary attribute was removed + emails, ok := removePrimaryResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + + var workEmail map[string]interface{} + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + if email["type"] == "work" { + workEmail = email + break + } + } + + require.NotNil(t, workEmail, "Work email should exist") + _, hasPrimary := workEmail["primary"] + assert.False(t, hasPrimary, "Work email should not have primary attribute") + }) + // Test failure cases using table-driven tests t.Run("Email validation failure cases", func(t *testing.T) { // Define test cases @@ -2317,6 +2796,52 @@ func testPatchUserEmails(t *testing.T, s *Suite) { }, errorMessage: "A required value was missing", }, + { + name: "Add operation with invalid email path", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + // We only support type as a subattribute filter + "path": `emails[primary eq "true"]`, + "value": map[string]interface{}{ + "value": "new-email@example.com", + "type": "work", + "primary": true, + }, + }, + }, + }, + errorMessage: "Bad Request", + }, + { + name: "Add operation with missing values", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "emails", + "value": []map[string]interface{}{}, + }, + }, + }, + errorMessage: "Bad Request", + }, + { + name: "Remove operation with invalid email path", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": `emails[type eq "nonexistent"]`, + }, + }, + }, + errorMessage: "Bad Request", + }, } // Run each test case @@ -2520,6 +3045,236 @@ func testPatchUserAttributes(t *testing.T, s *Suite) { assert.Equal(t, false, patchActiveWithPathResp["active"], "active should be updated to false with explicit path") }) + t.Run("Add a new attribute", func(t *testing.T) { + // Add externalId attribute + addExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "externalId", + "value": "external-id-123", + }, + }, + } + + var addExternalIdResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addExternalIdPayload, http.StatusOK, &addExternalIdResp) + assert.Equal(t, "external-id-123", addExternalIdResp["externalId"], "externalId should be added") + }) + + t.Run("Delete an attribute", func(t *testing.T) { + // First, ensure the user has an externalId + setupExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "externalId": "external-id-to-delete", + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupExternalIdPayload, http.StatusOK, &setupResp) + assert.Equal(t, "external-id-to-delete", setupResp["externalId"], "externalId should be set") + + // Now delete the externalId + deleteExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "externalId", + }, + }, + } + + var deleteExternalIdResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), deleteExternalIdPayload, http.StatusOK, &deleteExternalIdResp) + _, hasExternalId := deleteExternalIdResp["externalId"] + assert.False(t, hasExternalId, "externalId should be deleted") + }) + + t.Run("Add a new attribute without path specified", func(t *testing.T) { + // First, ensure externalId is not present + removeExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "externalId", + }, + }, + } + + var removeResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), removeExternalIdPayload, http.StatusOK, &removeResp) + + // Add externalId attribute without path specified + addExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "value": map[string]interface{}{ + "externalId": "external-id-no-path", + }, + }, + }, + } + + var addExternalIdResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addExternalIdPayload, http.StatusOK, &addExternalIdResp) + assert.Equal(t, "external-id-no-path", addExternalIdResp["externalId"], "externalId should be added without path specified") + }) + + t.Run("Combined add and remove operations for attributes", func(t *testing.T) { + // Setup initial state with externalId and active attributes + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "externalId": "initial-external-id", + "active": true, + }, + }, + }, + } + + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), setupPayload, http.StatusOK, &setupResp) + assert.Equal(t, "initial-external-id", setupResp["externalId"], "externalId should be set") + assert.Equal(t, true, setupResp["active"], "active should be set to true") + + // Perform combined operations: remove externalId and add a new email + combinedOpsPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "externalId", + }, + { + "op": "add", + "path": "emails", + "value": []map[string]interface{}{ + { + "value": "new-combined-email@example.com", + "type": "work", + "primary": true, + }, + }, + }, + }, + } + + var combinedOpsResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), combinedOpsPayload, http.StatusOK, &combinedOpsResp) + + // Verify externalId is removed and new email is added + _, hasExternalId := combinedOpsResp["externalId"] + assert.False(t, hasExternalId, "externalId should be removed") + + emails, ok := combinedOpsResp["emails"].([]interface{}) + require.True(t, ok, "Response should have emails array") + + // Find the new email + var foundNewEmail bool + for _, e := range emails { + email, ok := e.(map[string]interface{}) + assert.True(t, ok, "Email should be an object") + if email["value"] == "new-combined-email@example.com" { + foundNewEmail = true + assert.Equal(t, "work", email["type"], "Email type should be work") + assert.Equal(t, true, email["primary"], "Email should be primary") + } + } + assert.True(t, foundNewEmail, "New email should be added") + }) + + t.Run("Add userName attribute", func(t *testing.T) { + // Add userName attribute + addUserNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "userName", + "value": "new-username@example.com", + }, + }, + } + + var addUserNameResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addUserNamePayload, http.StatusOK, &addUserNameResp) + assert.Equal(t, "new-username@example.com", addUserNameResp["userName"], "userName should be updated") + }) + + t.Run("Add name attributes", func(t *testing.T) { + // Add givenName attribute + addGivenNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "name.givenName", + "value": "NewGiven", + }, + }, + } + + var addGivenNameResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addGivenNamePayload, http.StatusOK, &addGivenNameResp) + name, ok := addGivenNameResp["name"].(map[string]interface{}) + require.True(t, ok, "Response should have name object") + assert.Equal(t, "NewGiven", name["givenName"], "givenName should be updated") + + // Add familyName attribute + addFamilyNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "name.familyName", + "value": "NewFamily", + }, + }, + } + + var addFamilyNameResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addFamilyNamePayload, http.StatusOK, &addFamilyNameResp) + name, ok = addFamilyNameResp["name"].(map[string]interface{}) + require.True(t, ok, "Response should have name object") + assert.Equal(t, "NewFamily", name["familyName"], "familyName should be updated") + + // Add entire name object + addNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "name", + "value": map[string]interface{}{ + "givenName": "CompletelyNew", + "familyName": "FullName", + }, + }, + }, + } + + var addNameResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Users/"+userID), addNamePayload, http.StatusOK, &addNameResp) + name, ok = addNameResp["name"].(map[string]interface{}) + require.True(t, ok, "Response should have name object") + assert.Equal(t, "CompletelyNew", name["givenName"], "givenName should be updated") + assert.Equal(t, "FullName", name["familyName"], "familyName should be updated") + }) + // Failure tests using table-driven approach t.Run("Failure cases", func(t *testing.T) { testCases := []struct { @@ -2642,18 +3397,18 @@ func testPatchUserAttributes(t *testing.T, s *Suite) { errorMessage: errors.ScimErrorInvalidValue.Detail, }, { - name: "unsupported operation (add instead of replace)", + name: "unsupported operation", payload: map[string]interface{}{ "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, "Operations": []map[string]interface{}{ { - "op": "add", // Only "replace" is supported + "op": "bad", "path": "active", "value": false, }, }, }, - errorMessage: "Bad Request", + errorMessage: errors.ScimErrorInvalidValue.Detail, }, { name: "no path and invalid value format", @@ -2683,6 +3438,99 @@ func testPatchUserAttributes(t *testing.T, s *Suite) { }, errorMessage: "A required value was missing", }, + { + name: "Add operation with invalid path", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "nonExistentAttribute", + "value": "some value", + }, + }, + }, + errorMessage: `The "path" attribute was invalid or malformed.`, + }, + { + name: "Add operation without value", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "externalId", + // Missing value + }, + }, + }, + errorMessage: "A required value was missing", + }, + { + name: "Remove operation without path", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + // Missing path + }, + }, + }, + errorMessage: "A required value was missing", + }, + { + name: "Remove required attribute - userName", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "userName", + }, + }, + }, + errorMessage: "Bad Request", + }, + { + name: "Remove required attribute - givenName", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "name.givenName", + }, + }, + }, + errorMessage: "Bad Request", + }, + { + name: "Remove required attribute - familyName", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "name.familyName", + }, + }, + }, + errorMessage: "Bad Request", + }, + { + name: "Remove required attribute - entire name object", + payload: map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "name", + }, + }, + }, + errorMessage: "Bad Request", + }, } for _, tc := range testCases { @@ -2697,6 +3545,646 @@ func testPatchUserAttributes(t *testing.T, s *Suite) { }) } +func testPatchGroupAttributes(t *testing.T, s *Suite) { + // Create a test user to be added as a member of the group + userID, _ := createTestUser(t, s, "group-patch-test@example.com") + + // Create a test group + createGroupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:schemas:core:2.0:Group"}, + "displayName": "Original Group Name", + "externalId": "original-external-id", + "members": []map[string]interface{}{ + { + "value": userID, + }, + }, + } + + var createResp map[string]interface{} + s.DoJSON(t, "POST", scimPath("/Groups"), createGroupPayload, http.StatusCreated, &createResp) + groupID := createResp["id"].(string) + + t.Run("Replace displayName with explicit path", func(t *testing.T) { + patchDisplayNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "displayName", + "value": "Updated Group Name", + }, + }, + } + + var patchResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchDisplayNamePayload, http.StatusOK, &patchResp) + assert.Equal(t, "Updated Group Name", patchResp["displayName"], "displayName should be updated") + }) + + t.Run("Replace externalId with explicit path", func(t *testing.T) { + patchExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "externalId", + "value": "updated-external-id", + }, + }, + } + + var patchResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchExternalIdPayload, http.StatusOK, &patchResp) + assert.Equal(t, "updated-external-id", patchResp["externalId"], "externalId should be updated") + }) + + t.Run("Remove and add operation for externalId", func(t *testing.T) { + // First, remove the externalId + removeExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "externalId", + }, + }, + } + + var removeResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), removeExternalIdPayload, http.StatusOK, &removeResp) + _, hasExternalId := removeResp["externalId"] + assert.False(t, hasExternalId, "externalId should be removed") + + // Now add a new externalId + addExternalIdPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "externalId", + "value": "added-external-id", + }, + }, + } + + var addResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), addExternalIdPayload, http.StatusOK, &addResp) + assert.Equal(t, "added-external-id", addResp["externalId"], "externalId should be added") + }) + + t.Run("Multiple operations in one request", func(t *testing.T) { + multiOperationPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "externalId", + "value": "multi-op-external-id", + }, + { + "op": "replace", + "path": "displayName", + "value": "Multi-Op Group Name", + }, + }, + } + + var multiOpResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), multiOperationPayload, http.StatusOK, &multiOpResp) + assert.Equal(t, "multi-op-external-id", multiOpResp["externalId"], "externalId should be added") + assert.Equal(t, "Multi-Op Group Name", multiOpResp["displayName"], "displayName should be updated") + }) + + t.Run("Operations without path attribute", func(t *testing.T) { + noPathPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "value": map[string]interface{}{ + "displayName": "No-Path Group Name", + "externalId": "no-path-external-id", + }, + }, + }, + } + + var noPathResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), noPathPayload, http.StatusOK, &noPathResp) + assert.Equal(t, "no-path-external-id", noPathResp["externalId"], "externalId should be updated") + assert.Equal(t, "No-Path Group Name", noPathResp["displayName"], "displayName should be updated") + }) + + t.Run("Attempt to remove required displayName attribute", func(t *testing.T) { + removeDisplayNamePayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "displayName", + }, + }, + } + + var errorResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), removeDisplayNamePayload, http.StatusBadRequest, &errorResp) + assert.Contains(t, errorResp["detail"], "Bad Request", "Should return error for removing required attribute") + }) + + t.Run("Add members to group", func(t *testing.T) { + // First, ensure the group has a known state with just the original user + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userID, + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have 1 member after setup") + + // Now add the second user to the group + secondUserID, _ := createTestUser(t, s, "second-group-patch-test@example.com") + addMembersPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "members", + "value": []map[string]interface{}{ + { + "value": secondUserID, + }, + }, + }, + }, + } + + var addResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), addMembersPayload, http.StatusOK, &addResp) + + // Verify both users are now in the group + members, ok = addResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 2, len(members), "Group should have 2 members after adding one") + + // Check that both users are present + memberValues := make([]string, 0, 2) + for _, m := range members { + member, ok := m.(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + memberValues = append(memberValues, member["value"].(string)) + } + assert.Contains(t, memberValues, userID, "Original user should still be in the group") + assert.Contains(t, memberValues, secondUserID, "Second user should be added to the group") + }) + + t.Run("Replace members in group", func(t *testing.T) { + // First, ensure the group has a known state with multiple members + setupUserID, _ := createTestUser(t, s, "setup-user@example.com") + defer s.Do(t, "DELETE", scimPath("/Users/"+setupUserID), nil, http.StatusNoContent) + + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userID, + }, + { + "value": setupUserID, + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 2, len(members), "Group should have 2 members after setup") + + // Now replace all members with just the new user + newUserID, _ := createTestUser(t, s, "replace-members-test@example.com") + replaceMembersPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": newUserID, + }, + }, + }, + }, + } + + var replaceResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), replaceMembersPayload, http.StatusOK, &replaceResp) + + // Verify only the new user is in the group + members, ok = replaceResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have only 1 member after replacing") + + // Check that only the new user is present + member, ok := members[0].(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + assert.Equal(t, newUserID, member["value"], "New user should be the only member") + }) + + t.Run("Remove all members from group", func(t *testing.T) { + // First, ensure the group has members to remove + setupUserID, _ := createTestUser(t, s, "remove-test-user@example.com") + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userID, + }, + { + "value": setupUserID, + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 2, len(members), "Group should have 2 members after setup") + + // Now remove all members + removeMembersPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "members", + }, + }, + } + + var removeResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), removeMembersPayload, http.StatusOK, &removeResp) + + // Verify no members are in the group + _, hasMembers := removeResp["members"] + assert.False(t, hasMembers, "Group should have no members after removing all") + }) + + t.Run("Add members without path attribute", func(t *testing.T) { + // First, ensure the group has no members + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": "members", + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + _, hasMembers := setupResp["members"] + assert.False(t, hasMembers, "Group should have no members after setup") + + // Now add a member without specifying path + addMembersNoPathPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "value": map[string]interface{}{ + "members": []map[string]interface{}{ + { + "value": userID, + }, + }, + }, + }, + }, + } + + var addNoPathResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), addMembersNoPathPayload, http.StatusOK, &addNoPathResp) + + // Verify the user is now in the group + var members []interface{} + var ok bool + members, ok = addNoPathResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have 1 member after adding") + + // Check that the user is present + member, ok := members[0].(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + assert.Equal(t, userID, member["value"], "Original user should be added back to the group") + }) + + t.Run("Invalid member ID format", func(t *testing.T) { + // Try to add a member with invalid ID format + invalidMemberPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "members", + "value": []map[string]interface{}{ + { + "value": "invalid-user-id", + }, + }, + }, + }, + } + + var errorResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), invalidMemberPayload, http.StatusBadRequest, &errorResp) + assert.Contains(t, errorResp["detail"], "Bad Request", "Should return error for invalid member ID format") + }) + + t.Run("Non-existent member ID", func(t *testing.T) { + // Try to add a member with non-existent ID + nonExistentMemberPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": "members", + "value": []map[string]interface{}{ + { + "value": "4294967295", // Non-existent user ID + }, + }, + }, + }, + } + + var errorResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), nonExistentMemberPayload, http.StatusBadRequest, &errorResp) + assert.Contains(t, errorResp["detail"], "Bad Request", "Should return error for non-existent member ID") + }) +} + +func testPatchGroupMembers(t *testing.T, s *Suite) { + // Create test users to be added as members + userIDs := make([]string, 0, 3) + for i := 1; i <= 3; i++ { + userName := fmt.Sprintf("group-patch-test-user-%d@example.com", i) + userID, _ := createTestUser(t, s, userName) + userIDs = append(userIDs, userID) + } + + // Create a group with the first user as a member + createGroupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:schemas:core:2.0:Group"}, + "displayName": "Patch Members Test Group", + "members": []map[string]interface{}{ + { + "value": userIDs[0], + }, + }, + } + + var createResp map[string]interface{} + s.DoJSON(t, "POST", scimPath("/Groups"), createGroupPayload, http.StatusCreated, &createResp) + groupID := createResp["id"].(string) + + t.Run("Add a member using path filtering", func(t *testing.T) { + // Setup: Ensure the group has only the first user + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userIDs[0], + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have 1 member after setup") + + // Test: Add the second user to the group using path filtering + patchAddMemberPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "add", + "path": fmt.Sprintf(`members[value eq "%s"]`, userIDs[1]), + "value": map[string]interface{}{}, + }, + }, + } + + var patchResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchAddMemberPayload, http.StatusOK, &patchResp) + + // Verify the member was added + members, ok = patchResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 2, len(members), "Group should now have 2 members") + + // Check that both users are in the members list + memberValues := make([]string, 0, 2) + for _, m := range members { + member, ok := m.(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + memberValues = append(memberValues, member["value"].(string)) + } + assert.Contains(t, memberValues, userIDs[0], "First user should still be a member") + assert.Contains(t, memberValues, userIDs[1], "Second user should be added as a member") + }) + + t.Run("Remove a member using path filtering", func(t *testing.T) { + // Setup: Ensure the group has both first and second users + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userIDs[0], + }, + { + "value": userIDs[1], + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 2, len(members), "Group should have 2 members after setup") + + // Test: Remove the first user from the group using path filtering + patchRemoveMemberPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": fmt.Sprintf(`members[value eq "%s"]`, userIDs[0]), + }, + }, + } + + var patchResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchRemoveMemberPayload, http.StatusOK, &patchResp) + + // Verify the member was removed + members, ok = patchResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should now have 1 member") + + // Check that only the second user is in the members list + member, ok := members[0].(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + assert.Equal(t, userIDs[1], member["value"], "Only the second user should remain as a member") + }) + + t.Run("Replace a member using path filtering", func(t *testing.T) { + // Setup: Ensure the group has only the second user + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userIDs[1], + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have 1 member after setup") + + // Test: Replace the second user with the third user using path filtering + patchReplaceMemberPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": fmt.Sprintf(`members[value eq "%s"]`, userIDs[1]), + }, + { + "op": "add", + "path": fmt.Sprintf(`members[value eq "%s"]`, userIDs[2]), + "value": map[string]interface{}{}, + }, + }, + } + + var patchResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchReplaceMemberPayload, http.StatusOK, &patchResp) + + // Verify the member was replaced + members, ok = patchResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should still have 1 member") + + // Check that only the third user is in the members list + member, ok := members[0].(map[string]interface{}) + assert.True(t, ok, "Member should be an object") + assert.Equal(t, userIDs[2], member["value"], "Only the third user should be a member") + }) + + t.Run("Try to remove a non-existent member", func(t *testing.T) { + // Setup: Ensure the group has only the third user + setupPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "replace", + "path": "members", + "value": []map[string]interface{}{ + { + "value": userIDs[2], + }, + }, + }, + }, + } + var setupResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), setupPayload, http.StatusOK, &setupResp) + + // Verify setup was successful + members, ok := setupResp["members"].([]interface{}) + require.True(t, ok, "Response should have members array") + assert.Equal(t, 1, len(members), "Group should have 1 member after setup") + + // Test: Try to remove a user that is not a member + patchRemoveNonExistentPayload := map[string]interface{}{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:PatchOp"}, + "Operations": []map[string]interface{}{ + { + "op": "remove", + "path": fmt.Sprintf(`members[value eq "%s"]`, userIDs[0]), + }, + }, + } + + var errorResp map[string]interface{} + s.DoJSON(t, "PATCH", scimPath("/Groups/"+groupID), patchRemoveNonExistentPayload, http.StatusBadRequest, &errorResp) + + // Verify the error response + assert.EqualValues(t, errorResp["schemas"], []interface{}{"urn:ietf:params:scim:api:messages:2.0:Error"}) + assert.Contains(t, errorResp["detail"], "Bad Request") + }) +} + func scimPath(suffix string) string { paths := []string{"/api/v1/fleet/scim", "/api/latest/fleet/scim"} prefix := paths[time.Now().UnixNano()%int64(len(paths))] diff --git a/ee/server/scim/groups.go b/ee/server/scim/groups.go index c23739cc81..ebbfd724a6 100644 --- a/ee/server/scim/groups.go +++ b/ee/server/scim/groups.go @@ -1,8 +1,10 @@ package scim import ( + "context" "fmt" "net/http" + "net/url" "strconv" "strings" @@ -12,6 +14,7 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" kitlog "github.com/go-kit/log" "github.com/go-kit/log/level" + "github.com/scim2/filter-parser/v2" ) const ( @@ -145,7 +148,6 @@ func createGroupResource(group *fleet.ScimGroup) scim.Resource { for _, userID := range group.ScimUsers { members = append(members, map[string]interface{}{ "value": scimUserID(userID), - "$ref": "Users/" + scimUserID(userID), "type": "User", }) } @@ -170,15 +172,37 @@ func (g *GroupHandler) GetAll(r *http.Request, params scim.ListRequestParams) (s count = maxResults } - opts := fleet.ScimListOptions{ - StartIndex: uint(startIndex), // nolint:gosec // ignore G115 - PerPage: uint(count), // nolint:gosec // ignore G115 + opts := fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: uint(startIndex), // nolint:gosec // ignore G115 + PerPage: uint(count), // nolint:gosec // ignore G115 + }, } resourceFilter := r.URL.Query().Get("filter") if resourceFilter != "" { - level.Info(g.logger).Log("msg", "group filter not supported", "filter", resourceFilter) - return scim.Page{}, nil + expr, err := filter.ParseAttrExp([]byte(resourceFilter)) + if err != nil { + level.Error(g.logger).Log("msg", "failed to parse filter", "filter", resourceFilter, "err", err) + return scim.Page{}, errors.ScimErrorInvalidFilter + } + if !strings.EqualFold(expr.AttributePath.String(), "displayName") || expr.Operator != "eq" { + level.Info(g.logger).Log("msg", "unsupported filter", "filter", resourceFilter) + return scim.Page{}, nil + } + displayName, ok := expr.CompareValue.(string) + if !ok { + level.Error(g.logger).Log("msg", "unsupported value", "value", expr.CompareValue) + return scim.Page{}, nil + } + + // Decode URL-encoded characters + displayName, err = url.QueryUnescape(displayName) + if err != nil { + level.Error(g.logger).Log("msg", "failed to decode displayName", "displayName", displayName, "err", err) + return scim.Page{}, nil + } + opts.DisplayNameFilter = &displayName } groups, totalResults, err := g.ds.ListScimGroups(r.Context(), opts) @@ -256,8 +280,7 @@ func (g *GroupHandler) Delete(r *http.Request, id string) error { } // Patch -// Only supporting replacing the "displayName" attribute. -// Note: Okta does not use PATCH endpoint to update groups (2025/04/01) +// Supporting add/replace/remove operations for "displayName", "externalId", and "members" attributes. func (g *GroupHandler) Patch(r *http.Request, id string, operations []scim.PatchOperation) (scim.Resource, error) { idUint, err := extractGroupIDFromValue(id) if err != nil { @@ -275,53 +298,312 @@ func (g *GroupHandler) Patch(r *http.Request, id string, operations []scim.Patch } for _, op := range operations { - if op.Op != "replace" { + if op.Op != scim.PatchOperationAdd && op.Op != scim.PatchOperationReplace && op.Op != scim.PatchOperationRemove { level.Info(g.logger).Log("msg", "unsupported patch operation", "op", op.Op) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } switch { case op.Path == nil: + if op.Op == scim.PatchOperationRemove { + level.Info(g.logger).Log("msg", "the 'path' attribute is REQUIRED for 'remove' operations", "op", op.Op) + return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } newValues, ok := op.Value.(map[string]interface{}) if !ok { level.Info(g.logger).Log("msg", "unsupported patch value", "value", op.Value) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } - if len(newValues) != 1 { - level.Info(g.logger).Log("msg", "too many patch values", "value", op.Value) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + for k, v := range newValues { + switch k { + case externalIdAttr: + err = g.patchExternalId(op.Op, v, group) + if err != nil { + return scim.Resource{}, err + } + case displayNameAttr: + err = g.patchDisplayName(op.Op, v, group) + if err != nil { + return scim.Resource{}, err + } + case membersAttr: + err = g.patchMembers(r.Context(), op.Op, v, group) + if err != nil { + return scim.Resource{}, err + } + default: + level.Info(g.logger).Log("msg", "unsupported patch value field", "field", k) + return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } } - displayName, err := getRequiredResource[string](newValues, displayNameAttr) + case op.Path.String() == externalIdAttr: + err = g.patchExternalId(op.Op, op.Value, group) if err != nil { - level.Info(g.logger).Log("msg", "failed to get active value", "value", op.Value) return scim.Resource{}, err } - group.DisplayName = displayName case op.Path.String() == displayNameAttr: - displayName, ok := op.Value.(string) - if !ok { - level.Error(g.logger).Log("msg", "unsupported 'displayName' patch value", "value", op.Value) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + err = g.patchDisplayName(op.Op, op.Value, group) + if err != nil { + return scim.Resource{}, err + } + case op.Path.String() == membersAttr: + err = g.patchMembers(r.Context(), op.Op, op.Value, group) + if err != nil { + return scim.Resource{}, err + } + case op.Path.AttributePath.String() == membersAttr: + err = g.patchMembersWithPathFiltering(r.Context(), op, group) + if err != nil { + return scim.Resource{}, err } - group.DisplayName = displayName default: level.Info(g.logger).Log("msg", "unsupported patch path", "path", op.Path) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } } - err = g.ds.ReplaceScimGroup(r.Context(), group) - switch { - case fleet.IsNotFound(err): - level.Info(g.logger).Log("msg", "failed to find group to patch", "id", id) - return scim.Resource{}, errors.ScimErrorResourceNotFound(id) - case err != nil: - level.Error(g.logger).Log("msg", "failed to patch group", "id", id, "err", err) - return scim.Resource{}, err + if len(operations) != 0 { + err = g.ds.ReplaceScimGroup(r.Context(), group) + switch { + case fleet.IsNotFound(err): + level.Info(g.logger).Log("msg", "failed to find group to patch", "id", id) + return scim.Resource{}, errors.ScimErrorResourceNotFound(id) + case err != nil: + level.Error(g.logger).Log("msg", "failed to patch group", "id", id, "err", err) + return scim.Resource{}, err + } } return createGroupResource(group), nil } +func (g *GroupHandler) patchExternalId(op string, v interface{}, group *fleet.ScimGroup) error { + if op == scim.PatchOperationRemove || v == nil { + group.ExternalID = nil + return nil + } + externalId, ok := v.(string) + if !ok { + level.Info(g.logger).Log("msg", fmt.Sprintf("unsupported '%s' value", externalIdAttr), "value", v) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", v)}) + } + group.ExternalID = &externalId + return nil +} + +func (g *GroupHandler) patchDisplayName(op string, v interface{}, group *fleet.ScimGroup) error { + if op == scim.PatchOperationRemove { + level.Info(g.logger).Log("msg", "cannot remove required attribute", "attribute", displayNameAttr) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + displayName, ok := v.(string) + if !ok { + level.Info(g.logger).Log("msg", fmt.Sprintf("unsupported '%s' value", displayNameAttr), "value", v) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", v)}) + } + if displayName == "" { + level.Info(g.logger).Log("msg", fmt.Sprintf("'%s' cannot be empty", displayNameAttr), "value", v) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", v)}) + } + group.DisplayName = displayName + return nil +} + +// patchMembers handles add/replace/remove operations for the members attribute +func (g *GroupHandler) patchMembers(ctx context.Context, op string, v interface{}, group *fleet.ScimGroup) error { + if op == scim.PatchOperationRemove { + // Remove all members + group.ScimUsers = []uint{} + return nil + } + + // For add and replace operations, we need to extract the member IDs + var membersList []interface{} + + // Handle different value formats + switch val := v.(type) { + case []interface{}: + // Direct array of members + membersList = val + case map[string]interface{}: + // Single member as a map + membersList = []interface{}{val} + case []map[string]interface{}: + // Array of member maps + for _, m := range val { + membersList = append(membersList, m) + } + default: + level.Info(g.logger).Log("msg", "unsupported members value format", "value", v) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", v)}) + } + + // Process the members + userIDs := make([]uint, 0, len(membersList)) + valueStrings := make([]string, 0, len(membersList)) + + for _, memberIntf := range membersList { + member, ok := memberIntf.(map[string]interface{}) + if !ok { + level.Info(g.logger).Log("msg", "member must be an object", "member", memberIntf) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", memberIntf)}) + } + + // Get the value attribute which contains the user ID + valueIntf, ok := member["value"] + if !ok || valueIntf == nil { + level.Info(g.logger).Log("msg", "member missing value attribute", "member", member) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", member)}) + } + + valueStr, ok := valueIntf.(string) + if !ok { + level.Info(g.logger).Log("msg", "member value must be a string", "value", valueIntf) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", valueIntf)}) + } + valueStrings = append(valueStrings, valueStr) + + // Extract user ID from the value + userID, err := extractUserIDFromValue(valueStr) + if err != nil { + level.Info(g.logger).Log("msg", "invalid user ID format", "value", valueStr, "err", err) + return errors.ScimErrorBadParams([]string{valueStr}) + } + + userIDs = append(userIDs, userID) + } + + // Verify all users exist in a single database call + if len(userIDs) > 0 { + allExist, err := g.ds.ScimUsersExist(ctx, userIDs) + if err != nil { + level.Error(g.logger).Log("msg", "error checking users existence", "err", err) + return err + } + if !allExist { + level.Info(g.logger).Log("msg", "one or more users not found", "userIDs", userIDs) + return errors.ScimErrorBadParams(valueStrings) + } + } + + // For add operation, append to existing members + if op == scim.PatchOperationAdd { + // Create a map to track existing user IDs to avoid duplicates + existingUsers := make(map[uint]bool) + for _, id := range group.ScimUsers { + existingUsers[id] = true + } + + // Add new users that don't already exist in the group + for _, id := range userIDs { + if !existingUsers[id] { + group.ScimUsers = append(group.ScimUsers, id) + existingUsers[id] = true + } + } + } else { + // For replace operation, replace all members + group.ScimUsers = userIDs + } + + return nil +} + +// patchMembersWithPathFiltering handles patch operations with path filtering for members +// This supports paths like members[value eq "422"] for add/replace/remove operations +func (g *GroupHandler) patchMembersWithPathFiltering(ctx context.Context, op scim.PatchOperation, group *fleet.ScimGroup) error { + memberID, err := g.getMemberID(op) + if err != nil { + return err + } + + // Check if the member exists in the group + memberFound := false + var memberIndex int + for i, id := range group.ScimUsers { + if id == memberID { + memberIndex = i + memberFound = true + break + } + } + + // For remove operations, remove the member if found + if op.Op == scim.PatchOperationRemove { + if !memberFound { + level.Info(g.logger).Log("msg", "member not found", "member_id", memberID, "op", fmt.Sprintf("%v", op)) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + group.ScimUsers = append(group.ScimUsers[:memberIndex], group.ScimUsers[memberIndex+1:]...) + return nil + } + + // For add operations, add the member if not found + if op.Op == scim.PatchOperationAdd && !memberFound { + // Verify the user exists + userExists, err := g.ds.ScimUsersExist(ctx, []uint{memberID}) + if err != nil { + level.Error(g.logger).Log("msg", "error checking user existence", "err", err) + return err + } + if !userExists { + level.Info(g.logger).Log("msg", "user not found", "user_id", memberID) + return errors.ScimErrorBadParams([]string{scimUserID(memberID)}) + } + group.ScimUsers = append(group.ScimUsers, memberID) + return nil + } + + // For replace operations with a value + if op.Op == scim.PatchOperationReplace { + if !memberFound { + level.Info(g.logger).Log("msg", "member not found for replace operation", "members.value", memberID, "op", fmt.Sprintf("%v", op)) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + + // If the value is nil or an empty object, remove the member + if op.Value == nil { + group.ScimUsers = append(group.ScimUsers[:memberIndex], group.ScimUsers[memberIndex+1:]...) + return nil + } + + // Otherwise, we don't change anything since we're already filtering by the member ID + // and there are no other attributes to modify for a member + return nil + } + + return nil +} + +// getMemberID extracts the member ID from a path expression like members[value eq "422"] +func (g *GroupHandler) getMemberID(op scim.PatchOperation) (uint, error) { + attrExpression, ok := op.Path.ValueExpression.(*filter.AttributeExpression) + if !ok { + level.Info(g.logger).Log("msg", "unsupported patch path", "path", op.Path) + return 0, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + + // Only matching by member value (user ID) is supported + if attrExpression.AttributePath.String() != valueAttr || attrExpression.Operator != filter.EQ { + level.Info(g.logger).Log("msg", "unsupported patch path", "path", op.Path, "expression", attrExpression.AttributePath.String()) + return 0, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + + memberIDStr, ok := attrExpression.CompareValue.(string) + if !ok { + level.Info(g.logger).Log("msg", "unsupported patch path", "path", op.Path, "compare_value", attrExpression.CompareValue) + return 0, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + + // Extract user ID from the value + userID, err := extractUserIDFromValue(memberIDStr) + if err != nil { + level.Info(g.logger).Log("msg", "invalid user ID format", "value", memberIDStr, "err", err) + return 0, errors.ScimErrorBadParams([]string{memberIDStr}) + } + + return userID, nil +} + func scimGroupID(groupID uint) string { return fmt.Sprintf("group-%d", groupID) } diff --git a/ee/server/scim/scim.go b/ee/server/scim/scim.go index e0a6bbe1f1..57e67848c5 100644 --- a/ee/server/scim/scim.go +++ b/ee/server/scim/scim.go @@ -33,6 +33,8 @@ func RegisterSCIM( config := scim.ServiceProviderConfig{ DocumentationURI: optional.NewString("https://fleetdm.com/docs/get-started/why-fleet"), MaxResults: maxResults, + SupportFiltering: true, + SupportPatch: true, } // The common attributes are id, externalId, and meta. @@ -136,18 +138,14 @@ func RegisterSCIM( Mutability: schema.AttributeMutabilityImmutable(), Name: "value", }), - schema.SimpleReferenceParams(schema.ReferenceParams{ - Description: optional.NewString("The URI corresponding to a SCIM resource that is a member of this Group."), - Mutability: schema.AttributeMutabilityImmutable(), - Name: "$ref", - ReferenceTypes: []schema.AttributeReferenceType{"User"}, - }), schema.SimpleStringParams(schema.StringParams{ CanonicalValues: []string{"User"}, Description: optional.NewString("A label indicating the type of resource, e.g., 'User' or 'Group'."), Mutability: schema.AttributeMutabilityImmutable(), Name: "type", }), + // Note (2025/05/06): Microsoft does not properly support $ref attribute on group members + // https://learn.microsoft.com/en-us/answers/questions/1457148/scim-validator-patch-group-remove-member-test-comp }, }), }, diff --git a/ee/server/scim/users.go b/ee/server/scim/users.go index 5083499033..a74ab513ad 100644 --- a/ee/server/scim/users.go +++ b/ee/server/scim/users.go @@ -406,9 +406,7 @@ func (u *UserHandler) Delete(r *http.Request, id string) error { return nil } -// Patch -// Okta only requires patching the "active" attribute: -// https://developer.okta.com/docs/api/openapi/okta-scim/guides/scim-20/#update-a-specific-user-patch +// Patch - https://datatracker.ietf.org/doc/html/rfc7644#section-3.5.2 func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchOperation) (scim.Resource, error) { idUint, err := extractUserIDFromValue(id) if err != nil { @@ -426,12 +424,17 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO } for _, op := range operations { - if op.Op != "replace" { + if op.Op != scim.PatchOperationAdd && op.Op != scim.PatchOperationReplace && op.Op != scim.PatchOperationRemove { level.Info(u.logger).Log("msg", "unsupported patch operation", "op", op.Op) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } switch { + // If path is not specified, we look for the path in the value attribute. case op.Path == nil: + if op.Op == scim.PatchOperationRemove { + level.Info(u.logger).Log("msg", "the 'path' attribute is REQUIRED for 'remove' operations", "op", op.Op) + return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } newValues, ok := op.Value.(map[string]interface{}) if !ok { level.Info(u.logger).Log("msg", "unsupported patch value", "value", op.Value) @@ -439,27 +442,28 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO } for k, v := range newValues { switch k { + case externalIdAttr: + err = u.patchExternalId(op.Op, v, user) + if err != nil { + return scim.Resource{}, err + } case userNameAttr: - err = u.patchUserName(v, user) + err = u.patchUserName(op.Op, v, user) if err != nil { return scim.Resource{}, err } case activeAttr: - if v == nil { - user.Active = nil - continue - } - err = u.patchActive(v, user) + err = u.patchActive(op.Op, v, user) if err != nil { return scim.Resource{}, err } case nameAttr + "." + givenNameAttr: - err = u.patchGivenName(v, user) + err = u.patchGivenName(op.Op, v, user) if err != nil { return scim.Resource{}, err } case nameAttr + "." + familyNameAttr: - err = u.patchFamilyName(v, user) + err = u.patchFamilyName(op.Op, v, user) if err != nil { return scim.Resource{}, err } @@ -478,27 +482,28 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } } + case op.Path.String() == externalIdAttr: + err = u.patchExternalId(op.Op, op.Value, user) + if err != nil { + return scim.Resource{}, err + } case op.Path.String() == userNameAttr: - err = u.patchUserName(op.Value, user) + err = u.patchUserName(op.Op, op.Value, user) if err != nil { return scim.Resource{}, err } case op.Path.String() == activeAttr: - if op.Value == nil { - user.Active = nil - continue - } - err = u.patchActive(op.Value, user) + err = u.patchActive(op.Op, op.Value, user) if err != nil { return scim.Resource{}, err } case op.Path.String() == nameAttr+"."+givenNameAttr: - err = u.patchGivenName(op.Value, user) + err = u.patchGivenName(op.Op, op.Value, user) if err != nil { return scim.Resource{}, err } case op.Path.String() == nameAttr+"."+familyNameAttr: - err = u.patchFamilyName(op.Value, user) + err = u.patchFamilyName(op.Op, op.Value, user) if err != nil { return scim.Resource{}, err } @@ -513,84 +518,10 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO return scim.Resource{}, err } case op.Path.AttributePath.String() == emailsAttr: - emailType, err := u.getEmailType(op) + err = u.patchEmailsWithPathFiltering(op, user) if err != nil { return scim.Resource{}, err } - emailFound := false - var emailIndex int - for i, email := range user.Emails { - if email.Type != nil && *email.Type == emailType { - emailIndex = i - emailFound = true - break - } - } - if !emailFound { - level.Info(u.logger).Log("msg", "email not found", "email_type", emailType) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) - } - if op.Path.SubAttribute == nil { - // The value for emails comes in as an array. - userEmails, ok := op.Value.([]interface{}) - if !ok { - level.Info(u.logger).Log("msg", fmt.Sprintf("unsupported '%s' patch value", emailsAttr), "value", op.Value) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) - } - if len(userEmails) == 0 { - user.Emails = slices.Delete(user.Emails, emailIndex, emailIndex) - continue - } - if len(userEmails) != 1 { - level.Info(u.logger).Log("msg", "only 1 email should be present for replacement", "emails", userEmails) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) - } - userEmail, err := u.extractEmail(userEmails[0], op) - if err != nil { - return scim.Resource{}, err - } - // If setting primary to true, then unset true from other emails - if userEmail.Primary != nil && *userEmail.Primary { - clearPrimaryFlagFromEmails(user) - } - user.Emails[emailIndex] = userEmail - continue - } - switch *op.Path.SubAttribute { - case primaryAttr: - if op.Value == nil { - user.Emails[emailIndex].Primary = nil - continue - } - primary, err := getConcreteType[bool](u, op.Value, primaryAttr) - if err != nil { - return scim.Resource{}, err - } - // If setting primary to true, then unset true from other emails - if primary { - clearPrimaryFlagFromEmails(user) - } - user.Emails[emailIndex].Primary = &primary - case valueAttr: - value, err := getConcreteType[string](u, op.Value, valueAttr) - if err != nil { - return scim.Resource{}, err - } - user.Emails[emailIndex].Email = value - case typeAttr: - if op.Value == nil { - user.Emails[emailIndex].Type = nil - continue - } - newEmailType, err := getConcreteType[string](u, op.Value, typeAttr) - if err != nil { - return scim.Resource{}, err - } - user.Emails[emailIndex].Type = &newEmailType - default: - level.Info(u.logger).Log("msg", "unsupported patch path", "path", op.Path) - return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) - } default: level.Info(u.logger).Log("msg", "unsupported patch path", "path", op.Path) return scim.Resource{}, errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) @@ -612,6 +543,146 @@ func (u *UserHandler) Patch(r *http.Request, id string, operations []scim.PatchO return createUserResource(user), nil } +func (u *UserHandler) patchEmailsWithPathFiltering(op scim.PatchOperation, user *fleet.ScimUser) error { + emailType, err := u.getEmailType(op) + if err != nil { + return err + } + emailFound := false + var emailIndex int + for i, email := range user.Emails { + if email.Type != nil && *email.Type == emailType { + emailIndex = i + emailFound = true + break + } + } + if !emailFound && op.Op != scim.PatchOperationAdd { + level.Info(u.logger).Log("msg", "email not found", "email_type", emailType, "op", fmt.Sprintf("%v", op)) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + if op.Path.SubAttribute == nil { + if op.Op == scim.PatchOperationRemove { + user.Emails = slices.Delete(user.Emails, emailIndex, emailIndex+1) + return nil + } + + // For add and replace operations, we need to extract the emails + var emailsList []interface{} + // Handle different value formats + switch val := op.Value.(type) { + case []interface{}: + // Direct array of members + emailsList = val + case map[string]interface{}: + // Single member as a map + emailsList = []interface{}{val} + default: + level.Info(u.logger).Log("msg", fmt.Sprintf("unsupported '%s' patch value", emailsAttr), "value", op.Value) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + + switch op.Op { + case scim.PatchOperationReplace: + if len(emailsList) == 0 { + user.Emails = slices.Delete(user.Emails, emailIndex, emailIndex+1) + return nil + } + if len(emailsList) != 1 { + level.Info(u.logger).Log("msg", "only 1 email should be present for replacement", "emails", emailsList) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + userEmail, err := u.extractEmail(emailsList[0], op) + if err != nil { + return err + } + // If setting primary to true, then unset true from other emails + if userEmail.Primary != nil && *userEmail.Primary { + clearPrimaryFlagFromEmails(user) + } + user.Emails[emailIndex] = userEmail + case scim.PatchOperationAdd: + if len(emailsList) == 0 { + level.Info(u.logger).Log("msg", "no emails provided to add", "emails", emailsList) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + var newEmails []fleet.ScimUserEmail + for e := range emailsList { + userEmail, err := u.extractEmail(emailsList[e], op) + if err != nil { + return err + } + userEmail.Type = &emailType + newEmails = append(newEmails, userEmail) + } + primaryExists, err := u.checkEmailPrimary(newEmails) + if err != nil { + return err + } + if primaryExists { + clearPrimaryFlagFromEmails(user) + } + user.Emails = append(user.Emails, newEmails...) + } + return nil + } + if op.Op == scim.PatchOperationAdd && !emailFound { + user.Emails = append(user.Emails, fleet.ScimUserEmail{ + Type: ptr.String(emailType), + }) + emailIndex = len(user.Emails) - 1 + } + switch *op.Path.SubAttribute { + case primaryAttr: + if op.Op == scim.PatchOperationRemove { + user.Emails[emailIndex].Primary = nil + return nil + } + if op.Value == nil { + user.Emails[emailIndex].Primary = nil + return nil + } + primary, err := getConcreteType[bool](u, op.Value, primaryAttr) + if err != nil { + return err + } + // If setting primary to true, then unset true from other emails + if primary { + clearPrimaryFlagFromEmails(user) + } + user.Emails[emailIndex].Primary = &primary + case valueAttr: + if op.Op == scim.PatchOperationRemove { + // The operation of removing an email value doesn't make sense, but we allow it. + user.Emails[emailIndex].Email = "" + return nil + } + value, err := getConcreteType[string](u, op.Value, valueAttr) + if err != nil { + return err + } + user.Emails[emailIndex].Email = value + case typeAttr: + if op.Op == scim.PatchOperationRemove { + user.Emails[emailIndex].Type = nil + return nil + } + if op.Value == nil { + user.Emails[emailIndex].Type = nil + return nil + } + newEmailType, err := getConcreteType[string](u, op.Value, typeAttr) + if err != nil { + return err + } + user.Emails[emailIndex].Type = &newEmailType + default: + level.Info(u.logger).Log("msg", "unsupported patch path", "path", op.Path) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } + return nil +} + func (u *UserHandler) getEmailType(op scim.PatchOperation) (string, error) { attrExpression, ok := op.Path.ValueExpression.(*filter.AttributeExpression) if !ok { @@ -641,7 +712,11 @@ func getConcreteType[T string | bool](u *UserHandler, v interface{}, name string return concreteType, nil } -func (u *UserHandler) patchFamilyName(v interface{}, user *fleet.ScimUser) error { +func (u *UserHandler) patchFamilyName(op string, v interface{}, user *fleet.ScimUser) error { + if op == scim.PatchOperationRemove { + level.Info(u.logger).Log("msg", "cannot remove required attribute", "attribute", nameAttr+"."+familyNameAttr) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } familyName, err := getConcreteType[string](u, v, nameAttr+"."+familyNameAttr) if err != nil { return err @@ -650,7 +725,11 @@ func (u *UserHandler) patchFamilyName(v interface{}, user *fleet.ScimUser) error return nil } -func (u *UserHandler) patchGivenName(v interface{}, user *fleet.ScimUser) error { +func (u *UserHandler) patchGivenName(op string, v interface{}, user *fleet.ScimUser) error { + if op == scim.PatchOperationRemove { + level.Info(u.logger).Log("msg", "cannot remove required attribute", "attribute", nameAttr+"."+givenNameAttr) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } givenName, err := getConcreteType[string](u, v, nameAttr+"."+givenNameAttr) if err != nil { return err @@ -659,7 +738,11 @@ func (u *UserHandler) patchGivenName(v interface{}, user *fleet.ScimUser) error return nil } -func (u *UserHandler) patchActive(v interface{}, user *fleet.ScimUser) error { +func (u *UserHandler) patchActive(op string, v interface{}, user *fleet.ScimUser) error { + if op == scim.PatchOperationRemove || v == nil { + user.Active = nil + return nil + } active, err := getConcreteType[bool](u, v, activeAttr) if err != nil { return err @@ -668,7 +751,24 @@ func (u *UserHandler) patchActive(v interface{}, user *fleet.ScimUser) error { return nil } -func (u *UserHandler) patchUserName(v interface{}, user *fleet.ScimUser) error { +func (u *UserHandler) patchExternalId(op string, v interface{}, user *fleet.ScimUser) error { + if op == scim.PatchOperationRemove || v == nil { + user.ExternalID = nil + return nil + } + externalId, err := getConcreteType[string](u, v, externalIdAttr) + if err != nil { + return err + } + user.ExternalID = ptr.String(externalId) + return nil +} + +func (u *UserHandler) patchUserName(op string, v interface{}, user *fleet.ScimUser) error { + if op == scim.PatchOperationRemove { + level.Info(u.logger).Log("msg", "cannot remove required attribute", "attribute", userNameAttr) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } userName, err := getConcreteType[string](u, v, userNameAttr) if err != nil { return err @@ -690,43 +790,69 @@ func clearPrimaryFlagFromEmails(user *fleet.ScimUser) { } func (u *UserHandler) patchEmails(v interface{}, op scim.PatchOperation, user *fleet.ScimUser) error { - emailsValue, ok := v.([]interface{}) - if !ok { + if op.Op == scim.PatchOperationRemove { + user.Emails = nil + return nil + } + + // For add and replace operations, we need to extract the emails + var emailsList []interface{} + // Handle different value formats + switch val := v.(type) { + case []interface{}: + // Direct array of members + emailsList = val + case map[string]interface{}: + // Single member as a map + emailsList = []interface{}{val} + default: level.Info(u.logger).Log("msg", fmt.Sprintf("unsupported '%s' patch value", emailsAttr), "value", op.Value) return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) } + + if op.Op == scim.PatchOperationAdd && len(emailsList) == 0 { + level.Info(u.logger).Log("msg", "no emails provided to add", "emails", emailsList) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } // Convert the emails to the expected format - userEmails := make([]fleet.ScimUserEmail, 0, len(emailsValue)) - for _, emailIntf := range emailsValue { + userEmails := make([]fleet.ScimUserEmail, 0, len(emailsList)) + for _, emailIntf := range emailsList { userEmail, err := u.extractEmail(emailIntf, op) if err != nil { return err } userEmails = append(userEmails, userEmail) } - - err := u.checkEmailPrimary(userEmails) + primaryExists, err := u.checkEmailPrimary(userEmails) if err != nil { return err } + if op.Op == scim.PatchOperationAdd { + if primaryExists { + // Clear the primary flag from current emails because we are merging the two email lists and a new email has that flag. + clearPrimaryFlagFromEmails(user) + } + userEmails = append(user.Emails, userEmails...) + } + user.Emails = userEmails return nil } // checkEmailPrimary ensures at most one email is marked as primary -func (u *UserHandler) checkEmailPrimary(userEmails []fleet.ScimUserEmail) error { +func (u *UserHandler) checkEmailPrimary(userEmails []fleet.ScimUserEmail) (bool, error) { primaryEmailCount := 0 for _, email := range userEmails { if email.Primary != nil && *email.Primary { primaryEmailCount++ if primaryEmailCount > 1 { level.Info(u.logger).Log("msg", "multiple primary emails found") - return errors.ScimErrorBadParams([]string{"Only one email can be marked as primary"}) + return false, errors.ScimErrorBadParams([]string{"Only one email can be marked as primary"}) } } } - return nil + return primaryEmailCount > 0, nil } func (u *UserHandler) extractEmail(emailIntf interface{}, op scim.PatchOperation) (fleet.ScimUserEmail, error) { @@ -770,6 +896,10 @@ func (u *UserHandler) extractEmail(emailIntf interface{}, op scim.PatchOperation } func (u *UserHandler) patchName(v interface{}, op scim.PatchOperation, user *fleet.ScimUser) error { + if op.Op == scim.PatchOperationRemove { + level.Info(u.logger).Log("msg", "cannot remove required attribute", "attribute", nameAttr) + return errors.ScimErrorBadParams([]string{fmt.Sprintf("%v", op)}) + } name, ok := v.(map[string]interface{}) if !ok { level.Info(u.logger).Log("msg", fmt.Sprintf("unsupported '%s' patch value", nameAttr), "value", op.Value) diff --git a/server/datastore/mysql/scim.go b/server/datastore/mysql/scim.go index c7a8605fbf..04bedb2393 100644 --- a/server/datastore/mysql/scim.go +++ b/server/datastore/mysql/scim.go @@ -222,6 +222,24 @@ func (ds *Datastore) ReplaceScimUser(ctx context.Context, user *fleet.ScimUser) return err } + // Validate that at most one email is marked as primary + primaryCount := 0 + for _, email := range user.Emails { + if email.Primary != nil && *email.Primary { + primaryCount++ + } + } + if primaryCount > 1 { + return ctxerr.New(ctx, "only one email can be marked as primary") + } + + // Get current emails and check if they need to be updated + currentEmails, err := ds.getScimUserEmails(ctx, user.ID) + if err != nil { + return err + } + emailsNeedUpdate := emailsRequireUpdate(currentEmails, user.Emails) + return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { // load the username before updating the user, to check if it changed var oldUsername string @@ -265,24 +283,23 @@ func (ds *Datastore) ReplaceScimUser(ctx context.Context, user *fleet.ScimUser) } usernameChanged := oldUsername != user.UserName - // We assume that email is not blank/null. - // However, we do not assume that email/type are unique for a user. To keep the code simple, we: - // 1. Delete all existing emails - // 2. Insert all new emails - // This is less efficient and can be optimized if we notice a load on these tables in production. + // Only update emails if they've changed + if emailsNeedUpdate { + // We assume that email is not blank/null. + // However, we do not assume that email/type are unique for a user. To keep the code simple, we: + // 1. Delete all existing emails + // 2. Insert all new emails + // This is less efficient and can be optimized if we notice a load on these tables in production. - // TODO: Check if emails need to be updated at all. If so, update the user updated_at timestamp if emails have been updated - // TODO: Check that only 1 email is primary - - const deleteEmailsQuery = `DELETE FROM scim_user_emails WHERE scim_user_id = ?` - _, err = tx.ExecContext(ctx, deleteEmailsQuery, user.ID) - if err != nil { - return ctxerr.Wrap(ctx, err, "delete scim user emails") - } - - err = insertEmails(ctx, tx, user) - if err != nil { - return err + const deleteEmailsQuery = `DELETE FROM scim_user_emails WHERE scim_user_id = ?` + _, err = tx.ExecContext(ctx, deleteEmailsQuery, user.ID) + if err != nil { + return ctxerr.Wrap(ctx, err, "delete scim user emails") + } + err = insertEmails(ctx, tx, user) + if err != nil { + return err + } } // Get the user's groups @@ -867,7 +884,7 @@ func (ds *Datastore) DeleteScimGroup(ctx context.Context, id uint) error { } // ListScimGroups retrieves a list of SCIM groups with pagination -func (ds *Datastore) ListScimGroups(ctx context.Context, opts fleet.ScimListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) { +func (ds *Datastore) ListScimGroups(ctx context.Context, opts fleet.ScimGroupsListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) { // Default pagination values if not provided if opts.StartIndex == 0 { opts.StartIndex = 1 @@ -883,16 +900,25 @@ func (ds *Datastore) ListScimGroups(ctx context.Context, opts fleet.ScimListOpti FROM scim_groups ` + // Add where clause based on filters + var whereClause string + var params []interface{} + + if opts.DisplayNameFilter != nil { + whereClause = " WHERE scim_groups.display_name = ?" + params = append(params, *opts.DisplayNameFilter) + } + // First, get the total count without pagination - countQuery := "SELECT COUNT(DISTINCT id) FROM (" + baseQuery + ") AS filtered_groups" - err = sqlx.GetContext(ctx, ds.reader(ctx), &totalResults, countQuery) + countQuery := "SELECT COUNT(DISTINCT id) FROM (" + baseQuery + whereClause + ") AS filtered_groups" + err = sqlx.GetContext(ctx, ds.reader(ctx), &totalResults, countQuery, params...) if err != nil { return nil, 0, ctxerr.Wrap(ctx, err, "count total scim groups") } // Add pagination to the main query - query := baseQuery + " ORDER BY scim_groups.id LIMIT ? OFFSET ?" - params := []interface{}{opts.PerPage, opts.StartIndex - 1} + query := baseQuery + whereClause + " ORDER BY scim_groups.id LIMIT ? OFFSET ?" + params = append(params, opts.PerPage, opts.StartIndex-1) // Execute the query err = sqlx.SelectContext(ctx, ds.reader(ctx), &groups, query, params...) @@ -1208,3 +1234,76 @@ func triggerResendProfilesUsingVariables(ctx context.Context, tx sqlx.ExtContext } return nil } + +// emailsRequireUpdate compares two slices of emails and returns true if they are different +// and require an update in the database. +func emailsRequireUpdate(currentEmails, newEmails []fleet.ScimUserEmail) bool { + if len(currentEmails) != len(newEmails) { + return true + } + + // Create maps for efficient comparison + currentEmailMap := make(map[string]fleet.ScimUserEmail) + for i := range currentEmails { + key := currentEmails[i].GenerateComparisonKey() + currentEmailMap[key] = currentEmails[i] + } + + // Check if all new emails exist in current emails with the same attributes + for i := range newEmails { + key := newEmails[i].GenerateComparisonKey() + if _, exists := currentEmailMap[key]; !exists { + return true + } + } + + return false +} + +// ScimUsersExist checks if all the provided SCIM user IDs exist in the datastore +// If the slice is empty, it returns true +// This method processes IDs in batches to handle large numbers of IDs efficiently +func (ds *Datastore) ScimUsersExist(ctx context.Context, ids []uint) (bool, error) { + if len(ids) == 0 { + return true, nil + } + + // Create a map to track which IDs we've found + foundIDs := make(map[uint]bool, len(ids)) + + batchSize := 10000 + err := common_mysql.BatchProcessSimple(ids, batchSize, func(batchIDs []uint) error { + query, args, err := sqlx.In(` + SELECT id + FROM scim_users + WHERE id IN (?) + `, batchIDs) + if err != nil { + return ctxerr.Wrap(ctx, err, "prepare scim users exist batch query") + } + + var foundBatchIDs []uint + err = sqlx.SelectContext(ctx, ds.reader(ctx), &foundBatchIDs, query, args...) + if err != nil { + return ctxerr.Wrap(ctx, err, "check if scim users exist in batch") + } + + // Mark found IDs + for _, id := range foundBatchIDs { + foundIDs[id] = true + } + return nil + }) + if err != nil { + return false, err + } + + // Check if all IDs were found + for _, id := range ids { + if !foundIDs[id] { + return false, nil + } + } + + return true, nil +} diff --git a/server/datastore/mysql/scim_test.go b/server/datastore/mysql/scim_test.go index f81e2da449..c01c527b9f 100644 --- a/server/datastore/mysql/scim_test.go +++ b/server/datastore/mysql/scim_test.go @@ -29,6 +29,7 @@ func TestScim(t *testing.T) { {"ScimUserByUserNameOrEmail", testScimUserByUserNameOrEmail}, {"ScimUserByHostID", testScimUserByHostID}, {"ReplaceScimUser", testReplaceScimUser}, + {"ReplaceScimUserEmails", testReplaceScimUserEmails}, {"ReplaceScimUserValidation", testScimUserReplaceValidation}, {"DeleteScimUser", testDeleteScimUser}, {"ListScimUsers", testListScimUsers}, @@ -41,6 +42,7 @@ func TestScim(t *testing.T) { {"DeleteScimGroup", testDeleteScimGroup}, {"ListScimGroups", testListScimGroups}, {"ScimLastRequest", testScimLastRequest}, + {"ScimUsersExist", testScimUsersExist}, {"TriggerResendIdPProfiles", testTriggerResendIdPProfiles}, {"TriggerResendIdPProfilesOnTeam", testTriggerResendIdPProfilesOnTeam}, } @@ -423,6 +425,171 @@ func testReplaceScimUser(t *testing.T, ds *Datastore) { assert.True(t, fleet.IsNotFound(err)) } +func testReplaceScimUserEmails(t *testing.T, ds *Datastore) { + // Create a test user + user := fleet.ScimUser{ + UserName: "email-test-user", + ExternalID: ptr.String("ext-email-123"), + GivenName: ptr.String("Email"), + FamilyName: ptr.String("Test"), + Active: ptr.Bool(true), + Emails: []fleet.ScimUserEmail{ + { + Email: "original.email@example.com", + Primary: ptr.Bool(true), + Type: ptr.String("work"), + }, + }, + } + + var err error + user.ID, err = ds.CreateScimUser(t.Context(), &user) + require.Nil(t, err) + + // Smoke test email optimization - replacing with the same emails should not update emails + // First, get the current user to have a reference point + currentUser, err := ds.ScimUserByID(t.Context(), user.ID) + require.NoError(t, err) + + // Create a copy of the user with the same emails + sameEmailsUser := fleet.ScimUser{ + ID: user.ID, + UserName: "multi-update@example.com", + ExternalID: ptr.String("ext-replace-456"), + GivenName: ptr.String("Multiple"), + FamilyName: ptr.String("Updates"), + Active: ptr.Bool(true), + Emails: currentUser.Emails, // Same emails as current user + } + + // Replace the user + err = ds.ReplaceScimUser(t.Context(), &sameEmailsUser) + require.NoError(t, err) + + // Verify the user was updated correctly but emails remain the same + sameEmailsResult, err := ds.ScimUserByID(t.Context(), user.ID) + require.NoError(t, err) + assert.Equal(t, sameEmailsUser.UserName, sameEmailsResult.UserName) + assert.Equal(t, sameEmailsUser.ExternalID, sameEmailsResult.ExternalID) + assert.Equal(t, sameEmailsUser.GivenName, sameEmailsResult.GivenName) + assert.Equal(t, sameEmailsUser.FamilyName, sameEmailsResult.FamilyName) + assert.Equal(t, sameEmailsUser.Active, sameEmailsResult.Active) + + // Verify emails are the same as before + assert.Equal(t, len(currentUser.Emails), len(sameEmailsResult.Emails)) + for i := range currentUser.Emails { + assert.Equal(t, currentUser.Emails[i].Email, sameEmailsResult.Emails[i].Email) + assert.Equal(t, currentUser.Emails[i].Type, sameEmailsResult.Emails[i].Type) + assert.Equal(t, currentUser.Emails[i].Primary, sameEmailsResult.Emails[i].Primary) + } + + // Test validation for multiple primary emails + multiPrimaryUser := fleet.ScimUser{ + ID: user.ID, + UserName: "multi-primary@example.com", + ExternalID: ptr.String("ext-multi-primary"), + GivenName: ptr.String("Multi"), + FamilyName: ptr.String("Primary"), + Active: ptr.Bool(true), + Emails: []fleet.ScimUserEmail{ + { + Email: "primary1@example.com", + Primary: ptr.Bool(true), // First primary + Type: ptr.String("work"), + }, + { + Email: "primary2@example.com", + Primary: ptr.Bool(true), // Second primary - should cause validation error + Type: ptr.String("home"), + }, + }, + } + + // This should fail with a validation error + err = ds.ReplaceScimUser(t.Context(), &multiPrimaryUser) + assert.Error(t, err) + assert.Contains(t, err.Error(), "only one email can be marked as primary") + + // Test email comparison behavior with different combinations of nil/non-nil fields + // First, create a user with an email that has all fields set + userWithAllFields := fleet.ScimUser{ + ID: user.ID, + UserName: "all-fields@example.com", + ExternalID: ptr.String("ext-all-fields"), + GivenName: ptr.String("All"), + FamilyName: ptr.String("Fields"), + Active: ptr.Bool(true), + Emails: []fleet.ScimUserEmail{ + { + Email: "all-fields@example.com", + Primary: ptr.Bool(true), + Type: ptr.String("work"), + }, + }, + } + + err = ds.ReplaceScimUser(t.Context(), &userWithAllFields) + require.NoError(t, err) + + // Now create a user with the same email but with nil Primary field + userWithNilPrimary := fleet.ScimUser{ + ID: user.ID, + UserName: "all-fields@example.com", + ExternalID: ptr.String("ext-all-fields"), + GivenName: ptr.String("All"), + FamilyName: ptr.String("Fields"), + Active: ptr.Bool(true), + Emails: []fleet.ScimUserEmail{ + { + Email: "all-fields@example.com", + Primary: nil, // Changed from true to nil + Type: ptr.String("work"), + }, + }, + } + + // This should update the emails since the Primary field changed + err = ds.ReplaceScimUser(t.Context(), &userWithNilPrimary) + require.NoError(t, err) + + // Verify the email was updated + var nilPrimaryUser *fleet.ScimUser + nilPrimaryUser, err = ds.ScimUserByID(t.Context(), user.ID) + require.NoError(t, err) + require.Len(t, nilPrimaryUser.Emails, 1) + assert.Equal(t, "all-fields@example.com", nilPrimaryUser.Emails[0].Email) + assert.Nil(t, nilPrimaryUser.Emails[0].Primary, "Primary field should be nil") + + // Now create a user with the same email but with nil Type field + userWithNilType := fleet.ScimUser{ + ID: user.ID, + UserName: "all-fields@example.com", + ExternalID: ptr.String("ext-all-fields"), + GivenName: ptr.String("All"), + FamilyName: ptr.String("Fields"), + Active: ptr.Bool(true), + Emails: []fleet.ScimUserEmail{ + { + Email: "all-fields@example.com", + Primary: nil, + Type: nil, // Changed from "work" to nil + }, + }, + } + + // This should update the emails since the Type field changed + err = ds.ReplaceScimUser(t.Context(), &userWithNilType) + require.NoError(t, err) + + // Verify the email was updated + var nilTypeUser *fleet.ScimUser + nilTypeUser, err = ds.ScimUserByID(t.Context(), user.ID) + require.NoError(t, err) + require.Len(t, nilTypeUser.Emails, 1) + assert.Equal(t, "all-fields@example.com", nilTypeUser.Emails[0].Email) + assert.Nil(t, nilTypeUser.Emails[0].Type, "Type field should be nil") +} + func testDeleteScimUser(t *testing.T, ds *Datastore) { // Create a test user user := fleet.ScimUser{ @@ -1047,9 +1214,11 @@ func testListScimGroups(t *testing.T, ds *Datastore) { } // Test 1: List all groups - allGroups, totalResults, err := ds.ListScimGroups(t.Context(), fleet.ScimListOptions{ - StartIndex: 1, - PerPage: 10, + allGroups, totalResults, err := ds.ListScimGroups(t.Context(), fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: 1, + PerPage: 10, + }, }) require.Nil(t, err) assert.GreaterOrEqual(t, len(allGroups), 3) // There might be other groups from previous tests @@ -1068,18 +1237,22 @@ func testListScimGroups(t *testing.T, ds *Datastore) { assert.Equal(t, 3, foundGroups) // Test 2: Pagination - first page with 2 items - page1Groups, totalPage1, err := ds.ListScimGroups(t.Context(), fleet.ScimListOptions{ - StartIndex: 1, - PerPage: 2, + page1Groups, totalPage1, err := ds.ListScimGroups(t.Context(), fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: 1, + PerPage: 2, + }, }) require.Nil(t, err) assert.Equal(t, 2, len(page1Groups)) assert.GreaterOrEqual(t, totalPage1, uint(3)) // Total should be at least 3 // Test 3: Pagination - second page with 2 items - page2Groups, totalPage2, err := ds.ListScimGroups(t.Context(), fleet.ScimListOptions{ - StartIndex: 3, // StartIndex is 1-based, so for the second page with 2 items per page, we start at index 3 - PerPage: 2, + page2Groups, totalPage2, err := ds.ListScimGroups(t.Context(), fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: 3, // StartIndex is 1-based, so for the second page with 2 items per page, we start at index 3 + PerPage: 2, + }, }) require.Nil(t, err) assert.GreaterOrEqual(t, len(page2Groups), 1) // At least 1 item on the second page @@ -1091,6 +1264,33 @@ func testListScimGroups(t *testing.T, ds *Datastore) { assert.NotEqual(t, p1Group.ID, p2Group.ID, "Groups should not appear on multiple pages") } } + + // Test 4: Filter by display name + displayName := "List Test Group 2" + filteredGroups, totalFilteredResults, err := ds.ListScimGroups(t.Context(), fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: 1, + PerPage: 10, + }, + DisplayNameFilter: &displayName, + }) + require.Nil(t, err) + assert.Equal(t, 1, len(filteredGroups), "Should find exactly one group with the specified display name") + assert.Equal(t, uint(1), totalFilteredResults) + assert.Equal(t, displayName, filteredGroups[0].DisplayName) + + // Test 5: Filter by non-existent display name + nonExistentName := "Non-Existent Group" + emptyResults, totalEmptyResults, err := ds.ListScimGroups(t.Context(), fleet.ScimGroupsListOptions{ + ScimListOptions: fleet.ScimListOptions{ + StartIndex: 1, + PerPage: 10, + }, + DisplayNameFilter: &nonExistentName, + }) + require.Nil(t, err) + assert.Empty(t, emptyResults, "Should find no groups with a non-existent display name") + assert.Equal(t, uint(0), totalEmptyResults) } func testScimUserCreateValidation(t *testing.T, ds *Datastore) { @@ -1967,3 +2167,60 @@ func forceSetHostProfileStatus(t *testing.T, ds *Datastore, hostUUID string, pro return err }) } + +func testScimUsersExist(t *testing.T, ds *Datastore) { + // Create test users + users := createTestScimUsers(t, ds) + userIDs := make([]uint, len(users)) + for i, user := range users { + userIDs[i] = user.ID + } + + // Test 1: Empty slice should return true + exist, err := ds.ScimUsersExist(t.Context(), []uint{}) + require.NoError(t, err) + assert.True(t, exist, "Empty slice should return true") + + // Test 2: All existing users should return true + exist, err = ds.ScimUsersExist(t.Context(), userIDs) + require.NoError(t, err) + assert.True(t, exist, "All existing users should return true") + + // Test 3: Mix of existing and non-existing users should return false + nonExistentIDs := userIDs + nonExistentIDs = append(nonExistentIDs, 99999) + exist, err = ds.ScimUsersExist(t.Context(), nonExistentIDs) + require.NoError(t, err) + assert.False(t, exist, "Mix of existing and non-existing users should return false") + + // Test 4: Only non-existing users should return false + exist, err = ds.ScimUsersExist(t.Context(), []uint{99999, 100000}) + require.NoError(t, err) + assert.False(t, exist, "Only non-existing users should return false") + + // Test 5: Test with a large number of IDs to verify batching works + // First, create a large number of test users + largeUserIDs := make([]uint, 0, 25000) + largeUserIDs = append(largeUserIDs, userIDs...) // Add existing users + + // Add some non-existent IDs to test batching with mixed results + for i := 0; i < 24990; i++ { + largeUserIDs = append(largeUserIDs, uint(1000000)+uint(i)) // nolint:gosec // dismiss G115 integer overflow + } + + exist, err = ds.ScimUsersExist(t.Context(), largeUserIDs) + require.NoError(t, err) + assert.False(t, exist, "Large batch with non-existing users should return false") + + // Test 6: Test with a large number of existing IDs + // This is a bit tricky to test thoroughly without creating thousands of users, + // so we'll just verify the function handles a large slice without errors + largeExistingIDs := make([]uint, 0, 25000) + for i := 0; i < 25000; i++ { + largeExistingIDs = append(largeExistingIDs, userIDs[i%len(userIDs)]) + } + + exist, err = ds.ScimUsersExist(t.Context(), largeExistingIDs) + require.NoError(t, err) + assert.True(t, exist, "Large batch with only existing users should return true") +} diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go index 8fe558883f..013d6891f8 100644 --- a/server/fleet/datastore.go +++ b/server/fleet/datastore.go @@ -2067,6 +2067,9 @@ type Datastore interface { ScimUserByUserNameOrEmail(ctx context.Context, userName string, email string) (*ScimUser, error) // ScimUserByHostID retrieves a SCIM user associated with a host ID ScimUserByHostID(ctx context.Context, hostID uint) (*ScimUser, error) + // ScimUsersExist checks if all the provided SCIM user IDs exist in the datastore + // If the slice is empty, it returns true + ScimUsersExist(ctx context.Context, ids []uint) (bool, error) // ReplaceScimUser replaces an existing SCIM user in the database ReplaceScimUser(ctx context.Context, user *ScimUser) error // DeleteScimUser deletes a SCIM user from the database @@ -2084,7 +2087,7 @@ type Datastore interface { // DeleteScimGroup deletes a SCIM group from the database DeleteScimGroup(ctx context.Context, id uint) error // ListScimGroups retrieves a list of SCIM groups with pagination - ListScimGroups(ctx context.Context, opts ScimListOptions) (groups []ScimGroup, totalResults uint, err error) + ListScimGroups(ctx context.Context, opts ScimGroupsListOptions) (groups []ScimGroup, totalResults uint, err error) // ScimLastRequest retrieves the last SCIM request info ScimLastRequest(ctx context.Context) (*ScimLastRequest, error) // UpdateScimLastRequest updates the last SCIM request info diff --git a/server/fleet/scim.go b/server/fleet/scim.go index f949033088..40f36ec598 100644 --- a/server/fleet/scim.go +++ b/server/fleet/scim.go @@ -48,6 +48,28 @@ type ScimUserEmail struct { Type *string `db:"type"` } +// GenerateComparisonKey generates a unique string representation of the email +// that can be used for comparison, properly handling nil values. +func (e ScimUserEmail) GenerateComparisonKey() string { + // Handle Type field which can be nil + typeValue := "nil" + if e.Type != nil { + typeValue = *e.Type + } + + // Handle Primary field which can be nil + primaryValue := "nil" + if e.Primary != nil { + if *e.Primary { + primaryValue = "true" + } else { + primaryValue = "false" + } + } + + return e.Email + ":" + typeValue + ":" + primaryValue +} + type ScimListOptions struct { // 1-based index of the first result to return (must be positive integer) StartIndex uint @@ -69,6 +91,13 @@ type ScimUsersListOptions struct { EmailValueFilter *string } +type ScimGroupsListOptions struct { + ScimListOptions + + // DisplayNameFilter filters by displayName + DisplayNameFilter *string +} + type ScimGroup struct { ID uint `db:"id"` ExternalID *string `db:"external_id"` diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go index a97d7fa701..3f46bec92c 100644 --- a/server/mock/datastore_mock.go +++ b/server/mock/datastore_mock.go @@ -1320,6 +1320,8 @@ type ScimUserByUserNameOrEmailFunc func(ctx context.Context, userName string, em type ScimUserByHostIDFunc func(ctx context.Context, hostID uint) (*fleet.ScimUser, error) +type ScimUsersExistFunc func(ctx context.Context, ids []uint) (bool, error) + type ReplaceScimUserFunc func(ctx context.Context, user *fleet.ScimUser) error type DeleteScimUserFunc func(ctx context.Context, id uint) error @@ -1336,7 +1338,7 @@ type ReplaceScimGroupFunc func(ctx context.Context, group *fleet.ScimGroup) erro type DeleteScimGroupFunc func(ctx context.Context, id uint) error -type ListScimGroupsFunc func(ctx context.Context, opts fleet.ScimListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) +type ListScimGroupsFunc func(ctx context.Context, opts fleet.ScimGroupsListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) type ScimLastRequestFunc func(ctx context.Context) (*fleet.ScimLastRequest, error) @@ -3290,6 +3292,9 @@ type DataStore struct { ScimUserByHostIDFunc ScimUserByHostIDFunc ScimUserByHostIDFuncInvoked bool + ScimUsersExistFunc ScimUsersExistFunc + ScimUsersExistFuncInvoked bool + ReplaceScimUserFunc ReplaceScimUserFunc ReplaceScimUserFuncInvoked bool @@ -7869,6 +7874,13 @@ func (s *DataStore) ScimUserByHostID(ctx context.Context, hostID uint) (*fleet.S return s.ScimUserByHostIDFunc(ctx, hostID) } +func (s *DataStore) ScimUsersExist(ctx context.Context, ids []uint) (bool, error) { + s.mu.Lock() + s.ScimUsersExistFuncInvoked = true + s.mu.Unlock() + return s.ScimUsersExistFunc(ctx, ids) +} + func (s *DataStore) ReplaceScimUser(ctx context.Context, user *fleet.ScimUser) error { s.mu.Lock() s.ReplaceScimUserFuncInvoked = true @@ -7925,7 +7937,7 @@ func (s *DataStore) DeleteScimGroup(ctx context.Context, id uint) error { return s.DeleteScimGroupFunc(ctx, id) } -func (s *DataStore) ListScimGroups(ctx context.Context, opts fleet.ScimListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) { +func (s *DataStore) ListScimGroups(ctx context.Context, opts fleet.ScimGroupsListOptions) (groups []fleet.ScimGroup, totalResults uint, err error) { s.mu.Lock() s.ListScimGroupsFuncInvoked = true s.mu.Unlock()