Adds instance admins to organization_users as owners

- backfill to ensure all instance admins are added
  as owners of the default organization.
- this ensures that instance admins have access
  to the default organization for proper management

Changelog: added

app/models/organizations/organization_user.rb

@@ -17,15 +17,32 @@ class OrganizationUser < ApplicationRecord
 
     scope :owners, -> { where(access_level: Gitlab::Access::OWNER) }
 
-    def self.create_default_organization_record_for(user_id, access_level)
+    def self.create_default_organization_record_for(user_id, user_is_admin:)
       Organizations::OrganizationUser.upsert(
         {
           organization_id: Organizations::Organization::DEFAULT_ORGANIZATION_ID,
           user_id: user_id,
-          access_level: access_level
+          access_level: default_organization_access_level(user_is_admin: user_is_admin)
         },
         unique_by: [:organization_id, :user_id]
       )
     end
+
+    def self.update_default_organization_record_for(user_id, user_is_admin:)
+      find_or_initialize_by(
+        user_id: user_id, organization_id: Organizations::Organization::DEFAULT_ORGANIZATION_ID
+      ).tap do |record|
+        record.access_level = default_organization_access_level(user_is_admin: user_is_admin)
+        record.save
+      end
+    end
+
+    def self.default_organization_access_level(user_is_admin: false)
+      if user_is_admin
+        :owner
+      else
+        :default
+      end
+    end
   end
 end

app/models/user.rb

@@ -374,6 +374,7 @@ def update_tracked_fields!(request)
   end
 
   after_create_commit :create_default_organization_user
+  after_update_commit :update_default_organization_user, if: -> { saved_change_to_admin }
 
   # User's Layout preference
   enum layout: { fixed: 0, fluid: 1 }
@@ -2607,20 +2608,19 @@ def prefix_for_feed_token
   def create_default_organization_user
     return unless Feature.enabled?(:update_default_organization_users, self, type: :gitlab_com_derisk)
 
-    organization_access_level = if admin?
-                                  :owner
-                                else
-                                  :default
-                                end
+    Organizations::OrganizationUser.create_default_organization_record_for(id, user_is_admin: admin?)
+  end
+
+  def update_default_organization_user
+    return unless Feature.enabled?(:update_default_organization_users, self, type: :gitlab_com_derisk)
 
-    Organizations::OrganizationUser
-      .create_default_organization_record_for(id, organization_access_level)
+    Organizations::OrganizationUser.update_default_organization_record_for(id, user_is_admin: admin?)
   end
 
-  # method overriden in EE
+  # method overridden in EE
   def audit_lock_access(reason: nil); end
 
-  # method overriden in EE
+  # method overridden in EE
   def audit_unlock_access(author: self); end
 end
 

db/docs/batched_background_migrations/backfill_default_organization_owners.yml

+---
+migration_job_name: BackfillDefaultOrganizationOwners
+description: Populates organization_users with instance admins to the default organization.
+feature_category: cell
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141297
+milestone: '16.8'
+queued_migration_version: 20240108182342
+finalize_after: '2024-02-15'
+finalized_by: # version of the migration that ensured this bbm

db/post_migrate/20240108182342_queue_backfill_default_organization_owners.rb

+# frozen_string_literal: true
+
+class QueueBackfillDefaultOrganizationOwners < Gitlab::Database::Migration[2.2]
+  milestone '16.9'
+
+  MIGRATION = 'BackfillDefaultOrganizationOwners'
+  DELAY_INTERVAL = 2.minutes
+  BATCH_SIZE = 3_000
+  SUB_BATCH_SIZE = 250
+  MAX_BATCH_SIZE = 10_000
+
+  restrict_gitlab_migration gitlab_schema: :gitlab_main
+
+  def up
+    queue_batched_background_migration(
+      MIGRATION,
+      :users,
+      :id,
+      job_interval: DELAY_INTERVAL,
+      batch_size: BATCH_SIZE,
+      sub_batch_size: SUB_BATCH_SIZE,
+      max_batch_size: MAX_BATCH_SIZE
+    )
+  end
+
+  def down
+    delete_batched_background_migration(MIGRATION, :users, :id, [])
+  end
+end

db/schema_migrations/20240108182342

+487a17e2954ac054beae5fc9a1125f11bc179f13b7b3788e824248f81734048d
\ No newline at end of file

lib/gitlab/background_migration/backfill_default_organization_owners.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class BackfillDefaultOrganizationOwners < BatchedMigrationJob
+      operation_name :backfill_default_organization_owners # This is used as the key on collecting metrics
+      scope_to ->(relation) { relation.where(admin: true) }
+      feature_category :cell
+
+      module Organizations
+        class OrganizationUser < ApplicationRecord
+          self.table_name = 'organization_users'
+          self.inheritance_column = :_type_disabled
+        end
+      end
+
+      def perform
+        each_sub_batch do |sub_batch|
+          organization_users_attributes = sub_batch.map do |user|
+            {
+              user_id: user.id,
+              organization_id: ::Organizations::Organization::DEFAULT_ORGANIZATION_ID,
+              access_level: Gitlab::Access::OWNER
+            }
+          end
+
+          Organizations::OrganizationUser.upsert_all(
+            organization_users_attributes,
+            returning: false,
+            unique_by: [:organization_id, :user_id]
+          )
+        end
+      end
+    end
+  end
+end

spec/factories/users.rb

@@ -24,13 +24,7 @@
     end
 
     trait :without_default_org do
-      after(:build) do
-        User.skip_callback(:commit, :after, :create_default_organization_user)
-      end
-
-      after(:create) do
-        User.set_callback(:commit, :after, :create_default_organization_user)
-      end
+      before(:create) { |user| user.define_singleton_method(:create_default_organization_user) { nil } }
     end
 
     trait :with_namespace do

spec/lib/gitlab/background_migration/backfill_default_organization_owners_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::BackfillDefaultOrganizationOwners, schema: 20240108182342, feature_category: :cell do
+  let(:organization_users) { table(:organization_users) }
+  let(:users) { table(:users) }
+
+  let!(:first_admin) { users.create!(name: 'first', email: 'first_admin@example.com', projects_limit: 1, admin: true) }
+  let!(:last_admin) { users.create!(name: 'last', email: 'last_admin@example.com', projects_limit: 1, admin: true) }
+  let!(:user) { users.create!(name: 'user', email: 'user@example.com', projects_limit: 1) }
+
+  subject(:migration) do
+    described_class.new(
+      start_id: first_admin.id,
+      end_id: user.id,
+      batch_table: :users,
+      batch_column: :id,
+      sub_batch_size: 100,
+      pause_ms: 0,
+      connection: ApplicationRecord.connection
+    )
+  end
+
+  describe '#perform' do
+    context 'with no entries for admin user in organization_users' do
+      it 'adds admins correctly with the default organization to organization_users' do
+        expect(organization_users.count).to eq(0)
+
+        expect { migration.perform }.to change { organization_users.count }.by(2)
+
+        expect(organization_user_as_owner_exists?(first_admin.id)).to be(true)
+        expect(organization_user_as_owner_exists?(last_admin.id)).to be(true)
+      end
+    end
+
+    context 'when admin already exists in organization_users as a default user' do
+      before do
+        organization_users.create!(
+          organization_id: Organizations::Organization::DEFAULT_ORGANIZATION_ID,
+          user_id: first_admin.id,
+          access_level: Gitlab::Access::GUEST
+        )
+      end
+
+      it 'updates the organization_users entry to owner' do
+        expect(organization_users.count).to eq(1)
+
+        expect { migration.perform }.to change { organization_users.count }.by(1)
+
+        expect(organization_user_as_owner_exists?(first_admin.id)).to be(true)
+        expect(organization_user_as_owner_exists?(last_admin.id)).to be(true)
+      end
+    end
+  end
+
+  def organization_user_as_owner_exists?(user_id)
+    organization_users.exists?(
+      organization_id: Organizations::Organization::DEFAULT_ORGANIZATION_ID,
+      user_id: user_id,
+      access_level: Gitlab::Access::OWNER
+    )
+  end
+end

spec/migrations/20240108182342_queue_backfill_default_organization_owners_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe QueueBackfillDefaultOrganizationOwners, feature_category: :cell do
+  let!(:batched_migration) { described_class::MIGRATION }
+
+  it 'schedules a new batched migration' do
+    reversible_migration do |migration|
+      migration.before -> {
+        expect(batched_migration).not_to have_scheduled_batched_migration
+      }
+
+      migration.after -> {
+        expect(batched_migration).to have_scheduled_batched_migration(
+          table_name: :users,
+          column_name: :id,
+          interval: described_class::DELAY_INTERVAL,
+          batch_size: described_class::BATCH_SIZE,
+          sub_batch_size: described_class::SUB_BATCH_SIZE,
+          max_batch_size: described_class::MAX_BATCH_SIZE
+        )
+      }
+    end
+  end
+end

spec/models/organizations/organization_user_spec.rb

@@ -48,10 +48,12 @@
   describe '.create_default_organization_record_for' do
     let_it_be(:default_organization) { create(:organization, :default) }
     let_it_be(:user) { create(:user, :without_default_org) }
-    let(:access_level) { :default }
+    let(:user_is_admin) { false }
     let(:user_id) { user.id }
 
-    subject(:create_entry) { described_class.create_default_organization_record_for(user_id, access_level) }
+    subject(:create_entry) do
+      described_class.create_default_organization_record_for(user_id, user_is_admin: user_is_admin)
+    end
 
     context 'when creating as as default user' do
       it 'creates record with correct attributes' do
@@ -61,7 +63,7 @@
     end
 
     context 'when creating as an owner' do
-      let(:access_level) { :owner }
+      let(:user_is_admin) { true }
 
       it 'creates record with correct attributes' do
         expect { create_entry }.to change { described_class.count }.by(1)
@@ -77,7 +79,7 @@
       end
 
       context 'when access_level changes' do
-        let(:access_level) { :owner }
+        let(:user_is_admin) { true }
 
         it 'changes access_level on the existing record' do
           expect(default_organization.owner?(user)).to be(false)
@@ -89,20 +91,74 @@
       end
     end
 
-    context 'when creating with invalid access_level' do
-      let(:access_level) { :foo }
+    context 'when creating with invalid user_id' do
+      let(:user_id) { nil }
 
       it 'raises and error' do
         expect { create_entry }.to raise_error(ActiveRecord::NotNullViolation)
       end
     end
+  end
+
+  describe '.update_default_organization_record_for' do
+    let_it_be(:default_organization) { create(:organization, :default) }
+    let_it_be(:user) { create(:user, :without_default_org) }
+    let_it_be(:user_id) { user.id }
+    let(:user_is_admin) { false }
+
+    subject(:update_default_organization_record) do
+      described_class.update_default_organization_record_for(user_id, user_is_admin: user_is_admin)
+    end
+
+    context 'when record does not exist yet' do
+      it 'creates record with correct attributes' do
+        expect { update_default_organization_record }.to change { described_class.count }.by(1)
+        expect(default_organization.user?(user)).to be(true)
+      end
+    end
+
+    context 'when entry already exists' do
+      let_it_be(:organization_user) { create(:organization_user, user: user, organization: default_organization) }
+
+      it 'does not create or update existing record' do
+        expect { update_default_organization_record }.not_to change { described_class.count }
+      end
+
+      context 'when access_level changes' do
+        let(:user_is_admin) { true }
+
+        it 'changes access_level on the existing record' do
+          expect(default_organization.owner?(user)).to be(false)
+
+          expect { update_default_organization_record }.not_to change { described_class.count }
+
+          expect(default_organization.owner?(user)).to be(true)
+        end
+      end
+    end
 
     context 'when creating with invalid user_id' do
       let(:user_id) { nil }
 
-      it 'raises and error' do
-        expect { create_entry }.to raise_error(ActiveRecord::NotNullViolation)
+      it 'does not add a new record' do
+        expect { update_default_organization_record }.not_to change { described_class.count }
       end
     end
   end
+
+  describe '.default_organization_access_level' do
+    let(:user_is_admin) { true }
+
+    subject { described_class.default_organization_access_level(user_is_admin: user_is_admin) }
+
+    context 'when user is admin' do
+      it { is_expected.to eq(:owner) }
+    end
+
+    context 'when user is not admin' do
+      let(:user_is_admin) { false }
+
+      it { is_expected.to eq(:default) }
+    end
+  end
 end

spec/models/user_spec.rb

@@ -1755,6 +1755,53 @@
     end
   end
 
+  context 'when after_update_commit :update_default_organization_user on default organization' do
+    let_it_be(:default_organization) { create(:organization, :default) }
+
+    context 'when user is changed to an instance admin' do
+      let_it_be(:user) { create(:user) }
+
+      it 'changes user to owner in the organization' do
+        expect(default_organization.owner?(user)).to be(false)
+
+        expect { user.update!(admin: true) }.not_to change { Organizations::OrganizationUser.count }
+        expect(default_organization.owner?(user)).to be(true)
+      end
+
+      context 'when non admin attribute is updated' do
+        it 'does not change the organization_user' do
+          expect(default_organization.owner?(user)).to be(false)
+
+          expect { user.update!(name: 'Bob') }.not_to change { Organizations::OrganizationUser.count }
+          expect(default_organization.owner?(user)).to be(false)
+        end
+      end
+    end
+
+    context 'when user is changed from admin to regular user' do
+      let_it_be(:user) { create(:admin) }
+
+      it 'changes user to default access_level in organization' do
+        expect(default_organization.owner?(user)).to be(true)
+
+        expect { user.update!(admin: false) }.not_to change { Organizations::OrganizationUser.count }
+        expect(default_organization.owner?(user)).to be(false)
+        expect(default_organization.user?(user)).to be(true)
+      end
+    end
+
+    context 'when user did not already exist in the default organization' do
+      let_it_be(:user) { create(:user, :without_default_org) }
+
+      it 'changes user to owner in the organization' do
+        expect(default_organization.user?(user)).to be(false)
+
+        expect { user.update!(admin: true) }.to change { Organizations::OrganizationUser.count }
+        expect(default_organization.owner?(user)).to be(true)
+      end
+    end
+  end
+
   context 'when after_create_commit :create_default_organization_user on default organization' do
     let_it_be(:default_organization) { create(:organization, :default) }
     let(:user) { create(:user) }