Captain-P-Goldfish / SCIM-SDK

a scim implementation as described in RFC7643 and RFC7644
https://github.com/Captain-P-Goldfish/SCIM/wiki
BSD 3-Clause "New" or "Revised" License
122 stars 38 forks source link

Empty ref node calculation does not work for canonical 'User.groups[].type' value #176

Closed hamiltont closed 3 years ago

hamiltont commented 3 years ago

Running version 1.10.0.

Issue

The feature of adding a $ref value is super handy. Unfortunately it does not seem to work with the canonical values for listing groups a user belongs to, because it uses the RFC7643.TYPE field to try and lookup a resource type. This fails if you use the RFC7643 recommended values of "direct" or "indirect". Seems like a simple fix is possible - add a special case to check if the TYPE field is one of the canonical values of "direct" or "indirect", and map that to "Group".

For example:

    "groups": [
        {
            "value": "9d66400a-149e-40da-b55a-96d9bb6100aa",
            "display": "test group",
            "type": "indirect"
        }
    ],

Workaround

Use non-canonical resource name e.g. GroupNode.builder().type("Group") instead of GroupNode.builder().type("direct"). This works as expected:

    "groups": [
        {
            "value": "9d66400a-149e-40da-b55a-96d9bb6100aa",
            "$ref": "http://localhost:8089/scim/v2/Groups/9d66400a-149e-40da-b55a-96d9bb6100aa",
            "display": "test group",
            "type": "Group"
        }
    ],

RFC7643 section 4.1.2

groups A list of groups to which the user belongs, either through direct membership, through nested groups, or dynamically calculated. The values are meant to enable expression of common group-based or role-based access control models, although no explicit authorization model is defined. It is intended that the semantics of group membership and any behavior or authorization granted as a result of membership are defined by the service provider. The canonical types "direct" and "indirect" are defined to describe how the group membership was derived. Direct group membership indicates that the user is directly associated with the group and SHOULD indicate that clients may modify membership through the "Group" resource. Indirect membership indicates that user membership is transitive or dynamic and implies that clients cannot modify indirect group membership through the "Group" resource but MAY modify direct group membership through the "Group" resource, which may influence indirect memberships. If the 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. Since this attribute has a mutability of "readOnly", group membership changes MUST be applied via the "Group" Resource (Section 4.2). This attribute has a mutability of "readOnly".

Relevant code

See ResponseAttributeValidator#overrideEmptyReferenceNodeInComplex:

Captain-P-Goldfish commented 3 years ago

I see your point but I would need to think of a reliable solution. The custom case you mentioned is not an option for this implementation. The type could be used with values of "direct" and "indirect" in other custom resources too this would create a conflict causing reference urls of Group-resources to be entered in other resources. Would it be possible for you to define a custom attribute in the MEMBER-attribute that you use do place the "direct" and "indirect" values?

Alternatively I might be able to add a Function in the ResourceHandler supplier that you might use to add the url reference manually:

getResourceReferenceUrl.apply(resourceId)

what do you think? would this be an appropriate solution for you?

Captain-P-Goldfish commented 3 years ago

I just noticed that the previous noted solution with the Function is not so easily achieved. This would require an API change on the method structure of the ResourceHandler.

Captain-P-Goldfish commented 3 years ago

Please take a short look at the changes. I changed the method signature on the ResourceHandler methods. They will receive a Context object now that provides the Authorization object that was previously in the method signature itself and also 2 methods that should provide you with convenience functionality that should meet your requirements.

The references are not added automatically as you would have liked but I fear I have no better solution here.

hamiltont commented 3 years ago

Oh, I much prefer the Context based approach. In the future it would be much simpler to expand effectiveContext with new variables/objects if other feature requests come in.

It looks like it would force a code update on all users of the current API, (which I am fine with!) - that could be mitigated with some default facade methods that accept the new Context and by default call the corresponding 'auth-only' legacy methods. I personally do not need this, just mentioning it as a possibility

Thanks for the quick response!

Captain-P-Goldfish commented 3 years ago

I actually wanted to this for quite some time now since I was very unhappy with the Authorization-interface that was used as context so far. but great I consider this fixed then.