Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PATCH implementation's "to SCIM, from SCIM" behaviour is problematic #6

Closed
pond opened this issue Apr 7, 2021 · 5 comments
Closed
Labels
enhancement New feature or request

Comments

@pond
Copy link
Member

pond commented Apr 7, 2021

Scimitar has a mostly-working PATCH implementation which does a full to-SCIM, patch-that-Hash, from-SCIM cycle to read-modify-write data into a mapped resource. This was the best I could get my brain to figure out at the time.

Problems arise if a to-SCIM cycle on a resource does not contain all data, since the from-SCIM operation uses PUT / replace-like semantics intentionally, and if there's anything missing, it looks like a set-to-nil operation. This was recently revealed in a User mapping in an application using Scimitar, which supports Roles. Users can set up mappings between role names in the client system and role names in the Scimitar-based service providing application, and only mapped role data was being shown in the SCIM representation (since unmapped service provider roles would mean nothing to the client). When PATCHing e.g. just the 'is_active' field of a User, the result would be that the Roles collection got modified too, because any unmapped roles were omitted in the to-SCIM operation and the subsequent PUT-like from-SCIM operation treated this as a rewrite-roles-array-contents action.

I can't quite figure out how to make it work still and it'll take some time, but the right thing to do would be to traverse the to-SCIM data as we do today (required in order for filters in the path to work) at the same time as traversing the corresponding underlying class's attribute map. This is difficult for filters in particular - completely different filter application schemes will be needed for matching data, versus matching an attribute array static map, or an attribute array dynamic map. If that can be figured out, though, by the time a given PATCH path has finished traversal, we'll know both the SCIM attribute to be added/removed/altered and have attribute map data to allow that operation to be applied immediately via write accessors - no from-SCIM step would be required.

As a bonus, it'd be good if we could track the no-filter attribute map path arising so that the resource schema can be consulted and mutability flags read for a given attribute, helping to trap invalid PATCH attempts to in-context immutable attribute data that might otherwise be permitted.

@pond pond added the enhancement New feature or request label Apr 7, 2021
@pond pond changed the title PATCH implementation is problematic PATCH implementation's "to SCIM, from SCIM" behaviour is problematic Apr 7, 2021
@kuldeepaggarwal
Copy link

@pond Would you mind adding an example, maybe I can contribute. I am exploring this engine to use in our application, so would like to help to fix all existing issues.

@pelted
Copy link
Contributor

pelted commented Jul 21, 2023

I believe I'm running into these issues from Azure AD SCIM Validator. Looking at some ways to resolve this when I came across this issue.

@gsar
Copy link

gsar commented Jan 12, 2024

@pond @kuldeepaggarwal @pelted FWIW, i'm working around the problem with this rather ugly workaround. i don't think we should expect userName to be sent in anything except create.

module Scim
  class UsersController < Scimitar::ActiveRecordBackedResourcesController
    def update
      id = params[:id]
      # work around failure of PATCH endpoints in Microsoft Entra SCIM validator
      # see https://github.com/RIPAGlobal/scimitar/issues/6#issuecomment-1645714989
      unless params[:userName]
        resource = storage_scope.find_by(id: id)
        if resource
          params[:userName] = resource.scim_user_name
        end
      end
      super
    end
    ...
  end
end

@pond
Copy link
Member Author

pond commented Mar 25, 2024

I think #109 does enough work to essentially close this issue. It's not the prettiest approach but it ended up being less code than a full-on 100% attribute-map-based solution.

@pond
Copy link
Member Author

pond commented Mar 26, 2024

Fixed in v2.7.2 / v1.8.2.

@pond pond closed this as completed Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants