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

givenName and familyName not being updated on PATCH requests #123

Closed
eduardoborba opened this issue May 16, 2024 · 10 comments
Closed

givenName and familyName not being updated on PATCH requests #123

eduardoborba opened this issue May 16, 2024 · 10 comments

Comments

@eduardoborba
Copy link

I'm having this issue while testing the provisioning from Azure where the givenName and the familyName are not being stored in the database correctly. This is what my mapping looks like:

  def self.scim_attributes_map
    return {
      id: :uuid,
      externalId: :external_id,
      userName: :email_address,
      active: :active,
      name: {
        givenName:  :first_name,
        familyName: :last_name,
      },
      title: :job_title,
    }
  end

This is what the Azure request looks like:

        patch "/scim_v2/Users/#{user_id}",
              headers: {
                "Authorization" => "Bearer #{authorization_token}",
                "Content-Type" => "application/scim+json; charset=utf-8"
              },
              params: {
                "schemas" => [
                  "urn:ietf:params:scim:api:messages:2.0:PatchOp"
                ],
                "Operations" => [
                  {
                    "op" => "add",
                    "value" => {
                      "displayName" => "Foo Bar",
                      "title" => "Bar Foo",
                      "name.givenName" => "Foo",
                      "name.familyName" => "Bar",
                      "name.formatted"=>"Foo Bar",
                    },
                  },
                ]
              }.to_json

Expected output:

Store the givenName in the column first_name and the familyName in the last_name column.

Actual output:

Those are attributes are not being stored at all on PATCH requests.

Additional info:

On create requests, those fields are properly populated though, so the mapping is correct. It must be something on the from_scim_patch! method.

@eduardoborba
Copy link
Author

Hi @pond, do you have any idea on what is going on here? Are you able to replicate the issue?

@pond
Copy link
Member

pond commented May 26, 2024

Sorry for the delay @eduardoborba - super busy in the day job and this is now a one-man-band for maintenance and development sadly so it doesn't get the attention it should. I'll look into this as soon as I can, you're not being ignored and you've not been forgotten; sorry again for the length of time it's taken to even send this response, though.

@pond
Copy link
Member

pond commented May 27, 2024

@eduardoborba - a question; are you able to modify that payload? The specific issue is the dotted notation inside the attributes within the value object, which I can't see documented just about anywhere as valid for PATCH "add", but it kinda makes sense in context. However, I think you ought to have e.g. name.givenName in path and value is then just "Foo"; or path is name, then your value contains e.g. "givenName" => "Foo".

For the syntax of path, see:

The path is optional for add operations, but:

...says:

   The operation MUST contain a "value" member whose content specifies
   the value to be added.  The value MAY be a quoted value, or it may be
   a JSON object containing the sub-attributes of the complex attribute
   specified in the operation's "path"."

...which does not appear to permit a JSON object where relative attribute paths are specified; just sub-attributes of a given path.

@eduardoborba
Copy link
Author

@pond That's the payload we are getting from Azure, we don't really control that. This is what our attributes mapping looks like on the Azure provisioning settings:
Screenshot 2024-06-04 at 15 05 57

Let me know if you have any suggestions to fix that by changing the mapping.

@pond
Copy link
Member

pond commented Jun 9, 2024

I've spent an unhealthy amount of time ploughing through mile after mile over blustering, exceptionally over-complex documentation from Microsoft, all of which seems to totally miss the point and none of which includes any screenshots matching what you have above LOL. Unusually it's not a lack of documentation this time, but a totally overwhelming amount of documentation - but all of it seems very similar and there is no clear steer on which bit of it is relevant at any given time.

The closest thing to useful that I could find is, of course, not from Microsoft, but this document:

...which does show things like name.familyName as a user attribute mapping. It would seem that Microsoft's implementation just treats that as a blunt string with no understanding of what it means, throwing it directly into PATCH requests which are technically malformed (as far as I can tell - SCIM itself is an inconsistent and over-complex protocol that reeks of design-by-committee with nobody involved being responsible for producing a reference implementation and test suite).

This basically seems to mean that I somehow have to implement the nightmare of arbitrary paths inside already-extremely-awkward PATCH requests.

What I don't understand is why you're seeing this and nobody else is. We've had bug reports from the MS SCIM validator, and once fixed people have seemed happy. Perhaps they just gave up on Scimitar for Azure AD, or perhaps they got it to work - but in that case, why is your situation different? There's just no way I can devote the time to trying to understand the overwhelming mess of Microsoft's Azure-now-Entra to try and work that out.

@pond
Copy link
Member

pond commented Jun 11, 2024

Well, I think #125 should do it - under internal review now.

@pond
Copy link
Member

pond commented Jun 11, 2024

@eduardoborba Please could you give https://rubygems.org/gems/scimitar/versions/2.7.3 a try? I suspect we might need more than one iteration on this, but it'd be good to see if it at least moves things forward. Thanks!

@eduardoborba
Copy link
Author

Hey @pond,

Really appreciate the effort. I'm going to test that later today and will let you know.

@eduardoborba
Copy link
Author

@pond it's working perfectly now, thank you very much!

Feel free to close this issue :)

@pond
Copy link
Member

pond commented Jun 11, 2024

Fantastic news! Thanks for the detailed bug report and I'm glad that it's working.

@pond pond closed this as completed Jun 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

No branches or pull requests

2 participants