Merge pull request #133 from RIPAGlobal/feature/fix-schemas-endpoint

Fix "/Schemas" endpoint; configurable default resources

Gemfile

@@ -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

README.md

@@ -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])
+  # ...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).
@@ -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.
-
 * 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`.
@@ -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
@@ -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.
-
 * 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).

app/controllers/scimitar/schemas_controller.rb

@@ -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

lib/scimitar/engine.rb

@@ -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.
@@ -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
@@ -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

scimitar.gemspec

@@ -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

spec/controllers/scimitar/schemas_controller_spec.rb

@@ -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
@@ -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
@@ -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
 

spec/models/scimitar/lists/query_parser_spec.rb

@@ -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

spec/requests/engine_spec.rb

@@ -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

spec/spec_helper.rb

@@ -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'
 
 # ============================================================================