From 7b68c6cd992513a465b0e3be0ecabccbe99b8602 Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Fri, 5 Aug 2022 10:59:18 -0400 Subject: [PATCH 1/2] Utilize dormant user period in application settings Add the UI to set the dormant user period in application settings and have the user model look for it when it is generated. Update the documentation on dormant user. Changelog: added --- app/controllers/admin/users_controller.rb | 2 +- app/helpers/application_settings_helper.rb | 5 ++- app/models/user.rb | 5 ++- .../_account_and_limit.html.haml | 4 ++- .../application_settings/general.html.haml | 2 +- doc/user/admin_area/moderate_users.md | 6 ++-- lib/api/users.rb | 2 +- locale/gitlab.pot | 8 ++++- .../admin/users_controller_spec.rb | 6 ++-- spec/features/admin/admin_settings_spec.rb | 31 +++++++++++++++---- .../application_settings_helper_spec.rb | 8 +++++ spec/models/user_spec.rb | 12 +++---- spec/requests/api/users_spec.rb | 6 ++-- .../deactivate_dormant_users_worker_spec.rb | 10 +++--- 14 files changed, 73 insertions(+), 34 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 5cc0c8f3970a07..1a57d27127176c 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -105,7 +105,7 @@ def deactivate return redirect_back_or_admin_user(notice: _("Error occurred. A blocked user cannot be deactivated")) if user.blocked? return redirect_back_or_admin_user(notice: _("Successfully deactivated")) if user.deactivated? return redirect_back_or_admin_user(notice: _("Internal users cannot be deactivated")) if user.internal? - return redirect_back_or_admin_user(notice: _("The user you are trying to deactivate has been active in the past %{minimum_inactive_days} days and cannot be deactivated") % { minimum_inactive_days: ::User::MINIMUM_INACTIVE_DAYS }) unless user.can_be_deactivated? + return redirect_back_or_admin_user(notice: _("The user you are trying to deactivate has been active in the past %{minimum_inactive_days} days and cannot be deactivated") % { minimum_inactive_days: Gitlab::CurrentSettings.deactivate_dormant_users_period }) unless user.can_be_deactivated? user.deactivate redirect_back_or_admin_user(notice: _("Successfully deactivated")) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 321a6e9395e1e1..ba8909fa93a7da 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -436,7 +436,10 @@ def visible_attributes :project_runner_token_expiration_interval, :pipeline_limit_per_project_user_sha ].tap do |settings| - settings << :deactivate_dormant_users unless Gitlab.com? + next if Gitlab.com? + + settings << :deactivate_dormant_users + settings << :deactivate_dormant_users_period end end diff --git a/app/models/user.rb b/app/models/user.rb index afee2d70844162..2a766e3e9964ad 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -92,7 +92,6 @@ class User < ApplicationRecord include ForcedEmailConfirmation include RequireEmailVerification - MINIMUM_INACTIVE_DAYS = 90 MINIMUM_DAYS_CREATED = 7 # Override Devise::Models::Trackable#update_tracked_fields! @@ -488,7 +487,7 @@ def blocked? scope :order_oldest_sign_in, -> { reorder(arel_table[:current_sign_in_at].asc.nulls_last) } scope :order_recent_last_activity, -> { reorder(arel_table[:last_activity_on].desc.nulls_last, arel_table[:id].asc) } scope :order_oldest_last_activity, -> { reorder(arel_table[:last_activity_on].asc.nulls_first, arel_table[:id].desc) } - scope :dormant, -> { with_state(:active).human_or_service_user.where('last_activity_on <= ?', MINIMUM_INACTIVE_DAYS.day.ago.to_date) } + scope :dormant, -> { with_state(:active).human_or_service_user.where('last_activity_on <= ?', Gitlab::CurrentSettings.deactivate_dormant_users_period.day.ago.to_date) } scope :with_no_activity, -> { with_state(:active).human_or_service_user.where(last_activity_on: nil).where('created_at <= ?', MINIMUM_DAYS_CREATED.day.ago.to_date) } scope :by_provider_and_extern_uid, ->(provider, extern_uid) { joins(:identities).merge(Identity.with_extern_uid(provider, extern_uid)) } scope :by_ids_or_usernames, -> (ids, usernames) { where(username: usernames).or(where(id: ids)) } @@ -2325,7 +2324,7 @@ def groups_with_developer_maintainer_project_access end def no_recent_activity? - last_active_at.to_i <= MINIMUM_INACTIVE_DAYS.days.ago.to_i + last_active_at.to_i <= Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_i end def update_highest_role? diff --git a/app/views/admin/application_settings/_account_and_limit.html.haml b/app/views/admin/application_settings/_account_and_limit.html.haml index f914de138a9074..9b129ca4977cfe 100644 --- a/app/views/admin/application_settings/_account_and_limit.html.haml +++ b/app/views/admin/application_settings/_account_and_limit.html.haml @@ -52,7 +52,9 @@ = f.label :deactivate_dormant_users, _('Dormant users'), class: 'label-bold' - dormant_users_help_link = help_page_path('user/admin_area/moderate_users', anchor: 'automatically-deactivate-dormant-users') - dormant_users_help_link_start = ''.html_safe % { url: dormant_users_help_link } - = f.gitlab_ui_checkbox_component :deactivate_dormant_users, _('Deactivate dormant users after 90 days of inactivity'), help_text: _('Users can reactivate their account by signing in. %{link_start}Learn more%{link_end}').html_safe % { link_start: dormant_users_help_link_start, link_end: ''.html_safe } + = f.gitlab_ui_checkbox_component :deactivate_dormant_users, _('Deactivate dormant users after a period of inactivity'), help_text: _('Users can reactivate their account by signing in. %{link_start}Learn more%{link_end}').html_safe % { link_start: dormant_users_help_link_start, link_end: ''.html_safe } + = f.label :deactivate_dormant_users_period, _('Period of inactivity (days)'), class: 'label-light' + = f.number_field :deactivate_dormant_users_period, class: 'form-control gl-form-input', title: _('Period of inactivity before deactivation'), data: { toggle: 'tooltip', container: 'body' } .form-group = f.label :personal_access_token_prefix, _('Personal Access Token prefix'), class: 'label-light' = f.text_field :personal_access_token_prefix, placeholder: _('Maximum 20 characters'), class: 'form-control gl-form-input' diff --git a/app/views/admin/application_settings/general.html.haml b/app/views/admin/application_settings/general.html.haml index 7535980151bb3b..cd63873a8937b7 100644 --- a/app/views/admin/application_settings/general.html.haml +++ b/app/views/admin/application_settings/general.html.haml @@ -13,7 +13,7 @@ .settings-content = render 'visibility_and_access' -%section.settings.as-account-limit.no-animate#js-account-settings{ class: ('expanded' if expanded_by_default?), data: { qa_selector: 'account_and_limit_settings_content' } } +%section.settings.as-account-limit.no-animate#js-account-settings{ class: ('expanded' if expanded_by_default?), data: { qa_selector: 'account_and_limit_settings_content', testid: 'account-limit' } } .settings-header %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only = _('Account and limit') diff --git a/doc/user/admin_area/moderate_users.md b/doc/user/admin_area/moderate_users.md index ab581cd3aa8da4..bdb2fb30b06d6b 100644 --- a/doc/user/admin_area/moderate_users.md +++ b/doc/user/admin_area/moderate_users.md @@ -169,12 +169,13 @@ Users can also be deactivated using the [GitLab API](../../api/users.md#deactiva ### Automatically deactivate dormant users -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/320875) in GitLab 14.0. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/320875) in GitLab 14.0. +> - Customizable time period [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336747) in GitLab 15.4 Administrators can enable automatic deactivation of users who either: - Were created more than a week ago and have not signed in. -- Have no activity in the last 90 days. +- Have no activity for a specified period of time (defaults to 90 days). To do this: @@ -182,6 +183,7 @@ To do this: 1. On the left sidebar, select **Settings > General**. 1. Expand the **Account and limit** section. 1. Under **Dormant users**, check **Deactivate dormant users after 90 days of inactivity**. +1. Under **Period of inactivity (days)**, enter a period of time before deactivation. 1. Select **Save changes**. When this feature is enabled, GitLab runs a job once a day to deactivate the dormant users. diff --git a/lib/api/users.rb b/lib/api/users.rb index c93c0f601a0207..adaafa08aeaeb9 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -733,7 +733,7 @@ def reorder_users(users) unless user.can_be_deactivated? forbidden!('A blocked user cannot be deactivated by the API') if user.blocked? forbidden!('An internal user cannot be deactivated by the API') if user.internal? - forbidden!("The user you are trying to deactivate has been active in the past #{::User::MINIMUM_INACTIVE_DAYS} days and cannot be deactivated") + forbidden!("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") end if user.deactivate diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4b825afa2ec3e3..875dcc6b2c8dce 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12291,7 +12291,7 @@ msgstr "" msgid "Days to merge" msgstr "" -msgid "Deactivate dormant users after 90 days of inactivity" +msgid "Deactivate dormant users after a period of inactivity" msgstr "" msgid "Dear Administrator," @@ -28459,6 +28459,12 @@ msgstr "" msgid "Period in seconds" msgstr "" +msgid "Period of inactivity (days)" +msgstr "" + +msgid "Period of inactivity before deactivation" +msgstr "" + msgid "Permalink" msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 515ad9daf36ab5..bd70787e19f4a0 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -270,19 +270,19 @@ let(:user) { create(:user, **activity) } context 'with no recent activity' do - let(:activity) { { last_activity_on: ::User::MINIMUM_INACTIVE_DAYS.next.days.ago } } + let(:activity) { { last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.next.days.ago } } it_behaves_like 'a request that deactivates the user' end context 'with recent activity' do - let(:activity) { { last_activity_on: ::User::MINIMUM_INACTIVE_DAYS.pred.days.ago } } + let(:activity) { { last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.pred.days.ago } } it 'does not deactivate the user' do put :deactivate, params: { id: user.username } user.reload expect(user.deactivated?).to be_falsey - expect(flash[:notice]).to eq("The user you are trying to deactivate has been active in the past #{::User::MINIMUM_INACTIVE_DAYS} days and cannot be deactivated") + expect(flash[:notice]).to eq("The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") end end end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 8843e13026b71a..af1cd051fbcff2 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -102,7 +102,7 @@ end it 'change Account and Limit Settings' do - page.within('.as-account-limit') do + page.within(find('[data-testid="account-limit"]')) do uncheck 'Gravatar enabled' click_button 'Save changes' end @@ -112,7 +112,7 @@ end it 'change Maximum export size' do - page.within('.as-account-limit') do + page.within(find('[data-testid="account-limit"]')) do fill_in 'Maximum export size (MB)', with: 25 click_button 'Save changes' end @@ -122,7 +122,7 @@ end it 'change Maximum import size' do - page.within('.as-account-limit') do + page.within(find('[data-testid="account-limit"]')) do fill_in 'Maximum import size (MB)', with: 15 click_button 'Save changes' end @@ -150,16 +150,20 @@ it 'does not expose the setting' do expect(page).to have_no_selector('#application_setting_deactivate_dormant_users') end + + it 'does not expose the setting' do + expect(page).to have_no_selector('#application_setting_deactivate_dormant_users_period') + end end context 'when not Gitlab.com' do let(:dot_com?) { false } it 'change Dormant users' do - expect(page).to have_unchecked_field('Deactivate dormant users after 90 days of inactivity') + expect(page).to have_unchecked_field('Deactivate dormant users after a period of inactivity') expect(current_settings.deactivate_dormant_users).to be_falsey - page.within('.as-account-limit') do + page.within(find('[data-testid="account-limit"]')) do check 'application_setting_deactivate_dormant_users' click_button 'Save changes' end @@ -169,7 +173,22 @@ page.refresh expect(current_settings.deactivate_dormant_users).to be_truthy - expect(page).to have_checked_field('Deactivate dormant users after 90 days of inactivity') + expect(page).to have_checked_field('Deactivate dormant users after a period of inactivity') + end + + it 'change Dormant users period' do + expect(page).to have_field _('Period of inactivity (days)') + + page.within(find('[data-testid="account-limit"]')) do + fill_in _('application_setting_deactivate_dormant_users_period'), with: '35' + click_button 'Save changes' + end + + expect(page).to have_content "Application settings saved successfully" + + page.refresh + + expect(page).to have_field _('Period of inactivity (days)'), with: '35' end end end diff --git a/spec/helpers/application_settings_helper_spec.rb b/spec/helpers/application_settings_helper_spec.rb index 0304aac18aefcf..ba6caf7cf173d3 100644 --- a/spec/helpers/application_settings_helper_spec.rb +++ b/spec/helpers/application_settings_helper_spec.rb @@ -46,6 +46,10 @@ expect(helper.visible_attributes).to include(:deactivate_dormant_users) end + it 'contains :deactivate_dormant_users_period' do + expect(helper.visible_attributes).to include(:deactivate_dormant_users_period) + end + it 'contains rate limit parameters' do expect(helper.visible_attributes).to include(*%i( issues_create_limit notes_create_limit project_export_limit @@ -63,6 +67,10 @@ it 'does not contain :deactivate_dormant_users' do expect(helper.visible_attributes).not_to include(:deactivate_dormant_users) end + + it 'does not contain :deactivate_dormant_users_period' do + expect(helper.visible_attributes).not_to include(:deactivate_dormant_users_period) + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 801cc586b76202..d2aa2a63c1b7b0 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3810,8 +3810,8 @@ def login_method(login) describe '#can_be_deactivated?' do let(:activity) { {} } let(:user) { create(:user, name: 'John Smith', **activity) } - let(:day_within_minium_inactive_days_threshold) { User::MINIMUM_INACTIVE_DAYS.pred.days.ago } - let(:day_outside_minium_inactive_days_threshold) { User::MINIMUM_INACTIVE_DAYS.next.days.ago } + let(:day_within_minium_inactive_days_threshold) { Gitlab::CurrentSettings.deactivate_dormant_users_period.pred.days.ago } + let(:day_outside_minium_inactive_days_threshold) { Gitlab::CurrentSettings.deactivate_dormant_users_period.next.days.ago } shared_examples 'not eligible for deactivation' do it 'returns false' do @@ -7193,8 +7193,8 @@ def have_dismissed_callout describe '.dormant' do it 'returns dormant users' do freeze_time do - not_that_long_ago = (described_class::MINIMUM_INACTIVE_DAYS - 1).days.ago.to_date - too_long_ago = described_class::MINIMUM_INACTIVE_DAYS.days.ago.to_date + not_that_long_ago = (Gitlab::CurrentSettings.deactivate_dormant_users_period - 1).days.ago.to_date + too_long_ago = Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date create(:user, :deactivated, last_activity_on: too_long_ago) @@ -7214,8 +7214,8 @@ def have_dismissed_callout describe '.with_no_activity' do it 'returns users with no activity' do freeze_time do - active_not_that_long_ago = (described_class::MINIMUM_INACTIVE_DAYS - 1).days.ago.to_date - active_too_long_ago = described_class::MINIMUM_INACTIVE_DAYS.days.ago.to_date + active_not_that_long_ago = (Gitlab::CurrentSettings.deactivate_dormant_users_period - 1).days.ago.to_date + active_too_long_ago = Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date created_recently = (described_class::MINIMUM_DAYS_CREATED - 1).days.ago.to_date created_not_recently = described_class::MINIMUM_DAYS_CREATED.days.ago.to_date diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 26238a87209d50..331d8261dcfa8f 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -3238,7 +3238,7 @@ def update_password(user, admin, password = User.random_password) let(:user) { create(:user, **activity) } context 'with no recent activity' do - let(:activity) { { last_activity_on: ::User::MINIMUM_INACTIVE_DAYS.next.days.ago } } + let(:activity) { { last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.next.days.ago } } it 'deactivates an active user' do deactivate @@ -3249,13 +3249,13 @@ def update_password(user, admin, password = User.random_password) end context 'with recent activity' do - let(:activity) { { last_activity_on: ::User::MINIMUM_INACTIVE_DAYS.pred.days.ago } } + let(:activity) { { last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.pred.days.ago } } it 'does not deactivate an active user' do deactivate expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq("403 Forbidden - The user you are trying to deactivate has been active in the past #{::User::MINIMUM_INACTIVE_DAYS} days and cannot be deactivated") + expect(json_response['message']).to eq("403 Forbidden - The user you are trying to deactivate has been active in the past #{Gitlab::CurrentSettings.deactivate_dormant_users_period} days and cannot be deactivated") expect(user.reload.state).to eq('active') end end diff --git a/spec/workers/users/deactivate_dormant_users_worker_spec.rb b/spec/workers/users/deactivate_dormant_users_worker_spec.rb index 263ca31e0a0ee8..a8318de669bbf9 100644 --- a/spec/workers/users/deactivate_dormant_users_worker_spec.rb +++ b/spec/workers/users/deactivate_dormant_users_worker_spec.rb @@ -6,7 +6,7 @@ using RSpec::Parameterized::TableSyntax describe '#perform' do - let_it_be(:dormant) { create(:user, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date) } + let_it_be(:dormant) { create(:user, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date) } let_it_be(:inactive) { create(:user, last_activity_on: nil, created_at: User::MINIMUM_DAYS_CREATED.days.ago.to_date) } let_it_be(:inactive_recently_created) { create(:user, last_activity_on: nil, created_at: (User::MINIMUM_DAYS_CREATED - 1).days.ago.to_date) } @@ -14,7 +14,7 @@ it 'does not run for GitLab.com' do expect(Gitlab).to receive(:com?).and_return(true) - expect(Gitlab::CurrentSettings).not_to receive(:current_application_settings) + # Now makes a call to current settings to determine period of dormancy worker.perform @@ -48,7 +48,7 @@ end with_them do it 'deactivates certain user types' do - user = create(:user, user_type: user_type, state: :active, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date) + user = create(:user, user_type: user_type, state: :active, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date) worker.perform @@ -57,8 +57,8 @@ end it 'does not deactivate non-active users' do - human_user = create(:user, user_type: :human, state: :blocked, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date) - service_user = create(:user, user_type: :service_user, state: :blocked, last_activity_on: User::MINIMUM_INACTIVE_DAYS.days.ago.to_date) + human_user = create(:user, user_type: :human, state: :blocked, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date) + service_user = create(:user, user_type: :service_user, state: :blocked, last_activity_on: Gitlab::CurrentSettings.deactivate_dormant_users_period.days.ago.to_date) worker.perform -- GitLab From bc1e083aea54ffbcc35ec073b5a2c8844e95cf6d Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Tue, 23 Aug 2022 13:14:27 -0400 Subject: [PATCH 2/2] Updates from Review Add new help text instead of a placeholder for a field with a default value Update the readbility of the spec for dormant user period Add in a minimum value of 1 to the number field, disallowing 0 and negative numbers. --- .../application_settings/_account_and_limit.html.haml | 8 ++++++-- spec/features/admin/admin_settings_spec.rb | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/views/admin/application_settings/_account_and_limit.html.haml b/app/views/admin/application_settings/_account_and_limit.html.haml index 9b129ca4977cfe..3e0f1dc0a1ce64 100644 --- a/app/views/admin/application_settings/_account_and_limit.html.haml +++ b/app/views/admin/application_settings/_account_and_limit.html.haml @@ -53,8 +53,12 @@ - dormant_users_help_link = help_page_path('user/admin_area/moderate_users', anchor: 'automatically-deactivate-dormant-users') - dormant_users_help_link_start = ''.html_safe % { url: dormant_users_help_link } = f.gitlab_ui_checkbox_component :deactivate_dormant_users, _('Deactivate dormant users after a period of inactivity'), help_text: _('Users can reactivate their account by signing in. %{link_start}Learn more%{link_end}').html_safe % { link_start: dormant_users_help_link_start, link_end: ''.html_safe } - = f.label :deactivate_dormant_users_period, _('Period of inactivity (days)'), class: 'label-light' - = f.number_field :deactivate_dormant_users_period, class: 'form-control gl-form-input', title: _('Period of inactivity before deactivation'), data: { toggle: 'tooltip', container: 'body' } + .form-group + = f.label :deactivate_dormant_users_period, _('Period of inactivity (days)'), class: 'label-light' + = f.number_field :deactivate_dormant_users_period, class: 'form-control gl-form-input', min: '1' + .form-text.text-muted + = _('Period of inactivity before deactivation') + .form-group = f.label :personal_access_token_prefix, _('Personal Access Token prefix'), class: 'label-light' = f.text_field :personal_access_token_prefix, placeholder: _('Maximum 20 characters'), class: 'form-control gl-form-input' diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index af1cd051fbcff2..0f5d9892e668af 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -159,7 +159,7 @@ context 'when not Gitlab.com' do let(:dot_com?) { false } - it 'change Dormant users' do + it 'changes Dormant users' do expect(page).to have_unchecked_field('Deactivate dormant users after a period of inactivity') expect(current_settings.deactivate_dormant_users).to be_falsey -- GitLab