Merge branch 'jej/scim-params-parser-refactor' into 'master'

Refactor SCIM ParamsParser

See merge request gitlab-org/gitlab!19417

ee/app/finders/scim_finder.rb

@@ -10,7 +10,7 @@ class ScimFinder
   def search(params)
     return Identity.none unless saml_provider&.enabled?
 
-    parsed_hash = EE::Gitlab::Scim::ParamsParser.new(params).result
-    Identity.where_group_saml_uid(saml_provider, parsed_hash[:extern_uid])
+    parser = EE::Gitlab::Scim::ParamsParser.new(params)
+    Identity.where_group_saml_uid(saml_provider, parser.filter_params[:extern_uid])
   end
 end

ee/lib/api/scim.rb

@@ -62,7 +62,7 @@ module API
         # rubocop: disable CodeReuse/ActiveRecord
         def update_scim_user(identity)
           parser = EE::Gitlab::Scim::ParamsParser.new(params)
-          parsed_hash = parser.result
+          parsed_hash = parser.update_params
 
           if parser.deprovision_user?
             destroy_identity(identity)
@@ -129,7 +129,7 @@ module API
         post do
           group = find_and_authenticate_group!(params[:group])
           parser = EE::Gitlab::Scim::ParamsParser.new(params)
-          result = EE::Gitlab::Scim::ProvisioningService.new(group, parser.result).execute
+          result = EE::Gitlab::Scim::ProvisioningService.new(group, parser.post_params).execute
 
           case result.status
           when :success

ee/lib/ee/gitlab/scim/attribute_transform.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Scim
+      class AttributeTransform
+        MAPPINGS = {
+          id: :extern_uid,
+          displayName: :name,
+          'name.formatted': :name,
+          'emails[type eq "work"].value': :email,
+          active: :active,
+          externalId: :extern_uid,
+          userName: :username
+        }.with_indifferent_access.freeze
+
+        def initialize(key)
+          @key = key
+        end
+
+        def valid?
+          MAPPINGS.key?(@key)
+        end
+
+        def gitlab_key
+          MAPPINGS[@key]
+        end
+
+        def map_to(input)
+          return {} unless valid?
+
+          { gitlab_key => ValueParser.new(input).type_cast }
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/scim/filter_parser.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Scim
+      class FilterParser
+        FILTER_OPERATORS = %w[eq].freeze
+
+        attr_reader :attribute, :operator, :value
+
+        def initialize(filter)
+          @attribute, @operator, @value = filter&.split(' ')
+        end
+
+        def valid?
+          FILTER_OPERATORS.include?(operator) && attribute_transform.valid?
+        end
+
+        def params
+          @params ||= begin
+            return {} unless valid?
+
+            attribute_transform.map_to(value)
+          end
+        end
+
+        private
+
+        def attribute_transform
+          @attribute_transform ||= AttributeTransform.new(attribute)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/scim/params_parser.rb

@@ -4,81 +4,54 @@ module EE
   module Gitlab
     module Scim
       class ParamsParser
-        FILTER_OPERATORS = %w[eq].freeze
         OPERATIONS_OPERATORS = %w[Replace Add].freeze
 
-        ATTRIBUTE_MAP = {
-          id: :extern_uid,
-          displayName: :name,
-          'name.formatted': :name,
-          'emails[type eq "work"].value': :email,
-          active: :active,
-          externalId: :extern_uid,
-          userName: :username
-        }.with_indifferent_access.freeze
-
-        COERCED_VALUES = {
-          'True' => true,
-          'False' => false
-        }.freeze
-
         def initialize(params)
           @params = params.with_indifferent_access
           @hash = {}
         end
 
         def deprovision_user?
-          result[:active] == false
+          update_params[:active] == false
         end
 
-        def result
-          @result ||= process
+        def post_params
+          @post_params ||= process_post_params
         end
 
-        private
+        def update_params
+          @update_params ||= process_operations
+        end
 
-        def process
-          if @params[:filter]
-            process_filter
-          elsif @params[:Operations]
-            process_operations
-          else
-            # SCIM POST params
-            process_params
-          end
+        def filter_params
+          @filter_params ||= filter_parser.params
         end
 
-        def process_filter
-          attribute, operator, value = @params[:filter].split(' ')
+        def filter_operator
+          filter_parser.operator.to_sym if filter_parser.valid?
+        end
 
-          return {} unless FILTER_OPERATORS.include?(operator)
-          return {} unless ATTRIBUTE_MAP[attribute]
+        private
 
-          { ATTRIBUTE_MAP[attribute] => coerce(value) }
+        def filter_parser
+          @filter_parser ||= FilterParser.new(@params[:filter])
         end
 
         def process_operations
           @params[:Operations].each_with_object({}) do |operation, hash|
             next unless OPERATIONS_OPERATORS.include?(operation[:op])
 
-            attribute = ATTRIBUTE_MAP[operation[:path]]
-
-            hash[attribute] = coerce(operation[:value]) if attribute
+            hash.merge!(AttributeTransform.new(operation[:path]).map_to(operation[:value]))
           end
         end
 
-        def process_params
+        def process_post_params
           overwrites = { email: parse_emails, name: parse_name }.compact
-          parse_params.merge(overwrites)
-        end
 
-        def parse_params
           # compact can remove :active if the value for that is nil
-          @params.except(:email, :name).compact.each_with_object({}) do |(param, value), hash|
-            attribute = ATTRIBUTE_MAP[param]
-
-            hash[attribute] = coerce(value) if attribute
-          end
+          @params.except(overwrites.keys).compact.each_with_object({}) do |(param, value), hash|
+            hash.merge!(AttributeTransform.new(param).map_to(value))
+          end.merge(overwrites)
         end
 
         def parse_emails
@@ -99,15 +72,6 @@ module EE
           formatted_name ||= [name[:givenName], name[:familyName]].compact.join(' ')
           @hash[:name] = formatted_name
         end
-
-        def coerce(value)
-          return value unless value.is_a?(String)
-
-          value = value.delete('\"')
-          coerced = COERCED_VALUES[value]
-
-          coerced.nil? ? value : coerced
-        end
       end
     end
   end

ee/lib/ee/gitlab/scim/value_parser.rb

+# frozen_string_literal: true
+
+# Casts input from a SCIM compValue to a ruby object
+# This should be updated to accept the following JSON style inputs:
+#   false / null / true / number / string
+#
+# It also needs to accept capitalized True/False from Azure
+#
+# See https://tools.ietf.org/html/rfc7644#section-3.4.2.2
+module EE
+  module Gitlab
+    module Scim
+      class ValueParser
+        COERCED_VALUES = {
+          'true' => true,
+          'false' => false
+        }.freeze
+
+        def initialize(input)
+          @input = input
+        end
+
+        def type_cast
+          return @input unless @input.is_a?(String)
+
+          COERCED_VALUES.fetch(unquoted.downcase, unquoted)
+        end
+
+        private
+
+        def unquoted
+          @unquoted ||= @input.delete('\"')
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/scim/attribute_transform_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::Gitlab::Scim::AttributeTransform do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#valid?' do
+    it 'is true for accepted keys' do
+      expect(described_class.new(:userName)).to be_valid
+    end
+
+    it 'is false for unused keys' do
+      expect(described_class.new(:someUnknownKey)).not_to be_valid
+    end
+  end
+
+  describe '#gitlab_key' do
+    where(:scim_key, :expected) do
+      :id | :extern_uid
+      :displayName | :name
+      'name.formatted' | :name
+      'emails[type eq "work"].value' | :email
+      :active | :active
+      :externalId | :extern_uid
+      :userName | :username
+    end
+
+    with_them do
+      it do
+        expect(described_class.new(scim_key).gitlab_key).to eq expected
+      end
+    end
+  end
+
+  describe '#map_to' do
+    it 'returns an empty hash for unknown keys' do
+      expect(described_class.new(:abc).map_to(double)).to eq({})
+    end
+
+    it 'typecasts input' do
+      expect(described_class.new(:active).map_to('true')).to eq(active: true)
+    end
+
+    it 'creates a hash from transformed key to a typecasted value' do
+      expect(described_class.new(:userName).map_to('"my_handle"')).to eq(username: 'my_handle')
+    end
+  end
+end

ee/spec/lib/ee/gitlab/scim/filter_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::Gitlab::Scim::FilterParser do
+  describe '#operator' do
+    it 'is extracted from the filter' do
+      expect(described_class.new('displayName ne ""').operator).to eq 'ne'
+    end
+  end
+
+  describe '#valid?' do
+    it 'succeeds when the operator is supported' do
+      expect(described_class.new('userName eq "nick"')).to be_valid
+    end
+
+    it 'fails with unsupported operators' do
+      expect(described_class.new('userName is "nick"')).not_to be_valid
+    end
+
+    it 'fails when the attribute path is unsupported' do
+      expect(described_class.new('user_name eq "nick"')).not_to be_valid
+    end
+  end
+
+  describe '#params' do
+    it 'returns a mapping to filter on' do
+      expect(described_class.new('userName eq "nick"').params).to eq(username: 'nick')
+    end
+
+    it 'returns an empty hash when invalid' do
+      expect(described_class.new('userName is "nick"').params).to eq({})
+    end
+  end
+end

ee/spec/lib/ee/gitlab/scim/params_parser_spec.rb

@@ -3,37 +3,55 @@
 require 'spec_helper'
 
 describe EE::Gitlab::Scim::ParamsParser do
-  describe '#result' do
+  describe '#filter_params' do
     it 'returns the correct filter attributes' do
       filter = 'id eq "6ba81b08-77da"'
 
-      expect(described_class.new(filter: filter).result).to eq(extern_uid: '6ba81b08-77da')
+      expect(described_class.new(filter: filter).filter_params).to eq(extern_uid: '6ba81b08-77da')
     end
 
     it 'returns an empty hash for the wrong filter' do
       filter = 'blah eq "6ba81b08-77da"'
 
-      expect(described_class.new(filter: filter).result).to eq({})
+      expect(described_class.new(filter: filter).filter_params).to eq({})
     end
+  end
+
+  describe '#filter_operator' do
+    it 'returns the operator as a symbol' do
+      parser = described_class.new(filter: 'id eq 1')
+
+      expect(parser.filter_operator).to eq(:eq)
+    end
+
+    it 'returns nil if the filter is invalid' do
+      parser = described_class.new(filter: 'this eq that')
 
+      expect(parser.filter_operator).to eq(nil)
+    end
+  end
+
+  describe '#update_params' do
     it 'returns the correct operation attributes' do
       operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
 
-      expect(described_class.new(Operations: operations).result).to eq(active: false)
+      expect(described_class.new(Operations: operations).update_params).to eq(active: false)
     end
 
     it 'returns an empty hash for the wrong operations' do
       operations = [{ 'op': 'Replace', 'path': 'test', 'value': 'False' }]
 
-      expect(described_class.new(Operations: operations).result).to eq({})
+      expect(described_class.new(Operations: operations).update_params).to eq({})
     end
 
     it 'can update name from displayName' do
       operations = [{ 'op': 'Replace', 'path': 'displayName', 'value': 'My Name Is' }]
 
-      expect(described_class.new(Operations: operations).result).to include(name: 'My Name Is')
+      expect(described_class.new(Operations: operations).update_params).to include(name: 'My Name Is')
     end
+  end
 
+  describe '#post_params' do
     it 'returns a parsed hash for POST params' do
       params = {
         externalId: 'test',
@@ -48,22 +66,22 @@ describe EE::Gitlab::Scim::ParamsParser do
         extra: true
       }
 
-      expect(described_class.new(params).result).to eq(email: 'work@example.com',
-                                                       extern_uid: 'test',
-                                                       name: 'Test A. Name',
-                                                       username: 'username')
+      expect(described_class.new(params).post_params).to eq(email: 'work@example.com',
+                                                            extern_uid: 'test',
+                                                            name: 'Test A. Name',
+                                                            username: 'username')
     end
 
     it 'can construct a name from givenName and familyName' do
       params = { name: { givenName: 'Fred', familyName: 'Nurk' } }
 
-      expect(described_class.new(params).result).to include(name: 'Fred Nurk')
+      expect(described_class.new(params).post_params).to include(name: 'Fred Nurk')
     end
 
     it 'falls back to displayName when other names are missing' do
       params = { displayName: 'My Name' }
 
-      expect(described_class.new(params).result).to include(name: 'My Name')
+      expect(described_class.new(params).post_params).to include(name: 'My Name')
     end
   end
 

ee/spec/lib/ee/gitlab/scim/value_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::Gitlab::Scim::ValueParser do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#type_cast' do
+    where(:input, :expected_output) do
+      'True' | true
+      'true' | true
+      'False' | false
+      'false' | false
+      '"Quoted String"' | 'Quoted String'
+      true | true
+      false | false
+      123 | 123
+    end
+
+    with_them do
+      it do
+        expect(described_class.new(input).type_cast).to eq expected_output
+      end
+    end
+  end
+end