From 9c921ecafe93a58ade968ffb8f21f2a587b4444a Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Wed, 25 Mar 2026 08:49:16 +0100 Subject: [PATCH 1/7] feat: add ApplyPatch function for applying SCIM patch operations Add a generic ApplyPatch function that applies a sequence of patch operations (add, replace, remove) to resource attributes, resolving attribute paths against the provided schema and extensions. This avoids every consumer having to independently implement the RFC 7644 Section 3.5.2 patch semantics, which is error-prone and duplicative. --- examples_test.go | 66 +++++++ patch_apply.go | 455 ++++++++++++++++++++++++++++++++++++++++++++ patch_apply_test.go | 421 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 942 insertions(+) create mode 100644 patch_apply.go create mode 100644 patch_apply_test.go diff --git a/examples_test.go b/examples_test.go index 360764d..c124218 100644 --- a/examples_test.go +++ b/examples_test.go @@ -1,10 +1,76 @@ package scim import ( + "fmt" logger "log" "net/http" + + "github.com/elimity-com/scim/schema" + filter "github.com/scim2/filter-parser/v2" ) +func ExampleApplyPatch() { + // Define the schema for the resource. + userSchema := schema.Schema{ + ID: schema.UserSchema, + Attributes: schema.Attributes{ + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "userName", + })), + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "displayName", + })), + schema.ComplexCoreAttribute(schema.ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []schema.SimpleParams{ + schema.SimpleStringParams(schema.StringParams{Name: "value"}), + schema.SimpleStringParams(schema.StringParams{Name: "type"}), + }, + }), + }, + } + + // The resource to patch, typically fetched from a data store. + attrs := ResourceAttributes{ + "userName": "john", + "displayName": "John Doe", + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + }, + }, + } + + // Parse paths for the operations. + emailsPath, _ := filter.ParsePath([]byte("emails")) + workEmailValuePath, _ := filter.ParsePath([]byte(`emails[type eq "work"].value`)) + + // Apply a sequence of patch operations. + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: &emailsPath, Value: []interface{}{ + map[string]interface{}{"value": "john@home.com", "type": "home"}, + }}, + {Op: PatchOperationReplace, Path: &workEmailValuePath, Value: "john@newwork.com"}, + }, userSchema) + if err != nil { + panic(err) + } + + fmt.Println(result["userName"]) + emails := result["emails"].([]interface{}) + fmt.Println(len(emails)) + fmt.Println(emails[0].(map[string]interface{})["value"]) + fmt.Println(emails[1].(map[string]interface{})["value"]) + + // Output: + // john + // 2 + // john@newwork.com + // john@home.com +} + func ExampleNewServer() { args := &ServerArgs{ ServiceProviderConfig: &ServiceProviderConfig{}, diff --git a/patch_apply.go b/patch_apply.go new file mode 100644 index 0000000..df2066b --- /dev/null +++ b/patch_apply.go @@ -0,0 +1,455 @@ +package scim + +import ( + "fmt" + + internal "github.com/elimity-com/scim/filter" + "github.com/elimity-com/scim/schema" + filter "github.com/scim2/filter-parser/v2" +) + +func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, replace bool) ([]interface{}, error) { + result := make([]interface{}, len(list)) + copy(result, list) + + for i, elem := range result { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + if !matchesExpression(m, t.expr, t.attr, t.refSchema) { + continue + } + if t.subAttrName != "" { + updated := copyMap(m) + updated[t.subAttrName] = value + result[i] = updated + } else if replace { + if vm, ok := value.(map[string]interface{}); ok { + updated := copyMap(m) + for k, v := range vm { + updated[k] = v + } + result[i] = updated + } else { + result[i] = value + } + } else { + // Add to matching element. + if vm, ok := value.(map[string]interface{}); ok { + updated := copyMap(m) + for k, v := range vm { + if v != nil { + updated[k] = v + } + } + result[i] = updated + } else { + result[i] = value + } + } + } + return result, nil +} + +func copyMap(m map[string]interface{}) map[string]interface{} { + cp := make(map[string]interface{}, len(m)) + for k, v := range m { + cp[k] = v + } + return cp +} + +func matchesExpression(element map[string]interface{}, expr filter.Expression, attr schema.CoreAttribute, refSchema schema.Schema) bool { + v := internal.NewFilterValidator(expr, schema.Schema{ + ID: refSchema.ID, + Attributes: attr.SubAttributes(), + }) + return v.PassesFilter(element) == nil +} + +func mergeAdd(existing, value interface{}, attr schema.CoreAttribute) (interface{}, error) { + if attr.MultiValued() { + existingList, ok := existing.([]interface{}) + if !ok { + return nil, fmt.Errorf("expected multi-valued attribute to be a list") + } + switch v := value.(type) { + case []interface{}: + return append(existingList, v...), nil + default: + return append(existingList, v), nil + } + } + + if attr.HasSubAttributes() { + existingMap, ok := existing.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("expected complex attribute to be a map") + } + valueMap, ok := value.(map[string]interface{}) + if !ok { + // Not a map, just replace. + return value, nil + } + merged := copyMap(existingMap) + for k, v := range valueMap { + if v != nil { + merged[k] = v + } + } + return merged, nil + } + + // Simple single-valued: replace. + return value, nil +} + +func removeMatching(list []interface{}, t *valueExprTarget) ([]interface{}, error) { + var result []interface{} + for _, elem := range list { + m, ok := elem.(map[string]interface{}) + if !ok { + result = append(result, elem) + continue + } + if !matchesExpression(m, t.expr, t.attr, t.refSchema) { + result = append(result, elem) + continue + } + if t.subAttrName != "" { + updated := copyMap(m) + delete(updated, t.subAttrName) + result = append(result, updated) + } + // If no sub-attribute, the matching element is removed entirely. + } + return result, nil +} + +func resolveTarget(path *filter.Path, s schema.Schema, extensions []schema.Schema) (string, interface{}, error) { + attrPath := path.AttributePath + attrName := attrPath.AttributeName + + refSchema := s + if uri := attrPath.URI(); uri != "" { + found := false + if uri == s.ID { + found = true + } else { + for _, ext := range extensions { + if uri == ext.ID { + refSchema = ext + found = true + break + } + } + } + if !found { + return "", nil, fmt.Errorf("unknown schema URI: %s", uri) + } + } + + attr, ok := refSchema.Attributes.ContainsAttribute(attrName) + if !ok { + return "", nil, fmt.Errorf("attribute not found: %s", attrName) + } + // Use the canonical name from the schema. + attrName = attr.Name() + + // Extension attributes are stored under "schemaURI:attrName" in the resource. + if uri := attrPath.URI(); uri != "" && uri != s.ID { + attrName = uri + ":" + attrName + } + + if path.ValueExpression != nil { + subAttrName := path.SubAttributeName() + return attrName, &valueExprTarget{ + attr: attr, + expr: path.ValueExpression, + subAttrName: subAttrName, + refSchema: refSchema, + }, nil + } + + if subAttrName := attrPath.SubAttributeName(); subAttrName != "" { + // Resolve the canonical sub-attribute name. + if attr.HasSubAttributes() { + if sub, ok := attr.SubAttributes().ContainsAttribute(subAttrName); ok { + subAttrName = sub.Name() + } + } + return attrName, &subAttributeTarget{ + attr: attr, + subAttrName: subAttrName, + }, nil + } + + // Path-level sub-attribute (e.g., members[...].displayName has SubAttribute on Path). + if subAttrName := path.SubAttributeName(); subAttrName != "" { + if attr.HasSubAttributes() { + if sub, ok := attr.SubAttributes().ContainsAttribute(subAttrName); ok { + subAttrName = sub.Name() + } + } + return attrName, &subAttributeTarget{ + attr: attr, + subAttrName: subAttrName, + }, nil + } + + return attrName, &attributeTarget{attr: attr}, nil +} + +// ApplyPatch applies the given patch operations to the resource attributes. +// The schema and extensions are used to resolve attribute paths and validate +// value expressions. The operations are applied in order. The returned +// attributes are a modified copy of the input. +func ApplyPatch(attrs ResourceAttributes, ops []PatchOperation, s schema.Schema, extensions ...schema.Schema) (ResourceAttributes, error) { + result := copyMap(attrs) + for _, op := range ops { + var err error + result, err = applyOperation(result, op, s, extensions) + if err != nil { + return nil, err + } + } + return result, nil +} + +func applyAdd(attrs ResourceAttributes, op PatchOperation, s schema.Schema, extensions []schema.Schema) (ResourceAttributes, error) { + if op.Path == nil { + return applyAddRoot(attrs, op.Value) + } + + attrName, target, err := resolveTarget(op.Path, s, extensions) + if err != nil { + return nil, err + } + + switch t := target.(type) { + case *attributeTarget: + existing, exists := attrs[attrName] + if !exists { + attrs[attrName] = op.Value + return attrs, nil + } + merged, err := mergeAdd(existing, op.Value, t.attr) + if err != nil { + return nil, err + } + attrs[attrName] = merged + case *subAttributeTarget: + existing, exists := attrs[attrName] + if !exists { + attrs[attrName] = map[string]interface{}{ + t.subAttrName: op.Value, + } + return attrs, nil + } + m, ok := existing.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("expected complex attribute for %s", attrName) + } + m = copyMap(m) + m[t.subAttrName] = op.Value + attrs[attrName] = m + case *valueExprTarget: + existing, exists := attrs[attrName] + if !exists { + return attrs, nil + } + list, ok := existing.([]interface{}) + if !ok { + return nil, fmt.Errorf("expected multi-valued attribute for %s", attrName) + } + list, err := applyToMatching(list, t, op.Value, false) + if err != nil { + return nil, err + } + attrs[attrName] = list + } + return attrs, nil +} + +func applyAddRoot(attrs ResourceAttributes, value interface{}) (ResourceAttributes, error) { + m, ok := value.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("value must be a map when path is omitted") + } + for k, v := range m { + if v == nil { + continue + } + existing, exists := attrs[k] + if !exists { + attrs[k] = v + continue + } + // For multi-valued attributes, append. + if existingList, ok := existing.([]interface{}); ok { + if newList, ok := v.([]interface{}); ok { + attrs[k] = append(existingList, newList...) + continue + } + } + // For complex attributes, merge. + if existingMap, ok := existing.(map[string]interface{}); ok { + if newMap, ok := v.(map[string]interface{}); ok { + merged := copyMap(existingMap) + for mk, mv := range newMap { + if mv != nil { + merged[mk] = mv + } + } + attrs[k] = merged + continue + } + } + // Otherwise replace. + attrs[k] = v + } + return attrs, nil +} + +func applyOperation(attrs ResourceAttributes, op PatchOperation, s schema.Schema, extensions []schema.Schema) (ResourceAttributes, error) { + switch op.Op { + case PatchOperationAdd: + return applyAdd(attrs, op, s, extensions) + case PatchOperationReplace: + return applyReplace(attrs, op, s, extensions) + case PatchOperationRemove: + return applyRemove(attrs, op, s, extensions) + default: + return nil, fmt.Errorf("unknown patch operation: %s", op.Op) + } +} + +func applyRemove(attrs ResourceAttributes, op PatchOperation, s schema.Schema, extensions []schema.Schema) (ResourceAttributes, error) { + if op.Path == nil { + return nil, fmt.Errorf("path is required for remove operations") + } + + attrName, target, err := resolveTarget(op.Path, s, extensions) + if err != nil { + return nil, err + } + + switch t := target.(type) { + case *attributeTarget: + delete(attrs, attrName) + case *subAttributeTarget: + existing, exists := attrs[attrName] + if !exists { + return attrs, nil + } + m, ok := existing.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("expected complex attribute for %s", attrName) + } + m = copyMap(m) + delete(m, t.subAttrName) + if len(m) == 0 { + delete(attrs, attrName) + } else { + attrs[attrName] = m + } + case *valueExprTarget: + existing, exists := attrs[attrName] + if !exists { + return attrs, nil + } + list, ok := existing.([]interface{}) + if !ok { + return nil, fmt.Errorf("expected multi-valued attribute for %s", attrName) + } + list, err := removeMatching(list, t) + if err != nil { + return nil, err + } + if len(list) == 0 { + delete(attrs, attrName) + } else { + attrs[attrName] = list + } + } + return attrs, nil +} + +func applyReplace(attrs ResourceAttributes, op PatchOperation, s schema.Schema, extensions []schema.Schema) (ResourceAttributes, error) { + if op.Path == nil { + return applyReplaceRoot(attrs, op.Value) + } + + attrName, target, err := resolveTarget(op.Path, s, extensions) + if err != nil { + return nil, err + } + + switch t := target.(type) { + case *attributeTarget: + attrs[attrName] = op.Value + case *subAttributeTarget: + existing, exists := attrs[attrName] + if !exists { + attrs[attrName] = map[string]interface{}{ + t.subAttrName: op.Value, + } + return attrs, nil + } + m, ok := existing.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("expected complex attribute for %s", attrName) + } + m = copyMap(m) + m[t.subAttrName] = op.Value + attrs[attrName] = m + case *valueExprTarget: + existing, exists := attrs[attrName] + if !exists { + return attrs, nil + } + list, ok := existing.([]interface{}) + if !ok { + return nil, fmt.Errorf("expected multi-valued attribute for %s", attrName) + } + list, err := applyToMatching(list, t, op.Value, true) + if err != nil { + return nil, err + } + attrs[attrName] = list + } + return attrs, nil +} + +func applyReplaceRoot(attrs ResourceAttributes, value interface{}) (ResourceAttributes, error) { + m, ok := value.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("value must be a map when path is omitted") + } + for k, v := range m { + if v == nil { + delete(attrs, k) + continue + } + attrs[k] = v + } + return attrs, nil +} + +// target types returned by resolveTarget. +type attributeTarget struct { + attr schema.CoreAttribute +} + +type subAttributeTarget struct { + attr schema.CoreAttribute + subAttrName string +} + +type valueExprTarget struct { + attr schema.CoreAttribute + expr filter.Expression + subAttrName string + refSchema schema.Schema +} diff --git a/patch_apply_test.go b/patch_apply_test.go new file mode 100644 index 0000000..ff6cd76 --- /dev/null +++ b/patch_apply_test.go @@ -0,0 +1,421 @@ +package scim + +import ( + "testing" + + "github.com/elimity-com/scim/schema" + filterlib "github.com/scim2/filter-parser/v2" +) + +func TestApplyPatch_AddSimpleAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("displayName"), Value: "John Doe"}, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["displayName"] != "John Doe" { + t.Errorf("expected displayName to be 'John Doe', got %v", result["displayName"]) + } + // Original should not be modified. + if _, ok := attrs["displayName"]; ok { + t.Error("original attrs should not be modified") + } +} + +func TestApplyPatch_AddSubAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "name": map[string]interface{}{ + "givenName": "John", + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("name.familyName"), Value: "Doe"}, + }, s) + if err != nil { + t.Fatal(err) + } + + name, ok := result["name"].(map[string]interface{}) + if !ok { + t.Fatal("expected name to be a map") + } + if name["familyName"] != "Doe" { + t.Errorf("expected familyName to be 'Doe', got %v", name["familyName"]) + } + if name["givenName"] != "John" { + t.Errorf("expected givenName to remain 'John', got %v", name["givenName"]) + } +} + +func TestApplyPatch_AddToMultiValued(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@example.com", + "type": "work", + }, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("emails"), Value: []interface{}{ + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }}, + }, s) + if err != nil { + t.Fatal(err) + } + + emails, ok := result["emails"].([]interface{}) + if !ok { + t.Fatal("expected emails to be a list") + } + if len(emails) != 2 { + t.Fatalf("expected 2 emails, got %d", len(emails)) + } +} + +func TestApplyPatch_AddWithNoPath(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationAdd, + Value: map[string]interface{}{ + "displayName": "John Doe", + "userName": "johnny", + }, + }, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["displayName"] != "John Doe" { + t.Errorf("expected displayName to be 'John Doe', got %v", result["displayName"]) + } + if result["userName"] != "johnny" { + t.Errorf("expected userName to be 'johnny', got %v", result["userName"]) + } +} + +func TestApplyPatch_DoesNotMutateOriginal(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "name": map[string]interface{}{ + "givenName": "John", + }, + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("userName"), Value: "jane"}, + {Op: PatchOperationAdd, Path: mustParsePath("name.familyName"), Value: "Doe"}, + }, s) + if err != nil { + t.Fatal(err) + } + + if attrs["userName"] != "john" { + t.Error("original userName should not be modified") + } + name := attrs["name"].(map[string]interface{}) + if _, ok := name["familyName"]; ok { + t.Error("original name map should not be modified") + } +} + +func TestApplyPatch_MultipleOperations(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("displayName"), Value: "John Doe"}, + {Op: PatchOperationReplace, Path: mustParsePath("userName"), Value: "jane"}, + {Op: PatchOperationRemove, Path: mustParsePath("displayName")}, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["userName"] != "jane" { + t.Errorf("expected userName to be 'jane', got %v", result["userName"]) + } + if _, ok := result["displayName"]; ok { + t.Error("expected displayName to be removed after sequence of operations") + } +} + +func TestApplyPatch_RemoveAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "displayName": "John Doe", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove, Path: mustParsePath("displayName")}, + }, s) + if err != nil { + t.Fatal(err) + } + + if _, ok := result["displayName"]; ok { + t.Error("expected displayName to be removed") + } + if result["userName"] != "john" { + t.Error("expected userName to remain") + } +} + +func TestApplyPatch_RemoveSubAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "name": map[string]interface{}{ + "givenName": "John", + "familyName": "Doe", + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove, Path: mustParsePath("name.familyName")}, + }, s) + if err != nil { + t.Fatal(err) + } + + name, ok := result["name"].(map[string]interface{}) + if !ok { + t.Fatal("expected name to be a map") + } + if _, ok := name["familyName"]; ok { + t.Error("expected familyName to be removed") + } + if name["givenName"] != "John" { + t.Errorf("expected givenName to remain 'John'") + } +} + +func TestApplyPatch_RemoveSubAttributeFromValueExpression(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + "primary": true, + }, + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationRemove, + Path: mustParsePath(`emails[type eq "work"].primary`), + }, + }, s) + if err != nil { + t.Fatal(err) + } + + emails := result["emails"].([]interface{}) + workEmail := emails[0].(map[string]interface{}) + if _, ok := workEmail["primary"]; ok { + t.Error("expected primary to be removed from work email") + } + if workEmail["value"] != "john@work.com" { + t.Error("expected value to remain on work email") + } +} + +func TestApplyPatch_RemoveWithValueExpression(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + }, + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationRemove, + Path: mustParsePath(`emails[type eq "work"]`), + }, + }, s) + if err != nil { + t.Fatal(err) + } + + emails, ok := result["emails"].([]interface{}) + if !ok { + t.Fatal("expected emails to be a list") + } + if len(emails) != 1 { + t.Fatalf("expected 1 email, got %d", len(emails)) + } + remaining := emails[0].(map[string]interface{}) + if remaining["type"] != "home" { + t.Errorf("expected home email to remain, got %v", remaining["type"]) + } +} + +func TestApplyPatch_ReplaceSimpleAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("userName"), Value: "jane"}, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["userName"] != "jane" { + t.Errorf("expected userName to be 'jane', got %v", result["userName"]) + } +} + +func TestApplyPatch_ReplaceWithNoPath(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "displayName": "John Doe", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Value: map[string]interface{}{ + "displayName": nil, + "userName": "jane", + }, + }, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["userName"] != "jane" { + t.Errorf("expected userName to be 'jane', got %v", result["userName"]) + } + if _, ok := result["displayName"]; ok { + t.Error("expected displayName to be removed") + } +} + +func TestApplyPatch_ReplaceWithValueExpression(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + }, + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Path: mustParsePath(`emails[type eq "work"].value`), + Value: "john@newwork.com", + }, + }, s) + if err != nil { + t.Fatal(err) + } + + emails, ok := result["emails"].([]interface{}) + if !ok { + t.Fatal("expected emails to be a list") + } + workEmail := emails[0].(map[string]interface{}) + if workEmail["value"] != "john@newwork.com" { + t.Errorf("expected work email to be updated, got %v", workEmail["value"]) + } + homeEmail := emails[1].(map[string]interface{}) + if homeEmail["value"] != "john@home.com" { + t.Errorf("expected home email to remain, got %v", homeEmail["value"]) + } +} + +func mustParsePath(s string) *filterlib.Path { + p, err := filterlib.ParsePath([]byte(s)) + if err != nil { + panic(err) + } + return &p +} + +func testUserSchema() schema.Schema { + return schema.Schema{ + ID: schema.UserSchema, + Attributes: schema.Attributes{ + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "userName", + })), + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "displayName", + })), + schema.ComplexCoreAttribute(schema.ComplexParams{ + Name: "name", + SubAttributes: []schema.SimpleParams{ + schema.SimpleStringParams(schema.StringParams{Name: "givenName"}), + schema.SimpleStringParams(schema.StringParams{Name: "familyName"}), + }, + }), + schema.ComplexCoreAttribute(schema.ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []schema.SimpleParams{ + schema.SimpleStringParams(schema.StringParams{Name: "value"}), + schema.SimpleStringParams(schema.StringParams{Name: "type"}), + schema.SimpleBooleanParams(schema.BooleanParams{Name: "primary"}), + }, + }), + schema.ComplexCoreAttribute(schema.ComplexParams{ + Name: "members", + MultiValued: true, + SubAttributes: []schema.SimpleParams{ + schema.SimpleStringParams(schema.StringParams{Name: "value"}), + schema.SimpleStringParams(schema.StringParams{Name: "displayName"}), + }, + }), + }, + } +} From ab836ea090764524fefe2012363321009a5abb81 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Wed, 25 Mar 2026 09:47:34 +0100 Subject: [PATCH 2/7] fix: return noTarget error for replace with unmatched value expression --- patch_apply.go | 20 ++++++++++++++----- patch_apply_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++--- utils_test.go | 14 +++++++++++++ 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/patch_apply.go b/patch_apply.go index df2066b..518a82d 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -3,15 +3,17 @@ package scim import ( "fmt" + scimErrors "github.com/elimity-com/scim/errors" internal "github.com/elimity-com/scim/filter" "github.com/elimity-com/scim/schema" filter "github.com/scim2/filter-parser/v2" ) -func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, replace bool) ([]interface{}, error) { +func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, replace bool) ([]interface{}, bool, error) { result := make([]interface{}, len(list)) copy(result, list) + matched := false for i, elem := range result { m, ok := elem.(map[string]interface{}) if !ok { @@ -20,6 +22,7 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, if !matchesExpression(m, t.expr, t.attr, t.refSchema) { continue } + matched = true if t.subAttrName != "" { updated := copyMap(m) updated[t.subAttrName] = value @@ -49,7 +52,7 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, } } } - return result, nil + return result, matched, nil } func copyMap(m map[string]interface{}) map[string]interface{} { @@ -263,7 +266,7 @@ func applyAdd(attrs ResourceAttributes, op PatchOperation, s schema.Schema, exte if !ok { return nil, fmt.Errorf("expected multi-valued attribute for %s", attrName) } - list, err := applyToMatching(list, t, op.Value, false) + list, _, err := applyToMatching(list, t, op.Value, false) if err != nil { return nil, err } @@ -405,18 +408,25 @@ func applyReplace(attrs ResourceAttributes, op PatchOperation, s schema.Schema, m[t.subAttrName] = op.Value attrs[attrName] = m case *valueExprTarget: + // RFC 7644 Section 3.5.2.3: if the target location is a multi-valued + // attribute for which a value selection filter ("valuePath") has been + // supplied and no record match was made, the service provider SHALL + // return a 400 error with a scimType of noTarget. existing, exists := attrs[attrName] if !exists { - return attrs, nil + return nil, scimErrors.ScimErrorNoTarget } list, ok := existing.([]interface{}) if !ok { return nil, fmt.Errorf("expected multi-valued attribute for %s", attrName) } - list, err := applyToMatching(list, t, op.Value, true) + list, matched, err := applyToMatching(list, t, op.Value, true) if err != nil { return nil, err } + if !matched { + return nil, scimErrors.ScimErrorNoTarget + } attrs[attrName] = list } return attrs, nil diff --git a/patch_apply_test.go b/patch_apply_test.go index ff6cd76..246e849 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -3,8 +3,9 @@ package scim import ( "testing" + scimErrors "github.com/elimity-com/scim/errors" "github.com/elimity-com/scim/schema" - filterlib "github.com/scim2/filter-parser/v2" + filter "github.com/scim2/filter-parser/v2" ) func TestApplyPatch_AddSimpleAttribute(t *testing.T) { @@ -306,6 +307,47 @@ func TestApplyPatch_ReplaceSimpleAttribute(t *testing.T) { } } +// RFC 7644 Section 3.5.2.3: "If the target location is a multi-valued +// attribute for which a value selection filter ("valuePath") has been +// supplied and no record match was made, the service provider SHALL +// return a 400 error with a scimType of noTarget". +func TestApplyPatch_ReplaceValueExprNoTarget_AttributeMissing(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Path: mustParsePath(`emails[type eq "work"].value`), + Value: "new@work.com", + }, + }, s) + assertScimError(t, err, scimErrors.ScimTypeNoTarget) +} + +func TestApplyPatch_ReplaceValueExprNoTarget_NoMatch(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Path: mustParsePath(`emails[type eq "work"].value`), + Value: "new@work.com", + }, + }, s) + assertScimError(t, err, scimErrors.ScimTypeNoTarget) +} + func TestApplyPatch_ReplaceWithNoPath(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -374,8 +416,8 @@ func TestApplyPatch_ReplaceWithValueExpression(t *testing.T) { } } -func mustParsePath(s string) *filterlib.Path { - p, err := filterlib.ParsePath([]byte(s)) +func mustParsePath(s string) *filter.Path { + p, err := filter.ParsePath([]byte(s)) if err != nil { panic(err) } diff --git a/utils_test.go b/utils_test.go index 24ddd1d..ba2ac7a 100644 --- a/utils_test.go +++ b/utils_test.go @@ -69,6 +69,20 @@ func assertNotNil(t *testing.T, object interface{}, name string) { } } +func assertScimError(t *testing.T, err error, expectedType errors.ScimType) { + t.Helper() + if err == nil { + t.Fatalf("expected scimType %q error, got nil", expectedType) + } + scimErr, ok := err.(errors.ScimError) + if !ok { + t.Fatalf("expected ScimError, got %T: %v", err, err) + } + if scimErr.ScimType != expectedType { + t.Errorf("expected scimType %q, got %q", expectedType, scimErr.ScimType) + } +} + func assertTrue(t *testing.T, ok bool) { if !ok { t.Error("value should be true") From f118cdf7ea6b67030d7b603e5a62ee79251dceb0 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Wed, 25 Mar 2026 10:19:56 +0100 Subject: [PATCH 3/7] feat: add mutability checks and complex attribute merge for patch ops Validate readOnly and immutable attribute mutability per RFC 7644 Section 3.5.2 before applying add, remove, and replace operations. Implement sub-attribute merging for replace on singular complex attributes (Section 3.5.2.3) so unspecified sub-attributes are left unchanged. Return noTarget error for remove without a path (Section 3.5.2.2). --- patch_apply.go | 72 +++++++++++- patch_apply_test.go | 267 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 333 insertions(+), 6 deletions(-) diff --git a/patch_apply.go b/patch_apply.go index 518a82d..4698c7d 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -55,6 +55,33 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, return result, matched, nil } +func attrFromTarget(target interface{}) schema.CoreAttribute { + switch t := target.(type) { + case *attributeTarget: + return t.attr + case *subAttributeTarget: + return t.attr + case *valueExprTarget: + return t.attr + } + panic("unknown target type") +} + +// checkMutability validates that the operation is compatible with the +// attribute's mutability. Returns a mutability ScimError if not. +func checkMutability(op string, attr schema.CoreAttribute, exists bool) error { + switch attr.Mutability() { + case "readOnly": + return scimErrors.ScimErrorMutability + case "immutable": + if op == PatchOperationAdd && !exists { + return nil + } + return scimErrors.ScimErrorMutability + } + return nil +} + func copyMap(m map[string]interface{}) map[string]interface{} { cp := make(map[string]interface{}, len(m)) for k, v := range m { @@ -230,6 +257,11 @@ func applyAdd(attrs ResourceAttributes, op PatchOperation, s schema.Schema, exte return nil, err } + _, exists := attrs[attrName] + if err := checkMutability(op.Op, attrFromTarget(target), exists); err != nil { + return nil, err + } + switch t := target.(type) { case *attributeTarget: existing, exists := attrs[attrName] @@ -330,7 +362,7 @@ func applyOperation(attrs ResourceAttributes, op PatchOperation, s schema.Schema func applyRemove(attrs ResourceAttributes, op PatchOperation, s schema.Schema, extensions []schema.Schema) (ResourceAttributes, error) { if op.Path == nil { - return nil, fmt.Errorf("path is required for remove operations") + return nil, scimErrors.ScimErrorNoTarget } attrName, target, err := resolveTarget(op.Path, s, extensions) @@ -338,6 +370,11 @@ func applyRemove(attrs ResourceAttributes, op PatchOperation, s schema.Schema, e return nil, err } + _, exists := attrs[attrName] + if err := checkMutability(op.Op, attrFromTarget(target), exists); err != nil { + return nil, err + } + switch t := target.(type) { case *attributeTarget: delete(attrs, attrName) @@ -389,8 +426,38 @@ func applyReplace(attrs ResourceAttributes, op PatchOperation, s schema.Schema, return nil, err } + _, exists := attrs[attrName] + if err := checkMutability(op.Op, attrFromTarget(target), exists); err != nil { + return nil, err + } + switch t := target.(type) { case *attributeTarget: + // RFC 7644 Section 3.5.2.3: if the target location path specifies + // an attribute that does not exist, the service provider SHALL + // treat the operation as an "add". + if _, exists := attrs[attrName]; !exists { + return applyAdd(attrs, op, s, extensions) + } + // RFC 7644 Section 3.5.2.3: "If the target location specifies a + // complex attribute, a set of sub-attributes SHALL be specified in + // the 'value' parameter, which replaces any existing values or adds + // where an attribute did not previously exist. Sub-attributes that + // are not specified in the 'value' parameter are left unchanged." + if t.attr.HasSubAttributes() && !t.attr.MultiValued() { + existingMap, ok := attrs[attrName].(map[string]interface{}) + if ok { + valueMap, ok := op.Value.(map[string]interface{}) + if ok { + merged := copyMap(existingMap) + for k, v := range valueMap { + merged[k] = v + } + attrs[attrName] = merged + return attrs, nil + } + } + } attrs[attrName] = op.Value case *subAttributeTarget: existing, exists := attrs[attrName] @@ -411,7 +478,8 @@ func applyReplace(attrs ResourceAttributes, op PatchOperation, s schema.Schema, // RFC 7644 Section 3.5.2.3: if the target location is a multi-valued // attribute for which a value selection filter ("valuePath") has been // supplied and no record match was made, the service provider SHALL - // return a 400 error with a scimType of noTarget. + // indicate failure by returning HTTP status code 400 and a "scimType" + // error code of "noTarget". existing, exists := attrs[attrName] if !exists { return nil, scimErrors.ScimErrorNoTarget diff --git a/patch_apply_test.go b/patch_apply_test.go index 246e849..0ed95b5 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -57,6 +57,28 @@ func TestApplyPatch_AddSubAttribute(t *testing.T) { } } +func TestApplyPatch_AddSubAttribute_ParentNotExist(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("name.familyName"), Value: "Doe"}, + }, s) + if err != nil { + t.Fatal(err) + } + + name, ok := result["name"].(map[string]interface{}) + if !ok { + t.Fatal("expected name to be a map") + } + if name["familyName"] != "Doe" { + t.Errorf("expected familyName 'Doe', got %v", name["familyName"]) + } +} + func TestApplyPatch_AddToMultiValued(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -116,6 +138,43 @@ func TestApplyPatch_AddWithNoPath(t *testing.T) { } } +func TestApplyPatch_AddWithValueExpression(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + }, + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationAdd, + Path: mustParsePath(`emails[type eq "work"].primary`), + Value: true, + }, + }, s) + if err != nil { + t.Fatal(err) + } + + emails := result["emails"].([]interface{}) + workEmail := emails[0].(map[string]interface{}) + if workEmail["primary"] != true { + t.Errorf("expected primary to be true on work email, got %v", workEmail["primary"]) + } + homeEmail := emails[1].(map[string]interface{}) + if _, ok := homeEmail["primary"]; ok { + t.Error("expected home email to not have primary set") + } +} + func TestApplyPatch_DoesNotMutateOriginal(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -142,6 +201,47 @@ func TestApplyPatch_DoesNotMutateOriginal(t *testing.T) { } } +// RFC 7644 Section 3.5.2: "a client MAY 'add' a value to an 'immutable' +// attribute if the attribute had no previous value". +func TestApplyPatch_ImmutableAttribute_AddNew_Succeeds(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{} + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("externalId"), Value: "ext-1"}, + }, s) + if err != nil { + t.Fatal(err) + } + if result["externalId"] != "ext-1" { + t.Errorf("expected externalId 'ext-1', got %v", result["externalId"]) + } +} + +func TestApplyPatch_ImmutableAttribute_Remove_ReturnsMutability(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "externalId": "ext-1", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove, Path: mustParsePath("externalId")}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeMutability) +} + +func TestApplyPatch_ImmutableAttribute_Replace_ReturnsMutability(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "externalId": "ext-1", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("externalId"), Value: "ext-2"}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeMutability) +} + func TestApplyPatch_MultipleOperations(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -165,6 +265,42 @@ func TestApplyPatch_MultipleOperations(t *testing.T) { } } +// RFC 7644 Section 3.5.2: "a client MUST NOT modify an attribute that has +// mutability 'readOnly'". +func TestApplyPatch_ReadOnlyAttribute_Add_ReturnsMutability(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{} + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("id"), Value: "123"}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeMutability) +} + +func TestApplyPatch_ReadOnlyAttribute_Remove_ReturnsMutability(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "id": "123", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove, Path: mustParsePath("id")}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeMutability) +} + +func TestApplyPatch_ReadOnlyAttribute_Replace_ReturnsMutability(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "id": "123", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("id"), Value: "456"}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeMutability) +} + func TestApplyPatch_RemoveAttribute(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -251,6 +387,20 @@ func TestApplyPatch_RemoveSubAttributeFromValueExpression(t *testing.T) { } } +// RFC 7644 Section 3.5.2.2: "If 'path' is unspecified, the operation fails +// with HTTP status code 400 and a 'scimType' error code of 'noTarget'". +func TestApplyPatch_RemoveWithNoPath_ReturnsNoTarget(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeNoTarget) +} + func TestApplyPatch_RemoveWithValueExpression(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -289,6 +439,107 @@ func TestApplyPatch_RemoveWithValueExpression(t *testing.T) { } } +// RFC 7644 Section 3.5.2.3: "If the target location specifies a complex +// attribute, a set of sub-attributes SHALL be specified in the 'value' +// parameter, which replaces any existing values or adds where an attribute did +// not previously exist. Sub-attributes that are not specified in the 'value' +// parameter are left unchanged". +func TestApplyPatch_ReplaceComplexAttribute_MergesSubAttributes(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "name": map[string]interface{}{ + "givenName": "John", + "familyName": "Doe", + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Path: mustParsePath("name"), + Value: map[string]interface{}{ + "familyName": "Smith", + }, + }, + }, s) + if err != nil { + t.Fatal(err) + } + + name, ok := result["name"].(map[string]interface{}) + if !ok { + t.Fatal("expected name to be a map") + } + if name["familyName"] != "Smith" { + t.Errorf("expected familyName to be 'Smith', got %v", name["familyName"]) + } + if name["givenName"] != "John" { + t.Errorf("expected givenName to remain 'John', got %v", name["givenName"]) + } +} + +func TestApplyPatch_ReplaceMultiValuedWithoutFilter(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{ + "value": "john@work.com", + "type": "work", + }, + map[string]interface{}{ + "value": "john@home.com", + "type": "home", + }, + }, + } + + newEmails := []interface{}{ + map[string]interface{}{ + "value": "jane@new.com", + "type": "new", + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("emails"), Value: newEmails}, + }, s) + if err != nil { + t.Fatal(err) + } + + emails, ok := result["emails"].([]interface{}) + if !ok { + t.Fatal("expected emails to be a list") + } + if len(emails) != 1 { + t.Fatalf("expected 1 email, got %d", len(emails)) + } + email := emails[0].(map[string]interface{}) + if email["value"] != "jane@new.com" { + t.Errorf("expected email 'jane@new.com', got %v", email["value"]) + } +} + +// RFC 7644 Section 3.5.2.3: "If the target location path specifies an attribute +// that does not exist, the service provider SHALL treat the operation as an 'add'". +func TestApplyPatch_ReplaceNonExistentAttribute_TreatedAsAdd(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("displayName"), Value: "John Doe"}, + }, s) + if err != nil { + t.Fatal(err) + } + + if result["displayName"] != "John Doe" { + t.Errorf("expected displayName to be 'John Doe', got %v", result["displayName"]) + } +} + func TestApplyPatch_ReplaceSimpleAttribute(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -307,10 +558,10 @@ func TestApplyPatch_ReplaceSimpleAttribute(t *testing.T) { } } -// RFC 7644 Section 3.5.2.3: "If the target location is a multi-valued -// attribute for which a value selection filter ("valuePath") has been -// supplied and no record match was made, the service provider SHALL -// return a 400 error with a scimType of noTarget". +// RFC 7644 Section 3.5.2.3: "If the target location is a multi-valued attribute +// for which a value selection filter ('valuePath') has been supplied and no +// record match was made, the service provider SHALL indicate failure by +// returning HTTP status code 400 and a 'scimType' error code of 'noTarget'". func TestApplyPatch_ReplaceValueExprNoTarget_AttributeMissing(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -434,6 +685,14 @@ func testUserSchema() schema.Schema { schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ Name: "displayName", })), + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "externalId", + Mutability: schema.AttributeMutabilityImmutable(), + })), + schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ + Name: "id", + Mutability: schema.AttributeMutabilityReadOnly(), + })), schema.ComplexCoreAttribute(schema.ComplexParams{ Name: "name", SubAttributes: []schema.SimpleParams{ From a3ffafaa5a51e1fc02684e8dfa3bb248ffa3adef Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Thu, 26 Mar 2026 16:58:13 +0100 Subject: [PATCH 4/7] fix: reject PATCH remove on required attributes with invalidValue --- patch_apply.go | 3 +++ patch_apply_test.go | 16 +++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/patch_apply.go b/patch_apply.go index 4698c7d..52ea7f1 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -377,6 +377,9 @@ func applyRemove(attrs ResourceAttributes, op PatchOperation, s schema.Schema, e switch t := target.(type) { case *attributeTarget: + if t.attr.Required() { + return nil, scimErrors.ScimErrorInvalidValue + } delete(attrs, attrName) case *subAttributeTarget: existing, exists := attrs[attrName] diff --git a/patch_apply_test.go b/patch_apply_test.go index 0ed95b5..ec2cf26 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -323,6 +323,19 @@ func TestApplyPatch_RemoveAttribute(t *testing.T) { } } +func TestApplyPatch_RemoveRequiredAttribute(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "displayName": "John Doe", + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationRemove, Path: mustParsePath("userName")}, + }, s) + assertScimError(t, err, scimErrors.ScimTypeInvalidValue) +} + func TestApplyPatch_RemoveSubAttribute(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -680,7 +693,8 @@ func testUserSchema() schema.Schema { ID: schema.UserSchema, Attributes: schema.Attributes{ schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ - Name: "userName", + Name: "userName", + Required: true, })), schema.SimpleCoreAttribute(schema.SimpleStringParams(schema.StringParams{ Name: "displayName", From ed588642e8e7be924d88e26a7b7ce7700ce70692 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Thu, 26 Mar 2026 17:06:30 +0100 Subject: [PATCH 5/7] feat: reject duplicate (type, value) pairs in multi-valued attributes Enforce uniqueness of (type, value) pairs in multi-valued complex attributes per RFC 7643 Section 2.4. Validation is applied both during schema validation and after patch operations, returning a uniqueness error when duplicates are detected. --- patch_apply.go | 28 ++++++++++++++++ patch_apply_test.go | 65 +++++++++++++++++++++++++++++++++++++ schema/core.go | 45 ++++++++++++++++++++++++++ schema/core_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 216 insertions(+) diff --git a/patch_apply.go b/patch_apply.go index 52ea7f1..88806a0 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -67,6 +67,31 @@ func attrFromTarget(target interface{}) schema.CoreAttribute { panic("unknown target type") } +func checkDuplicateTypeValuePairs(attrs ResourceAttributes, s schema.Schema, extensions []schema.Schema) error { + allAttrs := make([]schema.CoreAttribute, 0, len(s.Attributes)) + allAttrs = append(allAttrs, s.Attributes...) + for _, ext := range extensions { + allAttrs = append(allAttrs, ext.Attributes...) + } + for _, attr := range allAttrs { + if !attr.MultiValued() || !attr.HasSubAttributes() || !attr.HasTypeAndValueSubAttrs() { + continue + } + val, ok := attrs[attr.Name()] + if !ok { + continue + } + list, ok := val.([]interface{}) + if !ok { + continue + } + if schema.HasDuplicateTypeValuePairs(list) { + return scimErrors.ScimErrorUniqueness + } + } + return nil +} + // checkMutability validates that the operation is compatible with the // attribute's mutability. Returns a mutability ScimError if not. func checkMutability(op string, attr schema.CoreAttribute, exists bool) error { @@ -244,6 +269,9 @@ func ApplyPatch(attrs ResourceAttributes, ops []PatchOperation, s schema.Schema, return nil, err } } + if err := checkDuplicateTypeValuePairs(result, s, extensions); err != nil { + return nil, err + } return result, nil } diff --git a/patch_apply_test.go b/patch_apply_test.go index ec2cf26..2f7ac02 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -8,6 +8,51 @@ import ( filter "github.com/scim2/filter-parser/v2" ) +func TestApplyPatch_AddDistinctTypeValuePair(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "emails": []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("emails"), Value: []interface{}{ + map[string]interface{}{"type": "home", "value": "john@home.com"}, + }}, + }, s) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + emails, ok := result["emails"].([]interface{}) + if !ok { + t.Fatal("expected emails to be a list") + } + if len(emails) != 2 { + t.Errorf("expected 2 emails, got %d", len(emails)) + } +} + +func TestApplyPatch_AddDuplicateTypeValuePair(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "emails": []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }, + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("emails"), Value: []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }}, + }, s) + if err == nil { + t.Error("expected error for duplicate (type, value) pair after add") + } +} + func TestApplyPatch_AddSimpleAttribute(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ @@ -491,6 +536,26 @@ func TestApplyPatch_ReplaceComplexAttribute_MergesSubAttributes(t *testing.T) { } } +func TestApplyPatch_ReplaceDuplicateTypeValuePair(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "userName": "john", + "emails": []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }, + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationReplace, Path: mustParsePath("emails"), Value: []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }}, + }, s) + if err == nil { + t.Error("expected error for duplicate (type, value) pair after replace") + } +} + func TestApplyPatch_ReplaceMultiValuedWithoutFilter(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ diff --git a/schema/core.go b/schema/core.go index 61d5291..4dd654e 100644 --- a/schema/core.go +++ b/schema/core.go @@ -16,6 +16,30 @@ var ( schemaAllowStringValues = false ) +// HasDuplicateTypeValuePairs reports whether the given list of multi-valued +// complex attribute elements contains duplicate (type, value) pairs. +func HasDuplicateTypeValuePairs(elements []interface{}) bool { + type pair struct{ typ, val string } + seen := make(map[pair]bool) + for _, elem := range elements { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + typ, hasType := m["type"].(string) + val, hasValue := m["value"].(string) + if !hasType || !hasValue { + continue + } + p := pair{typ, val} + if seen[p] { + return true + } + seen[p] = true + } + return false +} + // SetAllowStringValues sets whether string values are allowed. // If enabled, string values are allowed for booleans, integer and decimal attributes. // NOTE: This is NOT a standard SCIM behaviour, and should only be used for compatibility with non-compliant SCIM @@ -137,6 +161,22 @@ func (a CoreAttribute) HasSubAttributes() bool { return a.typ == attributeDataTypeComplex && len(a.subAttributes) != 0 } +// HasTypeAndValueSubAttrs reports whether the attribute has both "type" and +// "value" sub-attributes, which is the combination RFC 7643 Section 2.4 uses +// for duplicate detection in multi-valued attributes. +func (a CoreAttribute) HasTypeAndValueSubAttrs() bool { + hasType, hasValue := false, false + for _, sub := range a.subAttributes { + switch strings.ToLower(sub.name) { + case "type": + hasType = true + case "value": + hasValue = true + } + } + return hasType && hasValue +} + // MultiValued returns whether the attribute is multi valued. func (a CoreAttribute) MultiValued() bool { return a.multiValued @@ -417,6 +457,11 @@ func (a CoreAttribute) validate(attribute interface{}) (interface{}, *errors.Sci } attributes = append(attributes, attr) } + if a.typ == attributeDataTypeComplex && a.HasTypeAndValueSubAttrs() { + if HasDuplicateTypeValuePairs(attributes) { + return nil, &errors.ScimErrorUniqueness + } + } return attributes, nil default: diff --git a/schema/core_test.go b/schema/core_test.go index 8fdbeff..7108bf8 100644 --- a/schema/core_test.go +++ b/schema/core_test.go @@ -73,3 +73,81 @@ func TestCoreAttribute_WithReturned(t *testing.T) { t.Error("WithReturned modified the original attribute") } } + +func TestCoreAttribute_validate_allowsDistinctTypeValuePairs(t *testing.T) { + emails := ComplexCoreAttribute(ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "type"}), + SimpleBooleanParams(BooleanParams{Name: "primary"}), + }, + }) + + _, scimErr := emails.validate([]interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + map[string]interface{}{"type": "home", "value": "john@home.com"}, + }) + if scimErr != nil { + t.Errorf("unexpected error for distinct (type, value) pairs: %v", scimErr) + } +} + +func TestCoreAttribute_validate_allowsDuplicateTypeWithDifferentValue(t *testing.T) { + emails := ComplexCoreAttribute(ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "type"}), + }, + }) + + _, scimErr := emails.validate([]interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + map[string]interface{}{"type": "work", "value": "jane@work.com"}, + }) + if scimErr != nil { + t.Errorf("unexpected error for same type with different value: %v", scimErr) + } +} + +func TestCoreAttribute_validate_rejectsDuplicateTypeValuePairs(t *testing.T) { + emails := ComplexCoreAttribute(ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "type"}), + SimpleBooleanParams(BooleanParams{Name: "primary"}), + }, + }) + + _, scimErr := emails.validate([]interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }) + if scimErr == nil { + t.Error("expected error for duplicate (type, value) pairs") + } +} + +func TestCoreAttribute_validate_skipsDuplicateCheckWithoutTypeSubAttr(t *testing.T) { + members := ComplexCoreAttribute(ComplexParams{ + Name: "members", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "displayName"}), + }, + }) + + _, scimErr := members.validate([]interface{}{ + map[string]interface{}{"value": "user1", "displayName": "User 1"}, + map[string]interface{}{"value": "user1", "displayName": "User 1"}, + }) + if scimErr != nil { + t.Errorf("unexpected error for attribute without type sub-attribute: %v", scimErr) + } +} From 1016bfaaffe26d0767d8dd040b87fd5b28c14235 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Thu, 26 Mar 2026 17:14:54 +0100 Subject: [PATCH 6/7] feat: enforce at-most-one primary constraint on multi-valued attributes RFC 7643 Section 2.4 requires that the primary attribute value "true" appears no more than once in a multi-valued attribute. Add HasDuplicatePrimary and HasPrimarySubAttr to detect violations during both schema validation and patch application. --- patch_apply.go | 11 +++++++---- patch_apply_test.go | 18 ++++++++++++++++++ schema/core.go | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/patch_apply.go b/patch_apply.go index 88806a0..81fd089 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -67,14 +67,14 @@ func attrFromTarget(target interface{}) schema.CoreAttribute { panic("unknown target type") } -func checkDuplicateTypeValuePairs(attrs ResourceAttributes, s schema.Schema, extensions []schema.Schema) error { +func checkMultiValuedUniqueness(attrs ResourceAttributes, s schema.Schema, extensions []schema.Schema) error { allAttrs := make([]schema.CoreAttribute, 0, len(s.Attributes)) allAttrs = append(allAttrs, s.Attributes...) for _, ext := range extensions { allAttrs = append(allAttrs, ext.Attributes...) } for _, attr := range allAttrs { - if !attr.MultiValued() || !attr.HasSubAttributes() || !attr.HasTypeAndValueSubAttrs() { + if !attr.MultiValued() || !attr.HasSubAttributes() { continue } val, ok := attrs[attr.Name()] @@ -85,7 +85,10 @@ func checkDuplicateTypeValuePairs(attrs ResourceAttributes, s schema.Schema, ext if !ok { continue } - if schema.HasDuplicateTypeValuePairs(list) { + if attr.HasTypeAndValueSubAttrs() && schema.HasDuplicateTypeValuePairs(list) { + return scimErrors.ScimErrorUniqueness + } + if attr.HasPrimarySubAttr() && schema.HasDuplicatePrimary(list) { return scimErrors.ScimErrorUniqueness } } @@ -269,7 +272,7 @@ func ApplyPatch(attrs ResourceAttributes, ops []PatchOperation, s schema.Schema, return nil, err } } - if err := checkDuplicateTypeValuePairs(result, s, extensions); err != nil { + if err := checkMultiValuedUniqueness(result, s, extensions); err != nil { return nil, err } return result, nil diff --git a/patch_apply_test.go b/patch_apply_test.go index 2f7ac02..6a4d337 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -34,6 +34,24 @@ func TestApplyPatch_AddDistinctTypeValuePair(t *testing.T) { } } +func TestApplyPatch_AddDuplicatePrimary(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com", "primary": true}, + }, + } + + _, err := ApplyPatch(attrs, []PatchOperation{ + {Op: PatchOperationAdd, Path: mustParsePath("emails"), Value: []interface{}{ + map[string]interface{}{"type": "home", "value": "john@home.com", "primary": true}, + }}, + }, s) + if err == nil { + t.Error("expected error for duplicate primary after add") + } +} + func TestApplyPatch_AddDuplicateTypeValuePair(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ diff --git a/schema/core.go b/schema/core.go index 4dd654e..f650371 100644 --- a/schema/core.go +++ b/schema/core.go @@ -16,6 +16,26 @@ var ( schemaAllowStringValues = false ) +// HasDuplicatePrimary reports whether more than one element in the given list +// has primary set to true. RFC 7643 Section 2.4: "The primary attribute value +// 'true' MUST appear no more than once". +func HasDuplicatePrimary(elements []interface{}) bool { + count := 0 + for _, elem := range elements { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + if p, ok := m["primary"].(bool); ok && p { + count++ + if count > 1 { + return true + } + } + } + return false +} + // HasDuplicateTypeValuePairs reports whether the given list of multi-valued // complex attribute elements contains duplicate (type, value) pairs. func HasDuplicateTypeValuePairs(elements []interface{}) bool { @@ -156,6 +176,17 @@ func (a CoreAttribute) Description() string { return a.description.Value() } +// HasPrimarySubAttr reports whether the attribute has a "primary" +// sub-attribute, which requires at-most-one-true enforcement per RFC 7643. +func (a CoreAttribute) HasPrimarySubAttr() bool { + for _, sub := range a.subAttributes { + if strings.EqualFold(sub.name, "primary") { + return true + } + } + return false +} + // HasSubAttributes returns whether the attribute is complex and has sub attributes. func (a CoreAttribute) HasSubAttributes() bool { return a.typ == attributeDataTypeComplex && len(a.subAttributes) != 0 @@ -457,8 +488,11 @@ func (a CoreAttribute) validate(attribute interface{}) (interface{}, *errors.Sci } attributes = append(attributes, attr) } - if a.typ == attributeDataTypeComplex && a.HasTypeAndValueSubAttrs() { - if HasDuplicateTypeValuePairs(attributes) { + if a.typ == attributeDataTypeComplex { + if a.HasTypeAndValueSubAttrs() && HasDuplicateTypeValuePairs(attributes) { + return nil, &errors.ScimErrorUniqueness + } + if a.HasPrimarySubAttr() && HasDuplicatePrimary(attributes) { return nil, &errors.ScimErrorUniqueness } } From d5e7ba4189467b932cee1c9187128f74dc9a849c Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Thu, 26 Mar 2026 21:51:57 +0100 Subject: [PATCH 7/7] feat: auto-clear primary on multi-valued attrs per RFC 7644 Section 3.5.2 Instead of rejecting duplicate primary values as an error during PATCH, the server now automatically clears primary on existing values when a new primary is added, as required by RFC 7644 Section 3.5.2. This applies both to add operations and replace via value expressions. Also changes duplicate (type, value) and duplicate primary validation errors from uniqueness to invalidValue, which better matches the error semantics described in RFC 7643. --- patch_apply.go | 78 ++++++++++++++++++++++++++++++++++++++++++--- patch_apply_test.go | 63 ++++++++++++++++++++++++++++++------ schema/core.go | 4 +-- schema/core_test.go | 53 ++++++++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 17 deletions(-) diff --git a/patch_apply.go b/patch_apply.go index 81fd089..b7333af 100644 --- a/patch_apply.go +++ b/patch_apply.go @@ -14,6 +14,7 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, copy(result, list) matched := false + matchedIndices := make(map[int]bool) for i, elem := range result { m, ok := elem.(map[string]interface{}) if !ok { @@ -23,6 +24,7 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, continue } matched = true + matchedIndices[i] = true if t.subAttrName != "" { updated := copyMap(m) updated[t.subAttrName] = value @@ -52,6 +54,11 @@ func applyToMatching(list []interface{}, t *valueExprTarget, value interface{}, } } } + + if t.attr.HasPrimarySubAttr() { + clearOtherPrimaries(result, matchedIndices) + } + return result, matched, nil } @@ -67,7 +74,7 @@ func attrFromTarget(target interface{}) schema.CoreAttribute { panic("unknown target type") } -func checkMultiValuedUniqueness(attrs ResourceAttributes, s schema.Schema, extensions []schema.Schema) error { +func checkMultiValuedConstraints(attrs ResourceAttributes, s schema.Schema, extensions []schema.Schema) error { allAttrs := make([]schema.CoreAttribute, 0, len(s.Attributes)) allAttrs = append(allAttrs, s.Attributes...) for _, ext := range extensions { @@ -86,10 +93,10 @@ func checkMultiValuedUniqueness(attrs ResourceAttributes, s schema.Schema, exten continue } if attr.HasTypeAndValueSubAttrs() && schema.HasDuplicateTypeValuePairs(list) { - return scimErrors.ScimErrorUniqueness + return scimErrors.ScimErrorInvalidValue } - if attr.HasPrimarySubAttr() && schema.HasDuplicatePrimary(list) { - return scimErrors.ScimErrorUniqueness + if attr.HasPrimarySubAttr() { + clearDuplicatePrimary(list) } } return nil @@ -110,6 +117,67 @@ func checkMutability(op string, attr schema.CoreAttribute, exists bool) error { return nil } +// clearDuplicatePrimary ensures at most one element has primary set to true. +// RFC 7644 Section 3.5.2: the server SHALL set the value of the existing +// "primary" attribute to false when a new primary value is added via PATCH. +// The last element with primary: true wins. +func clearDuplicatePrimary(list []interface{}) { + lastPrimary := -1 + for i, elem := range list { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + if p, ok := m["primary"].(bool); ok && p { + lastPrimary = i + } + } + if lastPrimary < 0 { + return + } + for i, elem := range list { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + if p, ok := m["primary"].(bool); ok && p && i != lastPrimary { + m["primary"] = false + } + } +} + +// clearOtherPrimaries clears primary on all elements outside modifiedIndices +// when any modified element has primary set to true. Among modified elements, +// only the last one with primary=true is kept. +func clearOtherPrimaries(list []interface{}, modifiedIndices map[int]bool) { + lastModifiedPrimary := -1 + for i, elem := range list { + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + p, _ := m["primary"].(bool) + if p && modifiedIndices[i] { + lastModifiedPrimary = i + } + } + if lastModifiedPrimary < 0 { + return + } + for i, elem := range list { + if i == lastModifiedPrimary { + continue + } + m, ok := elem.(map[string]interface{}) + if !ok { + continue + } + if p, _ := m["primary"].(bool); p { + m["primary"] = false + } + } +} + func copyMap(m map[string]interface{}) map[string]interface{} { cp := make(map[string]interface{}, len(m)) for k, v := range m { @@ -272,7 +340,7 @@ func ApplyPatch(attrs ResourceAttributes, ops []PatchOperation, s schema.Schema, return nil, err } } - if err := checkMultiValuedUniqueness(result, s, extensions); err != nil { + if err := checkMultiValuedConstraints(result, s, extensions); err != nil { return nil, err } return result, nil diff --git a/patch_apply_test.go b/patch_apply_test.go index 6a4d337..719dcb0 100644 --- a/patch_apply_test.go +++ b/patch_apply_test.go @@ -34,7 +34,9 @@ func TestApplyPatch_AddDistinctTypeValuePair(t *testing.T) { } } -func TestApplyPatch_AddDuplicatePrimary(t *testing.T) { +// RFC 7644 Section 3.5.2: the server SHALL set primary to false on the +// existing value when a new value with primary: true is added. +func TestApplyPatch_AddDuplicatePrimary_AutoClears(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ "emails": []interface{}{ @@ -42,13 +44,25 @@ func TestApplyPatch_AddDuplicatePrimary(t *testing.T) { }, } - _, err := ApplyPatch(attrs, []PatchOperation{ + result, err := ApplyPatch(attrs, []PatchOperation{ {Op: PatchOperationAdd, Path: mustParsePath("emails"), Value: []interface{}{ map[string]interface{}{"type": "home", "value": "john@home.com", "primary": true}, }}, }, s) - if err == nil { - t.Error("expected error for duplicate primary after add") + if err != nil { + t.Fatalf("expected auto-clear of old primary, got error: %v", err) + } + emails := result["emails"].([]interface{}) + if len(emails) != 2 { + t.Fatalf("expected 2 emails, got %d", len(emails)) + } + work := emails[0].(map[string]interface{}) + if p, _ := work["primary"].(bool); p { + t.Error("expected old primary (work) to be cleared") + } + home := emails[1].(map[string]interface{}) + if p, _ := home["primary"].(bool); !p { + t.Error("expected new primary (home) to remain true") } } @@ -66,9 +80,7 @@ func TestApplyPatch_AddDuplicateTypeValuePair(t *testing.T) { map[string]interface{}{"type": "work", "value": "john@work.com"}, }}, }, s) - if err == nil { - t.Error("expected error for duplicate (type, value) pair after add") - } + assertScimError(t, err, scimErrors.ScimTypeInvalidValue) } func TestApplyPatch_AddSimpleAttribute(t *testing.T) { @@ -569,9 +581,7 @@ func TestApplyPatch_ReplaceDuplicateTypeValuePair(t *testing.T) { map[string]interface{}{"type": "work", "value": "john@work.com"}, }}, }, s) - if err == nil { - t.Error("expected error for duplicate (type, value) pair after replace") - } + assertScimError(t, err, scimErrors.ScimTypeInvalidValue) } func TestApplyPatch_ReplaceMultiValuedWithoutFilter(t *testing.T) { @@ -636,6 +646,39 @@ func TestApplyPatch_ReplaceNonExistentAttribute_TreatedAsAdd(t *testing.T) { } } +// RFC 7644 Section 3.5.2: when primary is set to true via a value expression, +// the server SHALL clear primary on all other values, even when the modified +// element appears before the existing primary in the list. +func TestApplyPatch_ReplacePrimaryViaValueExpr_AutoClears(t *testing.T) { + s := testUserSchema() + attrs := ResourceAttributes{ + "emails": []interface{}{ + map[string]interface{}{"type": "home", "value": "john@home.com"}, + map[string]interface{}{"type": "work", "value": "john@work.com", "primary": true}, + }, + } + + result, err := ApplyPatch(attrs, []PatchOperation{ + { + Op: PatchOperationReplace, + Path: mustParsePath(`emails[type eq "home"].primary`), + Value: true, + }, + }, s) + if err != nil { + t.Fatalf("expected auto-clear, got error: %v", err) + } + emails := result["emails"].([]interface{}) + home := emails[0].(map[string]interface{}) + if p, _ := home["primary"].(bool); !p { + t.Error("expected home to become primary") + } + work := emails[1].(map[string]interface{}) + if p, _ := work["primary"].(bool); p { + t.Error("expected work primary to be cleared") + } +} + func TestApplyPatch_ReplaceSimpleAttribute(t *testing.T) { s := testUserSchema() attrs := ResourceAttributes{ diff --git a/schema/core.go b/schema/core.go index f650371..9131b13 100644 --- a/schema/core.go +++ b/schema/core.go @@ -490,10 +490,10 @@ func (a CoreAttribute) validate(attribute interface{}) (interface{}, *errors.Sci } if a.typ == attributeDataTypeComplex { if a.HasTypeAndValueSubAttrs() && HasDuplicateTypeValuePairs(attributes) { - return nil, &errors.ScimErrorUniqueness + return nil, &errors.ScimErrorInvalidValue } if a.HasPrimarySubAttr() && HasDuplicatePrimary(attributes) { - return nil, &errors.ScimErrorUniqueness + return nil, &errors.ScimErrorInvalidValue } } return attributes, nil diff --git a/schema/core_test.go b/schema/core_test.go index 7108bf8..ef0f80e 100644 --- a/schema/core_test.go +++ b/schema/core_test.go @@ -2,8 +2,10 @@ package schema import ( "fmt" + "net/http" "testing" + "github.com/elimity-com/scim/errors" "github.com/elimity-com/scim/optional" ) @@ -113,6 +115,32 @@ func TestCoreAttribute_validate_allowsDuplicateTypeWithDifferentValue(t *testing } } +func TestCoreAttribute_validate_rejectsDuplicatePrimary(t *testing.T) { + emails := ComplexCoreAttribute(ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "type"}), + SimpleBooleanParams(BooleanParams{Name: "primary"}), + }, + }) + + _, scimErr := emails.validate([]interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com", "primary": true}, + map[string]interface{}{"type": "home", "value": "john@home.com", "primary": true}, + }) + if scimErr == nil { + t.Fatal("expected error for duplicate primary") + } + if scimErr.Status != http.StatusBadRequest { + t.Errorf("expected 400, got %d", scimErr.Status) + } + if scimErr.ScimType != errors.ScimTypeInvalidValue { + t.Errorf("expected scimType %q, got %q", errors.ScimTypeInvalidValue, scimErr.ScimType) + } +} + func TestCoreAttribute_validate_rejectsDuplicateTypeValuePairs(t *testing.T) { emails := ComplexCoreAttribute(ComplexParams{ Name: "emails", @@ -133,6 +161,31 @@ func TestCoreAttribute_validate_rejectsDuplicateTypeValuePairs(t *testing.T) { } } +func TestCoreAttribute_validate_rejectsDuplicateTypeValuePairsWithBadRequest(t *testing.T) { + emails := ComplexCoreAttribute(ComplexParams{ + Name: "emails", + MultiValued: true, + SubAttributes: []SimpleParams{ + SimpleStringParams(StringParams{Name: "value"}), + SimpleStringParams(StringParams{Name: "type"}), + }, + }) + + _, scimErr := emails.validate([]interface{}{ + map[string]interface{}{"type": "work", "value": "john@work.com"}, + map[string]interface{}{"type": "work", "value": "john@work.com"}, + }) + if scimErr == nil { + t.Fatal("expected error for duplicate (type, value) pairs") + } + if scimErr.Status != http.StatusBadRequest { + t.Errorf("expected 400, got %d", scimErr.Status) + } + if scimErr.ScimType != errors.ScimTypeInvalidValue { + t.Errorf("expected scimType %q, got %q", errors.ScimTypeInvalidValue, scimErr.ScimType) + } +} + func TestCoreAttribute_validate_skipsDuplicateCheckWithoutTypeSubAttr(t *testing.T) { members := ComplexCoreAttribute(ComplexParams{ Name: "members",