jelhub / scimgateway

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

Fetching users returns users groups with all the groups members in them #71

Closed SamMurphyDev closed 2 years ago

SamMurphyDev commented 2 years ago

When fetching user (/Users), the groups' attribute is returned containing a list of all the groups the user is part of. Then each of those groups within the users returns a list of all their members. (Not representative of a complete response, here is an example of how the payload is currently structured)

{
  "id": "fbc56952-0c46-4c00-80b3-fc48fbef8eaa",
  "groups": [
    {
      "value": "383e9f70-a1af-4e26-b068-d2454aca0260",
      "members": [
        {
          "value": "fbc56952-0c46-4c00-80b3-fc48fbef8eaa"
        },
        {
          "value": "b6ccf7e6-ac9e-4db8-b81c-b0167d431a95"
        }
      ]
    }
  ]
}

This can create large responses when the user belongs to many groups with many users (in my case, 120MB for a single user).

I propose changing https://github.com/jelhub/scimgateway/blob/master/lib/scimgateway.js#L791 to remove the members.value. This could be breaking for people relying on it, or it could be hidden behind a feature flag.

Doing this is within the SCIM Schema specification. As outlined in section 4.1.2 https://www.rfc-editor.org/rfc/rfc7643#section-4.1.2, under the heading of groups,

SCIM service provider exposes a "Group" resource, the "value" sub-attribute MUST be the "id", and the "$ref" sub-attribute must be the URI of the corresponding "Group" resources to which the user belongs.

This, in my eyes, would mean that members is at least an optional attribute to return on the user response payload. This is further backed up by the full user representation listed here https://www.rfc-editor.org/rfc/rfc7643#section-8.2. While the specification refers to a non-normative example, it provides an insight into what the specification authors were expecting as part of the standard.

Let me know what you think. I've already implemented the change on my fork (without the feature flag). So happy to carry this forward to a PR if you agree with the above.

Cheers, Sam

jelhub commented 2 years ago

Hi,

You may test using plugin-loki

/Users groups attribute should have following syntax (value is mandatory) and only include groups the actual user is member of

    "groups": [{
            "value": "Employees",
            "display": "Employees",
            "type": "direct"
        }
    ],

If your plugin getUsers method do not return groups, scimgateway will automatically try /Groups?filter=members.value eq "userid"&attributes=members.value,id,displayName and include groups accordingly

I suspect your getUsers method includes groups and do not use correct format as shown above (but you also mention https://github.com/jelhub/scimgateway/blob/master/lib/scimgateway.js#L791 having the auto-apply logic when groups not included...)

You may also exclude groups from the response by using: /Users?excludedAttributes=groups or your getUsers could include empty groups "groups": []

Regards, Jarle

jelhub commented 2 years ago

Your issue might refer to /Groups?filter=members.value eq "userid"&attributes=members.value,id,displayName returning what you mention (includes all members for the group user is member of)

Your are correct, skipping members.value in attributes is better attributes=id,displayName and I might look into this change, but I don't think general plugins care much of query attributes used. They probably return all supported without any query check.

The reason for including members.value was to double check that actual user is included in case plugin do not return /Groups as expected.

But anyhow these other members will not be included in the groups returned by /Users

Jarle

jelhub commented 2 years ago

v4.1.10 have been published addressing this problem

Jarle