From 469fcc2eddb92767c10911617c9dc564c23949d9 Mon Sep 17 00:00:00 2001 From: Andrew Hodgkinson Date: Tue, 18 Jun 2024 15:31:58 +1200 Subject: [PATCH] Various fixes for extension schema --- README.md | 4 +- app/models/scimitar/resources/base.rb | 41 +++++++-- lib/scimitar/support/utilities.rb | 11 ++- spec/apps/dummy/app/models/mock_user.rb | 8 +- .../dummy/config/initializers/scimitar.rb | 30 ++++++- .../20210304014602_create_mock_users.rb | 1 + spec/apps/dummy/db/schema.rb | 1 + .../scimitar/schemas_controller_spec.rb | 4 +- spec/models/scimitar/resources/base_spec.rb | 14 +-- spec/models/scimitar/resources/mixin_spec.rb | 41 ++++++--- ...record_backed_resources_controller_spec.rb | 88 ++++++++++++++++++- spec/requests/engine_spec.rb | 1 + 12 files changed, 210 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index f7b25a0..49db2e9 100644 --- a/README.md +++ b/README.md @@ -457,7 +457,9 @@ You can extend schema with custom data by defining an extension class and callin * Must call `super` in `def initialize`, providing data as shown in the example below * Must define class methods for `::id` and `::scim_attributes` -The `::id` class method defines a unique schema ID that is used to namespace payloads or paths in JSON responses describing extended resources, JSON payloads creating them or PATCH paths modifying them. The SCIM RFCs would refer to this as the URN. For example, we might choose to use the [RFC-defined User extension schema](https://tools.ietf.org/html/rfc7643#section-4.3) to define a couple of extra fields our User model happens to support: +The `::id` class method defines a unique schema ID that is used to namespace payloads or paths in JSON responses describing extended resources, JSON payloads creating them or PATCH paths modifying them. The RFCs require this to be a URN ([see RFC 2141](https://tools.ietf.org/html/rfc2141)). Your extension's ID URN must be globally unique. Depending on your expected use case, you should review the [IANA registration considerations that RFC 7643 describes](https://tools.ietf.org//html/rfc7643#section-10) and definitely review the [syntactic structure declaration therein](https://tools.ietf.org/html/rfc7643#section-10.2.1) (`urn:ietf:params:scim:{type}:{name}{:other}`). + +For example, we might choose to use the [RFC-defined User extension schema](https://tools.ietf.org/html/rfc7643#section-4.3) to define a couple of extra fields our User model happens to support: ```ruby class UserEnterpriseExtension < Scimitar::Schema::Base diff --git a/app/models/scimitar/resources/base.rb b/app/models/scimitar/resources/base.rb index 33e99ba..d5ecebc 100644 --- a/app/models/scimitar/resources/base.rb +++ b/app/models/scimitar/resources/base.rb @@ -35,14 +35,45 @@ def initialize(options = {}) @errors = ActiveModel::Errors.new(self) end + # Scimitar has at present a general limitation in handling schema IDs, + # which just involves stripping them and requiring attributes across all + # extension schemas to be overall unique. + # + # This method takes an options payload for the initializer and strips out + # *recognised* schema IDs, so that the resulting attribute data matches + # the resource attribute map. + # + # +attributes+:: Attributes to assign via initializer; typically a POST + # payload of attributes that has been run through Rails + # strong parameters for safety. + # + # Returns a new object of the same class as +options+ with recognised + # schema IDs removed. + # def flatten_extension_attributes(options) - flattened = options.dup - self.class.extended_schemas.each do |extended_schema| - if extension_attrs = flattened.delete(extended_schema.id) - flattened.merge!(extension_attrs) + flattened = options.class.new + lower_case_schema_ids = self.class.extended_schemas.map do | schema | + schema.id.downcase() + end + + options.each do | key, value | + path = Scimitar::Support::Utilities::path_str_to_array( + self.class.extended_schemas, + key + ) + + if path.first.include?(':') && lower_case_schema_ids.include?(path.first.downcase) + path.shift() + end + + if path.empty? + flattened.merge!(value) + else + flattened[path.join('.')] = value end end - flattened + + return flattened end # Can be used to extend an existing resource type's schema. For example: diff --git a/lib/scimitar/support/utilities.rb b/lib/scimitar/support/utilities.rb index 87b8285..af3edeb 100644 --- a/lib/scimitar/support/utilities.rb +++ b/lib/scimitar/support/utilities.rb @@ -57,9 +57,10 @@ def self.dot_path(array, value) # scim_resource_type.extended_schemas value. The # Array should be empty if there are no extensions. # - # +path_str+:: Path string, e.g. "password", "name.givenName", + # +path_str+:: Path String, e.g. "password", "name.givenName", # "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User" (special case), # "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:organization" + # (if given a Symbol, it'll be converted to a String). # # Returns an array of components, e.g. ["password"], ["name", # "givenName"], @@ -74,6 +75,7 @@ def self.dot_path(array, value) # path-free payload. # def self.path_str_to_array(schemas, path_str) + path_str = path_str.to_s components = [] # Note the ":" separating the schema ID (URN) from the attribute. @@ -84,11 +86,14 @@ def self.path_str_to_array(schemas, path_str) # particular, https://tools.ietf.org/html/rfc7644#page-35. # if path_str.include?(':') + lower_case_path_str = path_str.downcase() + schemas.each do |schema| - attributes_after_schema_id = path_str.downcase.split(schema.id.downcase + ':').drop(1) + lower_case_schema_id = schema.id.downcase() + attributes_after_schema_id = lower_case_path_str.split(lower_case_schema_id + ':').drop(1) if attributes_after_schema_id.empty? - components += [schema.id] + components += [schema.id] if lower_case_path_str == lower_case_schema_id else attributes_after_schema_id.each do |component| components += [schema.id] + component.split('.') diff --git a/spec/apps/dummy/app/models/mock_user.rb b/spec/apps/dummy/app/models/mock_user.rb index 1464a82..bd13fc8 100644 --- a/spec/apps/dummy/app/models/mock_user.rb +++ b/spec/apps/dummy/app/models/mock_user.rb @@ -18,6 +18,7 @@ class MockUser < ActiveRecord::Base work_phone_number organization department + manager mock_groups } @@ -90,13 +91,15 @@ def self.scim_attributes_map } ], active: :is_active, + primaryEmail: :scim_primary_email, # Custom extension schema - see configuration in # "spec/apps/dummy/config/initializers/scimitar.rb". # organization: :organization, department: :department, - primaryEmail: :scim_primary_email, + manager: :manager, + userGroups: [ { list: :mock_groups, @@ -130,7 +133,8 @@ def self.scim_queryable_attributes } end - # reader + # Custom attribute reader + # def scim_primary_email work_email_address end diff --git a/spec/apps/dummy/config/initializers/scimitar.rb b/spec/apps/dummy/config/initializers/scimitar.rb index a802f94..c15ae93 100644 --- a/spec/apps/dummy/config/initializers/scimitar.rb +++ b/spec/apps/dummy/config/initializers/scimitar.rb @@ -33,10 +33,13 @@ def scim_resource_type_url(options) module ScimSchemaExtensions module User + + # This "looks like" part of the standard Enterprise extension. + # class Enterprise < Scimitar::Schema::Base def initialize(options = {}) super( - name: 'ExtendedUser', + name: 'EnterpriseExtendedUser', description: 'Enterprise extension for a User', id: self.class.id, scim_attributes: self.class.scim_attributes @@ -55,8 +58,33 @@ def self.scim_attributes ] end end + + # In https://github.com/RIPAGlobal/scimitar/issues/122 we learn that with + # more than one extension, things can go wrong - so now we test with two. + # + class Manager < Scimitar::Schema::Base + def initialize(options = {}) + super( + name: 'ManagementExtendedUser', + description: 'Management extension for a User', + id: self.class.id, + scim_attributes: self.class.scim_attributes + ) + end + + def self.id + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User' + end + + def self.scim_attributes + [ + Scimitar::Schema::Attribute.new(name: 'manager', type: 'string') + ] + end + end end end Scimitar::Resources::User.extend_schema ScimSchemaExtensions::User::Enterprise + Scimitar::Resources::User.extend_schema ScimSchemaExtensions::User::Manager end diff --git a/spec/apps/dummy/db/migrate/20210304014602_create_mock_users.rb b/spec/apps/dummy/db/migrate/20210304014602_create_mock_users.rb index 80c129b..77895c3 100644 --- a/spec/apps/dummy/db/migrate/20210304014602_create_mock_users.rb +++ b/spec/apps/dummy/db/migrate/20210304014602_create_mock_users.rb @@ -19,6 +19,7 @@ def change # t.text :organization t.text :department + t.text :manager end end end diff --git a/spec/apps/dummy/db/schema.rb b/spec/apps/dummy/db/schema.rb index b20a616..cb9a79c 100644 --- a/spec/apps/dummy/db/schema.rb +++ b/spec/apps/dummy/db/schema.rb @@ -41,6 +41,7 @@ t.text "work_phone_number" t.text "organization" t.text "department" + t.text "manager" end add_foreign_key "mock_groups_users", "mock_groups" diff --git a/spec/controllers/scimitar/schemas_controller_spec.rb b/spec/controllers/scimitar/schemas_controller_spec.rb index f5b9baf..0c49de4 100644 --- a/spec/controllers/scimitar/schemas_controller_spec.rb +++ b/spec/controllers/scimitar/schemas_controller_spec.rb @@ -30,10 +30,10 @@ def index expect(response).to be_ok parsed_body = JSON.parse(response.body) - expect(parsed_body['Resources']&.size).to eql(3) + expect(parsed_body['Resources']&.size).to eql(4) schema_names = parsed_body['Resources'].map {|schema| schema['name']} - expect(schema_names).to match_array(['User', 'ExtendedUser', 'Group']) + expect(schema_names).to match_array(['User', 'EnterpriseExtendedUser', 'ManagementExtendedUser', 'Group']) end it 'returns only the User schema when its id is provided' do diff --git a/spec/models/scimitar/resources/base_spec.rb b/spec/models/scimitar/resources/base_spec.rb index eac5aee..a4cdf59 100644 --- a/spec/models/scimitar/resources/base_spec.rb +++ b/spec/models/scimitar/resources/base_spec.rb @@ -305,7 +305,7 @@ def self.scim_attributes ExtensionSchema = Class.new(Scimitar::Schema::Base) do def self.id - 'extension-id' + 'urn:extension' end def self.scim_attributes @@ -333,13 +333,13 @@ def self.resource_type_id context '#initialize' do it 'allows setting extension attributes' do - resource = resource_class.new('extension-id' => {relationship: 'GAGA'}) + resource = resource_class.new('urn:extension' => {relationship: 'GAGA'}) expect(resource.relationship).to eql('GAGA') end it 'allows setting complex extension attributes' do user_groups = [{ value: '123' }, { value: '456'}] - resource = resource_class.new('extension-id' => {userGroups: user_groups}) + resource = resource_class.new('urn:extension' => {userGroups: user_groups}) expect(resource.userGroups.map(&:value)).to eql(['123', '456']) end end # "context '#initialize' do" @@ -348,8 +348,8 @@ def self.resource_type_id it 'namespaces the extension attributes' do resource = resource_class.new(relationship: 'GAGA') hash = resource.as_json - expect(hash["schemas"]).to eql(['custom-id', 'extension-id']) - expect(hash["extension-id"]).to eql("relationship" => 'GAGA') + expect(hash["schemas"]).to eql(['custom-id', 'urn:extension']) + expect(hash["urn:extension"]).to eql("relationship" => 'GAGA') end end # "context '#as_json' do" @@ -362,10 +362,10 @@ def self.resource_type_id context 'validation' do it 'validates into custom schema' do - resource = resource_class.new('extension-id' => {}) + resource = resource_class.new('urn:extension' => {}) expect(resource.valid?).to eql(false) - resource = resource_class.new('extension-id' => {relationship: 'GAGA'}) + resource = resource_class.new('urn:extension' => {relationship: 'GAGA'}) expect(resource.relationship).to eql('GAGA') expect(resource.valid?).to eql(true) end diff --git a/spec/models/scimitar/resources/mixin_spec.rb b/spec/models/scimitar/resources/mixin_spec.rb index 9d7c216..1f9c3d5 100644 --- a/spec/models/scimitar/resources/mixin_spec.rb +++ b/spec/models/scimitar/resources/mixin_spec.rb @@ -288,10 +288,15 @@ def self.scim_queryable_attributes 'name' => {'givenName'=>'Foo', 'familyName'=>'Bar'}, 'groups' => [{'display'=>g1.display_name, 'value'=>g1.id.to_s}, {'display'=>g3.display_name, 'value'=>g3.id.to_s}], 'meta' => {'location'=>"https://test.com/mock_users/#{uuid}", 'resourceType'=>'User'}, - 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User', 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User'], + 'schemas' => [ + 'urn:ietf:params:scim:schemas:core:2.0:User', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User', + ], 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => { 'organization' => 'SOMEORG', }, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User' => {}, }) end end # "context 'with list of requested attributes' do" @@ -333,13 +338,19 @@ def self.scim_queryable_attributes 'externalId' => 'AA02984', 'groups' => [{'display'=>g1.display_name, 'value'=>g1.id.to_s}, {'display'=>g3.display_name, 'value'=>g3.id.to_s}], 'meta' => {'location'=>"https://test.com/mock_users/#{uuid}", 'resourceType'=>'User'}, - 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User', 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User'], - + 'schemas' => [ + 'urn:ietf:params:scim:schemas:core:2.0:User', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User', + ], 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => { 'organization' => 'SOMEORG', 'department' => nil, - 'primaryEmail' => instance.work_email_address - } + 'primaryEmail' => instance.work_email_address, + }, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User' => { + 'manager' => nil + }, }) end end # "context 'with a UUID, renamed primary key column' do" @@ -463,9 +474,13 @@ def self.scim_timestamps_map ], 'meta' => {'location'=>'https://test.com/static_map_test', 'resourceType'=>'User'}, - 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User', 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User'], - - 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => {} + 'schemas' => [ + 'urn:ietf:params:scim:schemas:core:2.0:User', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User', + ], + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => {}, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User' => {}, }) end end # "context 'using static mappings' do" @@ -492,9 +507,13 @@ def self.scim_timestamps_map ], 'meta' => {'location'=>'https://test.com/dynamic_map_test', 'resourceType'=>'User'}, - 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User', 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User'], - - 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => {} + 'schemas' => [ + 'urn:ietf:params:scim:schemas:core:2.0:User', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User', + ], + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User' => {}, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User' => {}, }) end end # "context 'using dynamic lists' do" diff --git a/spec/requests/active_record_backed_resources_controller_spec.rb b/spec/requests/active_record_backed_resources_controller_spec.rb index 997bbfa..44c4ce7 100644 --- a/spec/requests/active_record_backed_resources_controller_spec.rb +++ b/spec/requests/active_record_backed_resources_controller_spec.rb @@ -122,7 +122,15 @@ expect(result['Resources'].size).to eql(3) keys = result['Resources'].map { |resource| resource.keys }.flatten.uniq - expect(keys).to match_array(%w[id meta name schemas urn:ietf:params:scim:schemas:extension:enterprise:2.0:User]) + + expect(keys).to match_array(%w[ + id + meta + name + schemas + urn:ietf:params:scim:schemas:extension:enterprise:2.0:User + urn:ietf:params:scim:schemas:extension:manager:1.0:User + ]) expect(result.dig('Resources', 0, 'id')).to eql @u1.primary_key.to_s expect(result.dig('Resources', 0, 'name', 'givenName')).to eql 'Foo' expect(result.dig('Resources', 0, 'name', 'familyName')).to eql 'Ark' @@ -433,6 +441,76 @@ expect(new_mock.home_email_address).to eql('home_4@test.com') expect(new_mock.work_email_address).to eql('work_4@test.com') end + + it 'with schema ID value keys without inline attributes' do + mock_before = MockUser.all.to_a + + attributes = { + userName: '4', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User': { + organization: 'Foo Bar!', + department: 'Bar Foo!' + }, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User': { + manager: 'Foo Baz!' + } + } + + attributes = spec_helper_hupcase(attributes) if force_upper_case + + expect { + post "/Users", params: attributes.merge(format: :scim) + }.to change { MockUser.count }.by(1) + + mock_after = MockUser.all.to_a + new_mock = (mock_after - mock_before).first + + expect(response.status ).to eql(201) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + + result = JSON.parse(response.body) + + expect(new_mock.organization).to eql('Foo Bar!') + expect(new_mock.department ).to eql('Bar Foo!') + expect(new_mock.manager ).to eql('Foo Baz!') + + expect(result['urn:ietf:params:scim:schemas:extension:enterprise:2.0:User']['organization']).to eql(new_mock.organization) + expect(result['urn:ietf:params:scim:schemas:extension:enterprise:2.0:User']['department' ]).to eql(new_mock.department ) + expect(result['urn:ietf:params:scim:schemas:extension:manager:1.0:User' ]['manager' ]).to eql(new_mock.manager ) + end + + it 'with schema ID value keys that have inline attributes' do + mock_before = MockUser.all.to_a + + attributes = { + userName: '4', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:organization': 'Foo Bar!', + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department': 'Bar Foo!', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User:manager': 'Foo Baz!' + } + + attributes = spec_helper_hupcase(attributes) if force_upper_case + + expect { + post "/Users", params: attributes.merge(format: :scim) + }.to change { MockUser.count }.by(1) + + mock_after = MockUser.all.to_a + new_mock = (mock_after - mock_before).first + + expect(response.status ).to eql(201) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + + result = JSON.parse(response.body) + + expect(new_mock.organization).to eql('Foo Bar!') + expect(new_mock.department ).to eql('Bar Foo!') + expect(new_mock.manager ).to eql('Foo Baz!') + + expect(result['urn:ietf:params:scim:schemas:extension:enterprise:2.0:User']['organization']).to eql(new_mock.organization) + expect(result['urn:ietf:params:scim:schemas:extension:enterprise:2.0:User']['department' ]).to eql(new_mock.department ) + expect(result['urn:ietf:params:scim:schemas:extension:manager:1.0:User' ]['manager' ]).to eql(new_mock.manager ) + end end # "shared_examples 'a creator' do | force_upper_case: |" context 'using schema-matched case' do @@ -855,6 +933,9 @@ 'organization' => 'Foo Bar!', 'department' => 'Bar Foo!' }, + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User': { + 'manager' => 'Foo Baz!' + } }, }, ] @@ -872,6 +953,7 @@ expect(@u2.organization).to eql('Foo Bar!') expect(@u2.department ).to eql('Bar Foo!') + expect(@u2.manager ).to eql('Foo Baz!') end end @@ -886,7 +968,8 @@ op: 'add', value: { 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:organization' => 'Foo Bar!', - 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department' => 'Bar Foo!' + 'urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department' => 'Bar Foo!', + 'urn:ietf:params:scim:schemas:extension:manager:1.0:User:manager' => 'Foo Baz!' }, }, ] @@ -904,6 +987,7 @@ expect(@u2.organization).to eql('Foo Bar!') expect(@u2.department ).to eql('Bar Foo!') + expect(@u2.manager ).to eql('Foo Baz!') end end diff --git a/spec/requests/engine_spec.rb b/spec/requests/engine_spec.rb index ed17ebb..2404133 100644 --- a/spec/requests/engine_spec.rb +++ b/spec/requests/engine_spec.rb @@ -111,6 +111,7 @@ def self.endpoint; '/License'; end expect(schema_classes).to match_array([ Scimitar::Schema::User, ScimSchemaExtensions::User::Enterprise, + ScimSchemaExtensions::User::Manager, @license_resource.schemas.first ]) end