diff --git a/CHANGELOG.md b/CHANGELOG.md index f11dc98..3e65929 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2.1.3 (2023-01-09) + +* Fix https://github.com/RIPAGlobal/scimitar/issues/36 - filters are case-sensitive for special cases of `id`, `externalId` and `meta.*` attributes. A model must still declare if and how these are searchable via its `::scim_queryable_attributes` implementation, just as with any other attribute. + # 2.1.2 (2023-01-09) * Fix https://github.com/RIPAGlobal/scimitar/issues/37 - filters now correctly support case insensitive attribute names. diff --git a/Gemfile.lock b/Gemfile.lock index 3671024..ce46ec9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: . specs: - scimitar (2.1.2) + scimitar (2.1.3) rails (~> 7.0) GEM diff --git a/README.md b/README.md index 1153dc5..d05157a 100644 --- a/README.md +++ b/README.md @@ -441,6 +441,8 @@ If you believe choices made in this section may be incorrect, please [create a G * 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). + * The `PATCH` mechanism is supported, but where filters are included, only a single "attribute eq value" is permitted - no other operators or combinations. For example, a work e-mail address's value could be replaced by a PATCH patch of `emails[type eq "work"].value`. For in-path filters such as this, other operators such as `ne` are not supported; combinations with "and"/"or" are not supported; negation with "not" is not supported. If you would like to see something listed in the session implemented, please [create a GitHub issue](https://github.com/RIPAGlobal/scimitar/issues/new) asking for it to be implemented, or if possible, implement the feature and send a Pull Request. diff --git a/app/models/scimitar/lists/query_parser.rb b/app/models/scimitar/lists/query_parser.rb index 8eeada0..cce07cf 100644 --- a/app/models/scimitar/lists/query_parser.rb +++ b/app/models/scimitar/lists/query_parser.rb @@ -605,6 +605,16 @@ def apply_scim_filter( raise Scimitar::FilterError unless all_supported + unless case_sensitive + lc_scim_attribute = scim_attribute.downcase() + + case_sensitive = ( + lc_scim_attribute == 'id' || + lc_scim_attribute == 'externalid' || + lc_scim_attribute.start_with?('meta.') + ) + end + column_names.each.with_index do | column_name, index | arel_column = arel_table[column_name] arel_operation = case scim_operator diff --git a/lib/scimitar/version.rb b/lib/scimitar/version.rb index 7d17ca3..b68ec63 100644 --- a/lib/scimitar/version.rb +++ b/lib/scimitar/version.rb @@ -3,7 +3,7 @@ module Scimitar # Gem version. If this changes, be sure to re-run "bundle install" or # "bundle update". # - VERSION = '2.1.2' + VERSION = '2.1.3' # Date for VERSION. If this changes, be sure to re-run "bundle install" # or "bundle update". diff --git a/spec/apps/dummy/app/models/mock_user.rb b/spec/apps/dummy/app/models/mock_user.rb index 1743a5b..091fee4 100644 --- a/spec/apps/dummy/app/models/mock_user.rb +++ b/spec/apps/dummy/app/models/mock_user.rb @@ -92,11 +92,14 @@ def self.scim_mutable_attributes def self.scim_queryable_attributes return { - 'name.givenName' => { column: :first_name }, - 'name.familyName' => { column: :last_name }, - 'emails' => { columns: [ :work_email_address, :home_email_address ] }, - 'emails.value' => { columns: [ :work_email_address, :home_email_address ] }, - 'emails.type' => { ignore: true } # We can't filter on that; it'll just search all e-mails + 'id' => { column: :id }, + 'externalId' => { column: :scim_uid }, + 'meta.lastModified' => { column: :updated_at }, + 'name.givenName' => { column: :first_name }, + 'name.familyName' => { column: :last_name }, + 'emails' => { columns: [ :work_email_address, :home_email_address ] }, + 'emails.value' => { columns: [ :work_email_address, :home_email_address ] }, + 'emails.type' => { ignore: true } # We can't filter on that; it'll just search all e-mails } end diff --git a/spec/apps/dummy/db/migrate/20230109012729_add_timestamps_to_mock_user.rb b/spec/apps/dummy/db/migrate/20230109012729_add_timestamps_to_mock_user.rb new file mode 100644 index 0000000..58737f2 --- /dev/null +++ b/spec/apps/dummy/db/migrate/20230109012729_add_timestamps_to_mock_user.rb @@ -0,0 +1,5 @@ +class AddTimestampsToMockUser < ActiveRecord::Migration[7.0] + def change + add_timestamps :mock_users + end +end diff --git a/spec/apps/dummy/db/schema.rb b/spec/apps/dummy/db/schema.rb index 83f88c6..bd8474f 100644 --- a/spec/apps/dummy/db/schema.rb +++ b/spec/apps/dummy/db/schema.rb @@ -10,8 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_03_08_044214) do - +ActiveRecord::Schema[7.0].define(version: 2023_01_09_012729) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -37,6 +36,8 @@ t.text "work_email_address" t.text "home_email_address" t.text "work_phone_number" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end end diff --git a/spec/models/scimitar/lists/query_parser_spec.rb b/spec/models/scimitar/lists/query_parser_spec.rb index 8839314..b0f8fff 100644 --- a/spec/models/scimitar/lists/query_parser_spec.rb +++ b/spec/models/scimitar/lists/query_parser_spec.rb @@ -590,7 +590,7 @@ end it 'complains if there is no column mapping available' do - expect { @instance.send(:activerecord_columns, 'externalId') }.to raise_error(Scimitar::FilterError) + expect { @instance.send(:activerecord_columns, 'userName') }.to raise_error(Scimitar::FilterError) end it 'complains about malformed declarations' do diff --git a/spec/models/scimitar/schema/attribute_spec.rb b/spec/models/scimitar/schema/attribute_spec.rb index c80f5f5..7cc0689 100644 --- a/spec/models/scimitar/schema/attribute_spec.rb +++ b/spec/models/scimitar/schema/attribute_spec.rb @@ -21,10 +21,8 @@ expect(name.type).to eql('complex') expect(name.subAttributes).to eql(Scimitar::Schema::Name.scim_attributes) end - end - context '#valid?' do it 'is invalid if attribute is required but value is blank' do attribute = described_class.new(name: 'userName', type: 'string', required: true) @@ -76,5 +74,4 @@ expect(described_class.new(name: 'startDate', type: 'dateTime').valid?('gaga')).to be(false) end end - end diff --git a/spec/requests/active_record_backed_resources_controller_spec.rb b/spec/requests/active_record_backed_resources_controller_spec.rb index 32ab073..52864ef 100644 --- a/spec/requests/active_record_backed_resources_controller_spec.rb +++ b/spec/requests/active_record_backed_resources_controller_spec.rb @@ -1,12 +1,15 @@ require 'spec_helper' +require 'time' RSpec.describe Scimitar::ActiveRecordBackedResourcesController do before :each do allow_any_instance_of(Scimitar::ApplicationController).to receive(:authenticated?).and_return(true) - @u1 = MockUser.create(username: '1', first_name: 'Foo', last_name: 'Ark', home_email_address: 'home_1@test.com') - @u2 = MockUser.create(username: '2', first_name: 'Foo', last_name: 'Bar', home_email_address: 'home_2@test.com') - @u3 = MockUser.create(username: '3', first_name: 'Foo', home_email_address: 'home_3@test.com') + lmt = Time.parse("2023-01-09 14:25:00 +1300") + + @u1 = MockUser.create(username: '1', first_name: 'Foo', last_name: 'Ark', home_email_address: 'home_1@test.com', scim_uid: '001', created_at: lmt, updated_at: lmt + 1) + @u2 = MockUser.create(username: '2', first_name: 'Foo', last_name: 'Bar', home_email_address: 'home_2@test.com', scim_uid: '002', created_at: lmt, updated_at: lmt + 2) + @u3 = MockUser.create(username: '3', first_name: 'Foo', home_email_address: 'home_3@test.com', scim_uid: '003', created_at: lmt, updated_at: lmt + 3) end # =========================================================================== @@ -48,7 +51,7 @@ it 'applies a filter, with case-insensitive value comparison' do get '/Users', params: { format: :scim, - filter: 'name.givenName eq "Foo" and name.familyName pr and emails ne "home_1@TEST.COM"' + filter: 'name.givenName eq "FOO" and name.familyName pr and emails ne "home_1@test.com"' } expect(response.status).to eql(200) @@ -83,6 +86,68 @@ expect(usernames).to match_array(['2']) end + # Strange attribute capitalisation in tests here builds on test coverage + # for now-fixed GitHub issue #37. + # + context '"meta" / IDs (GitHub issue #36)' do + it 'applies a filter on primary keys, using direct comparison (rather than e.g. case-insensitive operators)' do + get '/Users', params: { + format: :scim, + filter: "id eq \"#{@u3.id}\"" + } + + expect(response.status).to eql(200) + result = JSON.parse(response.body) + + expect(result['totalResults']).to eql(1) + expect(result['Resources'].size).to eql(1) + + ids = result['Resources'].map { |resource| resource['id'] } + expect(ids).to match_array([@u3.id.to_s]) + + usernames = result['Resources'].map { |resource| resource['userName'] } + expect(usernames).to match_array(['3']) + end + + it 'applies a filter on external IDs, using direct comparison' do + get '/Users', params: { + format: :scim, + filter: "externalID eq \"#{@u2.scim_uid}\"" + } + + expect(response.status).to eql(200) + result = JSON.parse(response.body) + + expect(result['totalResults']).to eql(1) + expect(result['Resources'].size).to eql(1) + + ids = result['Resources'].map { |resource| resource['id'] } + expect(ids).to match_array([@u2.id.to_s]) + + usernames = result['Resources'].map { |resource| resource['userName'] } + expect(usernames).to match_array(['2']) + end + + it 'applies a filter on "meta" entries, using direct comparison' do + get '/Users', params: { + format: :scim, + filter: "Meta.LastModified eq \"#{@u3.updated_at}\"" + } + + expect(response.status).to eql(200) + result = JSON.parse(response.body) + + expect(result['totalResults']).to eql(1) + expect(result['Resources'].size).to eql(1) + + ids = result['Resources'].map { |resource| resource['id'] } + expect(ids).to match_array([@u3.id.to_s]) + + usernames = result['Resources'].map { |resource| resource['userName'] } + expect(usernames).to match_array(['3']) + end + end # "context '"meta" / IDs (GitHub issue #36)' do" + it 'obeys a page size' do get '/Users', params: { format: :scim,