From 77ffe1c80d2b79a6ff22936f864d9c7496baae4e Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 28 Sep 2022 11:54:47 +0530 Subject: [PATCH] Group owners to disable 2FA auth for members Currently only users can disable 2FA for thmeselves or Admin user can disable it for others. With this change group owners will be able to disable 2fa for the group members as well. Changelog: added EE: true --- app/services/two_factor/base_service.rb | 4 +- app/services/two_factor/destroy_service.rb | 6 +- .../groups/two_factor_auths_controller.rb | 40 +++++ .../services/ee/two_factor/destroy_service.rb | 14 ++ .../group_owners_to_disable_two_factor.yml | 8 + ee/config/routes/group.rb | 1 + .../two_factor_auths_controller_spec.rb | 139 ++++++++++++++++++ .../ee/two_factor/destroy_service_spec.rb | 111 +++++++++++--- locale/gitlab.pot | 6 + 9 files changed, 305 insertions(+), 24 deletions(-) create mode 100644 ee/app/controllers/groups/two_factor_auths_controller.rb create mode 100644 ee/config/feature_flags/development/group_owners_to_disable_two_factor.yml create mode 100644 ee/spec/requests/groups/two_factor_auths_controller_spec.rb diff --git a/app/services/two_factor/base_service.rb b/app/services/two_factor/base_service.rb index 0957d7ebabdcf6..50a3a5c099ccdd 100644 --- a/app/services/two_factor/base_service.rb +++ b/app/services/two_factor/base_service.rb @@ -4,12 +4,12 @@ module TwoFactor class BaseService include BaseServiceUtility - attr_reader :current_user, :params, :user + attr_reader :current_user, :user, :group def initialize(current_user, params = {}) @current_user = current_user - @params = params @user = params.delete(:user) + @group = params.delete(:group) end end end diff --git a/app/services/two_factor/destroy_service.rb b/app/services/two_factor/destroy_service.rb index 859012c21531c9..3db9aefbe709d3 100644 --- a/app/services/two_factor/destroy_service.rb +++ b/app/services/two_factor/destroy_service.rb @@ -3,7 +3,7 @@ module TwoFactor class DestroyService < ::TwoFactor::BaseService def execute - return error(_('You are not authorized to perform this action')) unless can?(current_user, :disable_two_factor, user) + return error(_('You are not authorized to perform this action')) unless authorized? return error(_('Two-factor authentication is not enabled for this user')) unless user.two_factor_enabled? result = disable_two_factor @@ -15,6 +15,10 @@ def execute private + def authorized? + can?(current_user, :disable_two_factor, user) + end + def disable_two_factor ::Users::UpdateService.new(current_user, user: user).execute do |user| user.disable_two_factor! diff --git a/ee/app/controllers/groups/two_factor_auths_controller.rb b/ee/app/controllers/groups/two_factor_auths_controller.rb new file mode 100644 index 00000000000000..2303d7062f4ed4 --- /dev/null +++ b/ee/app/controllers/groups/two_factor_auths_controller.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true +module Groups + class TwoFactorAuthsController < Groups::ApplicationController + before_action :authorize_admin_group! + before_action :check_for_feature_flag + before_action :set_user + + before_action do + push_frontend_feature_flag(:group_owners_to_disable_two_factor) + end + + feature_category :authentication_and_authorization + + def destroy + result = TwoFactor::DestroyService.new(current_user, user: @user, group: group).execute + + if result[:status] == :success + redirect_to( + group_group_members_path(group), + status: :found, + notice: format( + format(_("Two-factor authentication has been disabled successfully for %{username}!"), + username: @user.username) + )) + else + redirect_to group_group_members_path(group), status: :found, alert: result[:message] + end + end + + private + + def set_user + @user = User.find(params[:user_id]) + end + + def check_for_feature_flag + render_404 unless Feature.enabled?('group_owners_to_disable_two_factor', group) + end + end +end diff --git a/ee/app/services/ee/two_factor/destroy_service.rb b/ee/app/services/ee/two_factor/destroy_service.rb index da478a7cea077a..6541dd49670a6c 100644 --- a/ee/app/services/ee/two_factor/destroy_service.rb +++ b/ee/app/services/ee/two_factor/destroy_service.rb @@ -7,6 +7,20 @@ module DestroyService private + override :authorized? + def authorized? + return super unless group + return false unless ::Feature.enabled?('group_owners_to_disable_two_factor', group) + + group.root? && + group_provisioned_user?(user) && + can?(current_user, :admin_group, group) + end + + def group_provisioned_user?(user) + user.provisioned_by_group_id == group.id + end + override :notify_on_success def notify_on_success(user) audit_context = { diff --git a/ee/config/feature_flags/development/group_owners_to_disable_two_factor.yml b/ee/config/feature_flags/development/group_owners_to_disable_two_factor.yml new file mode 100644 index 00000000000000..bd15c8e1d72302 --- /dev/null +++ b/ee/config/feature_flags/development/group_owners_to_disable_two_factor.yml @@ -0,0 +1,8 @@ +--- +name: group_owners_to_disable_two_factor +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/99129 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/378321 +milestone: '15.6' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index fa6c6f604ae98e..2ffb3111212451 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -22,6 +22,7 @@ end resources :compliance_frameworks, only: [:new, :edit] + resource :two_factor_auth, only: [:destroy] get '/analytics', to: redirect('groups/%{group_id}/-/analytics/value_stream_analytics') resource :contribution_analytics, only: [:show] diff --git a/ee/spec/requests/groups/two_factor_auths_controller_spec.rb b/ee/spec/requests/groups/two_factor_auths_controller_spec.rb new file mode 100644 index 00000000000000..0a4a2894b649d0 --- /dev/null +++ b/ee/spec/requests/groups/two_factor_auths_controller_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::TwoFactorAuthsController do + let_it_be(:group) { create(:group) } + let_it_be(:current_user) { create(:user, provisioned_by_group_id: group.id) } + + subject(:delete_two_factor) do + delete group_two_factor_auth_path(group_id: group.path, user_id: user.id) + end + + before do + sign_in(current_user) + end + + describe 'DELETE #destroy' do + context 'when signed in user is a group owner' do + before do + group.add_owner(current_user) + end + + context 'when the user has 2FA enabled' do + let(:user) { create(:user, :two_factor, provisioned_by_group_id: provisioned_by_group_id) } + + context 'when the user is provisioned by the current group' do + let_it_be(:provisioned_by_group_id) { group.id } + + it 'successfully disables 2FA and redirects with a success notice', :aggregate_failures do + delete_two_factor + + expect(user.reload.two_factor_enabled?).to eq(false) + expect(response).to redirect_to(group_group_members_path(group)) + expect(flash[:notice]) + .to eq format(_("Two-factor authentication has been disabled successfully for %{user_email}!"), + { user_email: user.username }) + end + + it 'returns not found for nil user_id' do + delete group_two_factor_auth_path(group_id: group.path, user_id: nil) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns not found for non-existent user_id' do + delete group_two_factor_auth_path(group_id: group.path, user_id: non_existing_record_id) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'shows unauthorized error when group is not a root group', :aggregate_failures do + parent = create(:group) + subgroup = create(:group, parent: parent) + subgroup.add_owner(current_user) + + delete group_two_factor_auth_path(group_id: subgroup.full_path, user_id: user.id) + + expect(response).to redirect_to(group_group_members_path(subgroup)) + expect(flash[:alert]) + .to eq format(_("You are not authorized to perform this action")) + end + + context 'when feature flag is not enabled' do + before do + stub_feature_flags(group_owners_to_disable_two_factor: false) + end + + it 'responds with a 404', :aggregate_failures do + delete_two_factor + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + context 'when user is not provisioned by current group' do + let_it_be(:provisioned_by_group_id) { create(:group) } + + it 'fails with unauthorized error', :aggregate_failures do + delete_two_factor + + expect(response).to redirect_to(group_group_members_path(group)) + expect(flash[:alert]) + .to eq format(_("You are not authorized to perform this action")) + end + end + end + + context 'when the user does not have 2FA enabled' do + let_it_be(:user) { create(:user, provisioned_by_group_id: group.id) } + + it 'redirects to group_group_members_path' do + delete_two_factor + + expect(response).to redirect_to(group_group_members_path(group)) + end + + it 'displays an alert on failure' do + delete_two_factor + + expect(flash[:alert]) + .to eq _('Two-factor authentication is not enabled for this user') + end + end + end + + context 'when signed in user is not a group owner' do + context 'when the user has 2FA enabled' do + let(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } + + it 'responds with a 404' do + group.add_maintainer(current_user) + + delete_two_factor + + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'when signed in user is not a group member' do + it 'responds with a 404' do + delete_two_factor + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context "when invalid group id is passed" do + def delete_two_factor_invalid_group + delete group_two_factor_auth_path(group_id: 'non_existent_group', user_id: user.id) + end + + it 'throws routing error' do + expect { delete_two_factor_invalid_group }.to raise_error(ActionController::RoutingError) + end + end + end + end + end +end diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb index b41add3733e9df..4610012397cebd 100644 --- a/ee/spec/services/ee/two_factor/destroy_service_spec.rb +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -4,37 +4,106 @@ RSpec.describe TwoFactor::DestroyService do let_it_be(:current_user) { create(:user, :two_factor) } + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } - subject(:disable_2fa) { described_class.new(current_user, user: current_user).execute } + subject(:disable_2fa_with_group) { described_class.new(current_user, user: user, group: group).execute } - context 'when disabling two-factor authentication succeeds' do - context 'when licensed' do + shared_examples_for 'throws unauthorized error' do + it 'returns error' do + expect(disable_2fa_with_group).to eq( + { + status: :error, + message: 'You are not authorized to perform this action' + } + ) + end + end + + context "when feature flag is disabled" do + before do + stub_feature_flags(group_owners_to_disable_two_factor: false) + end + + it_behaves_like 'throws unauthorized error' + end + + context "when feature flag is enabled" do + context "when current user is a group owner" do before do - stub_licensed_features(admin_audit_log: true, audit_events: true, extended_audit_events: true) + group.add_owner(current_user) end - it 'creates an audit event', :aggregate_failures do - expect { disable_2fa }.to change(AuditEvent, :count).by(1) - - expect(AuditEvent.last).to have_attributes( - author: current_user, - entity_id: current_user.id, - target_id: current_user.id, - target_type: current_user.class.name, - target_details: current_user.name, - details: include(custom_message: 'Disabled two-factor authentication') - ) + + context 'when unlicensed' do + before do + stub_licensed_features(admin_audit_log: false, audit_events: false, extended_audit_events: false) + end + + it 'does not track audit event' do + expect { disable_2fa_with_group }.not_to change { AuditEvent.count } + end end - end - context 'when unlicensed' do - before do - stub_licensed_features(admin_audit_log: false, audit_events: false, extended_audit_events: false) + context "when licensed" do + before do + stub_licensed_features(admin_audit_log: true, audit_events: true, extended_audit_events: true) + end + + it 'creates an audit event', :aggregate_failures do + expect { disable_2fa_with_group }.to change(AuditEvent, :count).by(1) + .and change { user.reload.two_factor_enabled? }.from(true).to(false) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_id: user.id, + target_id: user.id, + target_type: current_user.class.name, + target_details: user.name, + details: include(custom_message: 'Disabled two-factor authentication') + ) + end + + context "when user is not provisioned by current group" do + let(:new_group) { create(:group) } + let(:user) { create(:user, :two_factor, provisioned_by_group_id: new_group.id) } + + it_behaves_like 'throws unauthorized error' + end + + context "when group is non root" do + let(:parent) { build(:group) } + + before do + group.parent = parent + parent.add_owner(create(:user)) + end + + it_behaves_like 'throws unauthorized error' + end + + context "when user is not provisioned by group" do + let(:user) { create(:user, :two_factor) } + + it_behaves_like 'throws unauthorized error' + end end + end + + context "when user is not a group owner" do + it_behaves_like 'throws unauthorized error' + + context "when group is nil" do + let(:group) { nil } - it 'does not track audit event' do - expect { disable_2fa }.not_to change { AuditEvent.count } + it_behaves_like 'throws unauthorized error' end end + + context "when user passed is nil" do + let(:user) { nil } + + it_behaves_like 'throws unauthorized error' + end end context 'when disabling two-factor authentication fails' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 545a2ca4855aaf..d4307f86bc7b97 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -42818,6 +42818,12 @@ msgstr "" msgid "Two-factor authentication has been disabled for your GitLab account." msgstr "" +msgid "Two-factor authentication has been disabled successfully for %{user_email}!" +msgstr "" + +msgid "Two-factor authentication has been disabled successfully for %{username}!" +msgstr "" + msgid "Two-factor authentication has been disabled successfully!" msgstr "" -- GitLab