Update SCIM API and spec

ee/lib/api/scim.rb

@@ -11,39 +11,113 @@ module API
         requires :group, type: String
       end
 
+      helpers do
+        def logger
+          API.logger
+        end
+
+        def destroy_identity(identity)
+          GroupSaml::Identity::DestroyService.new(identity).execute
+
+          true
+        rescue => e
+          logger.error(e.message)
+
+          false
+        end
+
+        def scim_not_found!(message:)
+          error!({ with: EE::Gitlab::Scim::NotFound }.merge(detail: message), 404)
+        end
+
+        def scim_error!(message:)
+          error!({ with: EE::Gitlab::Scim::Error }.merge(detail: message), 409)
+        end
+      end
+
       resource :Users do
         before do
           check_group_saml_configured
-          authenticate!
         end
 
         desc 'Returns 200 if authenticated'
         get do
-          group = find_group!(params[:group])
+          group = find_group(params[:group])
+
+          scim_error!(message: 'Missing filter params') unless params[:filter]
 
-          authorize_manage_saml!(group)
+          parsed_hash = EE::Gitlab::Scim::ParamsParser.new(params).to_hash
+          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: parsed_hash[:extern_uid])
 
           status 200
 
-          {} # Dummy, just used to verify the connection by IdPs at the moment
+          present identity || {}, with: ::EE::Gitlab::Scim::Users
         end
 
-        desc 'Removes a SAML user'
-        params do
-          requires :external_id, type: Integer, desc: 'The external ID of the member'
+        get ':id' do
+          group = find_group(params[:group])
+
+          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
+
+          scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
+
+          status 200
+
+          present identity, with: ::EE::Gitlab::Scim::User
         end
-        delete ":external_id" do
-          group = find_group!(params[:group])
 
-          authorize_manage_saml!(group)
+        # rubocop: disable CodeReuse/ActiveRecord
+        patch ':id' do
+          scim_error!(message: 'Missing ID') unless params[:id]
+
+          group = find_group(params[:group])
+
+          parser = EE::Gitlab::Scim::ParamsParser.new(params)
+          parsed_hash = parser.to_hash
+          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
+
+          scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
+
+          updated = if parser.deprovision_user?
+                      destroy_identity(identity)
+                    elsif parsed_hash[:extern_uid]
+                      identity.update(parsed_hash.slice(:extern_uid))
+                    else
+                      scim_error!(message: 'Email has already been taken') if parsed_hash[:email] &&
+                          User.by_any_email(parsed_hash[:email].downcase).where.not(id: identity.user.id).count > 0
+
+                      result = ::Users::UpdateService.new(identity.user,
+                                                          parsed_hash.except(:extern_uid, :provider)
+                                                            .merge(user: identity.user)).execute
+
+                      result[:status] == :success
+                    end
+
+          if updated
+            status 204
+
+            {}
+          else
+            scim_error!(message: "Error updating #{identity.user.name} with #{parsed_hash.inspect}")
+          end
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        desc 'Removes a SAML user'
+        delete ":id" do
+          scim_error!(message: 'Missing ID') unless params[:id]
+
+          group = find_group(params[:group])
+
+          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
 
-          user = User.find_by_email(params[:external_id])
+          scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
-          not_found!('User') unless user
+          status 204
 
-          linked_identity = GroupSamlIdentityFinder.new(user: user).find_linked(group: group)
+          scim_not_found!(message: "Resource #{params[:id]} not found") unless destroy_identity(identity)
 
-          GroupSaml::Identity::DestroyService.new(linked_identity).execute
+          {}
         end
       end
     end

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

@@ -21,7 +21,7 @@ module EE
 
         def initialize(params)
           @filter = params[:filter]
-          @operations = params[:operations]
+          @operations = params[:Operations]
         end
 
         def deprovision_user?

ee/spec/requests/api/scim_spec.rb

@@ -6,7 +6,6 @@ describe API::Scim do
   let(:user) { create(:user) }
   let(:identity) { create(:group_saml_identity, user: user) }
   let(:group) { identity.saml_provider.group }
-  let(:token) { create(:personal_access_token, user: user) }
 
   before do
     stub_licensed_features(group_saml: true)
@@ -15,10 +14,127 @@ describe API::Scim do
   end
 
   describe 'GET api/scim/v2/groups/:group/Users' do
-    it 'responds with a 200' do
-      get api("scim/v2/groups/#{group.full_path}/Users", user, oauth_access_token: token, version: '')
+    it 'responds with an error if there is no filter' do
+      get api("scim/v2/groups/#{group.full_path}/Users", user, version: '')
 
-      expect(response).to have_gitlab_http_status(200)
+      expect(response).to have_gitlab_http_status(409)
+    end
+
+    context 'existing user' do
+      it 'responds with 200' do
+        get api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"#{identity.extern_uid}\"", user, version: '')
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['Resources']).not_to be_empty
+        expect(json_response['totalResults']).to eq(1)
+      end
+    end
+
+    context 'no user' do
+      it 'responds with 200' do
+        get api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"nonexistent\"", user, version: '')
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['Resources']).to be_empty
+        expect(json_response['totalResults']).to eq(0)
+      end
+    end
+  end
+
+  describe 'GET api/scim/v2/groups/:group/Users/:id' do
+    it 'responds with 404 if there is no user' do
+      get api("scim/v2/groups/#{group.full_path}/Users/123", user, version: '')
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    context 'existing user' do
+      it 'responds with 200' do
+        get api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}", user, version: '')
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['id']).to eq(identity.extern_uid)
+      end
+    end
+  end
+
+  describe 'PATCH api/scim/v2/groups/:group/Users/:id' do
+    it 'responds with 404 if there is no user' do
+      patch api("scim/v2/groups/#{group.full_path}/Users/123", user, version: '')
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    context 'existing user' do
+      context 'extern UID' do
+        before do
+          params = { Operations: [{ 'op': 'Replace', 'path': 'id', 'value': 'new_uid' }] }.to_query
+
+          patch api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}", user, version: '')
+        end
+
+        it 'responds with 204' do
+          expect(response).to have_gitlab_http_status(204)
+        end
+
+        it 'updates the extern_uid' do
+          expect(identity.reload.extern_uid).to eq('new_uid')
+        end
+      end
+
+      context 'name' do
+        before do
+          params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query
+
+          patch api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}", user, version: '')
+        end
+
+        it 'responds with 204' do
+          expect(response).to have_gitlab_http_status(204)
+        end
+
+        it 'updates the name' do
+          expect(user.reload.name).to eq('new_name')
+        end
+      end
+
+      context 'Remove user' do
+        before do
+          params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
+
+          patch api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}", user, version: '')
+        end
+
+        it 'responds with 204' do
+          expect(response).to have_gitlab_http_status(204)
+        end
+
+        it 'removes the identity link' do
+          expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        end
+      end
+    end
+  end
+
+  describe 'DELETE/scim/v2/groups/:group/Users/:id' do
+    context 'existing user' do
+      before do
+        delete api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}", user, version: '')
+      end
+
+      it 'responds with 204' do
+        expect(response).to have_gitlab_http_status(204)
+      end
+
+      it 'removes the identity link' do
+        expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
+    it 'responds with 404 if there is no user' do
+      delete api("scim/v2/groups/#{group.full_path}/Users/123", user, version: '')
+
+      expect(response).to have_gitlab_http_status(404)
     end
   end
 end