Merge pull request #128 from RIPAGlobal/feature/handle-bad-microsoft-…

…filters-in-get-requests

Handle bad Microsoft filters in GET request (see #115)

app/models/scimitar/lists/query_parser.rb

@@ -59,6 +59,11 @@ class QueryParser
       #
       BINARY_OPERATORS = Set.new(OPERATORS.keys.reject { |op| UNARY_OPERATORS.include?(op) }).freeze
 
+      # Precompiled expression that matches a valid attribute name according to
+      # https://tools.ietf.org/html/rfc7643#section-2.1.
+      #
+      ATTRNAME = /[[:alpha:]][[:alnum:]$-_]*/
+
       # =======================================================================
       # Tokenizing expressions
       # =======================================================================
@@ -291,6 +296,10 @@ def get_tree
         # the part before the "[" as a prefix - "emails[type" to "emails.type",
         # with similar substitutions therein.
         #
+        # Further, via https://github.com/RIPAGlobal/scimitar/issues/115 we see
+        # a requirement to support a broken form emitted by Microsoft; that is
+        # supported herein.
+        #
         # This method tries to flatten things thus. It throws exceptions if any
         # problems arise at all. Some limitations:
         #
@@ -314,6 +323,9 @@ def get_tree
         #     <- userType ne "Employee" and not (emails[value co "example.com" or (value co "example.org")]) and userName="foo"
         #     => userType ne "Employee" and not (emails.value co "example.com" or (emails.value co "example.org")) and userName="foo"
         #
+        #     <- emails[type eq "work"].value eq "[email protected]"   (Microsoft workaround)
+        #     => emails.type eq "work" and emails.value eq "[email protected]"
+        #
         # +filter+:: Input filter string. Returns a possibly modified String,
         #            with the hopefully equivalent but flattened filter inside.
         #
@@ -363,9 +375,53 @@ def flatten_filter(filter)
               end
 
             elsif (expecting_value)
-              matches = downcased.match(/([^\\])\]/) # Contains no-backslash-then-literal (unescaped) ']'
+              matches = downcased.match(/([^\\])\](.*)/) # Contains no-backslash-then-literal (unescaped) ']'; also capture anything after
               unless matches.nil? # Contains no-backslash-then-literal (unescaped) ']'
                 character_before_closing_bracket = matches[1]
+                characters_after_closing_bracket = matches[2]
+
+                # https://github.com/RIPAGlobal/scimitar/issues/115 - detect
+                # bad Microsoft filters. After the closing bracket, we expect a
+                # dot then valid attribute characters and at least one white
+                # space character and filter operator, but we split on spaces,
+                # so the next item in the components array must be a recognised
+                # operator for the special case code to kick in.
+                #
+                # If this all works out, we transform this section of the
+                # filter string into a local dotted form, reconstruct the
+                # overall filter with this substitution, and call back to this
+                # method with that modified filter, returning the result.
+                #
+                # So - NOTE RECURSION AND EARLY EXIT POSSIBILITY HEREIN.
+                #
+                if (
+                  ! attribute_prefix.nil? &&
+                  OPERATORS.key?(components[index + 1]&.downcase) &&
+                  characters_after_closing_bracket.match?(/^\.#{ATTRNAME}$/)
+                )
+                  # E.g. '"work"' and '.value' from input '"work"].value'
+                  #
+                  component_matches           = component.match(/^(.*?[^\\])\](.*)/)
+                  part_before_closing_bracket = component_matches[1]
+                  part_after_closing_bracket  = component_matches[2]
+
+                  # Produces e.g. '"work"] and emails.value'
+                  #
+                  dotted_version = "#{part_before_closing_bracket}] and #{attribute_prefix}#{part_after_closing_bracket}"
+
+                  # Overwrite the components array entry with this new version.
+                  #
+                  components[index] = dotted_version
+
+                  # Join it back again as a reconstructed valid filter string.
+                  #
+                  hopefully_valid_filter = components.join(' ')
+
+                  # NOTE EARLY EXIT
+                  #
+                  return flatten_filter(hopefully_valid_filter)
+                end
+
                 component.gsub!(/[^\\]\]/, character_before_closing_bracket)
 
                 if expecting_closing_bracket

spec/models/scimitar/lists/query_parser_spec.rb

@@ -346,6 +346,33 @@
         result = @instance.send(:flatten_filter, 'emails[type eq "work" and value co "@example.com"    ] or userType eq "Admin" or ims[type eq "xmpp" and value co "@foo.com"]')
         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
+
+      # https://github.com/RIPAGlobal/scimitar/issues/115
+      #
+      context 'broken filters from Microsoft (GitHub issue #115)' do
+        it 'work with "eq"' do
+          result = @instance.send(:flatten_filter, 'emails[type eq "work"].value eq "[email protected]"')
+          expect(result).to eql('emails.type eq "work" and emails.value eq "[email protected]"')
+        end
+
+        it 'work with "ne"' do # (just check a couple of operators, not all!)
+          result = @instance.send(:flatten_filter, 'emails[type eq "work"].value ne "[email protected]"')
+          expect(result).to eql('emails.type eq "work" and emails.value ne "[email protected]"')
+        end
+
+        it 'preserve input case' do
+          result = @instance.send(:flatten_filter, 'emaiLs[TYPE eq "work"].valUE eq "[email protected]"')
+          expect(result).to eql('emaiLs.TYPE eq "work" and emaiLs.valUE eq "[email protected]"')
+        end
+
+        # At the time of writing, this was used in a "belt and braces" request
+        # spec in 'active_record_backed_resources_controller_spec.rb'.
+        #
+        it 'handles more complex, hypothetical cases' do
+          result = @instance.send(:flatten_filter, 'name[givenName eq "FOO"].familyName pr and emails ne "[email protected]"')
+          expect(result).to eql('name.givenName eq "FOO" and name.familyName pr and emails ne "[email protected]"')
+        end
+      end # "context 'broken filters from Microsoft' do"
     end # "context 'when flattening is needed' do"
 
     context 'with bad filters' do

spec/requests/active_record_backed_resources_controller_spec.rb

@@ -128,6 +128,8 @@
         expect(result.dig('Resources', 0, 'name', 'familyName')).to eql 'Ark'
       end
 
+      # https://github.com/RIPAGlobal/scimitar/issues/37
+      #
       it 'applies a filter, with case-insensitive attribute matching (GitHub issue #37)' do
         get '/Users', params: {
           format: :scim,
@@ -149,6 +151,30 @@
         expect(usernames).to match_array(['2'])
       end
 
+      # https://github.com/RIPAGlobal/scimitar/issues/115
+      #
+      it 'handles broken Microsoft filters (GitHub issue #115)' do
+        get '/Users', params: {
+          format: :scim,
+          filter: 'name[givenName eq "FOO"].familyName pr and emails ne "[email protected]"'
+        }
+
+        expect(response.status                 ).to eql(200)
+        expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8')
+
+        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.primary_key.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.
       #