Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying of nested values in dotpath notation for path-less replace operations #69

Merged

Conversation

wooly
Copy link

@wooly wooly commented Oct 6, 2023

Hey @pond

Bit of a mouthful and I'm really just throwing spaghetti at the wall at this point to see if I'm barking up the right tree.

Microsoft's SCIM Validator strikes again. This time they're failing us on path-less replace operations not updating attributes when those attributes are in dotpath notation.

An example of a request is below:

PATCH https://app.colloquial.io/scim_v2/Users/326 1.1

Host: app.colloquial.io

Content-Type: application/scim+json; charset=utf-8

{
  "Operations": [
    {
      "op": "replace",
      "path": "emails[type eq \"work\"].value",
      "value": "trystan@bradtkekutch.us"
    },
    {
      "op": "replace",
      "value": {
        "userName": "lucienne_durgan@wiza.biz",
        "name.givenName": "Keith",
        "name.familyName": "Stella",
      }
    }
  ],
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ]
}

In this case their expectation is that the givenName is updated to Keith and the familyName is similarly updated. I've created a test case for this that's similar to an existing one. This test fails without the patch to create the dot_pathed_value hash, but is probably the wrong place for this to live.

Any thoughts or guidance on how I can get this over the line would be much appreciated!

Cheers
W

@pond
Copy link
Member

pond commented Oct 8, 2023

As far as I can tell from RFC 2744 that's an entirely illegal payload (there should never be dotted values there, those are for paths only) and indeed Microsoft's own documentation does not attempt this:

https://learn.microsoft.com/en-us/azure/active-directory/app-provisioning/use-scim-to-provision-users-and-groups#update-user-multi-valued-properties

Same sort of thing in many other references, in fact I was unable to find a single document that used the form you give above; one example amongst many, but this is good as it specifically calls out the complex case of name:

https://is.docs.wso2.com/en/6.0.0/apis/scim2-patch-operations/

So I'm really not sure what to do about that one except submit a bug report to Microsoft because their validator is attempting to validate illegal SCIM operations that don't even match their own documentation saying what their SCIM system does?

Microsoft, as one of the largest players, has the largest responsibility to write code that conforms to the standards it purports to implement, yet it always seems to do the opposite and we've no recourse but to work around issues. The SCIM RFCs are IMHO quite badly written with a lot left to interpretation and it's even possible that maybe the form you give above is perhaps considered valid in some quarters. You have working code to patch it, albeit rather heavyweight; I don't think we should have that extra little service method dot_path necessarily in there tho as it does pollute the included namespace from the mixin and risks collision with a same-named method in client code. I'll have a think about it. At least you have something working that you can use off a branch for your application for now.

(We've never had to use the Microsoft integration so never ran their validator - we obviously should have, but it's a question of available time and resources - we figured SCIM would be used a lot by our customers but in practice it's hardly ever enabled by them, for whatever reason).

@wooly
Copy link
Author

wooly commented Oct 9, 2023

Thanks for taking the time to respond. I was also unable to find any examples containing the dotted notation for updating single complex values, but then I also couldn't find anything in the spec prohibiting their use.

We have a workaround for now, so I'll go ahead and close this and have a think about how to do it in a neater way, as agree that a potential collision via mixin is asking for trouble.

@pond
Copy link
Member

pond commented Oct 23, 2023

I'm actually happy this is still open, because as usual if Microsoft is doing something - compliant with the specification or otherwise - there's usually little choice for the whole of the rest of the industry to burn engineering time and money on dealing with it!

Your solution works, I just wanted to solve that encapsulation issue. With your permission, I could tinker with the code here accordingly & we see how it looks?

@wooly
Copy link
Author

wooly commented Oct 23, 2023

By all means!

@pond
Copy link
Member

pond commented Oct 30, 2023

I don't have push permission to your own branch, so pushed to a same-name duplicate and created a PR under #79.

If you're happy with what's going on there, perhaps the cleanest way to do this is to pull commit 84242388f8b08684973d2eb5dc2c4a0f850dc396 into your own PR so we the maintain correct history and ownership of the PR, then merge from here (and I'll close #79).

@pond
Copy link
Member

pond commented Nov 14, 2023

@wooly Bump on this, if you have a chance to look at it (see comment above).

@wooly
Copy link
Author

wooly commented Nov 15, 2023

Hi @pond, sorry for the delay on this, will get to it as soon as I have some free cycles!

@pond
Copy link
Member

pond commented Nov 15, 2023

@wooly No worries, I'm the very last person that could possibly complain about delays due to being too busy with other stuff 😂

@wooly wooly force-pushed the topic/complex-values-replace branch from 33f2bb7 to a5b6dcc Compare November 15, 2023 22:30
@wooly wooly force-pushed the topic/complex-values-replace branch from a5b6dcc to 28816cd Compare November 15, 2023 22:33
@wooly
Copy link
Author

wooly commented Jan 10, 2024

@pond I think this is now ready?

@pond
Copy link
Member

pond commented Jan 11, 2024

Great. Thanks!

@pond pond merged commit 0317e8c into RIPAGlobal:main Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants