Repository: scimmyjs/scimmy (js)

Issue:
Extension attributes are filtered when there's excludedAttributes param
Hi @sleelin,

We found another issue while trying to integrate with Entra ID. When calling `GET /Users?excludedAttributes=userName` the extension attributes are filtered out in response. If we remove the `excludedAttributes` param then the response is fine.

The problem seems to be caused by:
https://github.com/scimmyjs/scimmy/blob/9e81f47ceab7d0b518bc516ad088278cd0137774/src/lib/types/definition.js#L342
https://github.com/scimmyjs/scimmy/blob/9e81f47ceab7d0b518bc516ad088278cd0137774/src/lib/types/definition.js#L372-L373
https://github.com/scimmyjs/scimmy/blob/9e81f47ceab7d0b518bc516ad088278cd0137774/src/lib/types/definition.js#L306-L310

First of all, the `inclusions` are names of `attributes`, which would be `EnterpriseUser` for the SCIMMY.Schemas.EnterpriseUser extension or `name` for `new SCIMMY.Types.SchemaDefinition(name, ...)`.

Then, the logic seems to expect the key of data returned by handler to be individual extension attributes like `'urn:ietf:params:scim:schemas:extension:enterprise:2.0:user:organization'`. However, `SchemaDefinition.coerce` constructs an object and stores it in `urn:ietf:params:scim:schemas:extension:enterprise:2.0:user`. 

I think we should check type of attribute and get the `id` for `SchemaDefinition` instances, and remove the `':'` at end of the key checking expression.

Pull request:
Fix handling of schema extensions when filtering coerced values in `SCIMMY.Types.SchemaDefinition`
Currently, filtering of included or excluded attributes by the `SchemaDefinition` type class before return does not correctly handle namespaced extension attributes. This is due to the double handling of extension schema filtering, and insufficient evaluation of overall filter structure. A specific example of this is evident when making a request for a `User` resource, extended with the `EnterpriseUser` schema, and setting `excludedAttributes` to any attribute from the `User` schema.

The internal filter method of the `SchemaDefinition` class has been rewritten to first capture all excluded and included attribute filters. If no explicit inclusions were specified, and all supplied filters are exclusions, all attributes and extensions not explicitly excluded will instead be included (fixes #37). Double handling of extension schema filtering has also been resolved, as inclusions and exclusions will only be evaluated when the filter condition is not for a namespaced attribute. Test fixtures for the `coerce` method of the `SchemaDefinition` class have also been updated to verify attribute, extension, and namespaced attribute filtering is behaving as expected.

Patch:
diff --git a/src/lib/types/definition.js b/src/lib/types/definition.js
index bad6eb6..38bdd21 100644
--- a/src/lib/types/definition.js
+++ b/src/lib/types/definition.js
@@ -1,6 +1,9 @@
 import {Attribute} from "./attribute.js";
 import {Filter} from "./filter.js";
 
+// Deeply inspect a filter object to see if it represents attributes to be excluded from a coerced value
+const isExcludedAttributesFilter = (v) => Array.isArray(v) ? v[0] === "np" : Object.values(v).every(isExcludedAttributesFilter);
+
 /**
  * SCIM Schema Definition Type
  * @alias SCIMMY.Types.SchemaDefinition
@@ -317,79 +320,92 @@ export class SchemaDefinition {
             }
         }
         
-        return SchemaDefinition.#filter(target, filter && {...filter}, this.attributes);
+        return SchemaDefinition.#filter(this, filter && {...filter}, target);
     }
     
     /**
      * Filter out desired or undesired attributes from a coerced schema value
-     * @param {Object|Object[]} [data] - the data to filter attributes from
+     * @param {SCIMMY.Types.SchemaDefinition} definition - the schema definition requesting the filtering
      * @param {Object} [filter] - the filter to apply to the coerced value
-     * @param {SCIMMY.Types.Attribute[]} [attributes] - set of attributes to match against
+     * @param {Object|Object[]} [data={}] - the data to filter attributes from
+     * @param {String} [prefix=""] - prefix to use when filtering on complex value subAttributes
      * @returns {Object} the coerced value with desired or undesired attributes filtered out
      * @private
      */
-    static #filter(data = {}, filter, attributes) {
+    static #filter(definition, filter, data = {}, prefix = "") {
         // If there's no filter, just return the data
-        if (filter === undefined) return data;
+        if (filter === undefined || !Object.keys(filter).length)
+            return data;
         // If the data is a set, only get values that match the filter
         else if (Array.isArray(data))
-            return data.map(data => SchemaDefinition.#filter(data, {...filter}, attributes)).filter(v => Object.keys(v).length);
+            return data.map(data => SchemaDefinition.#filter(definition, {...filter}, data, prefix)).filter(v => Object.keys(v).length);
         // Otherwise, filter the data!
         else {
             // Prepare resultant value storage
             const target = {};
-            const filterable = {...filter};
-            const inclusions = attributes.map(({name}) => name);
+            const inclusions = [];
+            const exclusions = [];
             
-            // Check for any negative filters
-            for (let key in {...filterable}) {
-                // Find the attribute by lower case name
-                const {name, config: {returned} = {}} = attributes.find(a => a.name.toLowerCase() === key.toLowerCase()) ?? {};
+            for (let key in filter) try {
+                // Find the attribute or extension definition using the filter key
+                const attribute = definition.attribute(prefix ? `${prefix}.${key}` : key);
                 
-                // Mark the property as omitted from the result, and remove the spent filter
-                if (returned !== "always" && Array.isArray(filterable[key]) && filterable[key][0] === "np") {
-                    inclusions.splice(inclusions.indexOf(name), 1);
-                    delete filterable[key];
+                // Only be concerned with filter expressions for attributes or extensions directly for now
+                if (Array.isArray(filter[key]) && (attribute instanceof SchemaDefinition || !key.startsWith("urn:"))) {
+                    // Get real name and handle potentially overlapping filter conditions
+                    const name = (attribute instanceof SchemaDefinition ? attribute.id : attribute.name);
+                    const condition = filter[key].map(c => Array.isArray(c) ? c[0] : c);
+                    
+                    // Mark the positively filtered property as included in the result
+                    if (condition.includes("pr"))
+                        inclusions.push(name);
+                    // Mark the negatively filtered property as excluded from the result
+                    else if (condition.includes("np"))
+                        exclusions.push(name);
                 }
+            } catch {
+                // If we've reached here, the filter refers to an unknown attribute and should be ignored
             }
             
-            // Check for remaining positive filters
-            if (Object.keys(filterable).length) {
-                // If there was a positive filter, ignore the negative filters
-                inclusions.splice(0, inclusions.length);
-                
-                // Mark the positively filtered property as included in the result, and remove the spent filter
-                for (let key in {...filterable}) if (Array.isArray(filterable[key]) && filterable[key][0] === "pr") { 
-                    inclusions.push(key);
-                    delete filterable[key];
+            // If there were no explicit inclusions, and all filter expressions were negative...
+            if (!inclusions.length && isExcludedAttributesFilter(filter)) {
+                // ...go through all subAttributes, or extension attributes...
+                for (let attribute of (prefix ? definition.attribute(prefix).subAttributes : definition.attributes)) {
+                    // ...and assume they should be included, if they weren't explicitly excluded
+                    if (attribute instanceof Attribute && !exclusions.includes(attribute.name)) inclusions.push(attribute.name);
                 }
             }
             
-            // Go through every value in the data and filter attributes
+            // Go through every value in the data and filter it
             for (let key in data) {
-                if (key.toLowerCase().startsWith("urn:")) {
-                    // If there is data in a namespaced key, and a filter for it, include it
-                    if (Object.keys(data[key]).length && inclusions.some(k => k.toLowerCase().startsWith(`${key.toLowerCase()}:`)))
+                // Get the matching attribute or extension definition for the key
+                const attribute = definition.attribute(prefix ? `${prefix}.${key}` : key) ?? {};
+                
+                if (attribute instanceof SchemaDefinition) {
+                    // If there is data in a namespaced key and no namespace filter, or there's an explicit inclusion filter...
+                    if ((Object.keys(data[key]).length && !Array.isArray(filter[key])) || (key in filter && inclusions.includes(key)))
+                        // ...include the extension data
                         target[key] = data[key];
                 } else {
-                    // Get the matching attribute definition and some relevant config values
-                    const attribute = attributes.find(a => a.name === key) ?? {};
-                    const {type, config: {returned, multiValued} = {}, subAttributes} = attribute;
+                    // Get some relevant config values from the attribute
+                    const {name, type, config: {returned, multiValued} = {}} = attribute;
                     
                     // If the attribute is always returned, add it to the result
                     if (returned === "always") target[key] = data[key];
                     // Otherwise, if the attribute was requested and ~can~ be returned, process it
                     else if (![false, "never"].includes(returned)) {
-                        // If there was a simple presence filter for the attribute, assign it
-                        if (inclusions.includes(key) && data[key] !== undefined) target[key] = data[key];
-                        // Otherwise, if there's an unhandled filter for a complex attribute, evaluate it
-                        else if (key in filter && type === "complex") {
-                            const value = SchemaDefinition.#filter(data[key], filter[key], subAttributes);
+                        // If there's a filter for a complex attribute, evaluate it
+                        if (key in filter && !Array.isArray(filter[key]) && type === "complex") {
+                            const value = SchemaDefinition.#filter(definition, filter[key], data[key], key);
                             
                             // Only set the value if it isn't empty
                             if ((!multiValued && value !== undefined) || (Array.isArray(value) && value.length))
                                 target[key] = value;
                         }
+                        // Otherwise, if there was a simple presence filter for the attribute, assign it
+                        else if (inclusions.includes(name) && data[key] !== undefined) {
+                            target[key] = data[key];
+                        }
                     }
                 }
             }


Test patch:
diff --git a/test/lib/types/definition.js b/test/lib/types/definition.js
index e838a0e..5a11676 100644
--- a/test/lib/types/definition.js
+++ b/test/lib/types/definition.js
@@ -14,6 +14,7 @@ const basepath = path.relative(process.cwd(), path.dirname(url.fileURLToPath(imp
 const fixtures = fs.readFile(path.join(basepath, "./definition.json"), "utf8").then((f) => JSON.parse(f));
 // Default parameter values to use in tests
 const params = {name: "Test", id: "urn:ietf:params:scim:schemas:Test"};
+const extensionId = params.id.replace("Test", "Extension");
 
 describe("SCIMMY.Types.SchemaDefinition", () => {
     describe("@constructor", () => {
@@ -474,7 +475,6 @@ describe("SCIMMY.Types.SchemaDefinition", () => {
         it("should expect coerce to be called on included schema extensions", () => {
             const stub = sinon.stub();
             const get = (t, p) => (p === "coerce" ? (...args) => (stub(...args) ?? t.coerce(...args)) : t[p]);
-            const extensionId = params.id.replace("Test", "Extension");
             const attributes = [new Attribute("string", "employeeNumber")];
             const extension = new SchemaDefinition("Extension", extensionId, "An Extension", attributes).truncate(["schemas", "meta"]);
             const definition = new SchemaDefinition(...Object.values(params)).extend(new Proxy(extension, {get}), true);
@@ -483,46 +483,46 @@ describe("SCIMMY.Types.SchemaDefinition", () => {
             
             assert.ok(stub.calledWithMatch({employeenumber: "1234"}),
                 "Instance method 'coerce' did not call coerce method on included schema extensions");
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
+            assert.deepStrictEqual({...actual}, expected,
                 "Instance method 'coerce' did not correctly coerce included schema extension value");
         });
         
         it("should expect required schema extensions to be defined", () => {
-            const extension = new SchemaDefinition("Extension", params.id.replace("Test", "Extension"), "An Extension", []);
-            const definition = new SchemaDefinition(...Object.values(params)).extend(extension, true);
+            const definition = new SchemaDefinition(...Object.values(params))
+                .extend(new SchemaDefinition("Extension", extensionId, "An Extension", []), true);
             
             assert.throws(() => definition.coerce({}),
-                {name: "TypeError", message: `Missing values for required schema extension '${extension.id}'`},
+                {name: "TypeError", message: `Missing values for required schema extension '${extensionId}'`},
                 "Instance method 'coerce' did not attempt to coerce required schema extension");
-            assert.throws(() => definition.coerce({[extension.id]: {}}),
-                {name: "TypeError", message: `Missing values for required schema extension '${extension.id}'`},
+            assert.throws(() => definition.coerce({[extensionId]: {}}),
+                {name: "TypeError", message: `Missing values for required schema extension '${extensionId}'`},
                 "Instance method 'coerce' did not attempt to coerce required schema extension");
         });
         
         it("should expect namespaced attributes or extensions to be coerced", () => {
             const attribute = new Attribute("string", "employeeNumber");
-            const extension = new SchemaDefinition("Extension", params.id.replace("Test", "Extension"), "An Extension", [attribute]);
-            const definition = new SchemaDefinition(...Object.values(params)).extend(extension, true);
-            const metadata = {schemas: [definition.id, extension.id], meta: {resourceType: definition.name}};
+            const extension = new SchemaDefinition("Extension", extensionId, "An Extension", [attribute]);
+            const definition = new SchemaDefinition(...Object.values(params)).truncate(["meta"]).extend(extension, true);
+            const metadata = {schemas: [definition.id, extension.id]};
             const expected = {[extension.id]: {employeeNumber: "1234"}};
             
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(definition.coerce(expected))), {...metadata, ...expected},
+            assert.deepStrictEqual({...definition.coerce(expected)}, {...metadata, ...expected},
                 "Instance method 'coerce' failed to coerce required schema extension value");
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(definition.coerce({[`${extension.id}:employeeNumber`]: "1234"}))), {...metadata, ...expected},
+            assert.deepStrictEqual({...definition.coerce({[`${extensionId}:employeeNumber`]: "1234"})}, {...metadata, ...expected},
                 "Instance method 'coerce' failed to coerce required schema extension value");
-            assert.throws(() => definition.coerce({[`${extension.id}:employeeNumber`]: false}),
+            assert.throws(() => definition.coerce({[`${extensionId}:employeeNumber`]: false}),
                 {name: "TypeError", message: `Attribute 'employeeNumber' expected value type 'string' but found type 'boolean' in schema extension '${extension.id}'`},
                 "Instance method 'coerce' did not attempt to coerce required schema extension's invalid value");
         });
         
         it("should expect deeply nested namespaced and extension attributes to be merged and coerced", () => {
             const attributes = [new Attribute("complex", "test", {}, [new Attribute("string", "name"), new Attribute("string", "value")])]
-            const extension = new SchemaDefinition("Extension", params.id.replace("Test", "Extension"), "An Extension", attributes);
+            const extension = new SchemaDefinition("Extension", extensionId, "An Extension", attributes);
             const definition = new SchemaDefinition(...Object.values(params)).extend(extension, true);
             const {[extension.id]: actual} = definition.coerce({[`${extension.id}:test.value`]: "Test", [extension.id]: {test: {name: "Test"}}});
             const expected = {test: {name: "Test", value: "Test"}};
             
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
+            assert.deepStrictEqual({...actual}, expected,
                 "Instance method 'coerce' did not expect deeply nested namespaced and extension attributes to be merged and coerced");
         });
         
@@ -537,51 +537,57 @@ describe("SCIMMY.Types.SchemaDefinition", () => {
                 "Instance method 'coerce' included attributes not specified for filter 'testName pr'");
         });
         
-        it("should expect negative filters to be applied to coerced results", () => {
-            const attributes = [new Attribute("complex", "test", {}, [new Attribute("string", "name"), new Attribute("string", "value")])];
+        it("should expect positive filters to be preferred over negative filters", () => {
+            const source = {testName: "a string", testValue: "another string", employeeNumber: "Test", costCenter: "Test"};
+            const attributes = [new Attribute("string", "testName"), new Attribute("string", "testValue"), new Attribute("string", "employeeNumber"), new Attribute("string", "costCenter")];
             const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes);
-            const actual = definition.coerce({test: {name: "Test", value: "False"}}, undefined, undefined, new Filter("test.value np"));
-            const expected = {test: {name: "Test"}}
+            const actual = definition.coerce(source, undefined, undefined, new Filter("employeeNumber np and testName np and testName pr"));
+            const expected = {testName: "a string"};
             
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
-                "Instance method 'coerce' did not expect negative filters to be applied to coerced results");
+            assert.deepStrictEqual({...actual}, expected,
+                "Instance method 'coerce' did not prefer positive filters over negative filters");
         });
         
-        it("should expect complex multi-valued attributes to be filtered positively", () => {
-            const attributes = [new Attribute("complex", "test", {multiValued: true}, [new Attribute("string", "name"), new Attribute("string", "value")])];
-            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes);
-            const source = {test: [{name: "Test", value: "Test"}, {value: "False"}]};
-            const actual = definition.coerce(source, undefined, undefined, new Filter("test[name pr]"));
-            const expected = {test: [{name: "Test"}]};
-            
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
-                "Instance method 'coerce' did not positively filter complex multi-valued attributes");
-        });
-        
-        it("should expect complex multi-valued attributes to be filtered negatively", () => {
-            const attributes = [new Attribute("complex", "test", {multiValued: true}, [new Attribute("string", "name"), new Attribute("string", "value")])];
-            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes);
-            const source = {test: [{name: "Test", value: "Test"}, {value: "False"}]};
-            const actual = definition.coerce(source, undefined, undefined, new Filter("test[name np]"));
-            const expected = {test: [{value: "Test"}, {value: "False"}]};
-            
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
-                "Instance method 'coerce' did not negatively filter complex multi-valued attributes");
-        });
-        
-        it("should expect namespaced attributes in the supplied filter to be applied to coerced result", () => {
-            const attribute = new Attribute("string", "employeeNumber");
+        it("should expect missing attributes in a filter to be ignored", () => {
             const attributes = [new Attribute("string", "employeeNumber"), new Attribute("string", "costCenter")];
-            const extension = new SchemaDefinition("Extension", params.id.replace("Test", "Extension"), "An Extension", attributes);
-            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", [attribute]).extend(extension);
-            const source = {employeeNumber: "Test", [`${extension.id}:employeeNumber`]: "1234", [`${extension.id}:costCenter`]: "Test"};
-            const actual = definition.coerce(source, undefined, undefined, new Filter(`${extension.id}:employeeNumber pr`));
-            const expected = {[extension.id]: {employeeNumber: "1234"}};
+            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes).truncate(["schemas", "meta"]);
+            const expected = {employeeNumber: "Test", costCenter: "Test"};
+            const actual = definition.coerce(expected, undefined, undefined, new Filter("userName np"));
+            
+            assert.deepStrictEqual({...actual}, expected,
+                "Instance method 'coerce' did not ignore missing attributes specified in filter");
+        });
+        
+        for (let [target, outcome, unexpected, expected, filter, multiValued] of [
+            ["complex attributes", "filtered positively", "unexpectedly included", {test: {value: "False"}}, "test.value pr"],
+            ["complex attributes", "filtered negatively", "unexpectedly excluded", {test: {name: "Test"}}, "test.value np"],
+            ["complex multi-value attributes", "filtered positively", "unexpectedly included", {test: [{name: "Test"}]}, "test.name pr", true],
+            ["complex multi-value attributes", "filtered negatively", "unexpectedly excluded", {test: [{value: "Test"}, {value: "False"}]}, "test.name np", true]
+        ]) it(`should expect ${target} to be ${outcome}`, () => {
+            const source = {test: multiValued ? [{name: "Test", value: "Test"}, {value: "False"}] : {name: "Test", value: "False"}};
+            const attributes = [new Attribute("complex", "test", {multiValued}, [new Attribute("string", "name"), new Attribute("string", "value")])];
+            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes).truncate(["schemas", "meta"]);
+            
+            assert.deepStrictEqual({...definition.coerce(source, undefined, undefined, new Filter(filter))}, expected,
+                `Instance method 'coerce' ${unexpected} ${target} not specified for filter`);
+        });
+        
+        for (let [target, outcome, unexpected, expected, filter] of [
+            ["namespaced attributes", "present in coerced result", "unexpectedly excluded", {costCenter: "Test", [extensionId]: {employeeNumber: "1234", costCenter: "Test"}}, "employeeNumber np"],
+            ["namespaced attributes", "filtered positively", "unexpectedly included", {[extensionId]: {employeeNumber: "1234"}}, `${extensionId}:employeeNumber pr`],
+            ["namespaced attributes", "filtered negatively", "unexpectedly excluded", {employeeNumber: "Test", costCenter: "Test", [extensionId]: {costCenter: "Test"}}, `${extensionId}:employeeNumber np`],
+            ["extension namespaces", "filtered positively", "unexpectedly included", {[extensionId]: {employeeNumber: "1234", costCenter: "Test"}}, `${extensionId} pr`],
+            ["extension namespaces", "filtered negatively", "unexpectedly excluded", {employeeNumber: "Test", costCenter: "Test"}, `${extensionId} np`],
+            ["direct and namespaced attributes", "filtered positively", "unexpectedly included", {costCenter: "Test", [extensionId]: {employeeNumber: "1234"}}, `costCenter pr and ${extensionId}:employeeNumber pr`],
+            ["direct and namespaced attributes", "filtered negatively", "unexpectedly excluded", {employeeNumber: "Test", [extensionId]: {costCenter: "Test"}}, `costCenter np and ${extensionId}:employeeNumber np`]
+        ]) it(`should expect ${target} to be ${outcome}`, () => {
+            const source = {employeeNumber: "Test", costCenter: "Test", [`${extensionId}:employeeNumber`]: "1234", [`${extensionId}:costCenter`]: "Test"};
+            const attributes = [new Attribute("string", "employeeNumber"), new Attribute("string", "costCenter")];
+            const definition = new SchemaDefinition(...Object.values(params), "Test Schema", attributes).truncate(["schemas", "meta"])
+                .extend(new SchemaDefinition("Extension", extensionId, "An Extension", attributes));
             
-            assert.strictEqual(actual[extension.id].employeeNumber, "1234",
-                "Instance method 'coerce' did not include namespaced attributes for filter");
-            assert.deepStrictEqual(JSON.parse(JSON.stringify(actual)), expected,
-                "Instance method 'coerce' included namespaced attributes not specified for filter");
+            assert.deepStrictEqual({...definition.coerce(source, undefined, undefined, new Filter(filter))}, expected,
+                `Instance method 'coerce' ${unexpected} ${target} not specified for filter`);
         });
     });
 });
\ No newline at end of file