elimity-com / scim

Golang Implementation of the SCIM v2 Specification
MIT License
168 stars 55 forks source link

Proposal: make the meta.Location field modifiable #172

Closed icamys closed 6 months ago

icamys commented 6 months ago

The SCIM specification says that the "meta.location" attribute must be present in most of the responses, and it should be the resource's URL. This package returns this attribute, according to the code. The value is composed out of the resource endpoint and the ID:

m := meta{
    ResourceType: resourceType.Name,
    Location:     fmt.Sprintf("%s/%s", resourceType.Endpoint[1:], url.PathEscape(r.ID)),
}

There are a couple of issues with this approach:

  1. The Location value is not a valid URL; it'll just contain a string like Users/123.
  2. In a multi-tenant setup, the location should include the tenant ID: http://example.com/scim/v2/tenants/456/Users/123. This is possible only if the HTTP callback can set the location on its own.

The meta.Location setup is hidden, and there's no way to change it. My question is whether it is reasonable to add a Location string field to the public Meta structure and use it while constructing the response if it is present? This will make it possible to fix the invalid URL and enable multi-tenancy support.

If you are open to discussing this, I can prepare a PR with a proposed solution.

q-uint commented 6 months ago

Thanks for opening this discussion!

A location in SCIM is also allowed to be a partial URI, which means that the relative part is enough. Of course this should be relative to the root and not to the SCIM instance itself.

Would it be enough to configure this on the server level?

http://example.com/scim/v2/tenants/456/Users/ is not valid in SCIM, it is required to have the v2 prefix e.g. http://example.com/tenants/456/scim/v2/Users/

icamys commented 6 months ago

@q-uint Thank you for the response!

A location in SCIM is also allowed to be a partial URI

To be honest, I did not find where it was in the specification. All public SCIM-related documentation provides only complete URLs. Could you please direct me to where I can read about allowed partial URI?

http://example.com/scim/v2/tenants/456/Users/ is not valid in SCIM, it is required to have the v2 prefix e.g. http://example.com/tenants/456/scim/v2/Users/

This is an interesting note. After studying the specs again, this is what I found in the Base URI definition:

For example, a SCIM server might have a prefix of "https://example.com/" or "https://example.com/scim/tenancypath/". Additionally, a client MAY apply a version number to the server root prefix (see Section 3.13).

And in the Section 3.13:

The Base URL MAY be appended with a version identifier as a separate segment in the URL path. At the time of this writing, the identifier is 'v2'. If specified, the version identifier MUST appear in the URL path immediately preceding the resource endpoint and conform to the following scheme: the character 'v' followed by the desired SCIM version number, e.g., a version 'v2' User request is specified as /v2/Users. When specified, service providers MUST perform the operation using the desired version or reject the request. When omitted, service providers SHOULD perform the operation using the most recent SCIM protocol version supported by the service provider.

This is what I see in these two passages:

  1. The tenancy path is a part of the base URI.
  2. The tenancy path goes after the /scim part. Update: since both are a part of the base URI, perhaps the sequence is not essential.
  3. The /v2 part is optional.

Considering this, the base URI for a multitenant SCIM provider should be https://scim-provider.com/scim/tenants/123/v2/ (or without v2/). In this case, the only question remains about whether the URI can be partial. If so, then this package does not need the proposed modification.

icamys commented 6 months ago

Another related question: is the Location header mandatory? Does it make sense to set the same value for the meta.location and the Location header on response construction?

q-uint commented 6 months ago

v2 prefix

I am totally mistaking, about the v2 prefix.

Location Header

As far as I know the Location header is not mandatory. If might make sense to set this if meta.location is present.

Partial URI

In RFC7643 Section 3.1 it says:

The URI of the resource being returned. This value MUST be the same as the "Content-Location" HTTP response header. Which is defined in RFC 7231 Section 3.1.4.2: Content-Location = absolute-URI / partial-URI

icamys commented 6 months ago

I can confirm that the current implementation works, at least with Azure Entra ID, when a partial URI is returned in the meta.location and none is returned in the Location header. So, making the location modifiable does not make sense now.