From 6c3c9e6e97fdcbdec3f25d7c5e1f2f8459fd64ad Mon Sep 17 00:00:00 2001 From: David Fernandez Date: Fri, 15 Sep 2023 17:52:11 +0000 Subject: [PATCH 01/13] Enforce that the policy is executed by the bot user This MR enforces that only the bot user can trigger a security policy pipeline Changelog: security EE: true --- ...orchestration_policy_rule_schedule_namespace_worker_spec.rb | 1 + .../security/orchestration_policy_rule_schedule_worker_spec.rb | 3 +++ 2 files changed, 4 insertions(+) diff --git a/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb b/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb index 93d4f974a57565..5d44295087fee1 100644 --- a/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb +++ b/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb @@ -37,6 +37,7 @@ end it 'creates async new policy bot user only when it is missing for the project' do + it 'does not invokes the rule schedule worker for all projects in the group' do expect(Security::OrchestrationConfigurationCreateBotWorker).to receive(:perform_async).with(project_1.id, nil) expect(Security::OrchestrationConfigurationCreateBotWorker).to receive(:perform_async).with(project_2.id, nil) expect { worker.perform(schedule_id) }.not_to change { User.count } diff --git a/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb b/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb index e2cbdd71b214ad..3b096d658362b6 100644 --- a/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb +++ b/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb @@ -25,6 +25,9 @@ expect { worker.perform }.not_to change { User.count } end + context 'when a project does have a security_policy_bot' do + let_it_be(:security_policy_bot) { create(:user, user_type: :security_policy_bot) } + it 'does not invoke the rule schedule worker when there is no security policy bot' do expect(Security::ScanExecutionPolicies::RuleScheduleWorker).not_to receive(:perform_async) -- GitLab From b0320e998d889e5d8ebf8bfa017cfdf296945941 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Wed, 6 Sep 2023 15:25:21 -0400 Subject: [PATCH 02/13] Cherry picking major commits into their own branch Changelog: added --- .../admin/users/components/actions/index.js | 4 ++ .../admin/users/components/actions/trust.vue | 61 +++++++++++++++++++ .../users/components/actions/untrust.vue | 55 +++++++++++++++++ .../javascripts/admin/users/constants.js | 2 + app/controllers/admin/users_controller.rb | 20 ++++++ app/helpers/admin/user_actions_helper.rb | 11 ++++ app/helpers/users_helper.rb | 4 +- app/models/user.rb | 8 +++ config/routes/admin.rb | 2 + 9 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/admin/users/components/actions/trust.vue create mode 100644 app/assets/javascripts/admin/users/components/actions/untrust.vue diff --git a/app/assets/javascripts/admin/users/components/actions/index.js b/app/assets/javascripts/admin/users/components/actions/index.js index 4e63a85df891f2..373d47d29d9c94 100644 --- a/app/assets/javascripts/admin/users/components/actions/index.js +++ b/app/assets/javascripts/admin/users/components/actions/index.js @@ -9,6 +9,8 @@ import Reject from './reject.vue'; import Unban from './unban.vue'; import Unblock from './unblock.vue'; import Unlock from './unlock.vue'; +import Trust from './trust.vue'; +import Untrust from './untrust.vue'; export default { Activate, @@ -22,4 +24,6 @@ export default { Unblock, Unlock, Reject, + Trust, + Untrust, }; diff --git a/app/assets/javascripts/admin/users/components/actions/trust.vue b/app/assets/javascripts/admin/users/components/actions/trust.vue new file mode 100644 index 00000000000000..2722442ae0d258 --- /dev/null +++ b/app/assets/javascripts/admin/users/components/actions/trust.vue @@ -0,0 +1,61 @@ + + + diff --git a/app/assets/javascripts/admin/users/components/actions/untrust.vue b/app/assets/javascripts/admin/users/components/actions/untrust.vue new file mode 100644 index 00000000000000..919e9dfc4f2066 --- /dev/null +++ b/app/assets/javascripts/admin/users/components/actions/untrust.vue @@ -0,0 +1,55 @@ + + + diff --git a/app/assets/javascripts/admin/users/constants.js b/app/assets/javascripts/admin/users/constants.js index 9cd61d6b1dbcc6..43c9a8749cde76 100644 --- a/app/assets/javascripts/admin/users/constants.js +++ b/app/assets/javascripts/admin/users/constants.js @@ -19,4 +19,6 @@ export const I18N_USER_ACTIONS = { deleteWithContributions: s__('AdminUsers|Delete user and contributions'), ban: s__('AdminUsers|Ban user'), unban: s__('AdminUsers|Unban user'), + trust: s__('AdminUsers|Trust user'), + untrust: s__('AdminUsers|Untrust user'), }; diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 1f05e4e7b21c26..9bb0d902119f26 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -164,6 +164,26 @@ def unlock end end + def trust + result = Users::TrustService.new(current_user).execute(user) + + if result[:status] == :success + redirect_back_or_admin_user(notice: _("Successfully trusted")) + else + redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) + end + end + + def untrust + result = Users::UntrustService.new(current_user).execute(user) + + if result[:status] == :success + redirect_back_or_admin_user(notice: _("Successfully untrusted")) + else + redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) + end + end + def confirm if update_user(&:force_confirm) redirect_back_or_admin_user(notice: _("Successfully confirmed")) diff --git a/app/helpers/admin/user_actions_helper.rb b/app/helpers/admin/user_actions_helper.rb index 969c5d5a0b5b14..63c4d3e0db5bca 100644 --- a/app/helpers/admin/user_actions_helper.rb +++ b/app/helpers/admin/user_actions_helper.rb @@ -16,6 +16,7 @@ def admin_actions(user) unlock_actions delete_actions ban_actions + trust_actions @actions end @@ -66,5 +67,15 @@ def ban_actions @actions << 'ban' end end + + def trust_actions + return if @user.internal? + + @actions << if @user.trusted? + 'untrust' + else + 'trust' + end + end end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index a892b6e6ac6f7c..81c41aee142a70 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -262,7 +262,9 @@ def admin_users_paths delete_with_contributions: admin_user_path(:id, hard_delete: true), admin_user: admin_user_path(:id), ban: ban_admin_user_path(:id), - unban: unban_admin_user_path(:id) + unban: unban_admin_user_path(:id), + trust: trust_admin_user_path(:id), + untrust: untrust_admin_user_path(:id) } end diff --git a/app/models/user.rb b/app/models/user.rb index bebd83163521b8..9c0e7a54e3fc87 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -600,6 +600,12 @@ def blocked? 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)) } scope :without_forbidden_states, -> { where.not(state: FORBIDDEN_SEARCH_STATES) } + scope :trusted, -> do + where('EXISTS (?)', ::UserCustomAttribute + .select(1) + .where('user_id = users.id') + .trusted_with_spam) + end strip_attributes! :name @@ -768,6 +774,8 @@ def filter_items(filter_name) external when 'deactivated' deactivated + when "trusted" + trusted else active_without_ghosts end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 5513ac1813a0a5..e8ad19624e9823 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -22,6 +22,8 @@ put :unlock put :confirm put :approve + put :trust + put :untrust delete :reject post :impersonate patch :disable_two_factor -- GitLab From 8e3675ab8aa197c78ad1a9ad9b09fb29a461c7a3 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Thu, 3 Aug 2023 15:52:08 -0400 Subject: [PATCH 03/13] Renamed vue component to comply with eslint rules (cherry picked from commit 66b5d4e55c8b12598733bbcb40088a79f90f621f) Pulling in the trust/untrust services Add trust specs for UserActionsHelper (cherry picked from commit a57b001b7e99275bbe33dea1c69643b93b48bfec) Add request specs (cherry picked from commit 87ff28cf5ee4501c347ef5c3656ba04c0e4139e5) Fix trust/untrust spec in model (cherry picked from commit edca4ae268af599e85c2e5249a52930e9a83ade3) Fix trust/untrust spec in model (cherry picked from commit edca4ae268af599e85c2e5249a52930e9a83ade3) (cherry picked from commit 5a680330755696bc2244d0b41897ab984867d1f2) Add more changes from original branch Updated locale file and json files --- .../admin/abuse_report/constants.js | 1 + .../admin/users/components/actions/index.js | 4 +- .../actions/{trust.vue => trust_user.vue} | 0 .../actions/{untrust.vue => untrust_user.vue} | 0 app/controllers/admin/users_controller.rb | 2 +- app/helpers/admin/user_actions_helper.rb | 6 +- locale/gitlab.pot | 33 ++++++++++ .../admin_users_data_attributes_paths.json | 62 ++++++++++++++----- spec/frontend/admin/users/constants.js | 4 ++ .../helpers/admin/user_actions_helper_spec.rb | 59 +++++++++++++++--- spec/requests/admin/users_controller_spec.rb | 20 ++++++ 11 files changed, 166 insertions(+), 25 deletions(-) rename app/assets/javascripts/admin/users/components/actions/{trust.vue => trust_user.vue} (100%) rename app/assets/javascripts/admin/users/components/actions/{untrust.vue => untrust_user.vue} (100%) diff --git a/app/assets/javascripts/admin/abuse_report/constants.js b/app/assets/javascripts/admin/abuse_report/constants.js index 94ef911e8532a1..941b22669c2140 100644 --- a/app/assets/javascripts/admin/abuse_report/constants.js +++ b/app/assets/javascripts/admin/abuse_report/constants.js @@ -51,6 +51,7 @@ export const REASON_OPTIONS = [ text: s__('AbuseReport|Confirmed violation of a copyright or a trademark'), }, { value: 'malware', text: s__('AbuseReport|Confirmed posting of malware') }, + { value: 'trusted', text: s__(`AbuseReport|Confirmed trusted user`) }, { value: 'other', text: s__('AbuseReport|Something else') }, { value: 'unconfirmed', text: s__('AbuseReport|Abuse unconfirmed') }, ]; diff --git a/app/assets/javascripts/admin/users/components/actions/index.js b/app/assets/javascripts/admin/users/components/actions/index.js index 373d47d29d9c94..633bc4d8b15c32 100644 --- a/app/assets/javascripts/admin/users/components/actions/index.js +++ b/app/assets/javascripts/admin/users/components/actions/index.js @@ -9,8 +9,8 @@ import Reject from './reject.vue'; import Unban from './unban.vue'; import Unblock from './unblock.vue'; import Unlock from './unlock.vue'; -import Trust from './trust.vue'; -import Untrust from './untrust.vue'; +import Trust from './trust_user.vue'; +import Untrust from './untrust_user.vue'; export default { Activate, diff --git a/app/assets/javascripts/admin/users/components/actions/trust.vue b/app/assets/javascripts/admin/users/components/actions/trust_user.vue similarity index 100% rename from app/assets/javascripts/admin/users/components/actions/trust.vue rename to app/assets/javascripts/admin/users/components/actions/trust_user.vue diff --git a/app/assets/javascripts/admin/users/components/actions/untrust.vue b/app/assets/javascripts/admin/users/components/actions/untrust_user.vue similarity index 100% rename from app/assets/javascripts/admin/users/components/actions/untrust.vue rename to app/assets/javascripts/admin/users/components/actions/untrust_user.vue diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 9bb0d902119f26..ae6311c4d09f0d 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -310,7 +310,7 @@ def paginate_without_count? end def users_with_included_associations(users) - users.includes(:authorized_projects) # rubocop: disable CodeReuse/ActiveRecord + users.includes(:authorized_projects, :trusted_with_spam_attribute) # rubocop: disable CodeReuse/ActiveRecord end def admin_making_changes_for_another_user? diff --git a/app/helpers/admin/user_actions_helper.rb b/app/helpers/admin/user_actions_helper.rb index 63c4d3e0db5bca..ba40b3c8a8df5a 100644 --- a/app/helpers/admin/user_actions_helper.rb +++ b/app/helpers/admin/user_actions_helper.rb @@ -69,7 +69,11 @@ def ban_actions end def trust_actions - return if @user.internal? + return if @user.internal? || + @user.blocked_pending_approval? || + @user.banned? || + @user.blocked? || + @user.deactivated? @actions << if @user.trusted? 'untrust' diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1d3174577aaeb0..aa02f67161a1ab 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3876,6 +3876,9 @@ msgstr "" msgid "AdminUsers|Admins" msgstr "" +msgid "AdminUsers|Allow user %{username} to create possible spam?" +msgstr "" + msgid "AdminUsers|An error occurred while fetching this user's contributions, and the request cannot return the number of issues, merge requests, groups, and projects linked to this user. If you proceed with deleting the user, all their contributions will still be deleted." msgstr "" @@ -3978,6 +3981,9 @@ msgstr "" msgid "AdminUsers|Delete user and contributions" msgstr "" +msgid "AdminUsers|Disallow user %{username} to create possible spam?" +msgstr "" + msgid "AdminUsers|Export permissions as CSV (max 100,000 users)" msgstr "" @@ -4092,6 +4098,9 @@ msgstr "" msgid "AdminUsers|The maximum compute minutes that jobs in this namespace can use on shared runners each month. Set 0 for unlimited. Set empty to inherit the global setting of %{minutes}" msgstr "" +msgid "AdminUsers|The user can create issues, notes, snippets, and merge requests without being blocked." +msgstr "" + msgid "AdminUsers|The user can't access git repositories." msgstr "" @@ -4122,6 +4131,9 @@ msgstr "" msgid "AdminUsers|To confirm, type %{username}." msgstr "" +msgid "AdminUsers|Trust user" +msgstr "" + msgid "AdminUsers|Unban user" msgstr "" @@ -4137,6 +4149,9 @@ msgstr "" msgid "AdminUsers|Unlock user %{username}?" msgstr "" +msgid "AdminUsers|Untrust user" +msgstr "" + msgid "AdminUsers|User administration" msgstr "" @@ -4167,6 +4182,9 @@ msgstr "" msgid "AdminUsers|What does this mean?" msgstr "" +msgid "AdminUsers|When allowed to create possible spam:" +msgstr "" + msgid "AdminUsers|When banned:" msgstr "" @@ -4185,6 +4203,9 @@ msgstr "" msgid "AdminUsers|You are about to permanently delete the user %{username}. This will delete all issues, merge requests, groups, and projects linked to them. To avoid data loss, consider using the %{strongStart}Block user%{strongEnd} feature instead. After you %{strongStart}Delete user%{strongEnd}, you cannot undo this action or recover the data." msgstr "" +msgid "AdminUsers|You can allow possible spam in the future if necessary." +msgstr "" + msgid "AdminUsers|You can always block their account again if needed." msgstr "" @@ -4200,6 +4221,9 @@ msgstr "" msgid "AdminUsers|You can ban their account in the future if necessary." msgstr "" +msgid "AdminUsers|You can disallow creating possible spam in the future." +msgstr "" + msgid "AdminUsers|You can unban their account in the future. Their data remains intact." msgstr "" @@ -19033,6 +19057,9 @@ msgstr "" msgid "Error occurred. User was not unlocked" msgstr "" +msgid "Error occurred. User was not updated" +msgstr "" + msgid "Error parsing CSV file. Please make sure it has" msgstr "" @@ -46226,6 +46253,9 @@ msgstr "" msgid "Successfully synced %{synced_timeago}." msgstr "" +msgid "Successfully trusted" +msgstr "" + msgid "Successfully unbanned" msgstr "" @@ -46238,6 +46268,9 @@ msgstr "" msgid "Successfully unlocked" msgstr "" +msgid "Successfully untrusted" +msgstr "" + msgid "Successfully updated %{last_updated_timeago}." msgstr "" diff --git a/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json b/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json index 44d8e48a972c48..61472b273e1342 100644 --- a/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json +++ b/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json @@ -1,19 +1,51 @@ { "type": "object", "properties": { - "edit": { "type": "string" }, - "approve": { "type": "string" }, - "reject": { "type": "string" }, - "unblock": { "type": "string" }, - "block": { "type": "string" }, - "deactivate": { "type": "string" }, - "activate": { "type": "string" }, - "unlock": { "type": "string" }, - "delete": { "type": "string" }, - "delete_with_contributions": { "type": "string" }, - "admin_user": { "type": "string" }, - "ban": { "type": "string" }, - "unban": { "type": "string" } + "edit": { + "type": "string" + }, + "approve": { + "type": "string" + }, + "reject": { + "type": "string" + }, + "unblock": { + "type": "string" + }, + "block": { + "type": "string" + }, + "deactivate": { + "type": "string" + }, + "activate": { + "type": "string" + }, + "unlock": { + "type": "string" + }, + "delete": { + "type": "string" + }, + "delete_with_contributions": { + "type": "string" + }, + "admin_user": { + "type": "string" + }, + "ban": { + "type": "string" + }, + "unban": { + "type": "string" + }, + "trust": { + "type": "string" + }, + "untrust": { + "type": "string" + } }, "required": [ "edit", @@ -28,7 +60,9 @@ "delete_with_contributions", "admin_user", "ban", - "unban" + "unban", + "trust", + "untrust" ], "additionalProperties": false } diff --git a/spec/frontend/admin/users/constants.js b/spec/frontend/admin/users/constants.js index d341eb03b1b33c..39e8e51f43ca0b 100644 --- a/spec/frontend/admin/users/constants.js +++ b/spec/frontend/admin/users/constants.js @@ -9,6 +9,8 @@ const REJECT = 'reject'; const APPROVE = 'approve'; const BAN = 'ban'; const UNBAN = 'unban'; +const TRUST = 'trust'; +const UNTRUST = 'untrust'; export const EDIT = 'edit'; @@ -24,6 +26,8 @@ export const CONFIRMATION_ACTIONS = [ UNBAN, APPROVE, REJECT, + TRUST, + UNTRUST, ]; export const DELETE_ACTIONS = [DELETE, DELETE_WITH_CONTRIBUTIONS]; diff --git a/spec/helpers/admin/user_actions_helper_spec.rb b/spec/helpers/admin/user_actions_helper_spec.rb index 87d2308690c617..abfdabf3413df9 100644 --- a/spec/helpers/admin/user_actions_helper_spec.rb +++ b/spec/helpers/admin/user_actions_helper_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe Admin::UserActionsHelper do +RSpec.describe Admin::UserActionsHelper, feature_category: :user_management do describe '#admin_actions' do let_it_be(:current_user) { build(:user) } @@ -29,13 +29,33 @@ context 'the user is a standard user' do let_it_be(:user) { create(:user) } - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete", "delete_with_contributions") } + it do + is_expected.to contain_exactly( + "edit", + "block", + "ban", + "deactivate", + "delete", + "delete_with_contributions", + "trust" + ) + end end context 'the user is an admin user' do let_it_be(:user) { create(:user, :admin) } - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete", "delete_with_contributions") } + it do + is_expected.to contain_exactly( + "edit", + "block", + "ban", + "deactivate", + "delete", + "delete_with_contributions", + "trust" + ) + end end context 'the user is blocked by LDAP' do @@ -59,7 +79,16 @@ context 'the user is deactivated' do let_it_be(:user) { create(:user, :deactivated) } - it { is_expected.to contain_exactly("edit", "block", "ban", "activate", "delete", "delete_with_contributions") } + it do + is_expected.to contain_exactly( + "edit", + "block", + "ban", + "activate", + "delete", + "delete_with_contributions" + ) + end end context 'the user is locked' do @@ -77,7 +106,8 @@ "deactivate", "unlock", "delete", - "delete_with_contributions" + "delete_with_contributions", + "trust" ) } end @@ -88,6 +118,21 @@ it { is_expected.to contain_exactly("edit", "unban", "delete", "delete_with_contributions") } end + context 'the user is trusted' do + let_it_be(:user) { create(:user, :trusted) } + + it do + is_expected.to contain_exactly("edit", + "block", + "deactivate", + "ban", + "delete", + "delete_with_contributions", + "untrust" + ) + end + end + context 'the current_user does not have permission to delete the user' do let_it_be(:user) { build(:user) } @@ -95,7 +140,7 @@ allow(helper).to receive(:can?).with(current_user, :destroy_user, user).and_return(false) end - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate") } + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "trust") } end context 'the user is a sole owner of a group' do @@ -106,7 +151,7 @@ group.add_owner(user) end - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete_with_contributions") } + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete_with_contributions", "trust") } end context 'the user is a bot' do diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index e525d615b50311..ef3a1c1375a535 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -74,4 +74,24 @@ expect { request }.to change { user.reload.access_locked? }.from(true).to(false) end end + + describe 'PUT #trust' do + subject(:request) { put trust_admin_user_path(user) } + + it 'trusts the user' do + expect { request }.to change { user.reload.trusted? }.from(false).to(true) + end + end + + describe 'PUT #untrust' do + before do + user.custom_attributes.create!(key: UserCustomAttribute::TRUSTED_BY, value: "placeholder") + end + + subject(:request) { put untrust_admin_user_path(user) } + + it 'trusts the user' do + expect { request }.to change { user.reload.trusted? }.from(true).to(false) + end + end end -- GitLab From 8dcac6c9d97e18dfa08cf6dd796826a1e191d4c3 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Fri, 8 Sep 2023 14:08:45 -0400 Subject: [PATCH 04/13] Refactor to simplify the setting of the custom attribute Use `let_it_be_with_reload` for the `user` to fix issues with setting the custom attribute. Added documentation Code review responses Updated docs with review suggestions. Utilize the `trusted_with_spam_attribute` associate Remove unnecessary files The action components for the users admin panel weren't meant to be part of this MR. Moving them out to the appropriate MR. Remove unneeded changes for controller and helpers Filter reason options if user selects `Trust user` Renamed vue component to comply with eslint rules (cherry picked from commit 66b5d4e55c8b12598733bbcb40088a79f90f621f) Pulling in the trust/untrust services Add trust specs for UserActionsHelper (cherry picked from commit a57b001b7e99275bbe33dea1c69643b93b48bfec) Add request specs (cherry picked from commit 87ff28cf5ee4501c347ef5c3656ba04c0e4139e5) Fix trust/untrust spec in model (cherry picked from commit edca4ae268af599e85c2e5249a52930e9a83ade3) Fix trust/untrust spec in model (cherry picked from commit edca4ae268af599e85c2e5249a52930e9a83ade3) (cherry picked from commit 5a680330755696bc2244d0b41897ab984867d1f2) Add more changes from original branch Updated locale file and json files --- .../admin/users/components/actions/index.js | 4 -- .../users/components/actions/trust_user.vue | 61 ------------------- .../users/components/actions/untrust_user.vue | 55 ----------------- .../javascripts/admin/users/constants.js | 2 - app/controllers/admin/users_controller.rb | 20 ------ app/helpers/admin/user_actions_helper.rb | 15 ----- app/helpers/users_helper.rb | 4 +- app/models/user.rb | 8 --- config/routes/admin.rb | 2 - 9 files changed, 1 insertion(+), 170 deletions(-) delete mode 100644 app/assets/javascripts/admin/users/components/actions/trust_user.vue delete mode 100644 app/assets/javascripts/admin/users/components/actions/untrust_user.vue diff --git a/app/assets/javascripts/admin/users/components/actions/index.js b/app/assets/javascripts/admin/users/components/actions/index.js index 633bc4d8b15c32..4e63a85df891f2 100644 --- a/app/assets/javascripts/admin/users/components/actions/index.js +++ b/app/assets/javascripts/admin/users/components/actions/index.js @@ -9,8 +9,6 @@ import Reject from './reject.vue'; import Unban from './unban.vue'; import Unblock from './unblock.vue'; import Unlock from './unlock.vue'; -import Trust from './trust_user.vue'; -import Untrust from './untrust_user.vue'; export default { Activate, @@ -24,6 +22,4 @@ export default { Unblock, Unlock, Reject, - Trust, - Untrust, }; diff --git a/app/assets/javascripts/admin/users/components/actions/trust_user.vue b/app/assets/javascripts/admin/users/components/actions/trust_user.vue deleted file mode 100644 index 2722442ae0d258..00000000000000 --- a/app/assets/javascripts/admin/users/components/actions/trust_user.vue +++ /dev/null @@ -1,61 +0,0 @@ - - - diff --git a/app/assets/javascripts/admin/users/components/actions/untrust_user.vue b/app/assets/javascripts/admin/users/components/actions/untrust_user.vue deleted file mode 100644 index 919e9dfc4f2066..00000000000000 --- a/app/assets/javascripts/admin/users/components/actions/untrust_user.vue +++ /dev/null @@ -1,55 +0,0 @@ - - - diff --git a/app/assets/javascripts/admin/users/constants.js b/app/assets/javascripts/admin/users/constants.js index 43c9a8749cde76..9cd61d6b1dbcc6 100644 --- a/app/assets/javascripts/admin/users/constants.js +++ b/app/assets/javascripts/admin/users/constants.js @@ -19,6 +19,4 @@ export const I18N_USER_ACTIONS = { deleteWithContributions: s__('AdminUsers|Delete user and contributions'), ban: s__('AdminUsers|Ban user'), unban: s__('AdminUsers|Unban user'), - trust: s__('AdminUsers|Trust user'), - untrust: s__('AdminUsers|Untrust user'), }; diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index ae6311c4d09f0d..69248a61d17385 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -164,26 +164,6 @@ def unlock end end - def trust - result = Users::TrustService.new(current_user).execute(user) - - if result[:status] == :success - redirect_back_or_admin_user(notice: _("Successfully trusted")) - else - redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) - end - end - - def untrust - result = Users::UntrustService.new(current_user).execute(user) - - if result[:status] == :success - redirect_back_or_admin_user(notice: _("Successfully untrusted")) - else - redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) - end - end - def confirm if update_user(&:force_confirm) redirect_back_or_admin_user(notice: _("Successfully confirmed")) diff --git a/app/helpers/admin/user_actions_helper.rb b/app/helpers/admin/user_actions_helper.rb index ba40b3c8a8df5a..969c5d5a0b5b14 100644 --- a/app/helpers/admin/user_actions_helper.rb +++ b/app/helpers/admin/user_actions_helper.rb @@ -16,7 +16,6 @@ def admin_actions(user) unlock_actions delete_actions ban_actions - trust_actions @actions end @@ -67,19 +66,5 @@ def ban_actions @actions << 'ban' end end - - def trust_actions - return if @user.internal? || - @user.blocked_pending_approval? || - @user.banned? || - @user.blocked? || - @user.deactivated? - - @actions << if @user.trusted? - 'untrust' - else - 'trust' - end - end end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 81c41aee142a70..a892b6e6ac6f7c 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -262,9 +262,7 @@ def admin_users_paths delete_with_contributions: admin_user_path(:id, hard_delete: true), admin_user: admin_user_path(:id), ban: ban_admin_user_path(:id), - unban: unban_admin_user_path(:id), - trust: trust_admin_user_path(:id), - untrust: untrust_admin_user_path(:id) + unban: unban_admin_user_path(:id) } end diff --git a/app/models/user.rb b/app/models/user.rb index 9c0e7a54e3fc87..bebd83163521b8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -600,12 +600,6 @@ def blocked? 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)) } scope :without_forbidden_states, -> { where.not(state: FORBIDDEN_SEARCH_STATES) } - scope :trusted, -> do - where('EXISTS (?)', ::UserCustomAttribute - .select(1) - .where('user_id = users.id') - .trusted_with_spam) - end strip_attributes! :name @@ -774,8 +768,6 @@ def filter_items(filter_name) external when 'deactivated' deactivated - when "trusted" - trusted else active_without_ghosts end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index e8ad19624e9823..5513ac1813a0a5 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -22,8 +22,6 @@ put :unlock put :confirm put :approve - put :trust - put :untrust delete :reject post :impersonate patch :disable_two_factor -- GitLab From e59a61d114fb24904f486da10b3181cac7626485 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Wed, 6 Sep 2023 15:29:42 -0400 Subject: [PATCH 05/13] Pulling in the trust/untrust services Refactor to simplify the setting of the custom attribute Use `let_it_be_with_reload` for the `user` to fix issues with setting the custom attribute. --- app/services/users/trust_service.rb | 4 ++++ app/services/users/untrust_service.rb | 4 ++++ .../master/gl-common-scanning-report.json | 8 ++------ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/services/users/trust_service.rb b/app/services/users/trust_service.rb index faf0b9c40eabc0..22902a2b94da0d 100644 --- a/app/services/users/trust_service.rb +++ b/app/services/users/trust_service.rb @@ -1,7 +1,11 @@ # frozen_string_literal: true module Users +<<<<<<<< HEAD:app/services/users/trust_service.rb class TrustService < BaseService +======== + class UntrustService < BaseService +>>>>>>>> 1d65bb65a2ae (Pulling in the trust/untrust services):app/services/users/untrust_service.rb def initialize(current_user) @current_user = current_user end diff --git a/app/services/users/untrust_service.rb b/app/services/users/untrust_service.rb index aa5de71b97f881..7ed75f3c8c4c26 100644 --- a/app/services/users/untrust_service.rb +++ b/app/services/users/untrust_service.rb @@ -1,7 +1,11 @@ # frozen_string_literal: true module Users +<<<<<<<< HEAD:app/services/users/trust_service.rb + class TrustService < BaseService +======== class UntrustService < BaseService +>>>>>>>> 1d65bb65a2ae (Pulling in the trust/untrust services):app/services/users/untrust_service.rb def initialize(current_user) @current_user = current_user end diff --git a/spec/fixtures/security_reports/master/gl-common-scanning-report.json b/spec/fixtures/security_reports/master/gl-common-scanning-report.json index 47e2a503b02bc7..cd17da5a7aec80 100644 --- a/spec/fixtures/security_reports/master/gl-common-scanning-report.json +++ b/spec/fixtures/security_reports/master/gl-common-scanning-report.json @@ -420,9 +420,7 @@ "value": "foo" } ], - "links": [ - - ] + "links": [] } ], "remediations": [ @@ -484,9 +482,7 @@ "diff": "dG90YWxseSBsZWdpdGltYXRlIGRpZmYsIDEwLzEwIHdvdWxkIGFwcGx5" } ], - "dependency_files": [ - - ], + "dependency_files": [], "scan": { "analyzer": { "id": "common-analyzer", -- GitLab From afa7839c1d8505be11164def68f7c0dfa82207b4 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Thu, 14 Sep 2023 11:33:13 -0400 Subject: [PATCH 06/13] Add trust/untrust menu to spam logs UI --- app/views/admin/spam_logs/_spam_log.html.haml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index 6aed8508a6a48f..bcd09a55ac1bf2 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -53,6 +53,18 @@ - else = render Pajamas::ButtonComponent.new(size: :small, button_options: { class: 'disabled gl-mb-3'}) do = _("Already blocked") + - if user && !user.trusted? + = render Pajamas::ButtonComponent.new(size: :small, + method: :put, + href: trust_admin_user_path(user), + button_options: { class: 'gl-mb-3', data: {confirm: _('USER WILL BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?')} }) do + = _('Trust user') + - else + = render Pajamas::ButtonComponent.new(size: :small, + method: :put, + href: untrust_admin_user_path(user), + button_options: { class: 'gl-mb-3', data: {confirm: _('USER WILL NOT BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?')} }) do + = _('Untrust user') = render Pajamas::ButtonComponent.new(size: :small, method: :delete, href: [:admin, spam_log], -- GitLab From a8e2fa6a2094c7201789efda4f0a1a1438cde532 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Thu, 14 Sep 2023 14:39:02 -0400 Subject: [PATCH 07/13] Fix N+1 query for spam logs Updated locale file. Remove unneeded changes for controller and helpers Filter reason options if user selects `Trust user` --- .../admin/abuse_report/constants.js | 1 - app/controllers/admin/spam_logs_controller.rb | 4 +- app/controllers/admin/users_controller.rb | 2 +- app/services/users/trust_service.rb | 4 -- app/services/users/untrust_service.rb | 4 -- config/routes/admin.rb | 2 + ...icy_rule_schedule_namespace_worker_spec.rb | 1 - ...ration_policy_rule_schedule_worker_spec.rb | 3 - locale/gitlab.pot | 45 ++++---------- .../admin/admin_browse_spam_logs_spec.rb | 1 + .../admin_users_data_attributes_paths.json | 62 +++++-------------- spec/frontend/admin/users/constants.js | 4 -- .../helpers/admin/user_actions_helper_spec.rb | 59 +++--------------- spec/requests/admin/users_controller_spec.rb | 20 ------ 14 files changed, 40 insertions(+), 172 deletions(-) diff --git a/app/assets/javascripts/admin/abuse_report/constants.js b/app/assets/javascripts/admin/abuse_report/constants.js index 941b22669c2140..94ef911e8532a1 100644 --- a/app/assets/javascripts/admin/abuse_report/constants.js +++ b/app/assets/javascripts/admin/abuse_report/constants.js @@ -51,7 +51,6 @@ export const REASON_OPTIONS = [ text: s__('AbuseReport|Confirmed violation of a copyright or a trademark'), }, { value: 'malware', text: s__('AbuseReport|Confirmed posting of malware') }, - { value: 'trusted', text: s__(`AbuseReport|Confirmed trusted user`) }, { value: 'other', text: s__('AbuseReport|Something else') }, { value: 'unconfirmed', text: s__('AbuseReport|Abuse unconfirmed') }, ]; diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index b27185a6addd29..d7ed6aa33ef209 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -5,7 +5,9 @@ class Admin::SpamLogsController < Admin::ApplicationController # rubocop: disable CodeReuse/ActiveRecord def index - @spam_logs = SpamLog.includes(:user).order(id: :desc).page(params[:page]).without_count + @spam_logs = SpamLog.preload(user: [:trusted_with_spam_attribute]) + .order(id: :desc) + .page(params[:page]).without_count end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 69248a61d17385..1f05e4e7b21c26 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -290,7 +290,7 @@ def paginate_without_count? end def users_with_included_associations(users) - users.includes(:authorized_projects, :trusted_with_spam_attribute) # rubocop: disable CodeReuse/ActiveRecord + users.includes(:authorized_projects) # rubocop: disable CodeReuse/ActiveRecord end def admin_making_changes_for_another_user? diff --git a/app/services/users/trust_service.rb b/app/services/users/trust_service.rb index 22902a2b94da0d..faf0b9c40eabc0 100644 --- a/app/services/users/trust_service.rb +++ b/app/services/users/trust_service.rb @@ -1,11 +1,7 @@ # frozen_string_literal: true module Users -<<<<<<<< HEAD:app/services/users/trust_service.rb class TrustService < BaseService -======== - class UntrustService < BaseService ->>>>>>>> 1d65bb65a2ae (Pulling in the trust/untrust services):app/services/users/untrust_service.rb def initialize(current_user) @current_user = current_user end diff --git a/app/services/users/untrust_service.rb b/app/services/users/untrust_service.rb index 7ed75f3c8c4c26..aa5de71b97f881 100644 --- a/app/services/users/untrust_service.rb +++ b/app/services/users/untrust_service.rb @@ -1,11 +1,7 @@ # frozen_string_literal: true module Users -<<<<<<<< HEAD:app/services/users/trust_service.rb - class TrustService < BaseService -======== class UntrustService < BaseService ->>>>>>>> 1d65bb65a2ae (Pulling in the trust/untrust services):app/services/users/untrust_service.rb def initialize(current_user) @current_user = current_user end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 5513ac1813a0a5..e8ad19624e9823 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -22,6 +22,8 @@ put :unlock put :confirm put :approve + put :trust + put :untrust delete :reject post :impersonate patch :disable_two_factor diff --git a/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb b/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb index 5d44295087fee1..93d4f974a57565 100644 --- a/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb +++ b/ee/spec/workers/security/orchestration_policy_rule_schedule_namespace_worker_spec.rb @@ -37,7 +37,6 @@ end it 'creates async new policy bot user only when it is missing for the project' do - it 'does not invokes the rule schedule worker for all projects in the group' do expect(Security::OrchestrationConfigurationCreateBotWorker).to receive(:perform_async).with(project_1.id, nil) expect(Security::OrchestrationConfigurationCreateBotWorker).to receive(:perform_async).with(project_2.id, nil) expect { worker.perform(schedule_id) }.not_to change { User.count } diff --git a/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb b/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb index 3b096d658362b6..e2cbdd71b214ad 100644 --- a/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb +++ b/ee/spec/workers/security/orchestration_policy_rule_schedule_worker_spec.rb @@ -25,9 +25,6 @@ expect { worker.perform }.not_to change { User.count } end - context 'when a project does have a security_policy_bot' do - let_it_be(:security_policy_bot) { create(:user, user_type: :security_policy_bot) } - it 'does not invoke the rule schedule worker when there is no security policy bot' do expect(Security::ScanExecutionPolicies::RuleScheduleWorker).not_to receive(:perform_async) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index aa02f67161a1ab..bfe4122ff4464a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3876,9 +3876,6 @@ msgstr "" msgid "AdminUsers|Admins" msgstr "" -msgid "AdminUsers|Allow user %{username} to create possible spam?" -msgstr "" - msgid "AdminUsers|An error occurred while fetching this user's contributions, and the request cannot return the number of issues, merge requests, groups, and projects linked to this user. If you proceed with deleting the user, all their contributions will still be deleted." msgstr "" @@ -3981,9 +3978,6 @@ msgstr "" msgid "AdminUsers|Delete user and contributions" msgstr "" -msgid "AdminUsers|Disallow user %{username} to create possible spam?" -msgstr "" - msgid "AdminUsers|Export permissions as CSV (max 100,000 users)" msgstr "" @@ -4098,9 +4092,6 @@ msgstr "" msgid "AdminUsers|The maximum compute minutes that jobs in this namespace can use on shared runners each month. Set 0 for unlimited. Set empty to inherit the global setting of %{minutes}" msgstr "" -msgid "AdminUsers|The user can create issues, notes, snippets, and merge requests without being blocked." -msgstr "" - msgid "AdminUsers|The user can't access git repositories." msgstr "" @@ -4131,9 +4122,6 @@ msgstr "" msgid "AdminUsers|To confirm, type %{username}." msgstr "" -msgid "AdminUsers|Trust user" -msgstr "" - msgid "AdminUsers|Unban user" msgstr "" @@ -4149,9 +4137,6 @@ msgstr "" msgid "AdminUsers|Unlock user %{username}?" msgstr "" -msgid "AdminUsers|Untrust user" -msgstr "" - msgid "AdminUsers|User administration" msgstr "" @@ -4182,9 +4167,6 @@ msgstr "" msgid "AdminUsers|What does this mean?" msgstr "" -msgid "AdminUsers|When allowed to create possible spam:" -msgstr "" - msgid "AdminUsers|When banned:" msgstr "" @@ -4203,9 +4185,6 @@ msgstr "" msgid "AdminUsers|You are about to permanently delete the user %{username}. This will delete all issues, merge requests, groups, and projects linked to them. To avoid data loss, consider using the %{strongStart}Block user%{strongEnd} feature instead. After you %{strongStart}Delete user%{strongEnd}, you cannot undo this action or recover the data." msgstr "" -msgid "AdminUsers|You can allow possible spam in the future if necessary." -msgstr "" - msgid "AdminUsers|You can always block their account again if needed." msgstr "" @@ -4221,9 +4200,6 @@ msgstr "" msgid "AdminUsers|You can ban their account in the future if necessary." msgstr "" -msgid "AdminUsers|You can disallow creating possible spam in the future." -msgstr "" - msgid "AdminUsers|You can unban their account in the future. Their data remains intact." msgstr "" @@ -19057,9 +19033,6 @@ msgstr "" msgid "Error occurred. User was not unlocked" msgstr "" -msgid "Error occurred. User was not updated" -msgstr "" - msgid "Error parsing CSV file. Please make sure it has" msgstr "" @@ -46253,9 +46226,6 @@ msgstr "" msgid "Successfully synced %{synced_timeago}." msgstr "" -msgid "Successfully trusted" -msgstr "" - msgid "Successfully unbanned" msgstr "" @@ -46268,9 +46238,6 @@ msgstr "" msgid "Successfully unlocked" msgstr "" -msgid "Successfully untrusted" -msgstr "" - msgid "Successfully updated %{last_updated_timeago}." msgstr "" @@ -50262,6 +50229,9 @@ msgstr "" msgid "Trigger|Trigger description" msgstr "" +msgid "Trust user" +msgstr "" + msgid "Trusted" msgstr "" @@ -50415,9 +50385,15 @@ msgstr "" msgid "USER %{user_name} WILL BE REMOVED! Are you sure?" msgstr "" +msgid "USER WILL BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?" +msgstr "" + msgid "USER WILL BE BLOCKED! Are you sure?" msgstr "" +msgid "USER WILL NOT BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?" +msgstr "" + msgid "UTC" msgstr "" @@ -50766,6 +50742,9 @@ msgstr "" msgid "Untitled" msgstr "" +msgid "Untrust user" +msgstr "" + msgid "Unused" msgstr "" diff --git a/spec/features/admin/admin_browse_spam_logs_spec.rb b/spec/features/admin/admin_browse_spam_logs_spec.rb index c272a8630b76de..9fab4f545c292b 100644 --- a/spec/features/admin/admin_browse_spam_logs_spec.rb +++ b/spec/features/admin/admin_browse_spam_logs_spec.rb @@ -22,6 +22,7 @@ expect(page).to have_content("#{spam_log.description[0...97]}...") expect(page).to have_link('Remove user') expect(page).to have_link('Block user') + expect(page).to have_link('Trust user') end it 'does not perform N+1 queries' do diff --git a/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json b/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json index 61472b273e1342..44d8e48a972c48 100644 --- a/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json +++ b/spec/fixtures/api/schemas/entities/admin_users_data_attributes_paths.json @@ -1,51 +1,19 @@ { "type": "object", "properties": { - "edit": { - "type": "string" - }, - "approve": { - "type": "string" - }, - "reject": { - "type": "string" - }, - "unblock": { - "type": "string" - }, - "block": { - "type": "string" - }, - "deactivate": { - "type": "string" - }, - "activate": { - "type": "string" - }, - "unlock": { - "type": "string" - }, - "delete": { - "type": "string" - }, - "delete_with_contributions": { - "type": "string" - }, - "admin_user": { - "type": "string" - }, - "ban": { - "type": "string" - }, - "unban": { - "type": "string" - }, - "trust": { - "type": "string" - }, - "untrust": { - "type": "string" - } + "edit": { "type": "string" }, + "approve": { "type": "string" }, + "reject": { "type": "string" }, + "unblock": { "type": "string" }, + "block": { "type": "string" }, + "deactivate": { "type": "string" }, + "activate": { "type": "string" }, + "unlock": { "type": "string" }, + "delete": { "type": "string" }, + "delete_with_contributions": { "type": "string" }, + "admin_user": { "type": "string" }, + "ban": { "type": "string" }, + "unban": { "type": "string" } }, "required": [ "edit", @@ -60,9 +28,7 @@ "delete_with_contributions", "admin_user", "ban", - "unban", - "trust", - "untrust" + "unban" ], "additionalProperties": false } diff --git a/spec/frontend/admin/users/constants.js b/spec/frontend/admin/users/constants.js index 39e8e51f43ca0b..d341eb03b1b33c 100644 --- a/spec/frontend/admin/users/constants.js +++ b/spec/frontend/admin/users/constants.js @@ -9,8 +9,6 @@ const REJECT = 'reject'; const APPROVE = 'approve'; const BAN = 'ban'; const UNBAN = 'unban'; -const TRUST = 'trust'; -const UNTRUST = 'untrust'; export const EDIT = 'edit'; @@ -26,8 +24,6 @@ export const CONFIRMATION_ACTIONS = [ UNBAN, APPROVE, REJECT, - TRUST, - UNTRUST, ]; export const DELETE_ACTIONS = [DELETE, DELETE_WITH_CONTRIBUTIONS]; diff --git a/spec/helpers/admin/user_actions_helper_spec.rb b/spec/helpers/admin/user_actions_helper_spec.rb index abfdabf3413df9..87d2308690c617 100644 --- a/spec/helpers/admin/user_actions_helper_spec.rb +++ b/spec/helpers/admin/user_actions_helper_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe Admin::UserActionsHelper, feature_category: :user_management do +RSpec.describe Admin::UserActionsHelper do describe '#admin_actions' do let_it_be(:current_user) { build(:user) } @@ -29,33 +29,13 @@ context 'the user is a standard user' do let_it_be(:user) { create(:user) } - it do - is_expected.to contain_exactly( - "edit", - "block", - "ban", - "deactivate", - "delete", - "delete_with_contributions", - "trust" - ) - end + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete", "delete_with_contributions") } end context 'the user is an admin user' do let_it_be(:user) { create(:user, :admin) } - it do - is_expected.to contain_exactly( - "edit", - "block", - "ban", - "deactivate", - "delete", - "delete_with_contributions", - "trust" - ) - end + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete", "delete_with_contributions") } end context 'the user is blocked by LDAP' do @@ -79,16 +59,7 @@ context 'the user is deactivated' do let_it_be(:user) { create(:user, :deactivated) } - it do - is_expected.to contain_exactly( - "edit", - "block", - "ban", - "activate", - "delete", - "delete_with_contributions" - ) - end + it { is_expected.to contain_exactly("edit", "block", "ban", "activate", "delete", "delete_with_contributions") } end context 'the user is locked' do @@ -106,8 +77,7 @@ "deactivate", "unlock", "delete", - "delete_with_contributions", - "trust" + "delete_with_contributions" ) } end @@ -118,21 +88,6 @@ it { is_expected.to contain_exactly("edit", "unban", "delete", "delete_with_contributions") } end - context 'the user is trusted' do - let_it_be(:user) { create(:user, :trusted) } - - it do - is_expected.to contain_exactly("edit", - "block", - "deactivate", - "ban", - "delete", - "delete_with_contributions", - "untrust" - ) - end - end - context 'the current_user does not have permission to delete the user' do let_it_be(:user) { build(:user) } @@ -140,7 +95,7 @@ allow(helper).to receive(:can?).with(current_user, :destroy_user, user).and_return(false) end - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "trust") } + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate") } end context 'the user is a sole owner of a group' do @@ -151,7 +106,7 @@ group.add_owner(user) end - it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete_with_contributions", "trust") } + it { is_expected.to contain_exactly("edit", "block", "ban", "deactivate", "delete_with_contributions") } end context 'the user is a bot' do diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index ef3a1c1375a535..e525d615b50311 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -74,24 +74,4 @@ expect { request }.to change { user.reload.access_locked? }.from(true).to(false) end end - - describe 'PUT #trust' do - subject(:request) { put trust_admin_user_path(user) } - - it 'trusts the user' do - expect { request }.to change { user.reload.trusted? }.from(false).to(true) - end - end - - describe 'PUT #untrust' do - before do - user.custom_attributes.create!(key: UserCustomAttribute::TRUSTED_BY, value: "placeholder") - end - - subject(:request) { put untrust_admin_user_path(user) } - - it 'trusts the user' do - expect { request }.to change { user.reload.trusted? }.from(true).to(false) - end - end end -- GitLab From cda1f4f85fcb7a9d45dfba9e15ea96a15ce906f6 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Fri, 13 Oct 2023 09:37:01 -0400 Subject: [PATCH 08/13] Add documentation for reviewing spam logs --- doc/administration/review_spam_logs.md | 71 ++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 doc/administration/review_spam_logs.md diff --git a/doc/administration/review_spam_logs.md b/doc/administration/review_spam_logs.md new file mode 100644 index 00000000000000..4f11dbda7020bb --- /dev/null +++ b/doc/administration/review_spam_logs.md @@ -0,0 +1,71 @@ +--- +stage: Govern +group: Anti-Abuse +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +type: reference, howto +--- + +# Review spam logs **(FREE SELF)** + +View and resolve spam logs from GitLab users. + +GitLab administrators can view and [resolve](#managing-spam-logs) spam logs in the Admin Area. + +## Managing spam logs + +> **Trust user** [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131812) in GitLab 16.5. + +To access spam logs: + +1. On the left sidebar, select **Search or go to**. +1. Select **Admin Area**. +1. Select **Spam Logs**. + +There are four ways to resolve an spam log, with a button for each method: + +- Remove user. This: + - [Deletes the reported user](../user/profile/account/delete_account.md) from the + instance. +- [Block user](#blocking-users). +- Remove log. This: + - Removes the spam log from the list. +- Trust user. This: + - Allows the user to create issues, notes, snippets, and merge requests without being blocked for spam. + - Prevents spam logs from being created for this user. + +### Blocking users + +A blocked user cannot sign in or access any repositories, but all of their data +remains. + +Blocking a user: + +- Leaves them in the spam log list. +- Changes the **Block user** button to a disabled **Already blocked** button. + +The user is notified with the following message: + +```plaintext +Your account has been blocked. If you believe this is in error, contact a staff member. +``` + +After blocking, you can still either: + +- Remove the user and report if necessary. +- Remove the spam log. + +NOTE: +Users can be [blocked](../api/users.md#block-user) and +[unblocked](../api/users.md#unblock-user) using the GitLab API. + + -- GitLab From 577ad5edd2931b549bbe04670db522556f72c5a6 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Mon, 16 Oct 2023 14:06:48 -0400 Subject: [PATCH 09/13] Add missing controller and spec changes --- app/controllers/admin/users_controller.rb | 20 +++++++++++++++++++ locale/gitlab.pot | 9 +++++++++ .../master/gl-common-scanning-report.json | 8 ++++++-- spec/requests/admin/users_controller_spec.rb | 20 +++++++++++++++++++ 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 1f05e4e7b21c26..9bb0d902119f26 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -164,6 +164,26 @@ def unlock end end + def trust + result = Users::TrustService.new(current_user).execute(user) + + if result[:status] == :success + redirect_back_or_admin_user(notice: _("Successfully trusted")) + else + redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) + end + end + + def untrust + result = Users::UntrustService.new(current_user).execute(user) + + if result[:status] == :success + redirect_back_or_admin_user(notice: _("Successfully untrusted")) + else + redirect_back_or_admin_user(alert: _("Error occurred. User was not updated")) + end + end + def confirm if update_user(&:force_confirm) redirect_back_or_admin_user(notice: _("Successfully confirmed")) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bfe4122ff4464a..11ff5e44bf49f0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19033,6 +19033,9 @@ msgstr "" msgid "Error occurred. User was not unlocked" msgstr "" +msgid "Error occurred. User was not updated" +msgstr "" + msgid "Error parsing CSV file. Please make sure it has" msgstr "" @@ -46226,6 +46229,9 @@ msgstr "" msgid "Successfully synced %{synced_timeago}." msgstr "" +msgid "Successfully trusted" +msgstr "" + msgid "Successfully unbanned" msgstr "" @@ -46238,6 +46244,9 @@ msgstr "" msgid "Successfully unlocked" msgstr "" +msgid "Successfully untrusted" +msgstr "" + msgid "Successfully updated %{last_updated_timeago}." msgstr "" diff --git a/spec/fixtures/security_reports/master/gl-common-scanning-report.json b/spec/fixtures/security_reports/master/gl-common-scanning-report.json index cd17da5a7aec80..47e2a503b02bc7 100644 --- a/spec/fixtures/security_reports/master/gl-common-scanning-report.json +++ b/spec/fixtures/security_reports/master/gl-common-scanning-report.json @@ -420,7 +420,9 @@ "value": "foo" } ], - "links": [] + "links": [ + + ] } ], "remediations": [ @@ -482,7 +484,9 @@ "diff": "dG90YWxseSBsZWdpdGltYXRlIGRpZmYsIDEwLzEwIHdvdWxkIGFwcGx5" } ], - "dependency_files": [], + "dependency_files": [ + + ], "scan": { "analyzer": { "id": "common-analyzer", diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index e525d615b50311..ef3a1c1375a535 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -74,4 +74,24 @@ expect { request }.to change { user.reload.access_locked? }.from(true).to(false) end end + + describe 'PUT #trust' do + subject(:request) { put trust_admin_user_path(user) } + + it 'trusts the user' do + expect { request }.to change { user.reload.trusted? }.from(false).to(true) + end + end + + describe 'PUT #untrust' do + before do + user.custom_attributes.create!(key: UserCustomAttribute::TRUSTED_BY, value: "placeholder") + end + + subject(:request) { put untrust_admin_user_path(user) } + + it 'trusts the user' do + expect { request }.to change { user.reload.trusted? }.from(true).to(false) + end + end end -- GitLab From 720f56e51ed2a9428cccdd4d44cac856c6071ff7 Mon Sep 17 00:00:00 2001 From: Phillip Wells Date: Mon, 16 Oct 2023 18:55:11 +0000 Subject: [PATCH 10/13] Adding documentation suggestions. --- doc/administration/review_spam_logs.md | 29 ++++++++++++++------------ locale/gitlab.pot | 4 ++-- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/doc/administration/review_spam_logs.md b/doc/administration/review_spam_logs.md index 4f11dbda7020bb..c606e9080aeb01 100644 --- a/doc/administration/review_spam_logs.md +++ b/doc/administration/review_spam_logs.md @@ -11,27 +11,30 @@ View and resolve spam logs from GitLab users. GitLab administrators can view and [resolve](#managing-spam-logs) spam logs in the Admin Area. -## Managing spam logs +## Manage spam logs > **Trust user** [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131812) in GitLab 16.5. -To access spam logs: +View and resolve spam logs to moderate user activity in your instance. + +To view spam logs: 1. On the left sidebar, select **Search or go to**. 1. Select **Admin Area**. 1. Select **Spam Logs**. +1. Optional. To resolve a spam log, select a log and then select **Remove user**, **Block user**, **Remove log**, or **Trust user**. + +### Resolving spam logs + +You can resolve a spam log with one of the following effects: + +| Option | Description | +|---------|-------------| +| **Remove user** | The user is [deleted](../user/profile/account/delete_account.md) from the instance. | +| **Block user** | The user is blocked from the instance. | +| **Remove log** | The spam log is removed from the list. | +| **Trust user** | The user is trusted, and can create issues, notes, snippets, and merge requests without being blocked for spam. Spam logs are not created for trusted users. | -There are four ways to resolve an spam log, with a button for each method: - -- Remove user. This: - - [Deletes the reported user](../user/profile/account/delete_account.md) from the - instance. -- [Block user](#blocking-users). -- Remove log. This: - - Removes the spam log from the list. -- Trust user. This: - - Allows the user to create issues, notes, snippets, and merge requests without being blocked for spam. - - Prevents spam logs from being created for this user. ### Blocking users diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 11ff5e44bf49f0..44d8b0470ecce2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50394,13 +50394,13 @@ msgstr "" msgid "USER %{user_name} WILL BE REMOVED! Are you sure?" msgstr "" -msgid "USER WILL BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?" +msgid "User activity will not be monitored for spam. Are you sure?" msgstr "" msgid "USER WILL BE BLOCKED! Are you sure?" msgstr "" -msgid "USER WILL NOT BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?" +msgid "User activity will be monitored for spam. Are you sure?" msgstr "" msgid "UTC" -- GitLab From 9e988b8d1a12cb569c1cc439ce8972e5c91554a3 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Mon, 16 Oct 2023 15:14:58 -0400 Subject: [PATCH 11/13] Adding more documentation cleanup --- app/views/admin/spam_logs/_spam_log.html.haml | 8 +++--- doc/administration/review_spam_logs.md | 28 ++----------------- locale/gitlab.pot | 24 ++++++++-------- 3 files changed, 19 insertions(+), 41 deletions(-) diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index bcd09a55ac1bf2..878692438d47c4 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -29,7 +29,7 @@ variant: :danger, method: :delete, href: admin_spam_log_path(spam_log, remove_user: true), - button_options: { data: { confirm: _("USER %{user_name} WILL BE REMOVED! Are you sure?") % { user_name: user.name }, confirm_btn_variant: 'danger' }, aria: { label: _('Remove user') } }) do + button_options: { data: { confirm: _("User %{user_name} will be removed! Are you sure?") % { user_name: user.name }, confirm_btn_variant: 'danger' }, aria: { label: _('Remove user') } }) do = _('Remove user') %td -# TODO: Remove conditonal once spamcheck supports this https://gitlab.com/gitlab-com/gl-security/engineering-and-research/automation-team/spam/spamcheck/-/issues/190 @@ -48,7 +48,7 @@ = render Pajamas::ButtonComponent.new(size: :small, method: :put, href: block_admin_user_path(user), - button_options: { class: 'gl-mb-3', data: {confirm: _('USER WILL BE BLOCKED! Are you sure?')} }) do + button_options: { class: 'gl-mb-3', data: {confirm: _('User will be blocked! Are you sure?')} }) do = _('Block user') - else = render Pajamas::ButtonComponent.new(size: :small, button_options: { class: 'disabled gl-mb-3'}) do @@ -57,13 +57,13 @@ = render Pajamas::ButtonComponent.new(size: :small, method: :put, href: trust_admin_user_path(user), - button_options: { class: 'gl-mb-3', data: {confirm: _('USER WILL BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?')} }) do + button_options: { class: 'gl-mb-3', data: {confirm: _('User will be allowed to create possible spam! Are you sure?')} }) do = _('Trust user') - else = render Pajamas::ButtonComponent.new(size: :small, method: :put, href: untrust_admin_user_path(user), - button_options: { class: 'gl-mb-3', data: {confirm: _('USER WILL NOT BE ALLOWED TO CREATE POSSIBLE SPAM! Are you sure?')} }) do + button_options: { class: 'gl-mb-3', data: {confirm: _('User will not be allowed to create possible spam! Are you sure?')} }) do = _('Untrust user') = render Pajamas::ButtonComponent.new(size: :small, method: :delete, diff --git a/doc/administration/review_spam_logs.md b/doc/administration/review_spam_logs.md index c606e9080aeb01..35cc78a9bf36d3 100644 --- a/doc/administration/review_spam_logs.md +++ b/doc/administration/review_spam_logs.md @@ -7,9 +7,9 @@ type: reference, howto # Review spam logs **(FREE SELF)** -View and resolve spam logs from GitLab users. +GitLab tracks user activity and flags certain behavior for potential spam. -GitLab administrators can view and [resolve](#managing-spam-logs) spam logs in the Admin Area. +In the Admin Area, a GitLab administrator can view and resolve spam logs. ## Manage spam logs @@ -31,32 +31,10 @@ You can resolve a spam log with one of the following effects: | Option | Description | |---------|-------------| | **Remove user** | The user is [deleted](../user/profile/account/delete_account.md) from the instance. | -| **Block user** | The user is blocked from the instance. | +| **Block user** | The user is blocked from the instance. The spam log remains in the list. | | **Remove log** | The spam log is removed from the list. | | **Trust user** | The user is trusted, and can create issues, notes, snippets, and merge requests without being blocked for spam. Spam logs are not created for trusted users. | - -### Blocking users - -A blocked user cannot sign in or access any repositories, but all of their data -remains. - -Blocking a user: - -- Leaves them in the spam log list. -- Changes the **Block user** button to a disabled **Already blocked** button. - -The user is notified with the following message: - -```plaintext -Your account has been blocked. If you believe this is in error, contact a staff member. -``` - -After blocking, you can still either: - -- Remove the user and report if necessary. -- Remove the spam log. - NOTE: Users can be [blocked](../api/users.md#block-user) and [unblocked](../api/users.md#unblock-user) using the GitLab API. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 44d8b0470ecce2..d37a5783774481 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50391,18 +50391,6 @@ msgstr "" msgid "URL or request ID" msgstr "" -msgid "USER %{user_name} WILL BE REMOVED! Are you sure?" -msgstr "" - -msgid "User activity will not be monitored for spam. Are you sure?" -msgstr "" - -msgid "USER WILL BE BLOCKED! Are you sure?" -msgstr "" - -msgid "User activity will be monitored for spam. Are you sure?" -msgstr "" - msgid "UTC" msgstr "" @@ -51425,6 +51413,9 @@ msgstr "" msgid "User %{current_user_username} has started impersonating %{username}" msgstr "" +msgid "User %{user_name} will be removed! Are you sure?" +msgstr "" + msgid "User %{username} was successfully removed." msgstr "" @@ -51533,6 +51524,15 @@ msgstr "" msgid "User was successfully updated." msgstr "" +msgid "User will be allowed to create possible spam! Are you sure?" +msgstr "" + +msgid "User will be blocked! Are you sure?" +msgstr "" + +msgid "User will not be allowed to create possible spam! Are you sure?" +msgstr "" + msgid "User-based escalation rules must have a user with access to the project" msgstr "" -- GitLab From 288fe46ec4264e370fa7d4d2a23f9c9b244a135e Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Mon, 16 Oct 2023 15:25:33 -0400 Subject: [PATCH 12/13] Add additional feature test --- spec/features/admin/admin_browse_spam_logs_spec.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/spec/features/admin/admin_browse_spam_logs_spec.rb b/spec/features/admin/admin_browse_spam_logs_spec.rb index 9fab4f545c292b..f781e2adf0748d 100644 --- a/spec/features/admin/admin_browse_spam_logs_spec.rb +++ b/spec/features/admin/admin_browse_spam_logs_spec.rb @@ -4,9 +4,9 @@ RSpec.describe 'Admin browse spam logs', feature_category: :shared do let!(:spam_log) { create(:spam_log, description: 'abcde ' * 20) } + let(:admin) { create(:admin) } before do - admin = create(:admin) sign_in(admin) gitlab_enable_admin_mode_sign_in(admin) end @@ -31,4 +31,15 @@ expect { visit admin_spam_logs_path }.not_to exceed_query_limit(control_queries) end + + context 'when user is trusted' do + before do + UserCustomAttribute.set_trusted_by(user: spam_log.user, trusted_by: admin) + end + + it 'allows admin to untrust the user' do + visit admin_spam_logs_path + expect(page).to have_link('Untrust user') + end + end end -- GitLab From f62d2b463e2cfdda2202d112bc426e5ca176d9ec Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Mon, 16 Oct 2023 17:59:00 -0400 Subject: [PATCH 13/13] Add most tests --- spec/requests/admin/users_controller_spec.rb | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index ef3a1c1375a535..2f8025691f43da 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -81,6 +81,21 @@ it 'trusts the user' do expect { request }.to change { user.reload.trusted? }.from(false).to(true) end + + context 'when setting trust fails' do + before do + allow_next_instance_of(Users::TrustService) do |instance| + allow(instance).to receive(:execute).and_return({ status: :failed }) + end + end + + it 'displays a flash alert' do + request + + expect(response).to redirect_to(admin_user_path(user)) + expect(flash[:alert]).to eq(s_('Error occurred. User was not updated')) + end + end end describe 'PUT #untrust' do @@ -93,5 +108,20 @@ it 'trusts the user' do expect { request }.to change { user.reload.trusted? }.from(true).to(false) end + + context 'when untrusting fails' do + before do + allow_next_instance_of(Users::UntrustService) do |instance| + allow(instance).to receive(:execute).and_return({ status: :failed }) + end + end + + it 'displays a flash alert' do + request + + expect(response).to redirect_to(admin_user_path(user)) + expect(flash[:alert]).to eq(s_('Error occurred. User was not updated')) + end + end end end -- GitLab