How to remove a group member?

[bettysteger]

I have the following methods in my Group model:

  def members
    users.map do |user|
      {
        value: user.id,
        display: user.name,
        # '$ref': Rails.application.routes.url_for(action: :show, id: user.becomes(Scim::User).id)
      }
    end
  end

  def members=(arr)
    return if arr.nil?
    self.user_ids = arr.map{|x| x['value']}
  end

Adding member works:

it 'adds member to group' do
 user = create(:user)
 patch "/scim/Groups/#{group.id}", headers: headers, params: {
   Operations: [{
     op: 'Add',
     path: 'members',
     value: [{
       '$ref': nil,
       value: user.id
     }]
   }]
 }.to_json

 expect(response).to have_http_status(:ok)
 expect(group.users).to include(user)
end

But removing a member from a group fails, because the array (in members=(arr)) is nil:

it 'removes member to group' do
 user = create(:user)
 group.users << user
 patch "/scim/Groups/#{group.id}", headers: headers, params: {
   Operations: [{
     op: 'Remove',
     path: 'members',
     value: [{
       '$ref': nil,
       value: user.id
     }]
   }]
 }.to_json

 expect(response).to have_http_status(:ok)
 expect(group.users).not_to include(user)
end

I am new to SCIM, how can I remove a member from a group? is this even possible?

[pond]

I'm not quite sure what is going on with the "members is nil" thing to which you refer; that sounds more like perhaps a bug in your model? Anyway, maybe the following will help you out.

NOTE - I made quite a few late edits to this a few hours later. I'd been led astray by some false-passes in our tests. Long story short, the form of SCIM removal you're attempting doesn't work - online resources saying that it should are wrong, or at least have special non-standard capabilities implemented in their back-end SCIM processing. I'll explain below.

The SCIM API side of things

I found a few different ways of removing a single user from a group according to some incorrect online resources. First up we have Salesforce, who get it very wrong - https://help.salesforce.com/s/articleView?id=sf.identity_scim_manage_groups.htm&type=5 - recommending this payload, which is similar to yours except you'll note members is repeated inside the value section, which doesn't make sense since path already contains it.

// This is wrong!

{
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
    {
      "op": "remove",
      "path": "members",
      "value": {
        "members": [
          {
            "value": "<user_id>"
          }
        ]
      }
    }
  ]
}

Other people have tried the same as you (e.g. see the original poster who filed the bug report at simpleidserver/SimpleIdServer#164), along these lines:

// This is also wrong!

{
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
    {
      "op": "remove",
      "path": "members",
      "value": [
        {
          "value": "<user_id>"
        }
      ]
    }
  ]
}

In the bug report I reference above, an explanation of why the above approach doesn't work is found in the answer at simpleidserver/SimpleIdServer#164 (comment). The SCIM specification itself at https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2.2 shows the correct form. Various other online resources do get this right - a somewhat random Google result by way of example: https://docs.snowflake.com/en/user-guide/scim-intro.html#patch-scim-v2-groups-id.

TL;DR You need to change to using a path with a filter.

parameters = {
  'schemas' => ['urn:ietf:params:scim:api:messages:2.0:PatchOp'],
  'Operations' => [
    {
      'op' => 'remove',
      'path' => "members[value eq \"#{user.id}\"]",
    }
  ]
}

We have test coverage using this approach to remove a single user from a group in our own application based on Scimitar; it performs as expected, removing a user from a group; it passes where either that was the last user leaving none, or when there are other users, leaving those other users present.

The Ruby side of things

There is additional complexity with SCIM - which is, frankly, kind of a horrible spec! - alluded to in comments in our implementation (which is commercial / proprietary, so I'm sharing only limited amounts and have changed some of our model and concept names to match your code):

# Groups in SCIM can contain users or other groups. That's why the
# :find_with key in the Hash returned by ::scim_attributes_map has to check
# the type of thing it needs to find. Since the mappings only support a
# single read/write accessor, we need custom accessors to do what SCIM is
# expecting by turning the Rails associations to/from mixed, flat arrays of
# Users and Groups.

...this comment referring to the following code - bear in mind that our internal model attribute names and your internal model attribute names will differ:

def self.scim_attributes_map
  return {
    id:          :id,
    externalId:  :scim_uid,
    displayName: :display_name,
    members:     [
      list:  :scim_users_and_groups, # <-- i.e. Group#scim_users_and_groups - see explanation below
      using: {
        value:   :id,                # <-- i.e. Group#scim_users_and_groups[n].id
        display: :scim_display_name, # <-- i.e. Group#scim_users_and_groups[n].scim_display_name
        type:    :scim_record_type   # <-- i.e. Group#scim_users_and_groups[n].scim_record_type
      },
      find_with: -> (scim_list_entry) {
        id   = scim_list_entry['value']
        type = scim_list_entry['type' ] || 'User' # Some online examples omit 'type' and believe 'User' will be assumed

        case type.downcase
          when 'user'
            User.find_by_id(id)
          when 'group'
            Group.find_by_id(id)
          else
            raise Scimitar::InvalidSyntaxError.new("Unrecognised type #{type.inspect}")
        end
      }
    ]
  }
end

...as a result, we have special read-write accessors for what I think is our equivalent of your Group#members / Group#members= instance methods:

# Return a concatenated Array of members and any child groups.
#
# LIMITATION: Only immediate children are returned. Anything deeper in the
#             tree is not currently included.
#
def scim_users_and_groups
  self.members.to_a + self.children.to_a
end

# Given a mixture of User and Group objects, set as group members and child
# group entities. This overwrites any prior associations entirely.
#
def scim_users_and_groups=(mixed_array)
  self.members  = mixed_array.select { |item| item.is_a?(User ) }
  self.children = mixed_array.select { |item| item.is_a?(Group) }
end

Note that this code is believed correct according to our tests, but not yet exercised by a true third party SCIM connection. It should work but in practice we might need changes to deal with quirks of inbound SCIM operations.

[pond]

As a followup to this I would very, very strongly recommend you familiarise yourself with the SCIM RFCs before attempting to write a SCIM implementation. It's not something you can do via guesswork. The RFC gives an example of removing a single user which follows the second approach above (the one with a path that contains the filter selecting a member by ID directly) and this is likely to be the more common and widely used approach as a result.

• https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2.2

The full set of RFCs are:

• https://www.rfc-editor.org/rfc/rfc7642
• https://www.rfc-editor.org/rfc/rfc7643
• https://www.rfc-editor.org/rfc/rfc7644

It is probably not necessary to read top-to-bottom all of those but certainly doing a degree of skip-reading, so that you have an idea of what the RFCs include and where to find information within them as a reference work in future, should save you time if you're struggling with aspects of the SCIM protocol.

[bettysteger]

@pond thank you very much for your detailed response, the problem is - we need a SCIM implementation just for Microsoft Azure AD, and when you look at their documentation about removing users: https://learn.microsoft.com/en-us/azure/active-directory/app-provisioning/use-scim-to-provision-users-and-groups#update-group-remove-members you see that is the exact payload in my test...

So you are saying Microsoft is doing the removal request wrong? But i am not able to change that, so i need to find another solution :/

[pond]

Yes, technically I am not saying 😄 but the standard RFC is saying, as far as I understand it, that this is a non-compliant payload. The inclusion of $ref: null is particularly odd. This is Microsoft, though, so I suppose we should be unsurprised that they expect the rest of the world to waste hours working around their quirks. Story of my career!

You might need to submit a PR for Scimitar to support that. If I have time I can look into it, but that's going to require significant engineering work and I don't have any spare time for that right now, so I can't give you any sort of promises or estimation on when it might happen.

(EDIT - skip down a couple of comments since we've re-prioritised to get working on this straight away).

[pond]

...one hacky work-around to consider for the short term - in your subclass of ActiveRecordBackedResourcesController, override #update. Its current implementation is:

super do |record_id, patch_hash|
  self.storage_class().transaction do
    record = self.find_record(record_id)
    record.from_scim_patch!(patch_hash: patch_hash)
    self.save!(record)
    record_to_scim(record)
  end
end

The parameters passed to the block here are part of the documented interface of the superclass ResourcesController, so at least the super do... part of that override should not be considered fragile/risky in the face of potential future internal changes to Scimitar; at last within major version 2, it's considered part of the gem public API so it could only be broken at a major version bump.

Inside super, you could introspect specifically for the Microsoft version of that 'remove' payload, looking at the Hash to check that it's a removal operation for a member with the ID given, then rewrite that Hash into the form that Scimitar currently handles before continuing processing as normal. For example, if we knew that Microsoft only ever sent one user at a time in that form, then this simple code would work I think:

def update
  super do |record_id, patch_hash|

    # Rewrite this form:
    #   https://learn.microsoft.com/en-us/azure/active-directory/app-provisioning/use-scim-to-provision-users-and-groups#update-group-remove-members
    #
    # ...to match SCIM specification requirements per:
    #   https://www.rfc-editor.org/rfc/rfc7644#section-3.5.2.2
    #
    if (
      patch_hash.dig('Operations', 0, 'op'  ) == 'Remove' && 
      patch_hash.dig('Operations', 0, 'path') == 'members'
    )
      member_id = patch_hash.dig('Operations', 0, 'value', 0, 'value')

      patch_hash['Operations'] = [{
        'op'   => 'remove',
        'path' => "members[value eq \"#{member_id}\"]"
      }]
    end

    self.storage_class().transaction do
      record = self.find_record(record_id)
      record.from_scim_patch!(patch_hash: patch_hash)
      self.save!(record)
      record_to_scim(record)
    end
  end
end

...but slightly more complex rewrite logic would be needed if they do send arrays with multiple users or arrays with multiple operations in single payloads, or another layer of complexity would be needed if they aren't consistently using that capitalisation (much of SCIM is case-insensitive).

[pond]

In the grand spirit of repeatedly contradicting myself in a public GitHub issue's commentary 😂 we've decided internally that this is high priority enough to work on straight away. The failure mode is too nasty - the RFC might be read to say "should remove all users from the group", but that's very destructive and not a good thing to do when the payload clearly has the intended outcome of only one user being removed.

I still don't know how long it'll take to sort, hopefully not too long, but happily I can say that we're going to implement support for the Microsoft case (and while I suspect the Salesforce payload is a documentation fault, I might - if not too grubby - implement a special case handler for theirs too, just in case).

This means you might want the work-around just to get going really quickly, but Scimitar itself should be handling that payload as Microsoft intend soon.

[pond]

So, all the bookkeeping is done though the code I've implemented is to an extent hypothetical, as I don't have an active Microsoft endpoint calling into an implementation I can test against. Version 2.4.0 (Rails 7) and 1.5.0 (Rails 6) are now on RubyGems:

https://rubygems.org/gems/scimitar/versions

...and hopefully that will get things working for you without any hackery. Please let me know how you get on - it's possible that one or two patch versions might be needed to get things working with "real" Microsoft calls.

[bettysteger]

@pond wow ok that was quick, i was not expecting that - thank you so so much!

I will be able to test this next week!

Yes, technically I am not saying 😄 but the standard RFC is saying, as far as I understand it, that this is a non-compliant payload. The inclusion of $ref: null is particularly odd. This is Microsoft, though, so I suppose we should be unsurprised that they expect the rest of the world to waste hours working around their quirks. Story of my career!

That's so sad, we should tell them what they are doing wrong. I also had a problem for SCIM users when setting the active and inactive with Microsoft Azure AD... The documentation there was wrong, they were sending a string instead of a boolean:

  def is_active=(value)
    return true if value.nil?

    # microsoft sends {"op"=>"Replace", "path"=>"active", "value"=>"False"}
    value = ActiveModel::Type::Boolean.new.cast(value.downcase) if value.is_a?(String)
    return value if kept? == value

    self.discarded_at = value ? nil : Time.current
  end

[pond]

Hmm, yeah I'm not sure how well we'll deal with string booleans either, but it shouldn't be too hard to check the schema and do an appropriate type translation - the use of ActiveModel::Type::Boolean.new.cast is a good use of the converter there too since it'll handle all manner of strange things someone might put in that are "truthy" or "falsy".

Basically it'll be a case of "test it with the real Azure backend and if things aren't working, submit another issue", I think.

Thanks for the bug report and details either way - all of this helps make the gem better and more interoperable with real-world SCIM API callers.

[bettysteger]

@pond The test works now when removing a group member, and it also works with Azure AD real life data! 🎉
Thank you very much :)

FYI this is what i get from Azure AD:

{
  "schemas"=>["urn:ietf:params:scim:api:messages:2.0:PatchOp"], 
  "Operations"=>[{"op"=>"Remove", "path"=>"members", "value"=>[{"value"=>"66c9d41c-34e7-43a7-9606-87097eaf641a"}]}]
}

I will close the issue now!

[pond]

Great stuff - thanks for the followup. I'm glad it's working as expected now.