Merge branch...

Merge branch '271635-new-project-access-tokens-do-not-show-in-active-project-access-tokens' into 'master'

Skip SSO/GMA validation when creating project access token

See merge request gitlab-org/gitlab!46257

app/controllers/projects/settings/access_tokens_controller.rb

@@ -23,7 +23,7 @@ module Projects
 
           redirect_to namespace_project_settings_access_tokens_path, notice: _("Your new project access token has been created.")
         else
-          render :index
+          redirect_to namespace_project_settings_access_tokens_path, alert: _("Failed to create new project access token: %{token_response_message}") % { token_response_message: token_response.message }
         end
       end
 

app/services/resource_access_tokens/create_service.rb

@@ -15,13 +15,20 @@ module ResourceAccessTokens
       user = create_user
 
       return error(user.errors.full_messages.to_sentence) unless user.persisted?
-      return error("Failed to provide maintainer access") unless provision_access(resource, user)
+
+      member = create_membership(resource, user)
+
+      unless member.persisted?
+        delete_failed_user(user)
+        return error("Could not provision maintainer access to project access token")
+      end
 
       token_response = create_personal_access_token(user)
 
       if token_response.success?
         success(token_response.payload[:personal_access_token])
       else
+        delete_failed_user(user)
         error(token_response.message)
       end
     end
@@ -43,6 +50,10 @@ module ResourceAccessTokens
       Users::CreateService.new(current_user, default_user_params).execute(skip_authorization: true)
     end
 
+    def delete_failed_user(user)
+      DeleteUserWorker.perform_async(current_user.id, user.id, hard_delete: true, skip_authorization: true)
+    end
+
     def default_user_params
       {
         name: params[:name] || "#{resource.name.to_s.humanize} bot",
@@ -88,7 +99,7 @@ module ResourceAccessTokens
       Gitlab::Auth.resource_bot_scopes
     end
 
-    def provision_access(resource, user)
+    def create_membership(resource, user)
       resource.add_user(user, :maintainer, expires_at: params[:expires_at])
     end
 

changelogs/unreleased/271635-new-project-access-tokens-do-not-show-in-active-project-access-tok.yml

+---
+title: Skip GMA and SSO validation when creating project access tokens for project bots
+merge_request: 46257
+author:
+type: fixed

ee/app/models/ee/project_member.rb

@@ -7,8 +7,8 @@ module EE
     prepended do
       extend ::Gitlab::Utils::Override
 
-      validate :sso_enforcement, if: :group
-      validate :gma_enforcement, if: :group
+      validate :sso_enforcement, if: :group, unless: :project_bot
+      validate :gma_enforcement, if: :group, unless: :project_bot
 
       before_destroy :delete_member_branch_protection
     end
@@ -17,6 +17,10 @@ module EE
       source&.group
     end
 
+    def project_bot
+      user&.project_bot?
+    end
+
     def delete_member_branch_protection
       if user.present? && project.present?
         project.protected_branches.merge_access_by_user(user).destroy_all # rubocop: disable Cop/DestroyAll

ee/spec/models/project_member_spec.rb

@@ -21,23 +21,67 @@ RSpec.describe ProjectMember do
         stub_licensed_features(group_saml: true)
       end
 
-      it 'allows adding the project member' do
+      it 'allows adding a user linked to the GMA account as project member' do
         user = create(:user, :group_managed, managing_group: group)
         member = entity.add_developer(user)
 
         expect(member).to be_valid
       end
 
-      it 'does not add the the project member' do
+      it 'does not allow adding a user not linked to the GMA account as project member' do
         member = entity.add_developer(create(:user))
 
         expect(member).not_to be_valid
         expect(member.errors.messages[:user]).to include('is not in the group enforcing Group Managed Account')
       end
+
+      it 'allows adding a project bot' do
+        member = entity.add_developer(create(:user, :project_bot))
+
+        expect(member).to be_valid
+      end
+    end
+
+    context "for SSO enforced groups" do
+      let(:group) { create(:group, :private) }
+      let!(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+      let(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+
+      before do
+        stub_licensed_features(group_saml: true)
+      end
+
+      it 'allows adding a user linked to the SAML account as project member' do
+        sso_user = identity.user
+        member = entity.add_developer(sso_user)
+
+        expect(member).to be_valid
+      end
+
+      it 'does not allow adding a user not linked to the SAML account as a project member' do
+        member = entity.add_developer(create(:user))
+
+        expect(member).not_to be_valid
+        expect(member.errors.messages[:user]).to include('is not linked to a SAML account')
+      end
+
+      it 'allows adding a project bot' do
+        member = entity.add_developer(create(:user, :project_bot))
+
+        expect(member).to be_valid
+      end
     end
 
     context 'enforced group managed account disabled' do
-      it 'allows adding the group member' do
+      it 'allows adding any user as project member' do
+        member = entity.add_developer(create(:user))
+
+        expect(member).to be_valid
+      end
+    end
+
+    context 'enforced SSO disabled' do
+      it 'allows adding any user as project member' do
         member = entity.add_developer(create(:user))
 
         expect(member).to be_valid

ee/spec/services/resource_access_tokens/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ResourceAccessTokens::CreateService do
+  subject { described_class.new(user, resource).execute }
+
+  let_it_be(:user) { create(:user) }
+
+  shared_examples 'token creation succeeds' do
+    let(:resource) { create(:project, group: group)}
+
+    before do
+      resource.add_maintainer(user)
+    end
+
+    it 'does not cause an error' do
+      response = subject
+
+      expect(response.error?).to be false
+    end
+
+    it 'adds the project bot as a member' do
+      expect { subject }.to change { resource.members.count }.by(1)
+    end
+
+    it 'creates a project bot user' do
+      expect { subject }.to change { User.bots.count }.by(1)
+    end
+  end
+
+  describe '#execute' do
+    context 'with enforced group managed account enabled' do
+      let(:group) { create(:group_with_managed_accounts, :private) }
+      let(:user) { create(:user, :group_managed, managing_group: group) }
+
+      before do
+        stub_feature_flags(group_managed_accounts: true)
+        stub_licensed_features(group_saml: true)
+      end
+
+      it_behaves_like 'token creation succeeds'
+    end
+
+    context "for SAML enabled groups" do
+      let(:group) { create(:group, :private) }
+      let!(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+      let(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+      let(:user) { identity.user }
+
+      before do
+        stub_licensed_features(group_saml: true)
+      end
+
+      it_behaves_like 'token creation succeeds'
+    end
+  end
+end

locale/gitlab.pot

@@ -11072,6 +11072,9 @@ msgstr ""
 msgid "Failed to create import label for jira import."
 msgstr ""
 
+msgid "Failed to create new project access token: %{token_response_message}"
+msgstr ""
+
 msgid "Failed to create repository"
 msgstr ""
 

spec/services/resource_access_tokens/create_service_spec.rb

@@ -11,16 +11,15 @@ RSpec.describe ResourceAccessTokens::CreateService do
 
   describe '#execute' do
     # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046
-    shared_examples 'fails when user does not have the permission to create a Resource Bot' do
-      before_all do
-        resource.add_developer(user)
-      end
+    shared_examples 'token creation fails' do
+      let(:resource) { create(:project)}
 
-      it 'returns error' do
-        response = subject
+      it 'does not add the project bot as a member' do
+        expect { subject }.not_to change { resource.members.count }
+      end
 
-        expect(response.error?).to be true
-        expect(response.message).to eq("User does not have permission to create #{resource_type} Access Token")
+      it 'immediately destroys the bot user if one was created', :sidekiq_inline do
+        expect { subject }.not_to change { User.bots.count }
       end
     end
 
@@ -154,24 +153,36 @@ RSpec.describe ResourceAccessTokens::CreateService do
           context 'when invalid scope is passed' do
             let_it_be(:params) { { scopes: [:invalid_scope] } }
 
-            it 'returns error' do
+            it_behaves_like 'token creation fails'
+
+            it 'returns the scope error message' do
               response = subject
 
               expect(response.error?).to be true
+              expect(response.errors).to include("Scopes can only contain available scopes")
             end
           end
         end
-      end
 
-      context 'when access provisioning fails' do
-        before do
-          allow(resource).to receive(:add_user).and_return(nil)
-        end
+        context "when access provisioning fails" do
+          let_it_be(:bot_user) { create(:user, :project_bot) }
+          let(:unpersisted_member) { build(:project_member, source: resource, user: bot_user) }
 
-        it 'returns error' do
-          response = subject
+          before do
+            allow_next_instance_of(ResourceAccessTokens::CreateService) do |service|
+              allow(service).to receive(:create_user).and_return(bot_user)
+              allow(service).to receive(:create_membership).and_return(unpersisted_member)
+            end
+          end
 
-          expect(response.error?).to be true
+          it_behaves_like 'token creation fails'
+
+          it 'returns the provisioning error message' do
+            response = subject
+
+            expect(response.error?).to be true
+            expect(response.errors).to include("Could not provision maintainer access to project access token")
+          end
         end
       end
     end
@@ -180,7 +191,16 @@ RSpec.describe ResourceAccessTokens::CreateService do
       let_it_be(:resource_type) { 'project' }
       let_it_be(:resource) { project }
 
-      it_behaves_like 'fails when user does not have the permission to create a Resource Bot'
+      context 'when user does not have permission to create a resource bot' do
+        it_behaves_like 'token creation fails'
+
+        it 'returns the permission error message' do
+          response = subject
+
+          expect(response.error?).to be true
+          expect(response.errors).to include("User does not have permission to create #{resource_type} Access Token")
+        end
+      end
 
       context 'user with valid permission' do
         before_all do

spec/support/shared_examples/controllers/access_tokens_controller_shared_examples.rb

@@ -73,7 +73,23 @@ RSpec.shared_examples 'project access tokens available #create' do
       end
     end
 
-    it { expect(subject).to render_template(:index) }
+    it 'does not create the token' do
+      expect { subject }.not_to change { PersonalAccessToken.count }
+    end
+
+    it 'does not add the project bot as a member' do
+      expect { subject }.not_to change { Member.count }
+    end
+
+    it 'does not create the project bot user' do
+      expect { subject }.not_to change { User.count }
+    end
+
+    it 'shows a failure alert' do
+      subject
+
+      expect(response.flash[:alert]).to match("Failed to create new project access token: Failed!")
+    end
   end
 end