From f5221e0aec6d0bbb573db685f16a2d5f6446b1c9 Mon Sep 17 00:00:00 2001
From: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
Date: Wed, 13 Nov 2024 14:04:20 +1100
Subject: [PATCH] SECURITY: Moderators cannot see user emails.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Unless `moderators_view_emails` SiteSetting is enabled, moderators should not be able to discover users’ emails.
---
.../admin/addon/templates/logs.hbs | 10 +++---
.../app/lib/sidebar/admin-nav-map.js | 3 +-
.../app/lib/sidebar/admin-sidebar.js | 12 +++++--
.../admin/screened_emails_controller.rb | 6 ++++
app/serializers/current_user_serializer.rb | 11 +++++-
lib/guardian.rb | 6 ++++
.../admin/screened_emails_controller_spec.rb | 36 ++++++++++++++++---
spec/requests/export_csv_controller_spec.rb | 17 +++++++++
spec/system/admin_sidebar_navigation_spec.rb | 23 ++++++++++++
9 files changed, 112 insertions(+), 12 deletions(-)
diff --git a/app/assets/javascripts/admin/addon/templates/logs.hbs b/app/assets/javascripts/admin/addon/templates/logs.hbs
index e530f3cc507..854b27d01cf 100644
--- a/app/assets/javascripts/admin/addon/templates/logs.hbs
+++ b/app/assets/javascripts/admin/addon/templates/logs.hbs
@@ -3,10 +3,12 @@
@route="adminLogs.staffActionLogs"
@label="admin.logs.staff_actions.title"
/>
- <NavItem
- @route="adminLogs.screenedEmails"
- @label="admin.logs.screened_emails.title"
- />
+ {{#if this.currentUser.can_see_emails}}
+ <NavItem
+ @route="adminLogs.screenedEmails"
+ @label="admin.logs.screened_emails.title"
+ />
+ {{/if}}
<NavItem
@route="adminLogs.screenedIpAddresses"
@label="admin.logs.screened_ips.title"
diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js b/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js
index 6a830a6d3aa..16b226fa34a 100644
--- a/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js
+++ b/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js
@@ -1,5 +1,6 @@
import getURL from "discourse-common/lib/get-url";
+export const LOGS_SCREENED_EMAILS_LINK_KEY = "admin_logs_screened_emails";
export const ADMIN_NAV_MAP = [
{
name: "account",
@@ -246,7 +247,7 @@ export const ADMIN_NAV_MAP = [
icon: "external-link-alt",
},
{
- name: "admin_logs_screened_emails",
+ name: LOGS_SCREENED_EMAILS_LINK_KEY,
route: "adminLogs.screenedEmails",
label: "admin.security.sidebar_link.screened_emails",
icon: "envelope",
diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
index 483d47a04e2..b13fd984517 100644
--- a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
+++ b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js
@@ -1,7 +1,10 @@
import { cached } from "@glimmer/tracking";
import { htmlSafe } from "@ember/template";
import PreloadStore from "discourse/lib/preload-store";
-import { ADMIN_NAV_MAP } from "discourse/lib/sidebar/admin-nav-map";
+import {
+ ADMIN_NAV_MAP,
+ LOGS_SCREENED_EMAILS_LINK_KEY,
+} from "discourse/lib/sidebar/admin-nav-map";
import BaseCustomSidebarPanel from "discourse/lib/sidebar/base-custom-sidebar-panel";
import BaseCustomSidebarSection from "discourse/lib/sidebar/base-custom-sidebar-section";
import BaseCustomSidebarSectionLink from "discourse/lib/sidebar/base-custom-sidebar-section-link";
@@ -340,7 +343,12 @@ export default class AdminSidebarPanel extends BaseCustomSidebarPanel {
if (!currentUser.admin && currentUser.moderator) {
navConfig.forEach((section) => {
- section.links = section.links.filterBy("moderator");
+ section.links = section.links.filter((link) => {
+ if (link.name === LOGS_SCREENED_EMAILS_LINK_KEY) {
+ return siteSettings.moderators_view_emails;
+ }
+ return link.moderator;
+ });
});
navConfig = navConfig.filterBy("links.length");
}
diff --git a/app/controllers/admin/screened_emails_controller.rb b/app/controllers/admin/screened_emails_controller.rb
index b8688a0998d..e38175cbe26 100644
--- a/app/controllers/admin/screened_emails_controller.rb
+++ b/app/controllers/admin/screened_emails_controller.rb
@@ -1,6 +1,8 @@
# frozen_string_literal: true
class Admin::ScreenedEmailsController < Admin::StaffController
+ before_action :ensure_can_see_emails
+
def index
screened_emails = ScreenedEmail.limit(200).order("last_match_at desc").to_a
render_serialized(screened_emails, ScreenedEmailSerializer)
@@ -11,4 +13,8 @@ class Admin::ScreenedEmailsController < Admin::StaffController
screen.destroy!
render json: success_json
end
+
+ def ensure_can_see_emails
+ guardian.ensure_can_see_emails!
+ end
end
diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb
index 9d9dccf0955..9ec2e31ec70 100644
--- a/app/serializers/current_user_serializer.rb
+++ b/app/serializers/current_user_serializer.rb
@@ -76,7 +76,8 @@ class CurrentUserSerializer < BasicUserSerializer
:can_view_raw_email,
:use_glimmer_topic_list?,
:login_method,
- :render_experimental_about_page
+ :render_experimental_about_page,
+ :can_see_emails
delegate :user_stat, to: :object, private: true
delegate :any_posts, :draft_count, :pending_posts_count, :read_faq?, to: :user_stat
@@ -320,4 +321,12 @@ class CurrentUserSerializer < BasicUserSerializer
def use_glimmer_topic_list?
scope.user.in_any_groups?(SiteSetting.experimental_glimmer_topic_list_groups_map)
end
+
+ def can_see_emails
+ scope.can_see_emails?
+ end
+
+ def include_can_see_emails?
+ object.staff?
+ end
end
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 523b398dd0f..dbd212ebfa7 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -534,6 +534,7 @@ class Guardian
def can_export_entity?(entity)
return false if anonymous?
return true if is_admin?
+ return can_see_emails? if entity == "screened_email"
return entity != "user_list" if is_moderator?
# Regular users can only export their archives
@@ -544,6 +545,11 @@ class Guardian
).count == 0
end
+ def can_see_emails?
+ return true if is_admin?
+ SiteSetting.moderators_view_emails && is_moderator?
+ end
+
def can_mute_user?(target_user)
can_mute_users? && @user.id != target_user.id && !target_user.staff?
end
diff --git a/spec/requests/admin/screened_emails_controller_spec.rb b/spec/requests/admin/screened_emails_controller_spec.rb
index e8477784232..bfecb1c459a 100644
--- a/spec/requests/admin/screened_emails_controller_spec.rb
+++ b/spec/requests/admin/screened_emails_controller_spec.rb
@@ -23,8 +23,11 @@ RSpec.describe Admin::ScreenedEmailsController do
include_examples "screened emails accessible"
end
- context "when logged in as a moderator" do
- before { sign_in(moderator) }
+ context "when logged in as a moderator and has permission to view emails" do
+ before do
+ sign_in(moderator)
+ SiteSetting.moderators_view_emails = true
+ end
include_examples "screened emails accessible"
end
@@ -39,6 +42,17 @@ RSpec.describe Admin::ScreenedEmailsController do
expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
end
end
+
+ context "when logged in as a moderator but no permission to view emails" do
+ before { sign_in(moderator) }
+
+ it "denies access with a 403 response" do
+ get "/admin/logs/screened_emails.json"
+
+ expect(response.status).to eq(403)
+ expect(response.parsed_body["errors"]).to include(I18n.t("invalid_access"))
+ end
+ end
end
describe "#destroy" do
@@ -58,8 +72,11 @@ RSpec.describe Admin::ScreenedEmailsController do
include_examples "screened email deletion possible"
end
- context "when logged in as a moderator" do
- before { sign_in(moderator) }
+ context "when logged in as a moderator and has permission to view emails" do
+ before do
+ sign_in(moderator)
+ SiteSetting.moderators_view_emails = true
+ end
include_examples "screened email deletion possible"
end
@@ -74,5 +91,16 @@ RSpec.describe Admin::ScreenedEmailsController do
expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
end
end
+
+ context "when logged in as a moderator but no permission to view emails" do
+ before { sign_in(moderator) }
+
+ it "prevents deletion with a 403 response" do
+ delete "/admin/logs/screened_emails/#{screened_email.id}.json"
+
+ expect(response.status).to eq(403)
+ expect(response.parsed_body["errors"]).to include(I18n.t("invalid_access"))
+ end
+ end
end
end
diff --git a/spec/requests/export_csv_controller_spec.rb b/spec/requests/export_csv_controller_spec.rb
index d55106cb60f..b54f90452ae 100644
--- a/spec/requests/export_csv_controller_spec.rb
+++ b/spec/requests/export_csv_controller_spec.rb
@@ -97,6 +97,23 @@ RSpec.describe ExportCsvController do
expect(response.status).to eq(422)
end
+ it "does not allow moderators to export screened_email if they has no permission to view emails" do
+ SiteSetting.moderators_view_emails = false
+ post "/export_csv/export_entity.json", params: { entity: "screened_email" }
+ expect(response.status).to eq(422)
+ end
+
+ it "allows moderator to export screened_email if they has permission to view emails" do
+ SiteSetting.moderators_view_emails = true
+ post "/export_csv/export_entity.json", params: { entity: "screened_email" }
+ expect(response.status).to eq(200)
+ expect(response.parsed_body["success"]).to eq("OK")
+
+ job_data = Jobs::ExportCsvFile.jobs.first["args"].first
+ expect(job_data["entity"]).to eq("screened_email")
+ expect(job_data["user_id"]).to eq(moderator.id)
+ end
+
it "allows moderator to export other entities" do
post "/export_csv/export_entity.json", params: { entity: "staff_action" }
expect(response.status).to eq(200)
diff --git a/spec/system/admin_sidebar_navigation_spec.rb b/spec/system/admin_sidebar_navigation_spec.rb
index 84ebdac0d99..7b9788fa227 100644
--- a/spec/system/admin_sidebar_navigation_spec.rb
+++ b/spec/system/admin_sidebar_navigation_spec.rb
@@ -274,6 +274,29 @@ describe "Admin Revamp | Sidebar Navigation", type: :system do
sidebar.toggle_all_sections
+ links = page.all(".sidebar-section-link-content-text")
+ expect(links.map(&:text)).to eq(
+ [
+ "Dashboard",
+ "Users",
+ "What's New",
+ "All",
+ "Watched Words",
+ "Screened IPs",
+ "Screened URLs",
+ "Search Logs",
+ "Staff Action Logs",
+ ],
+ )
+ end
+
+ it "displays limited links for moderator with screened emails if allowed" do
+ SiteSetting.moderators_view_emails = true
+ sign_in(moderator)
+ visit("/admin")
+
+ sidebar.toggle_all_sections
+
links = page.all(".sidebar-section-link-content-text")
expect(links.map(&:text)).to eq(
[