Merge branch 'dz-fix-sql-error-admin-users-2fa' into 'master'

Fix SQL error when sorting 2FA-enabled users by name in admin area

See merge request gitlab-org/gitlab-ce!21324

4 files changed, 23 insertions, 7 deletions

diff --git a/app/models/member.rb b/app/models/member.rb
index 05c0bc8cb97..d9b4e8d2ac6 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -103,7 +103,7 @@ class Member < ActiveRecord::Base
     def filter_by_2fa(value)
       case value
       when 'enabled'
-        left_join_users.merge(User.with_two_factor_indistinct)
+        left_join_users.merge(User.with_two_factor)
       when 'disabled'
         left_join_users.merge(User.without_two_factor)
       else
diff --git a/app/models/user.rb b/app/models/user.rb
index a6ba90794d6..f21ca1c569f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -289,13 +289,16 @@ class User < ActiveRecord::Base
     end
   end
 
-  def self.with_two_factor_indistinct
-    joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id")
-      .where("u2f.id IS NOT NULL OR users.otp_required_for_login = ?", true)
-  end
-
   def self.with_two_factor
-    with_two_factor_indistinct.distinct(arel_table[:id])
+    with_u2f_registrations = <<-SQL
+      EXISTS (
+        SELECT *
+        FROM u2f_registrations AS u2f
+        WHERE u2f.user_id = users.id
+      ) OR users.otp_required_for_login = ?
+    SQL
+
+    where(with_u2f_registrations, true)
   end
 
   def self.without_two_factor
diff --git a/changelogs/unreleased/dz-fix-sql-error-admin-users-2fa.yml b/changelogs/unreleased/dz-fix-sql-error-admin-users-2fa.yml
new file mode 100644
index 00000000000..67926f3738a
--- /dev/null
+++ b/changelogs/unreleased/dz-fix-sql-error-admin-users-2fa.yml
@@ -0,0 +1,5 @@
+---
+title: Fix SQL error when sorting 2FA-enabled users by name in admin area
+merge_request: 21324
+author:
+type: fixed
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 9763477a711..fd99acb3bb2 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -315,6 +315,14 @@ describe User do
         expect(users_with_two_factor).to eq([user_with_2fa.id])
         expect(users_with_two_factor).not_to include(user_without_2fa.id)
       end
+
+      it 'works with ORDER BY' do
+        user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f)
+
+        expect(described_class
+          .with_two_factor
+          .reorder_by_name).to eq([user_with_2fa])
+      end
     end
 
     describe ".without_two_factor" do