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

Fix "/Schemas" endpoint; configurable default resources #133

Merged
merged 6 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,4 @@ source "https://rubygems.org"
#
gem 'sdoc', :git => 'https://github.com/pond/sdoc.git', :branch => 'master'

group :test do
gem 'pry'
gem 'pry-nav'
end

gemspec
35 changes: 19 additions & 16 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,23 @@ And write to it like this:
}
```

### Helping with auto-discovery

If you have an API consumer entity querying your Scimitar-based SCIM API provider endpoint and want to enable a degree of auto-discovery for that entity, then depending on your implementation, there may be customisations you wish to make.

#### Default resources

By default, Scimitar advertises (via things like [the `/Schemas` endpoint](https://tools.ietf.org/html/rfc7644#section-4)) support for both a `User` and `Group` resource, but if you (say) only support a `User` concept, you override the default using code such as this in your `config/initializers/scimitar.rb` file:

```ruby
Rails.application.config.to_prepare do
Scimitar::Engine::set_default_resources([Scimitar::Resources::User])
Copy link
Contributor

@gsar gsar Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pond although this appears to work, there are linter complaints about writing it the more idiomatic way:

  Scimitar::Engine.set_default_resources([Scimitar::Resources::User])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair. Thanks. I'll do a PR in the near-ish future that's just a style change for the above onto main/v1 but won't release a new gem version given there's no functional change.

# ...other Scimitar configuration / initialisation code...
end
```



## Security

One vital feature of SCIM is its authorisation and security model. The best resource I've found to describe this in any detail is [section 2 of the protocol RFC, 7644](https://tools.ietf.org/html/rfc7644#section-2).
Expand All @@ -600,8 +617,6 @@ Often, you'll find that bearer tokens are in use by SCIM API consumers, but the

### Specification versus implementation

* The `name` complex type of a User has `givenName` and `familyName` fields which [the RFC 7643 core schema](https://tools.ietf.org/html/rfc7643#section-8.7.1) describes as optional. Scimitar marks these as required, in the belief that most user synchronisation scenarios between clients and a Scimitar-based provider would require at least those names for basic user management on the provider side, in conjunction with the in-spec-required `userName` field. That's only if the whole `name` type is given at all - at the top level, this itself remains optional per spec, but if you're going to bother specifying names at all, Scimitar wants at least those two pieces of data.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#113 disagreed and so this was altered, since it was a fairly arbitrary decision and did indeed not match the spec. I've kind of given up trying to figure out ways to have the SCIM spec make any kind of coherent sense now.


* Several complex types for User contain the same set of `value`, `display`, `type` and `primary` fields, all used in synonymous ways.

- The `value` field - which is e.g. an e-mail address or phone number - is described as optional by [the RFC 7643 core schema](https://tools.ietf.org/html/rfc7643#section-8.7.1), also using "SHOULD" rather than "MUST" in field descriptions elsewhere. Scimitar marks this as required by default, since there's not much point being sent (say) an e-mail section which has entries that don't provide the e-mail address. Some services might send `null` values here regardless so, if you need to be able to accept such data, you can set [engine configuration option `optional_value_fields_required`](https://github.com/RIPAGlobal/scimitar/blob/main/config/initializers/scimitar.rb) to `false`.
Expand All @@ -620,6 +635,8 @@ Often, you'll find that bearer tokens are in use by SCIM API consumers, but the

* [RFC 7644 indicates](https://tools.ietf.org/html/rfc7644#page-35) that a resource might only return its core schema in the `schemas` attribute if it was created without any extension fields used. Only if e.g. a subsequent `PATCH` operation added data provided by extension schema, would that extension also appear in `schemas`. This behaviour is extremely difficult to implement and Scimitar does not try - it will always return a resource's core schema and any/all defined extension schemas in the `schemas` array at all times.

* As noted earlier, extension schema attribute names must be unique across your entire combined schema, regardless of schema IDs (URNs) used.

If you believe choices made in this section may be incorrect, please [create a GitHub issue](https://github.com/RIPAGlobal/scimitar/issues/new) describing the problem.

### Omissions
Expand All @@ -632,20 +649,6 @@ If you believe choices made in this section may be incorrect, please [create a G

It's very strange just specifying `emails co...`, since this is an Array which contains complex types. Is the filter there meant to try and match every attribute of the nested types in all array entries? I.e. if `type` happened to contain `example.com`, is that meant to match? It's strongly implied, because the next part of the filter specifically says `emails.value`. Again, we have to reach a little and assume that `emails.value` means "in _any_ of the objects in the `emails` Array, match all things where `value` contains `example.org`. It seems likely that this is a specification error and both of the specifiers should be `emails.value`.

Adding even more complexity - the specification shows filters _which include filters within them_. In the same way that PATCH operations use paths to identify attributes not just by name, but by filter matches within collections - e.g. `emails[type eq "work"]`, for all e-mail objects inside the `emails` array with a `type` attribute that has a value of `work`) - so also can a filter _contain a filter_, which isn't supported. So, this [example from the RFC](https://tools.ietf.org/html/rfc7644#page-23) is not supported by Scimitar:

- `filter=userType eq "Employee" and emails[type eq "work" and value co "@example.com"]`

Another filter shows a potential workaround:

- `filter=userType eq "Employee" and (emails.type eq "work")`

...which is just a match on `emails.type`, so if you have a queryable attribute mapping defined for `emails.type`, that would become queryable. Likewise, you could rewrite the more complex prior example thus:

- `filter=userType eq "Employee" and emails.type eq "work" and emails.value co "@example.com"`

...so adding a mapping for `emails.value` would then allow a database query to be constructed.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The topmost example does now work (and I've added test coverage to prove it). Unsure exactly when this started working. It looks a bit like #115, but isn't.


* Currently filtering for lists is always matched case-insensitive regardless of schema declarations that might indicate otherwise, for `eq`, `ne`, `co`, `sw` and `ew` operators; for greater/less-thank style filters, case is maintained with simple `>`, `<` etc. database operations in use. The standard Group and User schema have `caseExact` set to `false` for just about anything readily queryable, so this hopefully would only ever potentially be an issue for custom schema.

* As an exception to the above, attributes `id`, `externalId` and `meta.*` are matched case-sensitive. Filters that use `eq` on such attributes will end up a comparison using `=` rather than e.g. `ILIKE` (arising from https://github.com/RIPAGlobal/scimitar/issues/36).
Expand Down
16 changes: 15 additions & 1 deletion app/controllers/scimitar/schemas_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,21 @@ def index
hash
end

render json: schemas_by_id[params[:name]] || schemas
list = if params.key?(:name)
[ schemas_by_id[params[:name]] ]
else
schemas
end

render(json: {
schemas: [
'urn:ietf:params:scim:api:messages:2.0:ListResponse'
],
totalResults: list.size,
startIndex: 1,
itemsPerPage: list.size,
Resources: list
})
end

end
Expand Down
62 changes: 50 additions & 12 deletions lib/scimitar/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,24 @@ class Engine < ::Rails::Engine
JSON.parse(body)
end

# Return an Array of all supported default and custom resource classes.
# See also :add_custom_resource and :set_default_resources.
#
def self.resources
default_resources + custom_resources
self.default_resources() + self.custom_resources()
end

# Returns a flat array of instances of all resource schema included in the
# resource classes returned by ::resources.
#
def self.schemas
self.resources().map(&:schemas).flatten.uniq.map(&:new)
end

# Returns the list of custom resources, if any.
#
def self.custom_resources
@custom_resources ||= []
end

# Can be used to add a new resource type which is not provided by the gem.
Expand All @@ -37,7 +53,7 @@ def self.resources
# Scimitar::Engine.add_custom_resource Scim::Resources::ShinyResource
#
def self.add_custom_resource(resource)
custom_resources << resource
self.custom_resources() << resource
end

# Resets the resource list to default. This is really only intended for use
Expand All @@ -47,23 +63,45 @@ def self.reset_custom_resources
@custom_resources = []
end

# Returns the list of custom resources, if any.
#
def self.custom_resources
@custom_resources ||= []
end

# Returns the default resources added in this gem:
# Returns the default resources added in this gem - by default, these are:
#
# * Scimitar::Resources::User
# * Scimitar::Resources::Group
#
# ...but if an implementation does not e.g. support Group, it can
# be overridden via ::set_default_resources to help with service
# auto-discovery.
#
def self.default_resources
[ Resources::User, Resources::Group ]
@standard_default_resources = [ Resources::User, Resources::Group ]
@default_resources ||= @standard_default_resources.dup()
end

def self.schemas
resources.map(&:schemas).flatten.uniq.map(&:new)
# Override the resources returned by ::default_resources.
#
# +resource_array+:: An Array containing one or both of
# Scimitar::Resources::User and/or
# Scimitar::Resources::Group, and nothing else.
#
def self.set_default_resources(resource_array)
self.default_resources()
unrecognised_resources = resource_array - @standard_default_resources

if unrecognised_resources.any?
raise "Scimitar::Engine::set_default_resources: Only #{@standard_default_resources.map(&:name).join(', ')} are supported"
elsif resource_array.empty?
raise 'Scimitar::Engine::set_default_resources: At least one resource must be given'
end

@default_resources = resource_array
end

# Resets the default resource list. This is really only intended for use
# during testing, to avoid one test polluting another.
#
def self.reset_default_resources
self.default_resources()
@default_resources = @standard_default_resources
end

end
Expand Down
6 changes: 3 additions & 3 deletions scimitar.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ Gem::Specification.new do |s|

s.add_dependency 'rails', '~> 7.0'

s.add_development_dependency 'rake', '~> 13.1'
s.add_development_dependency 'debug', '~> 1.9'
s.add_development_dependency 'rake', '~> 13.2'
s.add_development_dependency 'pg', '~> 1.5'
s.add_development_dependency 'simplecov-rcov', '~> 0.3'
s.add_development_dependency 'rdoc', '~> 6.6'
s.add_development_dependency 'rdoc', '~> 6.7'
s.add_development_dependency 'rspec-rails', '~> 6.1'
s.add_development_dependency 'byebug', '~> 11.1'
s.add_development_dependency 'doggo', '~> 1.3'
end
43 changes: 34 additions & 9 deletions spec/controllers/scimitar/schemas_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,60 @@ def index
super
end
end

context '#index' do
it 'returns a valid ListResponse' do
get :index, params: { format: :scim }
expect(response).to be_ok

parsed_body = JSON.parse(response.body)
schema_count = parsed_body['Resources']&.size

expect(parsed_body['schemas' ]).to match_array(['urn:ietf:params:scim:api:messages:2.0:ListResponse'])
expect(parsed_body['totalResults']).to eql(schema_count)
expect(parsed_body['itemsPerPage']).to eql(schema_count)
expect(parsed_body['startIndex' ]).to eql(1)
end

it 'returns a collection of supported schemas' do
get :index, params: { format: :scim }
expect(response).to be_ok

parsed_body = JSON.parse(response.body)
expect(parsed_body.length).to eql(3)
schema_names = parsed_body.map {|schema| schema['name']}
expect(parsed_body['Resources']&.size).to eql(3)

schema_names = parsed_body['Resources'].map {|schema| schema['name']}
expect(schema_names).to match_array(['User', 'ExtendedUser', 'Group'])
end

it 'returns only the User schema when its id is provided' do
get :index, params: { name: Scimitar::Schema::User.id, format: :scim }
expect(response).to be_ok

parsed_body = JSON.parse(response.body)
expect(parsed_body['name']).to eql('User')
expect(parsed_body.dig('Resources', 0, 'name')).to eql('User')
end

it 'includes the controller customised schema location' do
get :index, params: { name: Scimitar::Schema::User.id, format: :scim }
expect(response).to be_ok

parsed_body = JSON.parse(response.body)
expect(parsed_body.dig('meta', 'location')).to eq scim_schemas_url(name: Scimitar::Schema::User.id, test: 1)
expect(parsed_body.dig('Resources', 0, 'meta', 'location')).to eq scim_schemas_url(name: Scimitar::Schema::User.id, test: 1)
end

it 'returns only the Group schema when its id is provided' do
get :index, params: { name: Scimitar::Schema::Group.id, format: :scim }
expect(response).to be_ok

parsed_body = JSON.parse(response.body)
expect(parsed_body['name']).to eql('Group')

expect(parsed_body['Resources' ]&.size).to eql(1)
expect(parsed_body['totalResults'] ).to eql(1)
expect(parsed_body['itemsPerPage'] ).to eql(1)
expect(parsed_body['startIndex' ] ).to eql(1)

expect(parsed_body.dig('Resources', 0, 'name')).to eql('Group')
end

context 'with custom resource types' do
Expand All @@ -65,7 +90,7 @@ def self.scim_attributes

license_resource = Class.new(Scimitar::Resources::Base) do
set_schema license_schema
def self.endopint
def self.endpoint
'/Gaga'
end
end
Expand All @@ -75,9 +100,9 @@ def self.endopint
get :index, params: { name: license_schema.id, format: :scim }
expect(response).to be_ok
parsed_body = JSON.parse(response.body)
expect(parsed_body['name']).to eql('License')
expect(parsed_body.dig('Resources', 0, 'name')).to eql('License')
end
end
end
end # "context 'with custom resource types' do"
end # "context '#index' do
end

5 changes: 5 additions & 0 deletions spec/models/scimitar/lists/query_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,11 @@
expect(result).to eql('emails.type eq "work" and emails.value co "@example.com" or userType eq "Admin" or ims.type eq "xmpp" and ims.value co "@foo.com"')
end

it 'handles an example previously described as unsupported in README.md' do
result = @instance.send(:flatten_filter, 'filter=userType eq "Employee" and emails[type eq "work" and value co "@example.com"]')
expect(result).to eql('filter=userType eq "Employee" and emails.type eq "work" and emails.value co "@example.com"')
end

# https://github.com/RIPAGlobal/scimitar/issues/116
#
context 'with schema IDs (GitHub issue #116)' do
Expand Down
74 changes: 74 additions & 0 deletions spec/requests/engine_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,78 @@
expect(JSON.parse(response.body)['name']['familyName']).to eql('baz')
end
end # "context 'parameter parser' do"

# These are unit tests rather than request tests; seems like a reasonable
# place to put them in the absence of a standardised RSpec "engine" location.
#
context 'engine unit tests' do
around :each do | example |
license_schema = Class.new(Scimitar::Schema::Base) do
def initialize(options = {})
super(name: 'License', id: self.class.id(), description: 'Represents a License')
end
def self.id; 'License'; end
def self.scim_attributes; []; end
end

@license_resource = Class.new(Scimitar::Resources::Base) do
self.set_schema(license_schema)
def self.endpoint; '/License'; end
end

example.run()
ensure
Scimitar::Engine.reset_default_resources()
Scimitar::Engine.reset_custom_resources()
end

context '::resources, :add_custom_resource, ::set_default_resources' do
it 'returns default resources' do
expect(Scimitar::Engine.resources()).to match_array([Scimitar::Resources::User, Scimitar::Resources::Group])
end

it 'includes custom resources' do
Scimitar::Engine.add_custom_resource(@license_resource)
expect(Scimitar::Engine.resources()).to match_array([Scimitar::Resources::User, Scimitar::Resources::Group, @license_resource])
end

it 'notes changes to defaults' do
Scimitar::Engine::set_default_resources([Scimitar::Resources::User])
expect(Scimitar::Engine.resources()).to match_array([Scimitar::Resources::User])
end

it 'notes changes to defaults with custom resources added' do
Scimitar::Engine::set_default_resources([Scimitar::Resources::User])
Scimitar::Engine.add_custom_resource(@license_resource)
expect(Scimitar::Engine.resources()).to match_array([Scimitar::Resources::User, @license_resource])
end

it 'rejects bad defaults' do
expect {
Scimitar::Engine::set_default_resources([@license_resource])
}.to raise_error('Scimitar::Engine::set_default_resources: Only Scimitar::Resources::User, Scimitar::Resources::Group are supported')
end

it 'rejects empty defaults' do
expect {
Scimitar::Engine::set_default_resources([])
}.to raise_error('Scimitar::Engine::set_default_resources: At least one resource must be given')
end
end # "context '::resources, :add_custom_resource, ::set_default_resources' do"

context '#schemas' do
it 'returns schema instances from ::resources' do
expect(Scimitar::Engine).to receive(:resources).and_return([Scimitar::Resources::User, @license_resource])

schema_instances = Scimitar::Engine.schemas()
schema_classes = schema_instances.map(&:class)

expect(schema_classes).to match_array([
Scimitar::Schema::User,
ScimSchemaExtensions::User::Enterprise,
@license_resource.schemas.first
])
end
end # "context '#schemas' do"
end # "context 'engine unit tests' do"
end
2 changes: 1 addition & 1 deletion spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
abort("The Rails environment is running in production mode!") if Rails.env.production?

require 'rspec/rails'
require 'byebug'
require 'debug'
require 'scimitar'

# ============================================================================
Expand Down
Loading