From 35b748e7f4cc4dcd646873ed426e2ce73131bf41 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Tue, 20 Aug 2024 15:27:29 +0300 Subject: [PATCH] FIX: Don't show silence button on staff users and display similar users (#28423) This commit fixes a bug where the silence button is incorrectly displayed on the admin page of a staff user. It's not actually possible to silence a staff user because the backend correctly prevents it, but the frontend isn't checking if the button should be displayed. Another small bug that this commit fixes is the similar users list not showing up inside the silence/suspend modals due to also a bug in the frontend. I've also changed the way similar users are loaded so that they're not returned by the `admin/users#show` endpoint anymore and moved them into a new endpoint that the penalize modals (suspend and silence) can call directly to retrieve the list of users. This is done because the similar users list is never shown on the admin user page (`/admin/users/:user_id/:username`); they're only needed when the suspend or silence modals are opened. Internal topic: t/130014. --- .../admin-penalty-similar-users.hbs | 66 +++++++++--------- .../components/admin-penalty-similar-users.js | 51 +++++++++----- .../addon/components/modal/penalize-user.hbs | 7 +- .../addon/components/modal/penalize-user.js | 5 ++ .../admin/addon/templates/user-index.hbs | 16 +++-- .../stylesheets/common/admin/penalty.scss | 2 +- app/controllers/admin/users_controller.rb | 26 ++++--- app/models/user.rb | 7 ++ .../admin_detailed_user_serializer.rb | 16 +---- config/routes.rb | 1 + spec/requests/admin/users_controller_spec.rb | 22 ++++-- .../api/schemas/json/admin_user_response.json | 3 + spec/system/admin_user_spec.rb | 69 +++++++++++++++++++ .../page_objects/modals/penalize_user.rb | 19 +++++ spec/system/page_objects/pages/admin_user.rb | 39 +++++++++++ 15 files changed, 262 insertions(+), 87 deletions(-) create mode 100644 spec/system/admin_user_spec.rb create mode 100644 spec/system/page_objects/modals/penalize_user.rb create mode 100644 spec/system/page_objects/pages/admin_user.rb diff --git a/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.hbs b/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.hbs index 10c485bc4d2..134412666a7 100644 --- a/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.hbs +++ b/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.hbs @@ -3,42 +3,44 @@ {{html-safe (i18n "admin.user.other_matches" - (hash count=this.user.similar_users_count username=this.user.username) + (hash count=@user.similar_users_count username=@user.username) ) }}

- - - - - - - - - - - - - - - {{#each this.user.similar_users as |user|}} + +
{{i18n "username"}}{{i18n "last_seen"}}{{i18n "admin.user.topics_entered"}}{{i18n "admin.user.posts_read_count"}}{{i18n "admin.user.time_read"}}{{i18n "created"}}
+ - - - - - - - + + + + + + + - {{/each}} - -
- - {{avatar user imageSize="small"}} {{user.username}}{{format-duration user.last_seen_age}}{{number user.topics_entered}}{{number user.posts_read_count}}{{format-duration user.time_read}}{{format-duration user.created_at_age}}{{i18n "username"}}{{i18n "last_seen"}}{{i18n "admin.user.topics_entered"}}{{i18n "admin.user.posts_read_count"}}{{i18n "admin.user.time_read"}}{{i18n "created"}}
+ + + + {{#each this.similarUsers as |user|}} + + + + + {{avatar user imageSize="small"}} {{user.username}} + {{format-duration user.last_seen_age}} + {{number user.topics_entered}} + {{number user.posts_read_count}} + {{format-duration user.time_read}} + {{format-duration user.created_at_age}} + + {{/each}} + + + \ No newline at end of file diff --git a/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.js b/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.js index 4531b83d9d8..adaa615cf0f 100644 --- a/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.js +++ b/app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.js @@ -1,12 +1,21 @@ -import Component from "@ember/component"; +import Component from "@glimmer/component"; +import { tracked } from "@glimmer/tracking"; import { action } from "@ember/object"; -import { tagName } from "@ember-decorators/component"; -import discourseComputed from "discourse-common/utils/decorators"; +import { ajax } from "discourse/lib/ajax"; -@tagName("") export default class AdminPenaltySimilarUsers extends Component { - @discourseComputed("penaltyType") - penaltyField(penaltyType) { + @tracked isLoading; + @tracked similarUsers = []; + selectedUserIds = []; + + constructor() { + super(...arguments); + + this.loadSimilarUsers(); + } + + get penaltyField() { + const penaltyType = this.args.penaltyType; if (penaltyType === "suspend") { return "can_be_suspended"; } else if (penaltyType === "silence") { @@ -14,16 +23,26 @@ export default class AdminPenaltySimilarUsers extends Component { } } - @action - selectUserId(userId, event) { - if (!this.selectedUserIds) { - return; - } - - if (event.target.checked) { - this.selectedUserIds.pushObject(userId); - } else { - this.selectedUserIds.removeObject(userId); + async loadSimilarUsers() { + this.isLoading = true; + try { + const data = await ajax( + `/admin/users/${this.args.user.id}/similar-users.json` + ); + this.similarUsers = data.users; + } finally { + this.isLoading = false; } } + + @action + selectUserId(userId, event) { + if (event.target.checked) { + this.selectedUserIds.push(userId); + } else { + this.selectedUserIds = this.selectedUserIds.filter((id) => id !== userId); + } + + this.args.onUsersChanged(this.selectedUserIds); + } } diff --git a/app/assets/javascripts/admin/addon/components/modal/penalize-user.hbs b/app/assets/javascripts/admin/addon/components/modal/penalize-user.hbs index 783a20e610b..8372f207098 100644 --- a/app/assets/javascripts/admin/addon/components/modal/penalize-user.hbs +++ b/app/assets/javascripts/admin/addon/components/modal/penalize-user.hbs @@ -46,11 +46,12 @@ @postEdit={{this.postEdit}} /> {{/if}} - {{#if this.user.similar_users}} + {{#if @model.user.similar_users_count}} {{/if}} {{else}} @@ -60,9 +61,9 @@
{{i18n "admin.user.cant_silence"}}
{{/if}} {{/if}} -
{{html-safe this.penaltyHistory}}
<:footer> +
{{html-safe this.penaltyHistory}}
{{i18n "admin.user.silence_explanation"}} {{else}} - - {{i18n "admin.user.silence_explanation"}} + {{#if this.model.canSilence}} + + {{i18n "admin.user.silence_explanation"}} + {{/if}} {{/if}} diff --git a/app/assets/stylesheets/common/admin/penalty.scss b/app/assets/stylesheets/common/admin/penalty.scss index 325f59dcce5..3e5fa827b9e 100644 --- a/app/assets/stylesheets/common/admin/penalty.scss +++ b/app/assets/stylesheets/common/admin/penalty.scss @@ -91,7 +91,7 @@ position: sticky; bottom: 0; background-color: var(--secondary); - padding: 0.5em 0 1em 0; + padding: 0 0 1em 0; } .penalty-history::before { position: absolute; diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index a8e49aaee86..9be0bc0ba26 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -46,18 +46,28 @@ class Admin::UsersController < Admin::StaffController @user = User.find_by(id: params[:id]) raise Discourse::NotFound unless @user - similar_users = - User - .real - .where.not(id: @user.id) - .where(ip_address: @user.ip_address, admin: false, moderator: false) - render_serialized( @user, AdminDetailedUserSerializer, root: false, - similar_users: similar_users.limit(MAX_SIMILAR_USERS), - similar_users_count: similar_users.count, + similar_users_count: @user.similar_users.count, + ) + end + + def similar_users + @user = User.find_by(id: params[:user_id]) + raise Discourse::NotFound if !@user + + render_json_dump( + { + users: + ActiveModel::ArraySerializer.new( + @user.similar_users.limit(MAX_SIMILAR_USERS), + each_serializer: SimilarAdminUserSerializer, + scope: guardian, + root: false, + ), + }, ) end diff --git a/app/models/user.rb b/app/models/user.rb index cf9dc0009ae..520713200d4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1884,6 +1884,13 @@ class User < ActiveRecord::Base update(required_fields_version: UserRequiredFieldsVersion.current) end + def similar_users + User + .real + .where.not(id: self.id) + .where(ip_address: self.ip_address, admin: false, moderator: false) + end + protected def badge_grant diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb index e82bf8ac346..a47d3ab66ec 100644 --- a/app/serializers/admin_detailed_user_serializer.rb +++ b/app/serializers/admin_detailed_user_serializer.rb @@ -36,7 +36,6 @@ class AdminDetailedUserSerializer < AdminUserSerializer :can_delete_sso_record, :api_key_count, :external_ids, - :similar_users, :similar_users_count has_one :approved_by, serializer: BasicUserSerializer, embed: :objects @@ -157,25 +156,12 @@ class AdminDetailedUserSerializer < AdminUserSerializer external_ids end - def similar_users - ActiveModel::ArraySerializer.new( - @options[:similar_users], - each_serializer: SimilarAdminUserSerializer, - scope: scope, - root: false, - ).as_json - end - - def include_similar_users? - @options[:similar_users].present? - end - def similar_users_count @options[:similar_users_count] end def include_similar_users_count? - @options[:similar_users].present? + @options[:similar_users_count].present? end def can_delete_sso_record diff --git a/config/routes.rb b/config/routes.rb index 7b0c25b3049..64fbcb0ba24 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -161,6 +161,7 @@ Discourse::Application.routes.draw do post "reset-bounce-score" put "disable_second_factor" delete "sso_record" + get "similar-users.json" => "users#similar_users" end get "users/:id.json" => "users#show", :defaults => { format: "json" } get "users/:id/:username" => "users#show", diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index ea73e76036f..005f4a61140 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -109,7 +109,7 @@ RSpec.describe Admin::UsersController do expect(response.parsed_body["id"]).to eq(user.id) end - it "includes similar users who aren't admin or mods" do + it "includes count of similiar users" do Fabricate(:user, ip_address: "88.88.88.88") Fabricate(:admin, ip_address: user.ip_address) Fabricate(:moderator, ip_address: user.ip_address) @@ -118,11 +118,7 @@ RSpec.describe Admin::UsersController do get "/admin/users/#{user.id}.json" expect(response.status).to eq(200) - expect(response.parsed_body["id"]).to eq(user.id) expect(response.parsed_body["similar_users_count"]).to eq(1) - expect(response.parsed_body["similar_users"].map { |u| u["id"] }).to contain_exactly( - similar_user.id, - ) end end @@ -138,6 +134,22 @@ RSpec.describe Admin::UsersController do end end + describe "#similar_users" do + before { sign_in(admin) } + + it "includes similar users who aren't admin or mods" do + Fabricate(:user, ip_address: "88.88.88.88") + Fabricate(:admin, ip_address: user.ip_address) + Fabricate(:moderator, ip_address: user.ip_address) + similar_user = Fabricate(:user, ip_address: user.ip_address) + + get "/admin/users/#{user.id}/similar-users.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["users"].map { |u| u["id"] }).to contain_exactly(similar_user.id) + end + end + describe "#approve" do let(:evil_trout) { Fabricate(:evil_trout) } diff --git a/spec/requests/api/schemas/json/admin_user_response.json b/spec/requests/api/schemas/json/admin_user_response.json index 6b6d902c1fa..9bd90d2100c 100644 --- a/spec/requests/api/schemas/json/admin_user_response.json +++ b/spec/requests/api/schemas/json/admin_user_response.json @@ -170,6 +170,9 @@ "api_key_count": { "type": "integer" }, + "similar_users_count": { + "type": "integer" + }, "single_sign_on_record": { "type": ["string", "null"] }, diff --git a/spec/system/admin_user_spec.rb b/spec/system/admin_user_spec.rb new file mode 100644 index 00000000000..c24530b44f8 --- /dev/null +++ b/spec/system/admin_user_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +describe "Admin User Page", type: :system do + fab!(:current_user) { Fabricate(:admin) } + + let(:admin_user_page) { PageObjects::Pages::AdminUser.new } + let(:suspend_user_modal) { PageObjects::Modals::PenalizeUser.new("suspend") } + let(:silence_user_modal) { PageObjects::Modals::PenalizeUser.new("silence") } + + before { sign_in(current_user) } + + context "when visiting an admin's page" do + fab!(:admin) + + before { admin_user_page.visit(admin) } + + it "doesn't display the suspend or silence buttons" do + expect(admin_user_page).to have_no_suspend_button + expect(admin_user_page).to have_no_silence_button + end + end + + context "when visiting a moderator's page" do + fab!(:moderator) + + before { admin_user_page.visit(moderator) } + + it "doesn't display the suspend or silence buttons" do + expect(admin_user_page).to have_no_suspend_button + expect(admin_user_page).to have_no_silence_button + end + end + + context "when visting a regular user's page" do + fab!(:user) { Fabricate(:user, ip_address: "93.123.44.90") } + fab!(:similar_user) { Fabricate(:user, ip_address: user.ip_address) } + fab!(:another_mod) { Fabricate(:moderator, ip_address: user.ip_address) } + fab!(:another_admin) { Fabricate(:admin, ip_address: user.ip_address) } + + before { admin_user_page.visit(user) } + + it "displays the suspend and silence buttons" do + expect(admin_user_page).to have_suspend_button + expect(admin_user_page).to have_silence_button + end + + describe "the suspend user modal" do + it "displays the list of users who share the same IP but are not mods or admins" do + admin_user_page.click_suspend_button + + expect(suspend_user_modal.similar_users).to contain_exactly(similar_user.username) + expect(admin_user_page.similar_users_warning).to include( + I18n.t("admin_js.admin.user.other_matches", count: 1, username: user.username), + ) + end + end + + describe "the silence user modal" do + it "displays the list of users who share the same IP but are not mods or admins" do + admin_user_page.click_silence_button + + expect(silence_user_modal.similar_users).to contain_exactly(similar_user.username) + expect(admin_user_page.similar_users_warning).to include( + I18n.t("admin_js.admin.user.other_matches", count: 1, username: user.username), + ) + end + end + end +end diff --git a/spec/system/page_objects/modals/penalize_user.rb b/spec/system/page_objects/modals/penalize_user.rb new file mode 100644 index 00000000000..5ae2fb39ae2 --- /dev/null +++ b/spec/system/page_objects/modals/penalize_user.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module PageObjects + module Modals + class PenalizeUser < PageObjects::Modals::Base + def initialize(penalty_type) + @penalty_type = penalty_type + end + + def similar_users + modal.all("table tbody tr td:nth-child(2)").map(&:text) + end + + def modal + find(".d-modal.#{@penalty_type}-user-modal") + end + end + end +end diff --git a/spec/system/page_objects/pages/admin_user.rb b/spec/system/page_objects/pages/admin_user.rb new file mode 100644 index 00000000000..5e7dc54408f --- /dev/null +++ b/spec/system/page_objects/pages/admin_user.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module PageObjects + module Pages + class AdminUser < PageObjects::Pages::Base + def visit(user) + page.visit("/admin/users/#{user.id}/#{user.username}") + end + + def has_suspend_button? + has_css?(".btn-danger.suspend-user") + end + + def has_no_suspend_button? + has_no_css?(".btn-danger.suspend-user") + end + + def has_silence_button? + has_css?(".btn-danger.silence-user") + end + + def has_no_silence_button? + has_no_css?(".btn-danger.silence-user") + end + + def click_suspend_button + find(".btn-danger.suspend-user").click + end + + def click_silence_button + find(".btn-danger.silence-user").click + end + + def similar_users_warning + find(".penalty-similar-users .alert-warning")["innerHTML"] + end + end + end +end