Merge branch...

Merge branch '220433-user-stuck-in-2fa-setup-page-even-if-group-disable-2fa-enforce-migration' into 'master'

Resolve "User stuck in 2FA setup page even if group disable 2FA enforce"

See merge request gitlab-org/gitlab!47193

changelogs/unreleased/220433-user-stuck-in-2fa-setup-page-even-if-group-disable-2fa-enforce-mig.yml

+---
+title: Add migration that updated users that don't need to have 2fa established
+merge_request: 47193
+author:
+type: fixed

db/post_migrate/20201030121314_schedule_update_existing_users_that_require_two_factor_auth.rb

+# # frozen_string_literal: true
+
+class ScheduleUpdateExistingUsersThatRequireTwoFactorAuth < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  MIGRATION = 'UpdateExistingUsersThatRequireTwoFactorAuth'
+  DELAY_INTERVAL = 2.minutes
+  BATCH_SIZE = 1000
+  INDEX_NAME = 'index_users_on_require_two_factor_authentication_from_group'
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'users'
+  end
+
+  def up
+    add_concurrent_index :users,
+                         :require_two_factor_authentication_from_group,
+                         where: 'require_two_factor_authentication_from_group = TRUE',
+                         name: INDEX_NAME
+
+    relation = User.where(require_two_factor_authentication_from_group: true)
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      relation, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    remove_concurrent_index_by_name :users, INDEX_NAME
+  end
+end

db/schema_migrations/20201030121314

+4875c1def91676d73f14c2fbff9318fc4ab1f26535503fd9700044b687e9714e
\ No newline at end of file

db/structure.sql

@@ -22203,6 +22203,8 @@ CREATE INDEX index_users_on_name_trigram ON users USING gin (name gin_trgm_ops);
 
 CREATE INDEX index_users_on_public_email ON users USING btree (public_email) WHERE ((public_email)::text <> ''::text);
 
+CREATE INDEX index_users_on_require_two_factor_authentication_from_group ON users USING btree (require_two_factor_authentication_from_group);
+
 CREATE UNIQUE INDEX index_users_on_reset_password_token ON users USING btree (reset_password_token);
 
 CREATE INDEX index_users_on_state ON users USING btree (state);

lib/gitlab/background_migration/update_existing_users_that_require_two_factor_auth.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class UpdateExistingUsersThatRequireTwoFactorAuth # rubocop:disable Metrics/ClassLength
+      def perform(start_id, stop_id)
+        ActiveRecord::Base.connection.execute <<~SQL
+          UPDATE
+            users
+          SET
+            require_two_factor_authentication_from_group = FALSE
+          WHERE
+            users.id BETWEEN #{start_id}
+            AND #{stop_id}
+            AND users.require_two_factor_authentication_from_group = TRUE
+            AND users.id NOT IN ( SELECT DISTINCT
+                users_groups_query.user_id
+              FROM (
+                SELECT
+                  users.id AS user_id,
+                  members.source_id AS group_ids
+                FROM
+                  users
+                LEFT JOIN members ON members.source_type = 'Namespace'
+                  AND members.requested_at IS NULL
+                  AND members.user_id = users.id
+                  AND members.type = 'GroupMember'
+              WHERE
+                users.require_two_factor_authentication_from_group = TRUE
+                AND users.id BETWEEN #{start_id}
+                AND #{stop_id}) AS users_groups_query
+              INNER JOIN LATERAL ( WITH RECURSIVE "base_and_ancestors" AS (
+          (
+                    SELECT
+                      "namespaces"."type",
+                      "namespaces"."id",
+                      "namespaces"."parent_id",
+                      "namespaces"."require_two_factor_authentication"
+                    FROM
+                      "namespaces"
+                    WHERE
+                      "namespaces"."type" = 'Group'
+                      AND "namespaces"."id" = users_groups_query.group_ids)
+                  UNION (
+                    SELECT
+                      "namespaces"."type",
+                      "namespaces"."id",
+                      "namespaces"."parent_id",
+                      "namespaces"."require_two_factor_authentication"
+                    FROM
+                      "namespaces",
+                      "base_and_ancestors"
+                    WHERE
+                      "namespaces"."type" = 'Group'
+                      AND "namespaces"."id" = "base_and_ancestors"."parent_id")),
+                  "base_and_descendants" AS (
+          (
+                      SELECT
+                        "namespaces"."type",
+                        "namespaces"."id",
+                        "namespaces"."parent_id",
+                        "namespaces"."require_two_factor_authentication"
+                      FROM
+                        "namespaces"
+                      WHERE
+                        "namespaces"."type" = 'Group'
+                        AND "namespaces"."id" = users_groups_query.group_ids)
+                    UNION (
+                      SELECT
+                        "namespaces"."type",
+                        "namespaces"."id",
+                        "namespaces"."parent_id",
+                        "namespaces"."require_two_factor_authentication"
+                      FROM
+                        "namespaces",
+                        "base_and_descendants"
+                      WHERE
+                        "namespaces"."type" = 'Group'
+                        AND "namespaces"."parent_id" = "base_and_descendants"."id"))
+                    SELECT
+                      "namespaces".*
+                    FROM ((
+                        SELECT
+                          "namespaces"."type",
+                          "namespaces"."id",
+                          "namespaces"."parent_id",
+                          "namespaces"."require_two_factor_authentication"
+                        FROM
+                          "base_and_ancestors" AS "namespaces"
+                        WHERE
+                          "namespaces"."type" = 'Group')
+                      UNION (
+                        SELECT
+                          "namespaces"."type",
+                          "namespaces"."id",
+                          "namespaces"."parent_id",
+                          "namespaces"."require_two_factor_authentication"
+                        FROM
+                          "base_and_descendants" AS "namespaces"
+                        WHERE
+                          "namespaces"."type" = 'Group')) namespaces
+                    WHERE
+                      "namespaces"."type" = 'Group'
+                      AND "namespaces"."require_two_factor_authentication" = TRUE) AS hierarchy_tree ON TRUE);
+        SQL
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/update_existing_users_that_require_two_factor_auth_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::UpdateExistingUsersThatRequireTwoFactorAuth, schema: 20201030121314 do
+  include MigrationHelpers::NamespacesHelpers
+
+  let(:group_with_2fa_parent) { create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE) }
+  let(:group_with_2fa_child) { create_namespace('child', Gitlab::VisibilityLevel::PRIVATE, parent_id: group_with_2fa_parent.id) }
+  let(:members_table) { table(:members) }
+  let(:users_table) { table(:users) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    context 'with group members' do
+      let(:user_1) { create_user('user@example.com') }
+      let!(:member) { create_group_member(user_1, group_with_2fa_parent) }
+      let!(:user_without_group) { create_user('user_without@example.com') }
+      let(:user_other) { create_user('user_other@example.com') }
+      let!(:member_other) { create_group_member(user_other, group_with_2fa_parent) }
+
+      it 'updates user when user should not be required to establish two factor authentication' do
+        subject.perform(user_1.id, user_without_group.id)
+
+        expect(user_1.reload.require_two_factor_authentication_from_group).to eq(false)
+      end
+
+      it 'does not update user when user is member of group that requires two factor authentication' do
+        group = create_namespace('other', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: true)
+        create_group_member(user_1, group)
+
+        subject.perform(user_1.id, user_without_group.id)
+
+        expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true)
+      end
+
+      it 'does not update user who is not in current batch' do
+        subject.perform(user_1.id, user_without_group.id)
+
+        expect(user_other.reload.require_two_factor_authentication_from_group).to eq(true)
+      end
+
+      it 'updates all users in current batch' do
+        subject.perform(user_1.id, user_other.id)
+
+        expect(user_other.reload.require_two_factor_authentication_from_group).to eq(false)
+      end
+
+      it 'does not update user when user is member of group which parent group requires two factor authentication' do
+        group_with_2fa_parent.update!(require_two_factor_authentication: true)
+        subject.perform(user_1.id, user_other.id)
+
+        expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true)
+      end
+
+      it 'does not update user when user is member of group which has subgroup that requires two factor authentication' do
+        create_namespace('subgroup', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: true, parent_id: group_with_2fa_child.id)
+
+        subject.perform(user_1.id, user_other.id)
+
+        expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true)
+      end
+    end
+  end
+
+  def create_user(email, require_2fa: true)
+    users_table.create!(email: email, projects_limit: 10, require_two_factor_authentication_from_group: require_2fa)
+  end
+
+  def create_group_member(user, group)
+    members_table.create!(user_id: user.id, source_id: group.id, access_level: GroupMember::MAINTAINER, source_type: "Namespace", type: "GroupMember", notification_level: 3)
+  end
+end

spec/migrations/schedule_update_existing_users_that_require_two_factor_auth_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20201030121314_schedule_update_existing_users_that_require_two_factor_auth.rb')
+
+RSpec.describe ScheduleUpdateExistingUsersThatRequireTwoFactorAuth do
+  let(:users) { table(:users) }
+  let!(:user_1) { users.create!(require_two_factor_authentication_from_group: true, name: "user1", email: "user1@example.com", projects_limit: 1) }
+  let!(:user_2) { users.create!(require_two_factor_authentication_from_group: false, name: "user2", email: "user2@example.com", projects_limit: 1) }
+  let!(:user_3) { users.create!(require_two_factor_authentication_from_group: true, name: "user3", email: "user3@example.com", projects_limit: 1) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  it 'schedules jobs for users that require two factor authentication' do
+    Sidekiq::Testing.fake! do
+      freeze_time do
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(
+          2.minutes, user_1.id, user_1.id)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(
+          4.minutes, user_3.id, user_3.id)
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end