Merge branch 'dblessing_okta_scim' into 'master'

Add support for Okta as SCIM provider

See merge request gitlab-org/gitlab!25649

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