Merge branch 'security-fix-privelege-escalation-from-external-to-internal' into 'master'

Inherit user external status while creating project bots

See merge request gitlab-org/security/gitlab!1652

app/services/resource_access_tokens/create_service.rb

@@ -16,6 +16,8 @@ module ResourceAccessTokens
 
       return error(user.errors.full_messages.to_sentence) unless user.persisted?
 
+      user.update!(external: true) if current_user.external?
+
       access_level = params[:access_level] || Gitlab::Access::MAINTAINER
       member = create_membership(resource, user, access_level)
 

db/post_migrate/20210811122206_update_external_project_bots.rb

+# frozen_string_literal: true
+
+class UpdateExternalProjectBots < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+  end
+
+  disable_ddl_transaction!
+
+  TMP_INDEX_NAME = 'tmp_idx_update_external_project_bots'
+
+  def up
+    add_concurrent_index('users', 'id', name: TMP_INDEX_NAME, where: 'external = true')
+
+    ids = ActiveRecord::Base.connection
+                            .execute("SELECT u.id FROM users u JOIN users u2 on u2.id = u.created_by_id WHERE u.user_type = 6 AND u2.external = true")
+                            .map { |result| result['id'] }
+
+    ids.each_slice(10) do |group|
+      UpdateExternalProjectBots::User.where(id: group).update_all(external: true)
+    end
+
+    remove_concurrent_index_by_name('users', TMP_INDEX_NAME)
+  end
+
+  def down
+    remove_concurrent_index_by_name('users', TMP_INDEX_NAME) if index_exists_by_name?('users', TMP_INDEX_NAME)
+
+    # This migration is irreversible
+  end
+end

db/schema_migrations/20210811122206

+f6f5e081672fb42adde980fa12f696f5d8fd11921ee52c1472b3d745bb11a5ff
\ No newline at end of file

spec/migrations/20210811122206_update_external_project_bots_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!('update_external_project_bots')
+
+RSpec.describe UpdateExternalProjectBots, :migration do
+  def create_user(**extra_options)
+    defaults = { projects_limit: 0, email: "#{extra_options[:username]}@example.com" }
+
+    table(:users).create!(defaults.merge(extra_options))
+  end
+
+  it 'sets bot users as external if were created by external users' do
+    internal_user = create_user(username: 'foo')
+    external_user = create_user(username: 'bar', external: true)
+
+    internal_project_bot = create_user(username: 'foo2', user_type: 6, created_by_id: internal_user.id, external: false)
+    external_project_bot = create_user(username: 'bar2', user_type: 6, created_by_id: external_user.id, external: false)
+
+    migrate!
+
+    expect(table(:users).find(internal_project_bot.id).external).to eq false
+    expect(table(:users).find(external_project_bot.id).external).to eq true
+  end
+end

spec/services/resource_access_tokens/create_service_spec.rb

@@ -110,6 +110,18 @@ RSpec.describe ResourceAccessTokens::CreateService do
             expect(resource.members.developers.map(&:user_id)).to include(bot_user.id)
           end
         end
+
+        context 'when user is external' do
+          let(:user) { create(:user, :external) }
+
+          before do
+            project.add_maintainer(user)
+          end
+
+          it 'creates resource bot user with external status' do
+            expect(subject.payload[:access_token].user.external).to eq true
+          end
+        end
       end
 
       context 'personal access token' do