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

PUT requests are incorrectly returning 204 #35

Closed
davefend-okta opened this issue Apr 2, 2021 · 10 comments
Closed

PUT requests are incorrectly returning 204 #35

davefend-okta opened this issue Apr 2, 2021 · 10 comments

Comments

@davefend-okta
Copy link

modifyUser does not work correctly for PUT requests. It always returns a 204. That is only a valid response for PATCH requests. Per spec, a PUT request must return a 200 with the entire resource within the response body".

"Unless otherwise specified, a successful PUT operation returns a 200
OK response code and the entire resource within the response body,
enabling the client to correlate the client's and the service
provider's views of the updated resource."

@jelhub
Copy link
Owner

jelhub commented Apr 4, 2021

Now fixed in v3.2.3, please verify
Thanks, Jarle

@davefend-okta
Copy link
Author

davefend-okta commented Apr 7, 2021

User updates are now failing. Looks like an error is occurring when reading the email object returned. Type is set to "work" in the response object for getUser()- conversion is failing. Here is a sample error file (scrubbed for sensitivity):

2021-04-07T14:18:05.591 plugin-myplugin debug: name -> [object Object]
2021-04-07T14:18:05.592 plugin-myplugin debug: active -> true
2021-04-07T14:18:05.592 plugin-myplugin debug: emails -> [object Object]
2021-04-07T14:18:05.592 plugin-myplugin debug: elements name
2021-04-07T14:18:05.592 plugin-myplugin debug: elements active
2021-04-07T14:18:05.592 plugin-myplugin debug: element active true
2021-04-07T14:18:05.593 plugin-myplugin debug: elements emails
2021-04-07T14:18:05.594 plugin-myplugin error: scimgateway[plugin-myplugin] 500ms 52.200.197.156 user@example.com PUT http://127.0.0.1/Users/ebe4641e-cb97-eb11-b1ac-0022480a0a18 Inbound = {"schemas":["urn:ietf:params:scim:schemas:core:2.0:User"],"id":"ebe4641e-cb97-eb11-b1ac-0022480a0a18","userName":"00uhw200fdiY1ZHOI1t7","name":{"givenName":"Steve","familyName":"Smith"},"emails":[{"primary":true,"value":"steve.smith@mailinator.com","type":"work","display":"steve.smith@mailinator.com"}],"active":"true","meta":{"resourceType":"User","location":"http://127.0.0.1/Users/ebe4641e-cb97-eb11-b1ac-0022480a0a18"},"externalId":"00uhw200fdiY1ZHOI1t7","groups":[]} Outbound = {"statusCode":500,"statusMessage":"Internal Server Error","body":{"schemas":["urn:ietf:params:scim:api:messages:2.0:Error"],"detail":"ScimGateway[plugin-myplugin] TypeError: Cannot read property 'value' of undefined","status":500}}

@jelhub
Copy link
Owner

jelhub commented Apr 7, 2021

Your request works fine using plugin-loki having id, userName and externalId replaced with "bjensen"

Seems your plugin gives an exception when accessing <someobject>.value
This is probably related to emails, maybe you access emails as array like: emails[0].value

Complex attributes like emails, phoneNumbers, addresses, entitlements,... (but not roles) are sent to plugin as "type converted objects"

You will get something like below (emails.home exist on user but not included in the new PUT)

"emails": {
  "work": {
	"primary": true,
	"value": "steve.smith@mailinator.com",
	"type": "work",
	"display": "steve.smith@mailinator.com"
  },
  "home": {
	"operation": "delete",
	"value": "",
	"type": "home"
  }
}

So, you might have to do something like:

if (attrObj.emails && attrObj.emails.work && attrObj.emails.work.value !== undefined) email_endpoint = attrObj.emails.work.value

After successfully modifyUser, SCIM gateway will call your plugin method getGroupMembers for retrieving all groups to be included in response. If you don't use groups or have not implemented any logic, this method should return an empty array e.g.: return []

@davefend-okta
Copy link
Author

The issue seems to be tied to whether emails is passed as an array object when there is only a single object. I was able to resolve the issue in my plugin by first validating whether the emails element isArray but this doesn't seem like it should be necessary. Should't it always be an array?
Likewise, when validating this new update for PUT, we hit an issue where if in getUser the retObj has an emails element defined as an array with a single element, scimgateway.js throws an error trying to do a toLowerCase() on an undefined element. If the plugin returns the emails element as a single object (not array), it works fine.

@jelhub
Copy link
Owner

jelhub commented Apr 8, 2021

For PUT SCIM Gateway will do following:

  1. Call plugin getUser - getUser should always return according to SCIM (emails always array and should include type and value)
    e.g. you may test plugin-loki using http://localhost:8880/Users/bjensen

gives following emails array:

"emails": [
  {
    "value": "bjensen@example.com",
    "type": "work",
    "primary": true
    },
    {
      "value": "babs@jensen.org",
      "type": "home"
    }
]  
  1. Result from Compliance Tests Failing  #1 getUser will become cleared and merged with the PUT request

  2. Call plugin modifyUser using the merged object from Security vunerability - SOAP / EJS #2
    Note, gateway will always send emails as "type converted object" to plugin as long as "type" is used - not using SCIM array like shown in Compliance Tests Failing  #1

  3. Call plugin getUser to retrieve the updated user

  4. If groups not included, call plugin getGroupMembers for retrieving all groups

  5. Return the fully user object to caller

You should first verify that your getUser (no query attributes) returns the fully user object according to SCIM
test: http://127.0.0.1/Users/ebe4641e-cb97-eb11-b1ac-0022480a0a18

If emails are not according to SCIM, then this problem will also be included in the #3 modifyUser sent to plugin. Your code then have to deal with the format you previously used.

@davefend-okta
Copy link
Author

Not sure if my prior post was clear and if we talked passed each other on this issue. For your #1 comment above, when getUser() is called, if my plug-in returned emails as an array then scimgateway.js throws an exception trying to do a toLowerCase(). I have not tried to personally track down exactly where or why this happens but this appears to be an issue with scimgateway.js. This does not happen on PATCH requests, only PUT requests.

My plugin returns something like this which from what I read from your comments above should be correct since its per SCIM specification:
"emails": [
{
"value": "bjensen@example.com",
"type": "work",
"primary": true
}
]

On your comment #3, "gateway will always send emails as "type converted object" to plugin as long as type is use. I presume that is what caused my initial error which I've mitigated by checking if the emails element is an array first. However, how does the gateway send this to the plugin if there are multiple emails? I don't have to support this use case at this time and I have the issue mitigated so more of a curious request. Also, curious why you don't just pass through the array which at face value to me as a newbie seems more straightforward and expected.

@jelhub
Copy link
Owner

jelhub commented Apr 9, 2021

Multiple emails will be:

"emails": {
  "work": {...},
  "home": {...},
  "test": {...},
  "abc": {...}
}

But you cannot have more than one work email

Reason for objects instead of array is simplicity of checking attributes included.
I think most IdP's use PATCH instead of PUT and then sending only modified attributes.

SCIM Gateway actually only cares about some few mandatory SCIM attributes
User: id, userName and/or eksternalId
Group: id, displayName and members
All other attributes can be whatever and will be passed "as-is" to plugin, and it's up to plugin to decide what should be sent back to client. But as mentioned, there is "type converted object" logic

Do your client and plugin use type or not?
According to info given, both do.
It must be consisent.

If getUser response includes type, but not the PUT, I have reproduced the following error:
TypeError: Cannot read property 'toLowerCase' of undefined"

Merged object then includes emails having objects with and without type

"type converted object" logic checks the first object in array for type. If type then all objects will be
converted and the type will be lowercase, hence failing if type do not exist on one of the objects.
This code part needs to be fixed. If mix and we have a "none type", the type should probably be set to "undefined".

I assume you have a mix of type and none-type in the getUser response and the PUT request?

@davefend-okta
Copy link
Author

I validated your presumption that we are seeing type come in as null from the client / IdP on the PUT command. I'm not sure why this is the case and we will be submitting a case for that item to be investigated.

Honestly, would prefer if this type conversion wasn't occurring and would be nice if we were just able to provide the object to send back but understand that's not how this implementation was designed.

@jelhub
Copy link
Owner

jelhub commented Apr 13, 2021

Object sent back is not "type converted". Back must be according to SCIM or your client.

v3.2.5 is almost ready and will soon be published.

In this version a "blank type" will be converted to type="undefined" (plugin will newer get an array)
Duplicate types or blank's e.g several "work" or several "blank" will give give a human friendly error message.

You may also override this "type converted object" logic by a new configuration setting excludeTypeConvert

"scimgateway: {
  "scim": {
	"excludeTypeConvert": ["emails", "addresses"]
	...
  }
  ...
}

Note, endpointMapper logic do not currently support excludeTypeConvert, but you are not using endpointMapper (see plugin-ldap and plugin-azure)

Type converted or not, in your case you have a problem when user was created/modified with emails.work and then later on using a PUT having emails only (without type=work). You then have two different incoming SCIM attributes that should be mapped to the same endpoint attribute

@jelhub
Copy link
Owner

jelhub commented Apr 14, 2021

v3.2.5 have now been published

scim.skipTypeConvert = true, will disable "type converted object" logic

"scimgateway: {
  "scim": {
    "skipTypeConvert": true
    ...
}

This new version should not give error message mention in this issue.
When PUT don't use type, but getUser returns user with type, you will probably end up with following in modifyUser:

"emails": {
  "undefined": {"value": "steve.smith@mailinator.com"},
  "work": {"value": "", "type": "work", "operation": "delete"}
}

scim.skipTypeConvert = true

"emails": [
  {"value": "steve.smith@mailinator.com"},
  {"value": "", "type": "work", "operation": "delete"}
[

@jelhub jelhub closed this as completed Apr 14, 2021
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