From 1d926e88a9d81b973484b1c472615b7e25898be3 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 23 Dec 2022 11:45:29 +0800 Subject: [PATCH] FIX: Admin can't see user sidebar preferences of other users (#19570) --- .../app/templates/preferences/sidebar.hbs | 2 +- .../concerns/user_sidebar_mixin.rb | 48 ++++++ .../concerns/user_sidebar_tags_mixin.rb | 31 ---- app/serializers/current_user_serializer.rb | 22 +-- app/serializers/user_serializer.rb | 7 +- .../current_user_serializer_spec.rb | 101 +----------- spec/serializers/user_serializer_spec.rb | 54 ++----- .../user_sidebar_serializer_attributes.rb | 150 ++++++++++++++++++ spec/support/user_sidebar_tags_mixin.rb | 41 ----- .../pages/user_preferences_sidebar.rb | 27 ++++ .../viewing_sidebar_preferences_spec.rb | 36 +++++ 11 files changed, 283 insertions(+), 236 deletions(-) create mode 100644 app/serializers/concerns/user_sidebar_mixin.rb delete mode 100644 app/serializers/concerns/user_sidebar_tags_mixin.rb create mode 100644 spec/support/user_sidebar_serializer_attributes.rb delete mode 100644 spec/support/user_sidebar_tags_mixin.rb create mode 100644 spec/system/page_objects/pages/user_preferences_sidebar.rb create mode 100644 spec/system/viewing_sidebar_preferences_spec.rb diff --git a/app/assets/javascripts/discourse/app/templates/preferences/sidebar.hbs b/app/assets/javascripts/discourse/app/templates/preferences/sidebar.hbs index 70270db4004..a3d6eea71ba 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/sidebar.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/sidebar.hbs @@ -40,7 +40,7 @@
- +
diff --git a/app/serializers/concerns/user_sidebar_mixin.rb b/app/serializers/concerns/user_sidebar_mixin.rb new file mode 100644 index 00000000000..1149b639602 --- /dev/null +++ b/app/serializers/concerns/user_sidebar_mixin.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module UserSidebarMixin + def sidebar_tags + object.visible_sidebar_tags(scope) + .pluck(:name, :topic_count, :pm_topic_count) + .reduce([]) do |tags, sidebar_tag| + tags.push( + name: sidebar_tag[0], + pm_only: sidebar_tag[1] == 0 && sidebar_tag[2] > 0 + ) + end + end + + def display_sidebar_tags + DiscourseTagging.filter_visible(Tag, scope).exists? + end + + def include_display_sidebar_tags? + include_sidebar_tags? + end + + def include_sidebar_tags? + SiteSetting.tagging_enabled && sidebar_navigation_menu? + end + + def sidebar_category_ids + object.category_sidebar_section_links.pluck(:linkable_id) & scope.allowed_category_ids + end + + def include_sidebar_category_ids? + sidebar_navigation_menu? + end + + def sidebar_list_destination + object.user_option.sidebar_list_none_selected? ? SiteSetting.default_sidebar_list_destination : object.user_option.sidebar_list_destination + end + + def include_sidebar_list_destination? + sidebar_navigation_menu? + end + + private + + def sidebar_navigation_menu? + !SiteSetting.legacy_navigation_menu? + end +end diff --git a/app/serializers/concerns/user_sidebar_tags_mixin.rb b/app/serializers/concerns/user_sidebar_tags_mixin.rb deleted file mode 100644 index bcdfc3c93fb..00000000000 --- a/app/serializers/concerns/user_sidebar_tags_mixin.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module UserSidebarTagsMixin - def self.included(base) - base.attributes :display_sidebar_tags, - :sidebar_tags - end - - def sidebar_tags - object.visible_sidebar_tags(scope) - .pluck(:name, :topic_count, :pm_topic_count) - .reduce([]) do |tags, sidebar_tag| - tags.push( - name: sidebar_tag[0], - pm_only: sidebar_tag[1] == 0 && sidebar_tag[2] > 0 - ) - end - end - - def include_sidebar_tags? - include_display_sidebar_tags? - end - - def display_sidebar_tags - DiscourseTagging.filter_visible(Tag, scope).exists? - end - - def include_display_sidebar_tags? - SiteSetting.tagging_enabled && !SiteSetting.legacy_navigation_menu? - end -end diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 6a6f7f8febd..cb4153edefc 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -2,7 +2,7 @@ class CurrentUserSerializer < BasicUserSerializer include UserTagNotificationsMixin - include UserSidebarTagsMixin + include UserSidebarMixin attributes :name, :unread_notifications, @@ -62,12 +62,14 @@ class CurrentUserSerializer < BasicUserSerializer :draft_count, :pending_posts_count, :status, - :sidebar_category_ids, :grouped_unread_notifications, :redesigned_user_menu_enabled, :redesigned_user_page_nav_enabled, - :sidebar_list_destination, - :redesigned_topic_timeline_enabled + :redesigned_topic_timeline_enabled, + :display_sidebar_tags, + :sidebar_tags, + :sidebar_category_ids, + :sidebar_list_destination delegate :user_stat, to: :object, private: true delegate :any_posts, :draft_count, :pending_posts_count, :read_faq?, to: :user_stat @@ -100,10 +102,6 @@ class CurrentUserSerializer < BasicUserSerializer scope.can_create_group? end - def sidebar_list_destination - object.user_option.sidebar_list_none_selected? ? SiteSetting.default_sidebar_list_destination : object.user_option.sidebar_list_destination - end - def can_send_private_email_messages scope.can_send_private_messages_to_email? end @@ -253,14 +251,6 @@ class CurrentUserSerializer < BasicUserSerializer Draft.has_topic_draft(object) end - def sidebar_category_ids - object.category_sidebar_section_links.pluck(:linkable_id) & scope.allowed_category_ids - end - - def include_sidebar_category_ids? - !SiteSetting.legacy_navigation_menu? - end - def include_status? SiteSetting.enable_user_status && object.has_status? end diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 0e9f975032e..505d7364544 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -2,7 +2,7 @@ class UserSerializer < UserCardSerializer include UserTagNotificationsMixin - include UserSidebarTagsMixin + include UserSidebarMixin attributes :bio_raw, :bio_cooked, @@ -64,7 +64,10 @@ class UserSerializer < UserCardSerializer :user_auth_tokens, :user_notification_schedule, :use_logo_small_as_avatar, - :sidebar_tags + :sidebar_tags, + :sidebar_category_ids, + :sidebar_list_destination, + :display_sidebar_tags untrusted_attributes :bio_raw, :bio_cooked, diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index aec2d373320..9e2308739c7 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -220,96 +220,6 @@ RSpec.describe CurrentUserSerializer do end end - describe '#sidebar_tags' do - fab!(:tag) { Fabricate(:tag, name: "foo") } - fab!(:pm_tag) { Fabricate(:tag, name: "bar", pm_topic_count: 5, topic_count: 0) } - fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } - fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"]) } - fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } - fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user, linkable: pm_tag) } - fab!(:tag_sidebar_section_link_3) { Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag) } - - it "is not included when navigation menu is legacy" do - SiteSetting.navigation_menu = "legacy" - SiteSetting.tagging_enabled = true - - json = serializer.as_json - - expect(json[:sidebar_tags]).to eq(nil) - end - - it "is not included when tagging has not been enabled" do - SiteSetting.navigation_menu = "sidebar" - SiteSetting.tagging_enabled = false - - json = serializer.as_json - - expect(json[:sidebar_tags]).to eq(nil) - end - - it "serializes only the tags that the user can see when sidebar and tagging has been enabled" do - SiteSetting.navigation_menu = "sidebar" - SiteSetting.tagging_enabled = true - - json = serializer.as_json - - expect(json[:sidebar_tags]).to contain_exactly( - { name: tag.name, pm_only: false }, - { name: pm_tag.name, pm_only: true } - ) - - user.update!(admin: true) - - json = serializer.as_json - - expect(json[:sidebar_tags]).to contain_exactly( - { name: tag.name, pm_only: false }, - { name: pm_tag.name, pm_only: true }, - { name: hidden_tag.name, pm_only: false } - ) - end - end - - describe '#sidebar_category_ids' do - fab!(:group) { Fabricate(:group) } - fab!(:category) { Fabricate(:category) } - fab!(:category_2) { Fabricate(:category) } - fab!(:private_category) { Fabricate(:private_category, group: group) } - fab!(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category) } - fab!(:category_sidebar_section_link_2) { Fabricate(:category_sidebar_section_link, user: user, linkable: category_2) } - fab!(:category_sidebar_section_link_3) { Fabricate(:category_sidebar_section_link, user: user, linkable: private_category) } - - it "is not included when navigation menu is legacy" do - category_sidebar_section_link - SiteSetting.navigation_menu = "legacy" - - json = serializer.as_json - - expect(json[:sidebar_category_ids]).to eq(nil) - end - - it 'serializes only the categories that the user can see when sidebar and tagging has been enabled"' do - SiteSetting.navigation_menu = "sidebar" - - json = serializer.as_json - - expect(json[:sidebar_category_ids]).to eq([ - category.id, - category_2.id - ]) - - group.add(user) - serializer = described_class.new(user, scope: Guardian.new(user), root: false) - json = serializer.as_json - - expect(json[:sidebar_category_ids]).to eq([ - category.id, - category_2.id, - private_category.id - ]) - end - end - describe "#likes_notifications_disabled" do it "is true if the user disables likes notifications" do user.user_option.update!(like_notification_frequency: UserOption.like_notification_frequency_type[:never]) @@ -363,15 +273,6 @@ RSpec.describe CurrentUserSerializer do end end - describe "#sidebar_list_destination" do - it "returns choosen value or default" do - expect(serializer.as_json[:sidebar_list_destination]).to eq(SiteSetting.default_sidebar_list_destination) - - user.user_option.update!(sidebar_list_destination: "unread_new") - expect(serializer.as_json[:sidebar_list_destination]).to eq("unread_new") - end - end - describe "#new_personal_messages_notifications_count" do fab!(:notification) { Fabricate(:notification, user: user, read: false, notification_type: Notification.types[:private_message]) } @@ -388,5 +289,5 @@ RSpec.describe CurrentUserSerializer do end end - include_examples "#display_sidebar_tags", described_class + include_examples "User Sidebar Serializer Attributes", described_class end diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 44dd3c36f08..08fcb10aa41 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -372,53 +372,17 @@ RSpec.describe UserSerializer do end end - describe '#sidebar_tags' do - fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) } - fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) } + context 'for user sidebar attributes' do + include_examples "User Sidebar Serializer Attributes", described_class - context 'when viewing self' do - subject(:json) { UserSerializer.new(user, scope: Guardian.new(user), root: false).as_json } + it "does not include attributes when scoped to user that cannot edit user" do + user2 = Fabricate(:user) + serializer = described_class.new(user, scope: Guardian.new(user2), root: false) - it "is not included when navigation menu is set to legacy" do - SiteSetting.navigation_menu = "legacy" - SiteSetting.tagging_enabled = true - - expect(json[:sidebar_tags]).to eq(nil) - end - - it "is not included when SiteSetting.tagging_enabled is false" do - SiteSetting.navigation_menu = "sidebar" - SiteSetting.tagging_enabled = false - - expect(json[:sidebar_tags]).to eq(nil) - end - - it "is present when sidebar and tagging has been enabled" do - SiteSetting.navigation_menu = "sidebar" - SiteSetting.tagging_enabled = true - - tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0) - - expect(json[:sidebar_tags]).to contain_exactly( - { name: tag_sidebar_section_link.linkable.name, pm_only: false }, - { name: tag_sidebar_section_link_2.linkable.name, pm_only: true } - ) - end - end - - context 'when viewing another user' do - fab!(:user2) { Fabricate(:user) } - - subject(:json) { UserSerializer.new(user, scope: Guardian.new(user2), root: false).as_json } - - it "is not present even when sidebar and tagging has been enabled" do - SiteSetting.navigation_menu = "sidebar" - SiteSetting.tagging_enabled = true - - expect(json[:sidebar_tags]).to eq(nil) - end + expect(serializer.as_json[:sidebar_category_ids]).to eq(nil) + expect(serializer.as_json[:sidebar_tags]).to eq(nil) + expect(serializer.as_json[:sidebar_list_destination]).to eq(nil) + expect(serializer.as_json[:display_sidebar_tags]).to eq(nil) end end - - include_examples "#display_sidebar_tags", UserSerializer end diff --git a/spec/support/user_sidebar_serializer_attributes.rb b/spec/support/user_sidebar_serializer_attributes.rb new file mode 100644 index 00000000000..cf882207bd3 --- /dev/null +++ b/spec/support/user_sidebar_serializer_attributes.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| + fab!(:user) { Fabricate(:user) } + + let(:serializer) { serializer_klass.new(user, scope: Guardian.new(user), root: false) } + + before do + SiteSetting.navigation_menu = "sidebar" + end + + describe "#sidebar_list_destination" do + it 'is not included when navigation menu is legacy' do + SiteSetting.navigation_menu = "legacy" + + expect(serializer.as_json[:sidebar_list_destination]).to eq(nil) + end + + it "returns choosen value or default" do + expect(serializer.as_json[:sidebar_list_destination]).to eq(SiteSetting.default_sidebar_list_destination) + + user.user_option.update!(sidebar_list_destination: "unread_new") + + expect(serializer.as_json[:sidebar_list_destination]).to eq("unread_new") + end + end + + describe '#sidebar_category_ids' do + fab!(:group) { Fabricate(:group) } + fab!(:category) { Fabricate(:category) } + fab!(:category_2) { Fabricate(:category) } + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category) } + fab!(:category_sidebar_section_link_2) { Fabricate(:category_sidebar_section_link, user: user, linkable: category_2) } + fab!(:category_sidebar_section_link_3) { Fabricate(:category_sidebar_section_link, user: user, linkable: private_category) } + + it "is not included when navigation menu is legacy" do + SiteSetting.navigation_menu = "legacy" + + json = serializer.as_json + + expect(json[:sidebar_category_ids]).to eq(nil) + end + + it 'serializes only the categories that the user can see when sidebar has been enabled"' do + SiteSetting.navigation_menu = "sidebar" + + json = serializer.as_json + + expect(json[:sidebar_category_ids]).to eq([ + category.id, + category_2.id + ]) + + group.add(user) + serializer = serializer_klass.new(user, scope: Guardian.new(user), root: false) + json = serializer.as_json + + expect(json[:sidebar_category_ids]).to eq([ + category.id, + category_2.id, + private_category.id + ]) + end + end + + describe '#sidebar_tags' do + fab!(:tag) { Fabricate(:tag, name: "foo") } + fab!(:pm_tag) { Fabricate(:tag, name: "bar", pm_topic_count: 5, topic_count: 0) } + fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } + fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"]) } + fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } + fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user, linkable: pm_tag) } + fab!(:tag_sidebar_section_link_3) { Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag) } + + it "is not included when navigation menu is legacy" do + SiteSetting.navigation_menu = "legacy" + SiteSetting.tagging_enabled = true + + json = serializer.as_json + + expect(json[:sidebar_tags]).to eq(nil) + end + + it "is not included when tagging has not been enabled" do + SiteSetting.navigation_menu = "sidebar" + SiteSetting.tagging_enabled = false + + json = serializer.as_json + + expect(json[:sidebar_tags]).to eq(nil) + end + + it "serializes only the tags that the user can see when sidebar and tagging has been enabled" do + SiteSetting.navigation_menu = "sidebar" + SiteSetting.tagging_enabled = true + + json = serializer.as_json + + expect(json[:sidebar_tags]).to contain_exactly( + { name: tag.name, pm_only: false }, + { name: pm_tag.name, pm_only: true } + ) + + user.update!(admin: true) + + json = serializer.as_json + + expect(json[:sidebar_tags]).to contain_exactly( + { name: tag.name, pm_only: false }, + { name: pm_tag.name, pm_only: true }, + { name: hidden_tag.name, pm_only: false } + ) + end + end + + describe "#display_sidebar_tags" do + fab!(:tag) { Fabricate(:tag) } + + it 'should not be included in serialised object when navigation menu is legacy' do + SiteSetting.tagging_enabled = true + SiteSetting.navigation_menu = "legacy" + + expect(serializer.as_json[:display_sidebar_tags]).to eq(nil) + end + + it 'should not be included in serialised object when tagging has been disabled' do + SiteSetting.tagging_enabled = false + + expect(serializer.as_json[:display_sidebar_tags]).to eq(nil) + end + + it 'should be true when user has visible tags' do + SiteSetting.tagging_enabled = true + + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag.name]) + user.update!(admin: true) + + expect(serializer.as_json[:display_sidebar_tags]).to eq(true) + end + + it 'should be false when user has no visible tags' do + SiteSetting.tagging_enabled = true + + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag.name]) + + expect(serializer.as_json[:display_sidebar_tags]).to eq(false) + end + end +end diff --git a/spec/support/user_sidebar_tags_mixin.rb b/spec/support/user_sidebar_tags_mixin.rb deleted file mode 100644 index 65a1d113965..00000000000 --- a/spec/support/user_sidebar_tags_mixin.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_examples "#display_sidebar_tags" do |serializer_klass| - fab!(:tag) { Fabricate(:tag) } - fab!(:user) { Fabricate(:user) } - let(:serializer) { serializer_klass.new(user, scope: Guardian.new(user), root: false) } - - before do - SiteSetting.navigation_menu = "sidebar" - end - - it 'should not be included in serialised object when navigation menu is legacy' do - SiteSetting.tagging_enabled = true - SiteSetting.navigation_menu = "legacy" - - expect(serializer.as_json[:display_sidebar_tags]).to eq(nil) - end - - it 'should not be included in serialised object when tagging has been disabled' do - SiteSetting.tagging_enabled = false - - expect(serializer.as_json[:display_sidebar_tags]).to eq(nil) - end - - it 'should be true when user has visible tags' do - SiteSetting.tagging_enabled = true - - Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag.name]) - user.update!(admin: true) - - expect(serializer.as_json[:display_sidebar_tags]).to eq(true) - end - - it 'should be false when user has no visible tags' do - SiteSetting.tagging_enabled = true - - Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag.name]) - - expect(serializer.as_json[:display_sidebar_tags]).to eq(false) - end -end diff --git a/spec/system/page_objects/pages/user_preferences_sidebar.rb b/spec/system/page_objects/pages/user_preferences_sidebar.rb new file mode 100644 index 00000000000..d0448c5e915 --- /dev/null +++ b/spec/system/page_objects/pages/user_preferences_sidebar.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module PageObjects + module Pages + class UserPreferencesSidebar < PageObjects::Pages::Base + def visit(user) + page.visit("/u/#{user.username}/preferences/sidebar") + self + end + + def has_sidebar_categories_preference?(*categories) + category_selector_header = page.find(".category-selector .select-kit-header-wrapper") + category_selector_header.has_content?(categories.map(&:name).join(", ")) + end + + def has_sidebar_tags_preference?(*tags) + tag_selector_header = page.find(".tag-chooser .select-kit-header-wrapper") + tag_selector_header.has_content?(tags.map(&:name).join(", ")) + end + + def has_sidebar_list_destination_preference?(type) + list_selector_header = page.find(".preferences-sidebar-navigation__list-destination-selector .select-kit-header-wrapper") + list_selector_header.has_content?(I18n.t("js.user.experimental_sidebar.list_destination_#{type}")) + end + end + end +end diff --git a/spec/system/viewing_sidebar_preferences_spec.rb b/spec/system/viewing_sidebar_preferences_spec.rb new file mode 100644 index 00000000000..79938a88040 --- /dev/null +++ b/spec/system/viewing_sidebar_preferences_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +describe 'Viewing sidebar preferences', type: :system, js: true do + let(:user_preferences_sidebar_page) { PageObjects::Pages::UserPreferencesSidebar.new } + + before do + SiteSetting.navigation_menu = "sidebar" + end + + context 'as an admin' do + fab!(:admin) { Fabricate(:admin) } + fab!(:user) { Fabricate(:user) } + fab!(:category) { Fabricate(:category) } + fab!(:category2) { Fabricate(:category) } + fab!(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category) } + fab!(:category2_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category2) } + fab!(:tag) { Fabricate(:tag) } + fab!(:tag2) { Fabricate(:tag) } + fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } + fab!(:tag2_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag2) } + + before do + sign_in(admin) + end + + it 'should be able to view sidebar preferences of another user' do + user.user_option.update!(sidebar_list_destination: "unread_new") + + user_preferences_sidebar_page.visit(user) + + expect(user_preferences_sidebar_page).to have_sidebar_categories_preference(category, category2) + expect(user_preferences_sidebar_page).to have_sidebar_tags_preference(tag, tag2) + expect(user_preferences_sidebar_page).to have_sidebar_list_destination_preference("unread_new") + end + end +end