From 61e9a3dcc4d8d33ab2a5c2773acfce03db08a039 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 20 Apr 2018 12:25:22 +0300 Subject: [PATCH] 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 --- .../groups/group_members_controller.rb | 9 +++- app/models/member.rb | 11 ++++ app/models/user.rb | 10 ++-- .../groups/group_members/index.html.haml | 26 +++++---- .../members/_filter_2fa_dropdown.html.haml | 11 ++++ app/views/shared/members/_member.html.haml | 4 ++ changelogs/unreleased/dz-add-2fa-filter.yml | 5 ++ .../groups/members/filter_members_spec.rb | 54 +++++++++++++++++++ 8 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 app/views/shared/members/_filter_2fa_dropdown.html.haml create mode 100644 changelogs/unreleased/dz-add-2fa-filter.yml create mode 100644 spec/features/groups/members/filter_members_spec.rb diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 134b0dfc0db..ef3eba80154 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/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)) diff --git a/app/models/member.rb b/app/models/member.rb index eac4a22a03f..68572f2e33a 100644 --- a/app/models/member.rb +++ b/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) diff --git a/app/models/user.rb b/app/models/user.rb index a9cfd39f604..d74d5aade5a 100644 --- a/app/models/user.rb +++ b/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 # diff --git a/app/views/groups/group_members/index.html.haml b/app/views/groups/group_members/index.html.haml index ad9d5562ded..c8addc49117 100644 --- a/app/views/groups/group_members/index.html.haml +++ b/app/views/groups/group_members/index.html.haml @@ -1,10 +1,11 @@ - 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 - .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") - = render 'shared/members/sort_dropdown' .panel.panel-default - .panel-heading - Members with access to - %strong= @group.name + .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' %ul.content-list.members-list = render partial: 'shared/members/member', collection: @members, as: :member = paginate @members, theme: 'gitlab' diff --git a/app/views/shared/members/_filter_2fa_dropdown.html.haml b/app/views/shared/members/_filter_2fa_dropdown.html.haml new file mode 100644 index 00000000000..95c35c56b3c --- /dev/null +++ b/app/views/shared/members/_filter_2fa_dropdown.html.haml @@ -0,0 +1,11 @@ +- 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 diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 1c139827acf..1961ad6d616 100644 --- a/app/views/shared/members/_member.html.haml +++ b/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" diff --git a/changelogs/unreleased/dz-add-2fa-filter.yml b/changelogs/unreleased/dz-add-2fa-filter.yml new file mode 100644 index 00000000000..82d501d6604 --- /dev/null +++ b/changelogs/unreleased/dz-add-2fa-filter.yml @@ -0,0 +1,5 @@ +--- +title: Add 2FA filter to the group members page +merge_request: 18483 +author: +type: changed diff --git a/spec/features/groups/members/filter_members_spec.rb b/spec/features/groups/members/filter_members_spec.rb new file mode 100644 index 00000000000..5ddb5894624 --- /dev/null +++ b/spec/features/groups/members/filter_members_spec.rb @@ -0,0 +1,54 @@ +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