Captain-P-Goldfish / scim-for-keycloak

a third party module that extends keycloak by SCIM functionality
BSD 3-Clause "New" or "Revised" License
186 stars 48 forks source link

Scim add and remove member doesnt' work #16

Closed giovannialbero1992 closed 3 years ago

giovannialbero1992 commented 3 years ago

Hi @Captain-P-Goldfish thanks for your great work!

I've started to use your Keycloak plugin but I have an issue. I created by KC's admin panel a group and I tried to add a user (created before by user scim endpoint) to the group.

But the user doesn't appear in group membership.

The URL called with PATCH is: http://localhost:8080/auth/realms/myrealm/scim/v2/Groups/28bc3597-2046-40cc-ad4a-517049e303e4

This is my request body:

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ],
    "Operations": [
        {
            "op": "Add",
            "path": "members",
            "value": [
                {
                    "$ref": "http://localhost:8080/auth/realms/myrealm/scim/v2/Users/e0d3bf3b-f669-4eaf-a139-4b4b9b4df64b",
                    "value": "e0d3bf3b-f669-4eaf-a139-4b4b9b4df64b"
                }
            ]
        }
    ]
}

I receive the response:

{
    "id": "28bc3597-2046-40cc-ad4a-517049e303e4",
    "displayName": "Italy+Communications Italy",
    "members": [
        {
            "value": "c77bf49f-a026-49d5-bdf1-76da2ecc0163",
            "$ref": "http://localhost:8080/auth/realms/enel/scim/v2/Users/c77bf49f-a026-49d5-bdf1-76da2ecc0163",
            "type": "User"
        }
    ],
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:Group"
    ],
    "meta": {
        "resourceType": "Group",
        "created": "2021-07-15T16:32:26.106Z",
        "lastModified": "2021-07-15T16:32:26.106Z",
        "location": "http://localhost:8080/auth/realms/enel/scim/v2/Groups/28bc3597-2046-40cc-ad4a-517049e303e4"
    }
}

Keycloak version is: 12.0.2

In the response you can notice that the user doesn't appear :disappointed:

giovannialbero1992 commented 3 years ago

To add some more detail, I tried with Keycloak: 14.0.0 and plugin version 14-b2 and it works

Captain-P-Goldfish commented 3 years ago

I am rather irritated that it should work with kc-14-b2. This should result in the same problem. I must admit though that it is irritating that no error is raised in such a case. The problem is simply that:

"value": [
                {
                    "$ref": "http://localhost:8080/auth/realms/myrealm/scim/v2/Users/e0d3bf3b-f669-4eaf-a139-4b4b9b4df64b",
                    "value": "e0d3bf3b-f669-4eaf-a139-4b4b9b4df64b"
                }
            ]

the value node describing the member is missing the "type" attribute. This results in scim-for-keycloak not being able to determine to what type of resource the id should be linked. Is it a user or a group? The nature of the code - in how it was implemented - simiply bypasses members that have no "type"-attributes or even "type"-attributes that are not supported. You might get a glimpse of that from this code-snippet that shows how the user-members are selected:

              group.getMembers().stream()
                                             .filter(groupMember -> groupMember.getType().isPresent()
                                                                    && groupMember.getType()
                                                                                  .get()
                                                                                  .equalsIgnoreCase("User"))
                                             .map(groupMember -> groupMember.getValue().get())
                                             .collect(Collectors.toSet());

unfortunately I have no real good solution for cases in which the "type"-attribute is missing. But it should raise at least an error message if such incomplete member-definitions are present within the resource.

giovannialbero1992 commented 3 years ago

Maybe I've the same problem with KC 14.0.0, I talked too early 😅.

But reading RFC I didn't see any reference to the type attribute.

https://datatracker.ietf.org/doc/html/rfc7644#section-3.5.2

Captain-P-Goldfish commented 3 years ago

that is correct but how else would you suggest to determine if the ID belongs to a group or to an user? It would be possible to try on which resource the id exists but on the other hand no one could guarentee that the ids in use remain unique between both resource-tables so I cannot think of any other reliable way.

giovannialbero1992 commented 3 years ago

I didn't know that it is possible to add also another kind of resource, I believed that the only resource can be added to the groups is the user. Anyway you can consider the property $ref that contains information about the resource, right?

Captain-P-Goldfish commented 3 years ago

the $ref attribute would be as good as the type attribute with the difference that I would have to parse the type of resource out of the given URL. So it is a lot easier to use the type attribute for clients and the implementation.

giovannialbero1992 commented 3 years ago

Ok, I understand. But this is not a standard and when the endpoints are consumed by an external tool that can't be customized as we want it is a problem.

So I imagine that there isn't the possibility to have an endpoint rfc complaint 😔

Captain-P-Goldfish commented 3 years ago

is it possible for you to add the §ref-attribute with the tool? Alternatively I could add an optional handling to use either the type or $ref attribute to resolve the resource-type.

Otherwise - though I do not like it - we could say that if the type attribute is missing that it is expected to be a user. I would prefer the $ref-attribute solution though if this is possible for you

giovannialbero1992 commented 3 years ago

I think that the $ref attribute is always passed during this integration (but I'll discover definitively during my test) but, I'm pretty sure that the only resource that can be passed to a Group is a User in SCIM. Also reading several resources on the internet I didn't see the possibility to add a group to another group. Some resources here: https://docs.databricks.com/dev-tools/api/latest/scim/scim-groups.html https://help.quickbase.com/api-guide/scim_api_endpoint_details_syntax_groups.html https://docs.microsoft.com/en-us/azure/databricks/dev-tools/api/latest/scim/scim-groups

Tristanden commented 3 years ago

Hi, if that helps I have fixed this issue here : https://github.com/liksi/scim-for-keycloak/tree/feature/group-membership This is not very elegant but reliable enough for my use case (sync from Azure AD). Basically the ids that keycloak exposes to AAD are prefixed by u- or g- depending on whether this is a user or a group.

Captain-P-Goldfish commented 3 years ago

@giovannialbero1992 please see this (copied from RFC7643):

members A list of members of the Group. While values MAY be added or removed, sub-attributes of members are "immutable". The "value" sub-attribute contains the value of an "id" attribute of a SCIM resource, and the "$ref" sub-attribute must be the URI of a SCIM resource such as a "User", or a "Group". The intention of the "Group" type is to allow the service provider to support nested groups. Service providers MAY require clients to provide a non-empty value by setting the "required" attribute characteristic of a sub-attribute of the "members" attribute in the "Group" resource schema.

So by RFC7643 the members attribute is defined to support either "users" or "groups". But it is actually not restricted to those two. A valid Group-member is defined by the type of identity provider and its provided resources so a "client" might also be a valid resource if defined accordingly.

@Tristanden This solution works only if the IDs for resources are provided by the SCIM-API. I know at least about one case in which this is not the case so this solution would break things there.

giovannialbero1992 commented 3 years ago

Thanks @Captain-P-Goldfish, I found that I will have always the $ref property. can we evaluate $ref instead of type if the property type is missing?

Captain-P-Goldfish commented 3 years ago

this is possible. but i will give the type attribute precedence before the $ref attribute meaning if both are present tje type attribute will be used

Captain-P-Goldfish commented 3 years ago

Alright just added the new usecase but for kc-14 only.

This will now work. The id at the end of the $ref attribute will be ignored. The spec wants the value-attribute as the id and so it is used:

{
  "schemas" : [ "urn:ietf:params:scim:schemas:core:2.0:Group" ],
  "displayName" : "myGroup",
  "members" : [ {
    "value" : "e9dae6bb-7d08-4483-b00f-4bac1466728e",
    "$ref" : "http://localhost/scim/v2/Users/e9dae6bb-7d08-4483-b00f-4bac1466728e"
  }, {
    "value" : "63f957f9-6218-4d97-bb69-6dfcbb291166",
    "$ref" : "http://localhost/scim/v2/Groups/63f957f9-6218-4d97-bb69-6dfcbb291166"
  } ]
}
giovannialbero1992 commented 3 years ago

Thanks so much @Captain-P-Goldfish!

When will be released?

Captain-P-Goldfish commented 3 years ago

just released