Add support for Okta as SCIM provider

Minor adjustments were necessary to support Okta SCIM payloads.
The SCIM spec gives enough leeway that providers can differ slightly
in the payload format. These changes allow us to be compatible with
Azure and Okta, but probably additional providers as well.

changelogs/unreleased/dblessing_okta_scim.yml

+---
+title: Add support for Okta as a SCIM provider
+merge_request: 25649
+author:
+type: added

ee/app/finders/scim_finder.rb

@@ -49,6 +49,9 @@ class ScimFinder
     if eq_filter_on_extern_uid?(parser)
       by_extern_uid(parser.filter_params[:extern_uid])
     elsif eq_filter_on_username?(parser)
+      identity = by_extern_uid(parser.filter_params[:username])
+      return identity if identity.present?
+
       by_username(parser.filter_params[:username])
     else
       raise UnsupportedFilter

ee/lib/ee/api/entities/scim/user.rb

@@ -10,9 +10,10 @@ module EE
           expose :active
           expose :email_user, as: :emails, using: ::EE::API::Entities::Scim::Emails
 
-          expose 'name.formatted' do |identity, _options|
-            identity.user.name
+          expose :name, using: ::EE::API::Entities::Scim::UserName do |identity, _options|
+            identity.user
           end
+
           expose :meta do
             expose :resource_type, as: :resourceType
           end

ee/lib/ee/api/entities/scim/user_name.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Entities
+      module Scim
+        class UserName < Grape::Entity
+          expose :name, as: :formatted
+          expose :first_name, as: :givenName
+          expose :last_name, as: :familyName
+        end
+      end
+    end
+  end
+end

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

@@ -4,7 +4,7 @@ module EE
   module Gitlab
     module Scim
       class ParamsParser
-        OPERATIONS_OPERATORS = %w[Replace Add].freeze
+        OPERATIONS_OPERATORS = %w[replace add].freeze
 
         def initialize(params)
           @params = params.with_indifferent_access
@@ -43,9 +43,9 @@ module EE
 
         def process_operations
           @params[:Operations].each_with_object({}) do |operation, hash|
-            next unless OPERATIONS_OPERATORS.include?(operation[:op])
+            next unless OPERATIONS_OPERATORS.include?(operation[:op].downcase)
 
-            hash.merge!(AttributeTransform.new(operation[:path]).map_to(operation[:value]))
+            hash.merge!(transformed_operation(operation))
           end
         end
 
@@ -76,6 +76,19 @@ module EE
           formatted_name ||= [name[:givenName], name[:familyName]].compact.join(' ')
           @hash[:name] = formatted_name
         end
+
+        # The `path` key is optional per the SCIM spec.
+        # Ex. Azure uses `path` while Okta does not.
+        def transformed_operation(operation)
+          key, value =
+            if operation[:path]
+              operation.values_at(:path, :value)
+            else
+              [operation[:value].each_key.first, operation[:value].each_value.first]
+            end
+
+          AttributeTransform.new(key).map_to(value)
+        end
       end
     end
   end

ee/spec/finders/scim_finder_spec.rb

@@ -56,8 +56,18 @@ describe ScimFinder do
             expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id
           end
 
-          it 'allows lookup by userName when userName is an email address' do
-            expect(finder.search(filter: "userName eq #{id.user.email}").first).to eq id
+          context 'allows lookup by userName' do
+            it 'finds user when userName is an email address' do
+              expect(finder.search(filter: "userName eq #{id.user.email}").first).to eq id
+            end
+
+            it 'finds user by username' do
+              expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id
+            end
+
+            it 'finds user by extern_uid' do
+              expect(finder.search(filter: "userName eq \"#{id.extern_uid}\"").first).to eq id
+            end
           end
         end
 

ee/spec/lib/ee/api/entities/scim/user_name_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::EE::API::Entities::Scim::UserName do
+  let(:user) { build(:user) }
+
+  subject { described_class.new(user).as_json }
+
+  it 'contains the name' do
+    expect(subject[:formatted]).to eq(user.name)
+  end
+
+  it 'contains the first name' do
+    expect(subject[:givenName]).to eq(user.first_name)
+  end
+
+  it 'contains the last name' do
+    expect(subject[:familyName]).to eq(user.last_name)
+  end
+end

ee/spec/lib/ee/api/entities/scim/user_spec.rb

@@ -25,7 +25,15 @@ describe ::EE::API::Entities::Scim::User do
   end
 
   it 'contains the name' do
-    expect(subject[:'name.formatted']).to eq(user.name)
+    expect(subject[:name][:formatted]).to eq(user.name)
+  end
+
+  it 'contains the first name' do
+    expect(subject[:name][:givenName]).to eq(user.first_name)
+  end
+
+  it 'contains the last name' do
+    expect(subject[:name][:familyName]).to eq(user.last_name)
   end
 
   it 'contains the email' do

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

@@ -32,22 +32,56 @@ describe EE::Gitlab::Scim::ParamsParser do
   end
 
   describe '#update_params' do
-    it 'returns the correct operation attributes' do
-      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
+    shared_examples 'scim operation active false' do
+      it 'returns the correct operation attributes' do
+        expect(described_class.new(Operations: operations).update_params).to eq(active: false)
+      end
+    end
+
+    shared_examples 'scim operation empty' do
+      it 'returns an empty hash for the wrong operations' do
+        expect(described_class.new(Operations: operations).update_params).to eq({})
+      end
+    end
 
-      expect(described_class.new(Operations: operations).update_params).to eq(active: false)
+    shared_examples 'scim operation update name' do
+      it 'can update name from displayName' do
+        expect(described_class.new(Operations: operations).update_params).to include(name: 'My Name Is')
+      end
     end
 
-    it 'returns an empty hash for the wrong operations' do
-      operations = [{ 'op': 'Replace', 'path': 'test', 'value': 'False' }]
+    context 'when path key is present' do
+      it_behaves_like 'scim operation active false' do
+        let(:operations) { [{ 'op': 'replace', 'path': 'active', 'value': 'False' }] }
+      end
 
-      expect(described_class.new(Operations: operations).update_params).to eq({})
+      it_behaves_like 'scim operation empty' do
+        let(:operations) { [{ 'op': 'replace', 'path': 'test', 'value': 'False' }] }
+      end
+
+      it_behaves_like 'scim operation update name' do
+        let(:operations) { [{ 'op': 'replace', 'path': 'displayName', 'value': 'My Name Is' }] }
+      end
     end
 
-    it 'can update name from displayName' do
-      operations = [{ 'op': 'Replace', 'path': 'displayName', 'value': 'My Name Is' }]
+    context 'when path key is not present' do
+      it_behaves_like 'scim operation active false' do
+        let(:operations) { [{ 'op': 'replace', 'value': { 'active': false } }] }
+      end
+
+      it_behaves_like 'scim operation empty' do
+        let(:operations) { [{ 'op': 'replace', 'value': { 'test': false } }] }
+      end
 
-      expect(described_class.new(Operations: operations).update_params).to include(name: 'My Name Is')
+      it_behaves_like 'scim operation update name' do
+        let(:operations) { [{ 'op': 'replace', 'value': { 'displayName': 'My Name Is' } }] }
+      end
+    end
+
+    context 'with capitalized op values for Azure' do
+      it_behaves_like 'scim operation active false' do
+        let(:operations) { [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }
+      end
     end
   end
 
@@ -87,27 +121,33 @@ describe EE::Gitlab::Scim::ParamsParser do
 
   describe '#deprovision_user?' do
     it 'returns true when deprovisioning' do
-      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
+      operations = [{ 'op': 'replace', 'path': 'active', 'value': 'False' }]
 
       expect(described_class.new(Operations: operations).deprovision_user?).to be true
     end
 
     it 'returns false when not deprovisioning' do
-      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }]
+      operations = [{ 'op': 'replace', 'path': 'active', 'value': 'True' }]
 
       expect(described_class.new(Operations: operations).deprovision_user?).to be false
     end
+
+    it 'returns true when deprovisioning without a path key' do
+      operations = [{ 'op': 'replace', 'value': { 'active': false } }]
+
+      expect(described_class.new(Operations: operations).deprovision_user?).to be true
+    end
   end
 
   describe '#reprovision_user?' do
     it 'returns true when reprovisioning' do
-      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }]
+      operations = [{ 'op': 'replace', 'path': 'active', 'value': 'True' }]
 
       expect(described_class.new(Operations: operations).reprovision_user?).to be true
     end
 
     it 'returns false when not reprovisioning' do
-      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
+      operations = [{ 'op': 'replace', 'path': 'active', 'value': 'False' }]
 
       expect(described_class.new(Operations: operations).reprovision_user?).to be false
     end

ee/spec/requests/api/scim_spec.rb

@@ -277,18 +277,6 @@ describe API::Scim do
             end
           end
         end
-
-        context 'Remove user' do
-          before do
-            params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
-
-            patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
-          end
-
-          it 'responds with 204' do
-            expect(response).to have_gitlab_http_status(:no_content)
-          end
-        end
       end
     end
 
@@ -375,12 +363,34 @@ describe API::Scim do
 
     describe 'PATCH api/scim/v2/groups/:group/Users/:id' do
       context 'Remove user' do
-        it 'removes the identity link' do
-          params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
+        shared_examples 'remove user' do
+          it 'responds with 204' do
+            expect(response).to have_gitlab_http_status(:no_content)
+          end
 
-          patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          it 'removes the identity link' do
+            expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          end
+        end
 
-          expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        context 'when path key is present' do
+          before do
+            params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
+
+            patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          end
+
+          it_behaves_like 'remove user'
+        end
+
+        context 'when the path key is not present' do
+          before do
+            params = { Operations: [{ 'op': 'replace', 'value': { 'active': false } }] }.to_query
+
+            patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          end
+
+          it_behaves_like 'remove user'
         end
       end
     end