elimity-com / scim Goto Github PK

We should ensure that the value is a so-called relative-path reference (cf. https://tools.ietf.org/html/rfc3986#section-4.2), to eliminate cases where endpoints reside outside of the server's reach.

https://tools.ietf.org/html/rfc7644#section-3.12

Use the 'new' dependency management system that makes dependency version information explicit and easier to manage.

Percent-encode URLs as described in Section 2.1 of RFC3986.

• create server implementing http.Handler, so you can attach it to one endpoint and don't have to define 10.
• implement /Schemas and children if applicable

There are still a lot of (small) things which are not tested.
This should prevent mistakes like: #67.

Currently we require schemas, resource types and configuration to be loaded from JSON. This complicates validation and is not type-safe. I propose we use go structures to make the process of setting up a server more user-friendly.

Hi,

First off, thanks for the nice library.

According to the spec, when doing a GetAll a query argument of zero or negative would indicate that the resources need only be counted:
https://tools.ietf.org/html/rfc7644#section-3.4.2.4

Specifies the desired maximum number of query results per page, e.g., 10. A negative value SHALL be interpreted as "0". A value of "0" indicates that no resource results are to be returned except for "totalResults".

The same is documented in your godoc:
https://pkg.go.dev/github.com/elimity-com/scim#ListRequestParams

However, in reality a check in the server prevents the count value from ever being anything less than 1, hence such a query is impossible to perform:

Hi,
I am using this wonderful library to implement the SCIM server.

If /Bulk was not supported, I fear that the number of requests from SCIM clients would be quite high and the server might not be able to handle it.

Therefore, I want to know why you don't support /Bulk.

I look forward to your reply.

Curious about your thoughts on this. One last requirement I need to meet before deploying my SCIM implementation is more granular auth. In my implementation we are a service provider allowing customers the ability to provision/deprovision users with us. Each customer has a unique tenant
id. I need to pull claims out of the JWT to further filter GET requests, and validate create or update requests. Is there a way to access request context from within a handler? If not, would you support that sort of functionality being added?

Maybe not relevant in our case?

Link: Error Wrapping

All of the scim types from the specs
https://tools.ietf.org/html/rfc7644#section-3.12
are pre-defined in the library but not all of them are used.

I believe that it's possible to cover some of them on the side of the library as it has a validation logic.

• invalidFilter
  Currently errors.ScimErrorBadParams is returned but it should be just errors.ScimErrorInvalidFilter.

• mutability
  Mutability is not checked. It should return mutability error in case when user tries to change immutable attributes that are defined immutable via the schema.

• invalidPath
  Path isn't validated but might be validated with using the schema.

https://tools.ietf.org/html/rfc7643#section-3

Line should be removed.

I noticed the schema for complex types is not quite marshaling right. The subAttributes field is being marshalled as {}. Looks like an easy fix. Going to try getting a PR soon.

Looks like it just needs to be handled here:

Defining attribute as schema.AttributeMutabilityImmutable() doesn't work:
I created a user schema with the userName attribute as immutable so it can only be entered when creating a user, after performing a POST request I got the following error: {“schemas”:[“urn:ietf:params:scim:api:messages:2.0:Error”],“scimType”:“mutability”,“detail”:“The attempted modification is not compatible with the target attribute’s mutability or current state.“,”status”:“400”}

i think the issue is at resource_type.go validate function where the switch case should have been case http.MethodPost, http.MethodPut: and not case http.MethodGet, http.MethodPut:

• validate using json schema
• structs schema and attribute
• struct server with reference to schema(s): user schema, group schema and sp_config schema
• schema.FromPath(...) & schema.FromString(...) (+ validation)
• api to create new server: scim.New(userSchema, groupSchema Schema)

RFC: Section 2.3.7 | Section 7

The value MUST be the absolute or relative URI of the target resource.

Currently reference types are only checked whether they are a string.

AttrName = ALPHA *(nameChar)
nameChar = "-" / "_" / DIGIT / ALPHA

Great work on this library, loving it so far! I would like to contribute. I am planning on using this library to build out our SCIM implementation. There are 3 things missing that I need. Filtering, Pagination, and PATCH support. I noticed the contribution docs mention that I should discuss my intent to contribute with y'all, so here I am!

I have only given the internals a cursory glance but this would be my plan.

For filtering and pagination, I would just make sure the params get to the callbacks/handlers and then just make sure there is a mechanism to represent the proper details in the response.

For PATCH support I would like the library to support it, and do the proper validation but I am not sure what that would take. Perhaps for the PATCH endpoints, I just implement them in my app? Curious if you have any feedback?

Attempting to PATCH with a path filter always returns ScimErrorInvalidValue.

e.g. using the example from the RFC: https://tools.ietf.org/html/rfc7644#section-3.5.2.2

$ curl -si -X PATCH <server>/Groups/acbf3ae7-8463-...-9b4da3f908ce -d '{
  "schemas":
   ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
  "Operations":[{
    "op":"remove",
    "path":"members[value eq \"2819c223-7f76-...413861904646\"]"
  }]
}'
HTTP/1.1 400 Bad Request
Content-Type: application/scim+json
Date: Thu, 17 Sep 2020 05:46:58 GMT
Content-Length: 241

{"schemas":["urn:ietf:params:scim:api:messages:2.0:Error"],"scimType":"invalidValue","detail":"A required value was missing, or the value specified was not compatible with the operation or attribute type, or resource schema.","status":"400"}

This doesn't error when the "path" in the operation is simple, like "path":"members", or when a "value" is provided - but I believe "remove" operations shouldn't specify a "value" param.

It seems like the operation validation treats the path as if it is the name of an attribute, which causes the attribute validation to always return ScimErrorInvalidValue since there is no field members[value eq "2819c223-7f76-...413861904646"] in my schema.

The patch support added in #65 appeared to skip value validation if there was a path filter, but the latest code no longer skips the value validation like this.

• create resource handler callback interface (w/ get, delete, etc...)
• link callbacks w/ resource types
• abstract data structures for callback responses

Description

I would like to have more flexibility in the errors returned in the resource handlers.
Would you accept a PR with the following changes?

1. All ResourceHandler methods will return an error as the second return value.
2. Current errors will implement the error interface.
3. *Nil errors like GetErrorNil will be nil.
4. ScimError and its fields will be public
5. ScimType and the ScimType constants will be public.
6. The server will accept *ScimErrors, and the current errors by default.
7. The server will also accept errors implementing the interface interface{ StatusCoder() int } and will generate the proper ScimError with the detail message being the result of err.Error().

With a change like this I'll think I can make current code work as it's currently working and clean and make more flexible the resource handlers.

We currently use CircleCI to lint and run the tests. A third party is not needed anymore now that we have GitHub Actions. I recommend changing the current CI configuration to a new one using GitHub Actions.

• Remove the old .circleci/config.yml.
• Add a .github/workflows/test.yml
• Add a .github/workflows/lint.yml

Links:

• setup-go
• golangci

Link: [email protected]

RFC: https://tools.ietf.org/html/rfc7644#section-3.11

A service provider that does NOT support this feature SHOULD respond with HTTP status code 501 (Not Implemented).

If there is a Complex attribute that has multiple sub-attributes that are required, it is not possible to modify just one of those fields with a PATCH operation.

For example, let's consider a User schema where givenName and familyName are required. Then the following queries fail:

{
  "op": "replace",
  "path": "name.givenName",
  "value": "John"
}

{
  "op": "replace",
  "path": "name",
  "value": {
    "givenName": "John"
  }
}

This is due to the following piece of code:

The found variable is just used to check duplicates but does not handle when a field is missing. This might be correct for a PUT request, where you would expect both sub-attributes to be present, but for PATCH the specification states that it is ok to just modify one of the fields and the other should be left as is.

If the target location specifies a complex attribute, a set of sub-attributes SHALL be specified in the "value" parameter, which replaces any existing values or adds where an attribute did not previously exist. Sub-attributes that are not specified in the "value" parameter are left unchanged.
https://tools.ietf.org/html/rfc7644#section-3.5.2.3

Changing the code to the following solves the problem and both operations pass the validation:

if found {
    attr, scimErr := sub.validate(hit)
    if scimErr != nil {
        return nil, scimErr
    }
    attributes[sub.name] = attr
}

However this probably breaks the validation for PUT, so likely a better approach is needed.

"For purposes of readability, examples are not URL encoded. Implementers MUST percent-encode URLs as described in Section 2.1 of [RFC3986]"
https://tools.ietf.org/html/rfc7644#section-1.2

• create post endpoint for resources
• validate resources based on schema

• load sp_config at server creation
• use default items per page in list responses (maxResults)

are these required for a minimal scim implementation?

Hi, there.

I am trying to integrate My App which uses this library with AzureAD.

I do not know why this Patch request AzureAD sent is invalid.
It seems valid to me.

{
       "schemas": [
               "urn:ietf:params:scim:api:messages:2.0:PatchOp"
       ],
       "Operations": [
               {
                       "op": "Replace",
                       "path": "emails[type eq \"work\"].value",
                       "value": "[email protected]"
               }
       ]
}

This library treats this request as ScimErrorInvalidSyntax here.

I want to be clear if the request AzureAD sent is wrong or this library has bug.
Do you have any ides? @di-wu

• overlapping w/ '/Schemas' etc.
• starting w/ '/'

Hi there!

I am wondering why defaultStartIndex = 1.

Using defaultStartIndex as an offset to use as-is, I could not get the first record from PostgreSQL.

Is it should be 0??

I want to know your opinions.

RFC: Section 7

returned indicates when an attribute and associated values are returned.

Is this a check we want to do on the returned Resource that the user returns from the callback?
Or is it their responsibility to not pass sensitive information like a password which has a returned-value of never.

• add /v2/ to endpoints, $ref is not correct "/v2/ResourceType/User" fails
• relative paths i.e. https://elimity.com/scim/, the request url would be "/scim/v2/Schemas" which would fail.

It seems that externalId is not available on returned resources even tho it has been added as an attribute when you are using the Core schema (attributes not in the schema are filtered).

We are looking into adding this but seeing that this is a common attribute we would add it in the same locations as ID (and treat it the same way). Like this:

Does this sound like the approach you want to take as well @di-wu ?

RFC: Section 2

Internal functionality should not be visible/accessible to the end-user.

Solution: Internal Packages?

I noticed the effects of this line of code https://github.com/elimity-com/scim/blob/master/resource_handler.go#L44 and was wondering why this is overwriting the id field with the resource id? I was using the id field for my unique per user/group uuid. Can we remove this line of code?

Hi,

Would it be possible to have an option on the Server (for example), that would allow one to disable the validation? In particular, I am most interested in disabling the validation for the Patch endpoint. The main reason for this is that it seams that there isn't really a consensus on how it should work and this mostly boils down to the specification not been thorough enough. Let me try to explain.

Let's take Azure's SCIM implementation as an example. Their previous client implementation would send the following PATCH operation to update a user's givenName:

{
  "op": "replace",
  "path": "name.givenName",
  "value": "John"
}

and now their most recent implementation, uses the following model:

{
  "op": "replace",
  "value": {
    "name.givenName": "John"
  }
}

Now, neither of these passes the validation logic of this project. And this project is probably correct. When you look at the specification, the only mention of a complex attribute is the following:

If the target location specifies a complex attribute, a set of sub-attributes SHALL be specified in the "value" parameter, which replaces any existing values or adds where an attribute did not previously exist.
https://tools.ietf.org/html/rfc7644#section-3.5.2.3

So the correct approach would likely have to be:

{
  "op": "replace",
  "path": "name",
  "value": {
    "givenName": "John"
  }
}

And this one, I believe, passess the validation check.

However, the specification is just too vague on this. None of the examples they provide shows how singular complex attributes are modified and this leads to implementations having to guess or assume.

However, even this library probably makes a mistake. When it comes to boolean attributes (like active), it expects the value field to be of type boolean. However, if you read the specification it says that the value field will either be a quoted string (meaning "true", not true) or a JSON object:

The value MAY be a quoted value, or it may be a JSON object containing the sub-attributes of the complex attribute specified in the operation's "path".
https://tools.ietf.org/html/rfc7644#section-3.5.2.1

However, this understanding, that active should instead be a boolean, seems to be common in the community, as seen in this thread:
https://stackoverflow.com/questions/59264809/azure-user-group-provisioning-with-scim-problem-with-boolean-values
In fact, after a recent change, Azure changed their implementation so that active is sent as a boolean and not as a string.

Furthermore, there may be situations where it would be ok to ignore a PATCH operation on a field that is not part of the schema, in the name of flexibility / interoperability.

I guess, end of the day, I would really like to be able to support Identity Providers, even if their implementation is slightly out of spec.

• readme
• contributors
• test/example/library