Add sub-attribute to empty multi-valued attribute

[arnecs]

Describe the bug
Add/Replace a specific sub-attribute of a complex multi-valued attribute selected by "valuePath" filter is not working when current value if undefined/null or an empty array [] is provided.

To Reproduce

const scimPatch = require("scim-patch")
const assert = require("assert")

const resource = {
  schemas: ["urn:ietf:params:scim:schemas:core:2.0:User"],
  // addresses: [],
}

const patches = [
  { op: "Add", path: "addresses[type eq \"work\"].streetAddress", value: "1010 Broadway Ave"},
]

const expected = {
  schemas: ["urn:ietf:params:scim:schemas:core:2.0:User"],
  addresses: [{
    type: "work",
    streetAddress: "1010 Broadway Ave"
  }]
}


const patchedResource = scimPatch.scimPatch(resource, patches)

// throws
// InvalidScimPatchOp [Error]: Invalid SCIM Patch: Impossible to search on a mono valued attribute.

assert.deepStrictEqual(patchedResource, expected)

When empty array is provided, another error occurs

const resource = {
  schemas: ["urn:ietf:params:scim:schemas:core:2.0:User"],
  addresses: [],
}

const patches = [
  { op: "Add", path: "addresses[type eq \"work\"].streetAddress", value: "1010 Broadway Ave"},
]

scimPatch.scimPatch(resource, patches)

// throws
// /scim-patch/lib/src/scimPatch.js:115
//          resource[lastSubPath] = addOrReplaceAttribute(resource[lastSubPath], patch);
//                                                              ^
//
//  TypeError: Cannot read property 'streetAddress' of undefined

Expected behavior
The new "streetAddress" and "type" should be added to the resource.
Expeced value for the addresses attribute is:

[
  {
    "type": "work",
    "streetAddress": "1010 Broadway Ave"
  }
]

[thomaspoignant]

Thanks, @arnecs to have open this issue, which will help me a lot to improve the library.
I will check this problem soon, in the meantime if you want to open a pull request for this issue you are welcome.

[thomaspoignant]

I've looked at the RFC and it is unclear to me if your expected behavior is how SCIM is supposed to work.

   o  If omitted, the target location is assumed to be the resource
      itself.  The "value" parameter contains a set of attributes to be
      added to the resource.

   o  If the target location does not exist, the attribute and value are
      added.

   o  If the target location specifies a complex attribute, a set of
      sub-attributes SHALL be specified in the "value" parameter.

   o  If the target location specifies a multi-valued attribute, a new
      value is added to the attribute.

   o  If the target location specifies a single-valued attribute, the
      existing value is replaced.

   o  If the target location specifies an attribute that does not exist
      (has no value), the attribute is added with the new value.

   o  If the target location exists, the value is replaced.

   o  If the target location already contains the value specified, no
      changes SHOULD be made to the resource, and a success response
      SHOULD be returned.  Unless other operations change the resource,
      this operation SHALL NOT change the modify timestamp of the
      resource.

Do you have any idea where this complex case refers to the RFC?

BTW, even if we do not implement the feature, we should at least not failed in that case.

[arnecs]

   o  If the target location does not exist, the attribute and value are
      added.

   o  If the target location specifies an attribute that does not exist
      (has no value), the attribute is added with the new value.

Do you have any idea where this complex case refers to the RFC?

Isn't this adding the new value to the resource?

I agree that the RFC is a bit unclear in regards to using a filter in the path.

[arnecs]

RFC7643 SCIM Schema

Unassigned attributes, the null value, or an empty array (in the case
of a multi-valued attribute) SHALL be considered to be equivalent in
"state".

When a resource is expressed in
JSON format, unassigned attributes, although they are defined in
schema, MAY be omitted for compactness.

The SCIM represantation undefined, null or [] for a multi-valued should be considered equivalent.
scim-patch should not throw InvalidScimPatchOp, but consider the value to be an empty array instead.

Since the library has no knowledge of the schema definition it is impossible to know if the path specified in patch operation is in fact a multi-valued attribute.

[arnecs]

From 3.5.2.3 Replace Operation

   o  If the target location is a complex multi-valued attribute with a
      value selection filter ("valuePath") and a specific sub-attribute
      (e.g., "addresses[type eq "work"].streetAddress"), the matching
      sub-attribute of all matching records is replaced.

   o  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 the filter is not matching any records it should not be treated as an add operation but respond with error "noTarget".

[thomaspoignant]

So if I follow correctly in your case we should respond with a noTarget?

[arnecs]

At least for the replace operation.
For add operation it should add the specified target location.

[thomaspoignant]

@arnecs I've done a fix for the replace operation (see PR #44), for the add operation, I return the same exception.
It is available in the v0.3.0 version of scim-patch.
Thanks again for your feedback it helps me a lot to improve the library.

I was looking more in detail on what you purpose for the ADD operation and I have a doubt that what you want is doable.
In your example:

const resource = { schemas: ["urn:ietf:params:scim:schemas:core:2.0:User"], addresses: [] }
const patches = [{ op: "Add", path: "addresses[type eq \"work\"].streetAddress", value: "1010 Broadway Ave"}]

You expect to have this result:

[
  {
    "type": "work",
    "streetAddress": "1010 Broadway Ave"
  }
]

This case seems doable because you are using the filter eq, but if we want to have something like:

const resource = { schemas: ["urn:ietf:params:scim:schemas:core:2.0:User"], addresses: [] }
const patches = [{ op: "Add", path: "addresses[type ne \"work\"].streetAddress", value: "1010 Broadway Ave"}]

In that example we are using ne, we are not in a position where we can determine the type, so it is hard to know what is the object we want to insert here.
We will have this problem with all the conditions except eq (ne, co, sw, ew, gt, lt, ge, le).

And I am not sure we can ignore the type completely by adding something like:

[  {    "streetAddress": "1010 Broadway Ave"  } ]

because if you try to do the same thing again your filter will still not match the element you've added.

[arnecs]

Considering all different filters may be a problem. afaik, the RFC does not explicitly state what do in these cases.

---

Discovered the issue when working with SCIM user provisioning from Azure AD.
A user with no address info are created without any addresses.
Later, if an address-attribute is set, e.g. streetAddress, a PATCH request with the following operation is issued:

{"op":"Add","path":"addresses[type eq \"work\"].streetAddress","value":"1010 Broadway Ave"}]}

[thomaspoignant]

If this is the way that Azure AD is working it's maybe worth to do it only for the add operation.

I think this is a bit hacky but I think that being compatible with Azure AD is important.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[maxdeviant]

I'm seeing this issue with Azure AD as well:

{
  "op": "Add",
  "path": "addresses[type eq \"work\"].formatted",
  "value": "1111 Street Rd"
}

[thomaspoignant]

I’ll re-open the issue.
@maxdeviant if you want to contribute you’re welcome.

[thomaspoignant]

@arnecs @maxdeviant I've opened a PR #106 to fix this problem.
I will release a version later today.

[thomaspoignant]

The version v0.5.0 contains the fix for this issue and is available on npm.

[maxdeviant]

@thomaspoignant Thanks so much!

[leifdejong]

Thank you for the fix!