Merge branch '11540-support-emails-as-id-in-scim' into 'master'

Resolve "Support emails as ID in SCIM"

Closes #11540

See merge request gitlab-org/gitlab-ee!14625

ee/changelogs/unreleased/11540-support-emails-as-id-in-scim.yml

+---
+title: Support emails as ID in SCIM
+merge_request: 14625
+author:
+type: fixed

ee/lib/api/scim.rb

@@ -5,6 +5,7 @@ module API
     prefix 'api/scim'
     version 'v2'
     content_type :json, 'application/scim+json'
+    USER_ID_REQUIREMENTS = { id: /.+/ }.freeze
 
     namespace 'groups/:group' do
       params do
@@ -107,7 +108,7 @@ module API
         desc 'Get a SAML user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
-        get ':id' do
+        get ':id', requirements: USER_ID_REQUIREMENTS do
           group = find_and_authenticate_group!(params[:group])
 
           identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
@@ -142,7 +143,7 @@ module API
         desc 'Updates a SAML user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
-        patch ':id' do
+        patch ':id', requirements: USER_ID_REQUIREMENTS do
           scim_error!(message: 'Missing ID') unless params[:id]
 
           group = find_and_authenticate_group!(params[:group])
@@ -164,7 +165,7 @@ module API
         desc 'Removes a SAML user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
-        delete ":id" do
+        delete ':id', requirements: USER_ID_REQUIREMENTS do
           scim_error!(message: 'Missing ID') unless params[:id]
 
           group = find_and_authenticate_group!(params[:group])

ee/spec/requests/api/scim_spec.rb

@@ -4,7 +4,6 @@ require 'spec_helper'
 
 describe API::Scim do
   let(:user) { create(:user) }
-  let(:identity) { create(:group_saml_identity, user: user) }
   let(:group) { identity.saml_provider.group }
   let(:scim_token) { create(:scim_oauth_access_token, group: group) }
 
@@ -14,216 +13,234 @@ describe API::Scim do
     group.add_owner(user)
   end
 
-  describe 'GET api/scim/v2/groups/:group/Users' do
-    context 'without token auth' do
-      it 'responds with 401' do
-        get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"#{identity.extern_uid}\"", token: false)
+  shared_examples 'SCIM API Endpoints' do
+    describe 'GET api/scim/v2/groups/:group/Users' do
+      context 'without token auth' do
+        it 'responds with 401' do
+          get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"#{identity.extern_uid}\"", token: false)
 
-        expect(response).to have_gitlab_http_status(401)
+          expect(response).to have_gitlab_http_status(401)
+        end
       end
-    end
 
-    it 'responds with an error if there is no filter' do
-      get scim_api("scim/v2/groups/#{group.full_path}/Users")
-
-      expect(response).to have_gitlab_http_status(412)
-    end
+      it 'responds with an error if there is no filter' do
+        get scim_api("scim/v2/groups/#{group.full_path}/Users")
 
-    context 'existing user' do
-      it 'responds with 200' do
-        get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"#{identity.extern_uid}\"")
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['Resources']).not_to be_empty
-        expect(json_response['totalResults']).to eq(1)
+        expect(response).to have_gitlab_http_status(412)
       end
-    end
 
-    context 'no user' do
-      it 'responds with 200' do
-        get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"nonexistent\"")
+      context 'existing user' do
+        it 'responds with 200' do
+          get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"#{identity.extern_uid}\"")
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['Resources']).to be_empty
-        expect(json_response['totalResults']).to eq(0)
+          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
-    end
-  end
-
-  describe 'GET api/scim/v2/groups/:group/Users/:id' do
-    it 'responds with 404 if there is no user' do
-      get scim_api("scim/v2/groups/#{group.full_path}/Users/123")
 
-      expect(response).to have_gitlab_http_status(404)
-    end
-
-    context 'existing user' do
-      it 'responds with 200' do
-        get scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
+      context 'no user' do
+        it 'responds with 200' do
+          get scim_api("scim/v2/groups/#{group.full_path}/Users?filter=id eq \"nonexistent\"")
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['id']).to eq(identity.extern_uid)
+          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
-  end
-
-  describe 'POST api/scim/v2/groups/:group/Users' do
-    let(:post_params) do
-      {
-        externalId: 'test_uid',
-        active: nil,
-        userName: 'username',
-        emails: [
-          { primary: true, type: 'work', value: 'work@example.com' }
-        ],
-        name: { formatted: 'Test Name', familyName: 'Name', givenName: 'Test' }
-      }.to_query
-    end
-
-    context 'without an existing user' do
-      let(:new_user) { User.find_by_email('work@example.com') }
-      let(:member) { GroupMember.find_by(user: new_user, group: group) }
-
-      before do
-        post scim_api("scim/v2/groups/#{group.full_path}/Users?params=#{post_params}")
-      end
 
-      it 'responds with 201' do
-        expect(response).to have_gitlab_http_status(201)
-      end
-
-      it 'has the user external ID' do
-        expect(json_response['id']).to eq('test_uid')
-      end
+    describe 'GET api/scim/v2/groups/:group/Users/:id' do
+      it 'responds with 404 if there is no user' do
+        get scim_api("scim/v2/groups/#{group.full_path}/Users/123")
 
-      it 'has the email' do
-        expect(json_response['emails'].first['value']).to eq('work@example.com')
+        expect(response).to have_gitlab_http_status(404)
       end
 
-      it 'created the identity' do
-        expect(Identity.find_by_extern_uid(:group_saml, 'test_uid')).not_to be_nil
-      end
-
-      it 'has the right saml provider' do
-        identity = Identity.find_by_extern_uid(:group_saml, 'test_uid')
+      context 'existing user' do
+        it 'responds with 200' do
+          get scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
 
-        expect(identity.saml_provider_id).to eq(group.saml_provider.id)
-      end
-
-      it 'created the user' do
-        expect(new_user).not_to be_nil
-      end
-
-      it 'created the right member' do
-        expect(member.access_level).to eq(::Gitlab::Access::GUEST)
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['id']).to eq(identity.extern_uid)
+        end
       end
     end
 
-    context 'existing user' do
-      before do
-        old_user = create(:user, email: 'work@example.com')
+    describe 'POST api/scim/v2/groups/:group/Users' do
+      let(:post_params) do
+        {
+          externalId: 'test_uid',
+          active: nil,
+          userName: 'username',
+          emails: [
+            { primary: true, type: 'work', value: 'work@example.com' }
+          ],
+          name: { formatted: 'Test Name', familyName: 'Name', givenName: 'Test' }
+        }.to_query
+      end
 
-        create(:group_saml_identity, user: old_user, extern_uid: 'test_uid')
-        group.add_guest(old_user)
+      context 'without an existing user' do
+        let(:new_user) { User.find_by_email('work@example.com') }
+        let(:member) { GroupMember.find_by(user: new_user, group: group) }
 
-        post scim_api("scim/v2/groups/#{group.full_path}/Users?params=#{post_params}")
-      end
+        before do
+          post scim_api("scim/v2/groups/#{group.full_path}/Users?params=#{post_params}")
+        end
 
-      it 'responds with 201' do
-        expect(response).to have_gitlab_http_status(201)
-      end
+        it 'responds with 201' do
+          expect(response).to have_gitlab_http_status(201)
+        end
 
-      it 'has the user external ID' do
-        expect(json_response['id']).to eq('test_uid')
-      end
-    end
-  end
+        it 'has the user external ID' do
+          expect(json_response['id']).to eq('test_uid')
+        end
 
-  describe 'PATCH api/scim/v2/groups/:group/Users/:id' do
-    it 'responds with 404 if there is no user' do
-      patch scim_api("scim/v2/groups/#{group.full_path}/Users/123")
+        it 'has the email' do
+          expect(json_response['emails'].first['value']).to eq('work@example.com')
+        end
 
-      expect(response).to have_gitlab_http_status(404)
-    end
+        it 'created the identity' do
+          expect(Identity.find_by_extern_uid(:group_saml, 'test_uid')).not_to be_nil
+        end
 
-    context 'existing user' do
-      context 'extern UID' do
-        before do
-          params = { Operations: [{ 'op': 'Replace', 'path': 'id', 'value': 'new_uid' }] }.to_query
+        it 'has the right saml provider' do
+          identity = Identity.find_by_extern_uid(:group_saml, 'test_uid')
 
-          patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          expect(identity.saml_provider_id).to eq(group.saml_provider.id)
         end
 
-        it 'responds with 204' do
-          expect(response).to have_gitlab_http_status(204)
+        it 'created the user' do
+          expect(new_user).not_to be_nil
         end
 
-        it 'updates the extern_uid' do
-          expect(identity.reload.extern_uid).to eq('new_uid')
+        it 'created the right member' do
+          expect(member.access_level).to eq(::Gitlab::Access::GUEST)
         end
       end
 
-      context 'name' do
+      context 'existing user' do
         before do
-          params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query
+          old_user = create(:user, email: 'work@example.com')
 
-          patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
-        end
+          create(:group_saml_identity, user: old_user, extern_uid: 'test_uid')
+          group.add_guest(old_user)
 
-        it 'responds with 204' do
-          expect(response).to have_gitlab_http_status(204)
+          post scim_api("scim/v2/groups/#{group.full_path}/Users?params=#{post_params}")
         end
 
-        it 'updates the name' do
-          expect(user.reload.name).to eq('new_name')
+        it 'responds with 201' do
+          expect(response).to have_gitlab_http_status(201)
         end
 
-        it 'responds with an empty response' do
-          expect(json_response).to eq({})
+        it 'has the user external ID' do
+          expect(json_response['id']).to eq('test_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 scim_api("scim/v2/groups/#{group.full_path}/Users/123")
+
+        expect(response).to have_gitlab_http_status(404)
+      end
 
-      context 'email' do
-        context 'non existent email' do
+      context 'existing user' do
+        context 'extern UID' do
           before do
-            params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+            params = { Operations: [{ 'op': 'Replace', 'path': 'id', 'value': 'new_uid' }] }.to_query
 
             patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
           end
 
-          it 'updates the email' do
-            expect(user.reload.unconfirmed_email).to eq('new@mail.com')
+          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 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(204)
           end
+
+          it 'updates the name' do
+            expect(user.reload.name).to eq('new_name')
+          end
+
+          it 'responds with an empty response' do
+            expect(json_response).to eq({})
+          end
         end
 
-        context 'existent email' do
-          before do
-            create(:user, email: 'new@mail.com')
+        context 'email' do
+          context 'non existent email' do
+            before do
+              params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+
+              patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+            end
+
+            it 'updates the email' do
+              expect(user.reload.unconfirmed_email).to eq('new@mail.com')
+            end
+
+            it 'responds with 204' do
+              expect(response).to have_gitlab_http_status(204)
+            end
+          end
 
-            params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+          context 'existent email' do
+            before do
+              create(:user, email: 'new@mail.com')
+
+              params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+
+              patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+            end
+
+            it 'does not update a duplicated email' do
+              expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
+            end
+
+            it 'responds with 209' do
+              expect(response).to have_gitlab_http_status(409)
+            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 'does not update a duplicated email' do
-            expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
+          it 'responds with 204' do
+            expect(response).to have_gitlab_http_status(204)
           end
 
-          it 'responds with 209' do
-            expect(response).to have_gitlab_http_status(409)
+          it 'removes the identity link' do
+            expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
           end
         end
       end
+    end
 
-      context 'Remove user' do
+    describe 'DELETE /scim/v2/groups/:group/Users/:id' do
+      context 'existing 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}")
+          delete scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
         end
 
         it 'responds with 204' do
@@ -233,37 +250,33 @@ describe API::Scim do
         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 scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
+        it 'responds with an empty response' do
+          expect(json_response).to eq({})
+        end
       end
 
-      it 'responds with 204' do
-        expect(response).to have_gitlab_http_status(204)
-      end
+      it 'responds with 404 if there is no user' do
+        delete scim_api("scim/v2/groups/#{group.full_path}/Users/123")
 
-      it 'removes the identity link' do
-        expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        expect(response).to have_gitlab_http_status(404)
       end
+    end
 
-      it 'responds with an empty response' do
-        expect(json_response).to eq({})
-      end
+    def scim_api(url, token: true)
+      api(url, user, version: '', oauth_access_token: token ? scim_token : nil)
     end
+  end
 
-    it 'responds with 404 if there is no user' do
-      delete scim_api("scim/v2/groups/#{group.full_path}/Users/123")
+  context 'user with an alphanumeric extern_uid' do
+    let(:identity) { create(:group_saml_identity, user: user, extern_uid: generate(:username)) }
 
-      expect(response).to have_gitlab_http_status(404)
-    end
+    it_behaves_like 'SCIM API Endpoints'
   end
 
-  def scim_api(url, token: true)
-    api(url, user, version: '', oauth_access_token: token ? scim_token : nil)
+  context 'user with an email extern_uid' do
+    let(:identity) { create(:group_saml_identity, user: user, extern_uid: user.email) }
+
+    it_behaves_like 'SCIM API Endpoints'
   end
 end