jelhub / scimgateway

Using SCIM protocol as a gateway for user provisioning to other endpoints
MIT License
176 stars 57 forks source link

Azure modifyUser throws exception if deleting a user #94

Closed osbornk closed 1 year ago

osbornk commented 1 year ago

We use Azure, which uses a PATCH operation as a soft delete by setting active = false. But our scim plugin interprets this as an actual delete operation. So, we actually delete the user and it now no longer exists. It does not have any harmful effects, but does cause the request to return a 500.

The problem is this line:

https://github.com/jelhub/scimgateway/blob/master/lib/scimgateway.js#L1304

In contrast, the modifyUser PUT operation handles this without error.

if (res && res.Resources && Array.isArray(res.Resources) && res.Resources.length === 1) scimdata = res.Resources[0]
        else if (Array.isArray(res) && res.length === 1) scimdata = res[0]
        else if (res && typeof (res) === 'object' && Object.keys(res).length > 0) scimdata = res
        else throw Error(`put using method ${handle.getMethod} got unexpected response: ${JSON.stringify(res)}`)

Notice the res.Resources.length === 1. So we end up in the third conditional and just return an empty array.

I can make a change and create a pull request, but I don't want to undo intentional code and this looks intentional. But it is throwing an exception if PATCH modifyUser causes the user to be removed.

jelhub commented 1 year ago

I will fix this, basically removing line https://github.com/jelhub/scimgateway/blob/master/lib/scimgateway.js#L1304

modifyUser (PATCH) logic becomes somehow odd when after modifying there is a user lookup for returning the user object. Now user is not found, and we do not throw this as an exception anymore. Anyhow modifyUser was OK, assume returning an empty response will not become a failure message in Azure?

osbornk commented 1 year ago

The 500 itself does not become a failure in Azure today. So I don't think it really cares about the response. It is more about the cleanliness of the application and not throwing exceptions in what is actually a successful state.

But once you push up a commit I can certainly test it out for you.

Thanks.

jelhub commented 1 year ago

What is your res content (getUser result)? I have problems to see why PUT is working

jelhub commented 1 year ago

Please ignore above update. I now see why PUT is working.

jelhub commented 1 year ago

Fix now included in v4.2.7 release

osbornk commented 1 year ago

Looks good now. Thanks.