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

Prevent returning password field from SCIM #56

Closed
kuldeepaggarwal opened this issue Jun 2, 2023 · 5 comments
Closed

Prevent returning password field from SCIM #56

kuldeepaggarwal opened this issue Jun 2, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@kuldeepaggarwal
Copy link

kuldeepaggarwal commented Jun 2, 2023

Scimitar::Schema::User has a password attribute:

Attribute.new(name: 'password', type: 'string', mutability: 'writeOnly', returned: 'never'),

and as per the definition, it should never be returned in the response. However, if client sends a password and Service provider stores the password using the scim_attributes_map then SCIMitar is responding back with the password value.

I believe, the issue is in Scimitar::Resources::Base#as_json, when we are calling

original_hash = super(options).except('errors')

without excluding those files which has a property returned='never'.

Proposed Solution

def non_returnable_attributes
  self.class.schemas.flat_map(&:scim_attributes).select { |attribute| attribute.returned == 'never' }.map(&:name)
end

def as_json(options = {})
  ...
  original_hash = super(options).except('errors', *non_returnable_attributes)
  ...
end
@pond
Copy link
Member

pond commented Jun 5, 2023

I'll get to this as soon as I can. Proposed idea in general looks OK though might need a bit of work to avoid lots of on-the-fly calculated hashes and intermediate objects. Sorry for the delay in responding - I was in the UK for 2 weeks (where my family lives - first trip back there for 5 years!) but thanks to lots of people coughing without masks on both of the long haul flights home, I came down with flu the day after I got back. Illness combined with 11 hours of jet lag (to NZ) hit rather hard and I'm only just starting to catch back up on everything now.

@pond pond self-assigned this Jun 5, 2023
@pond pond added the bug Something isn't working label Jun 5, 2023
@kuldeepaggarwal
Copy link
Author

Sorry to hear that you are affected with flu, @pond! Thanks for acknowledging this as a bug. I just realized this hack won't work unless one more change. Here is the monkey patch that I added in my application when I completed E2E testing.

record.from_scim!(scim_hash: scim_resource.as_json())

module ScimitarResourceJSONOverride
  def as_json(options = {})
    return super unless options.key?(:layout) # this change is needed to parse password SCIM params from request when passing from Controller to UserResource else password will not be passed from Controller to model.

    non_returnable_attributes = self.class.schemas.flat_map(&:scim_attributes).filter_map { |attribute| attribute.name if attribute.returned == 'never' }
    super.except(*non_returnable_attributes)
  end
end


Scimitar::Resources::Base.include(ScimitarResourceJSONOverride)

@pond
Copy link
Member

pond commented Oct 31, 2023

@kuldeepaggarwal This took far too long, sorry. Once I got back there was a backlog of work and things snowballed from there.

#80 implements (I think!) your initial suggestion, but without the amendment you have patched in with #56 (comment) - I don't think it's necessary (but don't entirely understand it so could be very wrong). Please can you check the PR and see if that looks OK, and/or otherwise elaborate on the :layout key check you made in the monkey patch?

pond added a commit that referenced this issue Nov 14, 2023
…e-fields

Implement solution to issue #56, as suggested therein
@pond
Copy link
Member

pond commented Nov 15, 2023

@kuldeepaggarwal This should now be fixed via 2.6.0/2.6.1, or 1.7.0/1.7.1 all now on RubyGems. Please let me know if you agree and, if so, we can close this issue :-)

@pond
Copy link
Member

pond commented Jan 11, 2024

I think it's been long enough on this to just go ahead and close it. If you see problems, please open a new issue. Thanks!

@pond pond closed this as completed Jan 11, 2024
xjunior added a commit to xjunior/scimitar that referenced this issue Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants