From 94d330b4af58e843bbeefa55ef120d3cb425429c Mon Sep 17 00:00:00 2001 From: leonardo-speranzon <46976486+leonardo-speranzon@users.noreply.github.com> Date: Thu, 30 Nov 2023 11:43:30 +0100 Subject: [PATCH] Path with a complex filter match only the first occurrence (#560) * Build done * Changed navigate() signature: - navigate return a list of sub-schemas - `applyRemoveOperation` and `applyAddOrReplaceOperation` adapted to the new signature (and logic) * Add tests for cases where valuePath match multiple elements * Revert "Build done" This reverts commit e8354a64a4ec66d1d7f0eceaf536377c9914f6af. * Fixed case special case of replace: "REPLACE: replace on multiValued objects without complete path" * Removed commented console logs --------- Co-authored-by: Thomas Poignant --- src/scimPatch.ts | 138 ++++++++++++++++++++++++----------------- test/scimPatch.test.ts | 137 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+), 57 deletions(-) diff --git a/src/scimPatch.ts b/src/scimPatch.ts index 3454b3fa..e2b6d209 100644 --- a/src/scimPatch.ts +++ b/src/scimPatch.ts @@ -168,14 +168,14 @@ function resolvePaths(path: string): string[] { function applyRemoveOperation(scimResource: T, patch: ScimPatchRemoveOperation): T { // We manipulate the object directly without knowing his property, that's why we use any. - let resource: Record = scimResource; + let resources_scoped: Record[]; validatePatchOperation(patch); // Path is supposed to be set, there are a validation in the validateOperation function. const paths = resolvePaths(patch.path); try { - resource = navigate(resource, paths, {isRemoveOp: true}); + resources_scoped = navigate(scimResource, paths, {isRemoveOp: true}); } catch (error) { if (error instanceof InvalidRemoveOpPath) { return scimResource; @@ -188,26 +188,29 @@ function applyRemoveOperation(scimResource: T, patch: Sc if (!IS_ARRAY_SEARCH.test(lastSubPath)) { // This is a mono valued property - if (!patch.value) { - // No value in the remove operation, we delete it. - delete resource[lastSubPath]; - return scimResource; + for (const resource of resources_scoped) { + if (!patch.value) { + // No value in the remove operation, we delete it. + delete resource[lastSubPath]; + } else { + // Value in the remove operation, we remove the children by value. + resource[lastSubPath] = removeWithPatchValue(resource[lastSubPath], patch.value); + } } - - // Value in the remove operation, we remove the children by value. - resource[lastSubPath] = removeWithPatchValue(resource[lastSubPath], patch.value); return scimResource; } + for (const resource of resources_scoped) { - // The last element is an Array request. - const {attrName, valuePath, array} = extractArray(lastSubPath, resource); + // The last element is an Array request. + const {attrName, valuePath, array} = extractArray(lastSubPath, resource); - // We keep only items who don't match the query if supplied. - resource[attrName] = filterWithQuery(array, valuePath, {excludeIfMatchFilter: true}); + // We keep only items who don't match the query if supplied. + resource[attrName] = filterWithQuery(array, valuePath, {excludeIfMatchFilter: true}); - // If the complex multi-valued attribute has no remaining records, the attribute SHALL be considered unassigned. - if (resource[attrName].length === 0) - delete resource[attrName]; + // If the complex multi-valued attribute has no remaining records, the attribute SHALL be considered unassigned. + if (resource[attrName].length === 0) + delete resource[attrName]; + } return scimResource; } @@ -215,7 +218,8 @@ function applyRemoveOperation(scimResource: T, patch: Sc function applyAddOrReplaceOperation(scimResource: T, patch: ScimPatchAddReplaceOperation, treatMissingAsAdd: boolean): T { // We manipulate the object directly without knowing his property, that's why we use any. - let resource: Record = scimResource; + // let resource: Record = scimResource; + let resources_scoped: Record[]; validatePatchOperation(patch); if (!patch.path) @@ -226,10 +230,10 @@ function applyAddOrReplaceOperation(scimResource: T, pat const lastSubPath = paths[paths.length - 1]; try { - resource = navigate(resource, paths); + resources_scoped = navigate(scimResource, paths); } catch(e) { if (e instanceof FilterOnEmptyArray || e instanceof FilterArrayTargetNotFound) { - resource = e.schema; + const resource: Record = e.schema; // check issue https://github.com/thomaspoignant/scim-patch/issues/42 to see why we should add this const parsedPath = parse(e.valuePath); if (isAddOperation(patch.op) && @@ -261,29 +265,37 @@ function applyAddOrReplaceOperation(scimResource: T, pat } throw e; } - if (!IS_ARRAY_SEARCH.test(lastSubPath)) { - resource[lastSubPath] = addOrReplaceAttribute(resource[lastSubPath], patch); - return scimResource; + for (const resource of resources_scoped) { + resource[lastSubPath] = addOrReplaceAttribute(resource[lastSubPath], patch); + } + return scimResource; } + + // The last element is an Array request. - const {valuePath, array} = extractArray(lastSubPath, resource); + for (const resource of resources_scoped) { + + const {valuePath, array} = extractArray(lastSubPath, resource); - // Get the list of items who are successful for the search query. - const matchFilter = filterWithQuery(array, valuePath); + // Get the list of items who are successful for the search query. + const matchFilter = filterWithQuery(array, valuePath); - // 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". - if (isReplaceOperation(patch.op) && matchFilter.length === 0) { - throw new NoTarget(patch.path); + // 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". + if (isReplaceOperation(patch.op) && matchFilter.length === 0) { + throw new NoTarget(patch.path); + } + for (let i = 0; i < array.length; i++) { + // We are sure to find at least one index because matchFilter comes from array. + if(matchFilter.includes(array[i])){ + array[i] = addOrReplaceAttribute(array[i], patch); + } + } } - // We are sure to find an index because matchFilter comes from array. - const index = array.findIndex(item => matchFilter.includes(item)); - array[index] = addOrReplaceAttribute(array[index], patch); - return scimResource; } @@ -315,38 +327,45 @@ function extractArray(subPath: string, schema: any): ScimSearchQuery { * @param options options used while calling navigate * @return the parent object of the element we want to edit */ -function navigate(inputSchema: any, paths: string[], options: NavigateOptions = {}): Record { - let schema = inputSchema; +function navigate(inputSchema: any, paths: string[], options: NavigateOptions = {}): Record[] { + let schemas: any[] = [inputSchema]; for (let i = 0; i < paths.length - 1; i++) { const subPath = paths[i]; // We check if the element is an array with query (ex: emails[primary eq true). - if (IS_ARRAY_SEARCH.test(subPath)) { - try { - const {attrName, valuePath, array} = extractArray(subPath, schema); - // Get the item who is successful for the search query. - const matchFilter = filterWithQuery(array, valuePath); - // We are sure to find an index because matchFilter comes from array. - const index = array.findIndex(item => matchFilter.includes(item)); - if (index < 0) { - throw new FilterArrayTargetNotFound('A matching array entry was not found using the supplied filter.', attrName, valuePath, schema); - } - schema = array[index]; - } catch (error) { - if(error instanceof FilterOnEmptyArray){ - error.schema = schema; + if (IS_ARRAY_SEARCH.test(subPath)) { + schemas = schemas.flatMap((schema)=>{ + try { + const {attrName, valuePath, array} = extractArray(subPath, schema); + // Get the item who is successful for the search query. + const matchFilter = filterWithQuery(array, valuePath); + if (matchFilter.length === 0) { + throw new FilterArrayTargetNotFound('A matching array entry was not found using the supplied filter.', attrName, valuePath, schema); + } + return matchFilter; + } catch (error) { + /* + FIXME In some edge cases, not fully compliant with SCIM, this can prematurely throw en error + For example if we have a structure with multiValued complex + attribute inside of a multiValued complex attribute, in + this case this throw even if only a "branch" of the search fail. + */ + if(error instanceof FilterOnEmptyArray){ + error.schema = schema; + } + throw error; } - throw error; - } + }); } else { - // The element is not an array. - if (!schema[subPath] && options.isRemoveOp) - throw new InvalidRemoveOpPath(); + schemas = schemas.flatMap((schema)=>{ + if (!schema[subPath] && options.isRemoveOp) + throw new InvalidRemoveOpPath(); - schema = schema[subPath] || (schema[subPath] = {}); + return schema[subPath] || (schema[subPath] = {}); + }); } } - return schema; + return schemas; } /** @@ -368,6 +387,11 @@ function addOrReplaceAttribute(property: any, patch: ScimPatchAddReplaceOperatio return patch.value; } + if(isReplaceOperation(patch.op)){ + return property.map( + (it) => addOrReplaceAttribute(it,patch,multiValuedPathFilter) + ); + } const a = property; if (!deepIncludes(a, patch.value)) a.push(patch.value); diff --git a/test/scimPatch.test.ts b/test/scimPatch.test.ts index cea9e5b4..e961d043 100644 --- a/test/scimPatch.test.ts +++ b/test/scimPatch.test.ts @@ -347,6 +347,74 @@ describe('SCIM PATCH', () => { return done(); }); + it('REPLACE: replace on multiValued objects', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre"}, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: expected, path: 'emails.newProperty'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.eq(expected); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + return done(); + }); + it('REPLACE: replace on multiValued objects without complete path', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre"}, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: {newProperty: expected}, path: 'emails'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.eq(expected); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + expect(afterPatch.emails.length).to.be.eq(4); + return done(); + }); + + it('REPLACE: filter that match multiple elements of complex multiValued attribute', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre"}, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: expected, path: 'emails[primary eq false].newProperty'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.eq("pre"); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + return done(); + }); + + it('REPLACE: filter that match multiple elements of complex multiValued attribute without complete path', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre"}, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'replace', value: {newProperty: expected}, path: 'emails[primary eq false]'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.eq("pre"); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + return done(); + }); + // see https://github.com/thomaspoignant/scim-patch/issues/489 it('REPLACE: Replace op with value of empty object results in circular reference', done => { const expected = [ @@ -832,6 +900,41 @@ describe('SCIM PATCH', () => { expect(afterPatch.emails.length).to.be.eq(2); return done(); }); + + + it('ADD: filter that match multiple elements of complex multiValued attribute', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true }, + { value: "addr2@email.com", primary: false }, + { value: "addr3@email.com", primary: false }, + { value: "addr4@email.com", primary: false }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'add', value: expected, path: 'emails[primary eq false].newProperty'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + return done(); + }); + + it('ADD: filter that match multiple elements of complex multiValued attribute without complete path', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true }, + { value: "addr2@email.com", primary: false }, + { value: "addr3@email.com", primary: false }, + { value: "addr4@email.com", primary: false }, + ]; + const expected = "post"; + const patch: ScimPatchAddReplaceOperation = {op: 'add', value: {newProperty: expected}, path: 'emails[primary eq false]'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[1].newProperty).to.be.eq(expected); + expect(afterPatch.emails[2].newProperty).to.be.eq(expected); + expect(afterPatch.emails[3].newProperty).to.be.eq(expected); + return done(); + }); }); describe('remove', () => { it('REMOVE: with no path', done => { @@ -1041,6 +1144,40 @@ describe('SCIM PATCH', () => { expect(scimUser).not.to.eq(afterPatch); return done(); }); + + + + it('REMOVE: remove all sub-attributes in a complex multi-valued attribute', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre" }, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const patch: ScimPatchRemoveOperation = {op: 'remove', path: 'emails.newProperty'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[1].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[2].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[3].newProperty).to.be.an('undefined'); + return done(); + }); + + it('REMOVE: filter that match multiple elements of complex multiValued attribute', done => { + scimUser.emails = [ + { value: "addr1@email.com", primary: true, newProperty: "pre" }, + { value: "addr2@email.com", primary: false, newProperty: "pre" }, + { value: "addr3@email.com", primary: false, newProperty: "pre" }, + { value: "addr4@email.com", primary: false, newProperty: "pre" }, + ]; + const patch: ScimPatchRemoveOperation = {op: 'remove', path: 'emails[primary eq false].newProperty'}; + const afterPatch = scimPatch(scimUser, [patch]); + expect(afterPatch.emails[0].newProperty).to.be.eq("pre"); + expect(afterPatch.emails[1].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[2].newProperty).to.be.an('undefined'); + expect(afterPatch.emails[3].newProperty).to.be.an('undefined'); + return done(); + }); }); describe('invalid requests', () => {