Add 2FA filter to group members page

* Show 2fa badge on a group members page
* Make group members page UI consistent with project members page
* Fix ambiguous sql in User.with/without_two_factor methods
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/controllers/groups/group_members_controller.rb

@@ -11,13 +11,20 @@ class Groups::GroupMembersController < Groups::ApplicationController
                                   :override
 
   def index
+    can_manage_members = can?(current_user, :admin_group_member, @group)
+
     @sort = params[:sort].presence || sort_value_name
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
 
     @members = GroupMembersFinder.new(@group).execute
-    @members = @members.non_invite unless can?(current_user, :admin_group, @group)
+    @members = @members.non_invite unless can_manage_members
     @members = @members.search(params[:search]) if params[:search].present?
     @members = @members.sort_by_attribute(@sort)
+
+    if can_manage_members && params[:two_factor].present?
+      @members = @members.filter_by_2fa(params[:two_factor])
+    end
+
     @members = @members.page(params[:page]).per(50)
     @members = present_members(@members.includes(:user))
 

app/models/member.rb

@@ -96,6 +96,17 @@ class Member < ActiveRecord::Base
       joins(:user).merge(User.search(query))
     end
 
+    def filter_by_2fa(value)
+      case value
+      when 'enabled'
+        left_join_users.merge(User.with_two_factor_indistinct)
+      when 'disabled'
+        left_join_users.merge(User.without_two_factor)
+      else
+        all
+      end
+    end
+
     def sort_by_attribute(method)
       case method.to_s
       when 'access_level_asc' then reorder(access_level: :asc)

app/models/user.rb

@@ -237,14 +237,18 @@ class User < ActiveRecord::Base
   scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }
   scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
 
-  def self.with_two_factor
+  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 otp_required_for_login = ?", true).distinct(arel_table[: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])
   end
 
   def self.without_two_factor
     joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id")
-      .where("u2f.id IS NULL AND otp_required_for_login = ?", false)
+      .where("u2f.id IS NULL AND users.otp_required_for_login = ?", false)
   end
 
   #

app/views/groups/group_members/index.html.haml

 - page_title "Members"
+- can_manage_members = can?(current_user, :admin_group_member, @group)
 
 .project-members-page.prepend-top-default
   %h4
     Members
   %hr
-  - if can?(current_user, :admin_group_member, @group)
+  - if can_manage_members
     .project-members-new.append-bottom-default
       %p.clearfix
         Add new member to
@@ -13,20 +14,23 @@
 
     = render 'shared/members/requests', membership_source: @group, requesters: @requesters
 
-  .append-bottom-default.clearfix
+  .clearfix
     %h5.member.existing-title
       Existing members
-    = form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form'  do
+  .panel.panel-default
+    .panel-heading.flex-project-members-panel
+      %span.flex-project-title
+        Members with access to
+        %strong= @group.name
+      %span.badge= @members.total_count
+      = form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form flex-project-members-form'  do
         .form-group
           = search_field_tag :search, params[:search], { placeholder: 'Find existing members by name', class: 'form-control', spellcheck: false }
           %button.member-search-btn{ type: "submit", "aria-label" => "Submit search" }
             = icon("search")
+          - if can_manage_members
+            = render 'shared/members/filter_2fa_dropdown'
           = render 'shared/members/sort_dropdown'
-  .panel.panel-default
-    .panel-heading
-      Members with access to
-      %strong= @group.name
-      %span.badge= @members.total_count
     %ul.content-list.members-list
       = render partial: 'shared/members/member', collection: @members, as: :member
     = paginate @members, theme: 'gitlab'

app/views/shared/members/_filter_2fa_dropdown.html.haml

+- filter = params[:two_factor] || 'everyone'
+- filter_options = { 'everyone' => 'Everyone', 'enabled' => 'Enabled', 'disabled' => 'Disabled' }
+.dropdown.inline.member-filter-2fa-dropdown
+  = dropdown_toggle('2FA: ' + filter_options[filter], { toggle: 'dropdown' })
+  %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable
+    %li.dropdown-header
+      Filter by two-factor authentication
+    - filter_options.each do |value, title|
+      %li
+        = link_to filter_group_project_member_path(two_factor: value), class: ("is-active" if filter == value) do
+          = title

app/views/shared/members/_member.html.haml

@@ -20,6 +20,10 @@
           %label.label.label-danger
             %strong Blocked
 
+        - if user.two_factor_enabled?
+          %label.label.label-info
+            2FA
+
         - if source.instance_of?(Group) && source != @group
           ·
           = link_to source.full_name, source, class: "member-group-link"

changelogs/unreleased/dz-add-2fa-filter.yml

+---
+title: Add 2FA filter to the group members page
+merge_request: 18483
+author:
+type: changed

spec/features/groups/members/filter_members_spec.rb

+require 'spec_helper'
+
+feature 'Groups > Members > Filter members' do
+  let(:user)          { create(:user) }
+  let(:user_with_2fa) { create(:user, :two_factor_via_otp) }
+  let(:group)         { create(:group) }
+
+  background do
+    group.add_owner(user)
+    group.add_master(user_with_2fa)
+
+    sign_in(user)
+  end
+
+  scenario 'shows all members' do
+    visit_members_list
+
+    expect(first_member).to include(user.name)
+    expect(second_member).to include(user_with_2fa.name)
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Everyone')
+  end
+
+  scenario 'shows only 2FA members' do
+    visit_members_list(two_factor: 'enabled')
+
+    expect(first_member).to include(user_with_2fa.name)
+    expect(members_list.size).to eq(1)
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Enabled')
+  end
+
+  scenario 'shows only non 2FA members' do
+    visit_members_list(two_factor: 'disabled')
+
+    expect(first_member).to include(user.name)
+    expect(members_list.size).to eq(1)
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Disabled')
+  end
+
+  def visit_members_list(options = {})
+    visit group_group_members_path(group.to_param, options)
+  end
+
+  def members_list
+    page.all('ul.content-list > li')
+  end
+
+  def first_member
+    members_list.first.text
+  end
+
+  def second_member
+    members_list.last.text
+  end
+end