Skip SSO GMA validation if project bot

Do not validate SSO or GMA enforcement if the user is a project bot

Change function name

Add changelog entry

Add back ampersand

Return user persisted check

Render alert when access token creation fails

Add spec for failure alert on unsuccessful creation

Add spec for access provisioning failure

Fix access provisioning spec

Fix bot not persisted spec

Add counts when token not created

Create EE spec file

EE spec file to test a EE feature flag even though there is no EE create
service file

Add newline to end of file

Clean up spec

Remove extra sidekiq inlien

Fix static analysis errors

Address MR review comments

Improve specs

Address MR review comments

Rename file, get specs passing, better specs

Add another context for SAML groups

Create a shared example

Move lets into individual contexts

Add specs for pass case

Remove extra new line

Revert "Add back ampersand"

This reverts commit 3aada357a573fb20e76f2d68d3b665a501037406.

Add ampersand back

Use destroy! since return value not checked

Update MR number in changelog

Move specs to CE file

Change spec names

Make spec titles more explicit

Remove extra new lines

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