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
121 stars 38 forks source link

Attribute names are being treated as case sensitive #301

Closed mjohnstonkura closed 2 years ago

mjohnstonkura commented 2 years ago

Hi, I have been using your library and ran into an issue when running the AzureAD test suite. The UserName attribute is being ignored by the library - due to a case sensitive comparison.

Reading the SCIM spec - "Attribute names are case insensitive and are often "camel-cased"(e.g., "camelCase")" https://datatracker.ietf.org/doc/html/rfc7643#section-2.1

Testing suite https://github.com/AzureAD/SCIMReferenceCode/wiki/Test-Your-SCIM-Endpoint

Example Post - the non camel-cased attributes are ignored. { "UserName": "UserName123", "Active": true, "DisplayName": "BobIsAmazing", "schemas": [ "urn:ietf:params:scim:schemas:core:2.0:User" ],

Have I missed a configuration option to enable case insensitive comparison ?

I had a bit of a look at the code and Jackson object mapper supports a case insensitive comparison -https://stackoverflow.com/a/36767648. Would the best approach be to change this in JsonHelper.java?

Captain-P-Goldfish commented 2 years ago

You are right. I located the problem here in class AbstractSchemaValidator:

protected ScimObjectNode validateDocument(ScimObjectNode validatedResource, Schema resourceSchema, JsonNode resource)
  {
    for ( SchemaAttribute schemaAttribute : resourceSchema.getAttributes() )
    {
      log.trace("Validating attribute '{}'", schemaAttribute.getScimNodeName());
      final String attributeName = schemaAttribute.getName();
      JsonNode attribute = resource.get(attributeName);
      Optional<JsonNode> validatedAttributeOptional = validateAttribute(schemaAttribute, attribute);
      validatedAttributeOptional.ifPresent(validatedAttribute -> {
        validatedResource.set(attributeName, validatedAttribute);
      });
    }
    return validatedResource;
  }

A jackson ObjectNode saves its children within a LinkedHashMap where the key of the map matches the name within the received document. So in your case the hash maps key would be UserName instead of the schemas definition userName and therefore no node is returned. Nodes within the json document that are not within the schemas definition will simply be ignored and removed from the validated document. This is done to prevent further processing of potentially malicious data.

I think I can fix this behaviour by writing a new class that will be responsible for extracting the attributes from the received json document but only if the caseExcact-attribute within the SchemaAttribute definition is set to false

EDIT: There would be a problem though: The POJO elements for the user resource for example would still try to extract the data by their SchemaAttribute definition name. So in order to fix this the new class would also replace the attribute-keys with the defined name within the SchemaAttributes definition. In other words, this:

{
  "UserName": "hello",
  "DisplayName": "world"
}

would become:

{
  "userName": "hello",
  "displayName": "world"
}

would this be a problem in your case?

EDIT 2:

The fix would not work for patch requests.

mjohnstonkura commented 2 years ago

Thanks for looking into this.

Reading the specification, I believe that caseExact only applies to to the attribute values - not attribute names (which are always case insensitive). So I think it should be extracting all attribute names.

would this be a problem in your case?

Nope - this is perfect.

This library has been around for a long time (thanks), and I couldn't see this issue reported by anyone else. I wonder if I've stumbled into some testing data that doesn't represent real world IdP's; e.g. all IdP's use the camel case naming convention.

Captain-P-Goldfish commented 2 years ago

Please test if everything works as expected. To enable case-insensitive attribute checks enable it via the service provider configuration:

ServiceProvider.builder().caseInsensitiveValidation(true).build();
mjohnstonkura commented 2 years ago

I've been testing your changes and it's working perfectly for me 🎉. I have mostly been testing users, but I'll take a look at group as well. Thanks.