Merge pull request #40 from RIPAGlobal/feature/backport-fixes-v2.1.2-…

…v2.1.3

Integrate changes from #38 and #39

CHANGELOG.md

@@ -1,3 +1,11 @@
+# 1.3.3 (2023-01-10)
+
+* Back-ports fixes from v2.1.3 for Rails 6 users.
+
+# 1.3.2 (2023-01-10)
+
+* Back-ports fixes from v2.1.2 for Rails 6 users.
+
 # 1.3.1 (2022-11-04)
 
 * Back-ports features from v2.1.1 for Rails 6 users.

Gemfile.lock

@@ -9,7 +9,7 @@ GIT
 PATH
   remote: .
   specs:
-    scimitar (1.3.1)
+    scimitar (1.3.3)
       rails (~> 6.0)
 
 GEM
@@ -78,34 +78,51 @@ GEM
     byebug (11.1.3)
     concurrent-ruby (1.1.10)
     crass (1.0.6)
+    date (3.3.3)
     diff-lcs (1.5.0)
     docile (1.4.0)
     doggo (1.2.0)
       rspec-core (~> 3.10)
-    erubi (1.11.0)
+    erubi (1.12.0)
     globalid (1.0.0)
       activesupport (>= 5.0)
     i18n (1.12.0)
       concurrent-ruby (~> 1.0)
-    loofah (2.19.0)
+    loofah (2.19.1)
       crass (~> 1.0.2)
       nokogiri (>= 1.5.9)
-    mail (2.7.1)
+    mail (2.8.0)
       mini_mime (>= 0.1.1)
+      net-imap
+      net-pop
+      net-smtp
     marcel (1.0.2)
     method_source (1.0.0)
     mini_mime (1.1.2)
-    mini_portile2 (2.8.0)
-    minitest (5.16.3)
+    mini_portile2 (2.8.1)
+    minitest (5.17.0)
+    net-imap (0.3.4)
+      date
+      net-protocol
+    net-pop (0.1.2)
+      net-protocol
+    net-protocol (0.2.1)
+      timeout
+    net-smtp (0.3.3)
+      net-protocol
     nio4r (2.5.8)
-    nokogiri (1.13.9)
+    nokogiri (1.13.10)
       mini_portile2 (~> 2.8.0)
       racc (~> 1.4)
-    pg (1.4.4)
-    psych (4.0.6)
+    nokogiri (1.13.10-x86_64-darwin)
+      racc (~> 1.4)
+    nokogiri (1.13.10-x86_64-linux)
+      racc (~> 1.4)
+    pg (1.4.5)
+    psych (5.0.1)
       stringio
-    racc (1.6.0)
-    rack (2.2.4)
+    racc (1.6.2)
+    rack (2.2.5)
     rack-test (2.0.2)
       rack (>= 1.3)
     rails (6.1.7)
@@ -126,23 +143,23 @@ GEM
     rails-dom-testing (2.0.3)
       activesupport (>= 4.2.0)
       nokogiri (>= 1.6)
-    rails-html-sanitizer (1.4.3)
-      loofah (~> 2.3)
+    rails-html-sanitizer (1.4.4)
+      loofah (~> 2.19, >= 2.19.1)
     railties (6.1.7)
       actionpack (= 6.1.7)
       activesupport (= 6.1.7)
       method_source
       rake (>= 12.2)
       thor (~> 1.0)
     rake (13.0.6)
-    rdoc (6.4.0)
+    rdoc (6.5.0)
       psych (>= 4.0.0)
     rspec-core (3.12.0)
       rspec-support (~> 3.12.0)
-    rspec-expectations (3.12.0)
+    rspec-expectations (3.12.2)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.12.0)
-    rspec-mocks (3.12.0)
+    rspec-mocks (3.12.2)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.12.0)
     rspec-rails (5.1.2)
@@ -154,29 +171,30 @@ GEM
       rspec-mocks (~> 3.10)
       rspec-support (~> 3.10)
     rspec-support (3.12.0)
-    simplecov (0.21.2)
+    simplecov (0.22.0)
       docile (~> 1.1)
       simplecov-html (~> 0.11)
       simplecov_json_formatter (~> 0.1)
     simplecov-html (0.12.3)
     simplecov-rcov (0.3.1)
       simplecov (>= 0.4.1)
     simplecov_json_formatter (0.1.4)
-    sprockets (4.1.1)
+    sprockets (4.2.0)
       concurrent-ruby (~> 1.0)
-      rack (> 1, < 3)
+      rack (>= 2.2.4, < 4)
     sprockets-rails (3.4.2)
       actionpack (>= 5.2)
       activesupport (>= 5.2)
       sprockets (>= 3.0.0)
-    stringio (3.0.2)
+    stringio (3.0.4)
     thor (1.2.1)
+    timeout (0.3.1)
     tzinfo (2.0.5)
       concurrent-ruby (~> 1.0)
     websocket-driver (0.7.5)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
-    zeitwerk (2.6.4)
+    zeitwerk (2.6.6)
 
 PLATFORMS
   ruby

README.md

@@ -429,6 +429,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.

app/controllers/scimitar/active_record_backed_resources_controller.rb

@@ -37,6 +37,7 @@ def index
       pagination_info = scim_pagination_info(query.count())
 
       page_of_results = query
+        .order(id: :asc)
         .offset(pagination_info.offset)
         .limit(pagination_info.limit)
         .to_a()

app/models/scimitar/lists/query_parser.rb

@@ -78,7 +78,7 @@ class QueryParser
       #                   method's return value here.
       #
       def initialize(attribute_map)
-        @attribute_map = attribute_map
+        @attribute_map = attribute_map.with_indifferent_case_insensitive_access()
       end
 
       # Parse SCIM filter query into RPN stack
@@ -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

lib/scimitar/version.rb

@@ -3,11 +3,11 @@ module Scimitar
   # Gem version. If this changes, be sure to re-run "bundle install" or
   # "bundle update".
   #
-  VERSION = '1.3.1'
+  VERSION = '1.3.3'
 
   # Date for VERSION. If this changes, be sure to re-run "bundle install"
   # or "bundle update".
   #
-  DATE = '2022-11-04'
+  DATE = '2023-01-10'
 
 end

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
 

spec/apps/dummy/db/migrate/20230109012729_add_timestamps_to_mock_user.rb

@@ -0,0 +1,5 @@
+class AddTimestampsToMockUser < ActiveRecord::Migration[6.1]
+  def change
+    add_timestamps :mock_users
+  end
+end

spec/apps/dummy/db/schema.rb

@@ -10,7 +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.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 +37,8 @@
     t.text "work_email_address"
     t.text "home_email_address"
     t.text "work_phone_number"
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
   end
 
 end

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

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

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)
@@ -64,6 +67,87 @@
         expect(usernames).to match_array(['2'])
       end
 
+      it 'applies a filter, with case-insensitive attribute matching (GitHub issue #37)' do
+        get '/Users', params: {
+          format: :scim,
+          filter: 'name.GIVENNAME eq "Foo" and name.Familyname pr and emails ne "home_1@test.com"'
+        }
+
+        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
+
+      # 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,