From 38742bc2080eb4ba3d55ff05288b1b2507747774 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 14 May 2021 09:45:14 +1000 Subject: [PATCH] FIX: Wrong scope used for notification levels user serializer (#13039) This is a recent regression introduced by https://github.com/discourse/discourse/pull/12937 which makes it so that when looking at a user profile that is not your own, specifically the category and tag notification settings, you would see your own settings instead of the target user. This is only a problem for admins because regular users cannot see these details for other users. The issue was that we were using `scope` in the serializer, which refers to the current user, rather than using a scope for the target user via `Guardian.new(user)`. However, on further inspection the `notification_levels_for` method for `TagUser` and `CategoryUser` did not actually need to be accepting an instance of Guardian, all that it was using it for was to check guardian.anonymous? which is just a fancy way of saying user.blank?. Changed this method to just accept a user instead and send the user in from the serializer. --- app/models/category_list.rb | 2 +- app/models/category_user.rb | 6 +-- app/models/site.rb | 2 +- app/models/tag_user.rb | 6 +-- app/serializers/basic_user_serializer.rb | 8 +++- app/serializers/current_user_serializer.rb | 2 +- app/serializers/user_serializer.rb | 12 +++--- spec/models/category_user_spec.rb | 5 +-- spec/models/group_user_spec.rb | 6 +-- spec/models/tag_user_spec.rb | 5 +-- spec/serializers/user_serializer_spec.rb | 45 ++++++++++++++++++++++ 11 files changed, 73 insertions(+), 26 deletions(-) diff --git a/app/models/category_list.rb b/app/models/category_list.rb index babb15164a0..4144f79b423 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -100,7 +100,7 @@ class CategoryList @categories = @categories.to_a - notification_levels = CategoryUser.notification_levels_for(@guardian) + notification_levels = CategoryUser.notification_levels_for(@guardian.user) default_notification_level = CategoryUser.default_notification_level allowed_topic_create = Set.new(Category.topic_create_allowed(@guardian).pluck(:id)) diff --git a/app/models/category_user.rb b/app/models/category_user.rb index 8a664c7fd05..84d3744db08 100644 --- a/app/models/category_user.rb +++ b/app/models/category_user.rb @@ -201,10 +201,10 @@ class CategoryUser < ActiveRecord::Base SiteSetting.mute_all_categories_by_default ? notification_levels[:muted] : notification_levels[:regular] end - def self.notification_levels_for(guardian) + def self.notification_levels_for(user) # Anonymous users have all default categories set to regular tracking, # except for default muted categories which stay muted. - if guardian.anonymous? + if user.blank? notification_levels = [ SiteSetting.default_categories_watching.split("|"), SiteSetting.default_categories_tracking.split("|"), @@ -218,7 +218,7 @@ class CategoryUser < ActiveRecord::Base [id.to_i, self.notification_levels[:muted]] end else - notification_levels = CategoryUser.where(user: guardian.user).pluck(:category_id, :notification_level) + notification_levels = CategoryUser.where(user: user).pluck(:category_id, :notification_level) end Hash[*notification_levels.flatten] diff --git a/app/models/site.rb b/app/models/site.rb index de8cfe0be58..c7c2d7a1c14 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -55,7 +55,7 @@ class Site by_id = {} - notification_levels = CategoryUser.notification_levels_for(@guardian) + notification_levels = CategoryUser.notification_levels_for(@guardian.user) default_notification_level = CategoryUser.default_notification_level categories.each do |category| diff --git a/app/models/tag_user.rb b/app/models/tag_user.rb index e44c458448d..6870ffbb5b5 100644 --- a/app/models/tag_user.rb +++ b/app/models/tag_user.rb @@ -192,10 +192,10 @@ class TagUser < ActiveRecord::Base auto_track_tag: TopicUser.notification_reasons[:auto_track_tag]) end - def self.notification_levels_for(guardian) + def self.notification_levels_for(user) # Anonymous users have all default tags set to regular tracking, # except for default muted tags which stay muted. - if guardian.anonymous? + if user.blank? notification_levels = [ SiteSetting.default_tags_watching_first_post.split("|"), SiteSetting.default_tags_watching.split("|"), @@ -208,7 +208,7 @@ class TagUser < ActiveRecord::Base [name, self.notification_levels[:muted]] end else - notification_levels = TagUser.where(user: guardian.user).joins(:tag).pluck("tags.name", :notification_level) + notification_levels = TagUser.where(user: user).joins(:tag).pluck("tags.name", :notification_level) end Hash[*notification_levels.flatten] diff --git a/app/serializers/basic_user_serializer.rb b/app/serializers/basic_user_serializer.rb index b6daa48a06f..26249bb4833 100644 --- a/app/serializers/basic_user_serializer.rb +++ b/app/serializers/basic_user_serializer.rb @@ -23,6 +23,10 @@ class BasicUserSerializer < ApplicationSerializer object[:user] || object.try(:user) || object end + def user_is_current_user + object.id == scope.user&.id + end + def categories_with_notification_level(lookup_level) category_user_notification_levels.select do |id, level| level == CategoryUser.notification_levels[lookup_level] @@ -30,7 +34,7 @@ class BasicUserSerializer < ApplicationSerializer end def category_user_notification_levels - @category_user_notification_levels ||= CategoryUser.notification_levels_for(scope) + @category_user_notification_levels ||= CategoryUser.notification_levels_for(user) end def tags_with_notification_level(lookup_level) @@ -40,6 +44,6 @@ class BasicUserSerializer < ApplicationSerializer end def tag_user_notification_levels - @tag_user_notification_levels ||= TagUser.notification_levels_for(scope) + @tag_user_notification_levels ||= TagUser.notification_levels_for(user) end end diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 053a8055a82..6b6f015876e 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -225,7 +225,7 @@ class CurrentUserSerializer < BasicUserSerializer end def tracked_tags - tags_with_notification_level(:tracked) + tags_with_notification_level(:tracking) end def watching_first_post_tags diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index d1bb28dc4e2..1c9afecd4e1 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -89,15 +89,15 @@ class UserSerializer < UserCardSerializer end def include_group_users? - (object.id && object.id == scope.user.try(:id)) || scope.is_admin? + user_is_current_user || scope.is_admin? end def include_associated_accounts? - (object.id && object.id == scope.user.try(:id)) + user_is_current_user end def include_second_factor_enabled? - (object&.id == scope.user&.id) || scope.is_admin? + user_is_current_user || scope.is_admin? end def second_factor_enabled @@ -105,7 +105,7 @@ class UserSerializer < UserCardSerializer end def include_second_factor_backup_enabled? - object&.id == scope.user&.id + user_is_current_user end def second_factor_backup_enabled @@ -113,7 +113,7 @@ class UserSerializer < UserCardSerializer end def include_second_factor_remaining_backup_codes? - (object&.id == scope.user&.id) && object.backup_codes_enabled? + user_is_current_user && object.backup_codes_enabled? end def second_factor_remaining_backup_codes @@ -211,7 +211,7 @@ class UserSerializer < UserCardSerializer end def tracked_tags - tags_with_notification_level(:tracked) + tags_with_notification_level(:tracking) end def watching_first_post_tags diff --git a/spec/models/category_user_spec.rb b/spec/models/category_user_spec.rb index dc05ab12e85..58be7482491 100644 --- a/spec/models/category_user_spec.rb +++ b/spec/models/category_user_spec.rb @@ -222,7 +222,6 @@ describe CategoryUser do end describe "#notification_levels_for" do - let(:guardian) { Guardian.new(user) } let!(:category1) { Fabricate(:category) } let!(:category2) { Fabricate(:category) } let!(:category3) { Fabricate(:category) } @@ -239,7 +238,7 @@ describe CategoryUser do SiteSetting.default_categories_muted = category5.id.to_s end it "every category from the default_categories_* site settings get overridden to regular, except for muted" do - levels = CategoryUser.notification_levels_for(guardian) + levels = CategoryUser.notification_levels_for(user) expect(levels[category1.id]).to eq(CategoryUser.notification_levels[:regular]) expect(levels[category2.id]).to eq(CategoryUser.notification_levels[:regular]) expect(levels[category3.id]).to eq(CategoryUser.notification_levels[:regular]) @@ -259,7 +258,7 @@ describe CategoryUser do it "gets the category_user notification levels for all categories the user is tracking and does not include categories the user is not tracking at all" do category6 = Fabricate(:category) - levels = CategoryUser.notification_levels_for(guardian) + levels = CategoryUser.notification_levels_for(user) expect(levels[category1.id]).to eq(CategoryUser.notification_levels[:watching]) expect(levels[category2.id]).to eq(CategoryUser.notification_levels[:tracking]) expect(levels[category3.id]).to eq(CategoryUser.notification_levels[:watching_first_post]) diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index 22b01023b1e..e6b3e55af42 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -57,7 +57,7 @@ describe GroupUser do group.watching_first_post_category_ids = [category5.id] group.save! expect { group.add(user) }.to change { CategoryUser.count }.by(5) - h = CategoryUser.notification_levels_for(Guardian.new(user)) + h = CategoryUser.notification_levels_for(user) expect(h[category1.id]).to eq(levels[:muted]) expect(h[category2.id]).to eq(levels[:regular]) expect(h[category3.id]).to eq(levels[:tracking]) @@ -74,7 +74,7 @@ describe GroupUser do group.watching_first_post_category_ids = [category2.id, category3.id, category4.id] group.save! group.add(user) - h = CategoryUser.notification_levels_for(Guardian.new(user)) + h = CategoryUser.notification_levels_for(user) expect(h[category1.id]).to eq(levels[:regular]) expect(h[category2.id]).to eq(levels[:watching_first_post]) expect(h[category3.id]).to eq(levels[:watching_first_post]) @@ -89,7 +89,7 @@ describe GroupUser do group.tracking_category_ids = [category4.id] group.save! group.add(user) - h = CategoryUser.notification_levels_for(Guardian.new(user)) + h = CategoryUser.notification_levels_for(user) expect(h[category1.id]).to eq(levels[:tracking]) expect(h[category2.id]).to eq(levels[:watching]) expect(h[category3.id]).to eq(levels[:muted]) diff --git a/spec/models/tag_user_spec.rb b/spec/models/tag_user_spec.rb index 8b6a2933d26..3777d8c8f08 100644 --- a/spec/models/tag_user_spec.rb +++ b/spec/models/tag_user_spec.rb @@ -234,7 +234,6 @@ describe TagUser do end describe "#notification_levels_for" do - let(:guardian) { Guardian.new(user) } let!(:tag1) { Fabricate(:tag) } let!(:tag2) { Fabricate(:tag) } let!(:tag3) { Fabricate(:tag) } @@ -249,7 +248,7 @@ describe TagUser do SiteSetting.default_tags_muted = tag4.name end it "every tag from the default_tags_* site settings get overridden to watching_first_post, except for muted" do - levels = TagUser.notification_levels_for(guardian) + levels = TagUser.notification_levels_for(user) expect(levels[tag1.name]).to eq(TagUser.notification_levels[:regular]) expect(levels[tag2.name]).to eq(TagUser.notification_levels[:regular]) expect(levels[tag3.name]).to eq(TagUser.notification_levels[:regular]) @@ -268,7 +267,7 @@ describe TagUser do it "gets the tag_user notification levels for all tags the user is tracking and does not include tags the user is not tracking at all" do tag5 = Fabricate(:tag) - levels = TagUser.notification_levels_for(guardian) + levels = TagUser.notification_levels_for(user) expect(levels[tag1.name]).to eq(TagUser.notification_levels[:watching]) expect(levels[tag2.name]).to eq(TagUser.notification_levels[:tracking]) expect(levels[tag3.name]).to eq(TagUser.notification_levels[:watching_first_post]) diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index cad83e3ac8e..7179a9985eb 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -60,6 +60,7 @@ describe UserSerializer do end context "with a user" do + let(:admin_user) { Fabricate(:admin) } let(:scope) { Guardian.new } fab!(:user) { Fabricate(:user) } let(:serializer) { UserSerializer.new(user, scope: scope, root: false) } @@ -67,6 +68,50 @@ describe UserSerializer do fab!(:upload) { Fabricate(:upload) } fab!(:upload2) { Fabricate(:upload) } + context "when the scope user is admin" do + let(:scope) { Guardian.new(admin_user) } + + it "returns the user's category notification levels, not the scope user's" do + category1 = Fabricate(:category) + category2 = Fabricate(:category) + category3 = Fabricate(:category) + category4 = Fabricate(:category) + CategoryUser.create(category: category1, user: user, notification_level: CategoryUser.notification_levels[:muted]) + CategoryUser.create(category: Fabricate(:category), user: admin_user, notification_level: CategoryUser.notification_levels[:muted]) + CategoryUser.create(category: category2, user: user, notification_level: CategoryUser.notification_levels[:tracking]) + CategoryUser.create(category: Fabricate(:category), user: admin_user, notification_level: CategoryUser.notification_levels[:tracking]) + CategoryUser.create(category: category3, user: user, notification_level: CategoryUser.notification_levels[:watching]) + CategoryUser.create(category: Fabricate(:category), user: admin_user, notification_level: CategoryUser.notification_levels[:watching]) + CategoryUser.create(category: category4, user: user, notification_level: CategoryUser.notification_levels[:regular]) + CategoryUser.create(category: Fabricate(:category), user: admin_user, notification_level: CategoryUser.notification_levels[:regular]) + + expect(json[:muted_category_ids]).to eq([category1.id]) + expect(json[:tracked_category_ids]).to eq([category2.id]) + expect(json[:watched_category_ids]).to eq([category3.id]) + expect(json[:regular_category_ids]).to eq([category4.id]) + end + + it "returns the user's tag notification levels, not the scope user's" do + tag1 = Fabricate(:tag) + tag2 = Fabricate(:tag) + tag3 = Fabricate(:tag) + tag4 = Fabricate(:tag) + TagUser.create(tag: tag1, user: user, notification_level: TagUser.notification_levels[:muted]) + TagUser.create(tag: Fabricate(:tag), user: admin_user, notification_level: TagUser.notification_levels[:muted]) + TagUser.create(tag: tag2, user: user, notification_level: TagUser.notification_levels[:tracking]) + TagUser.create(tag: Fabricate(:tag), user: admin_user, notification_level: TagUser.notification_levels[:tracking]) + TagUser.create(tag: tag3, user: user, notification_level: TagUser.notification_levels[:watching]) + TagUser.create(tag: Fabricate(:tag), user: admin_user, notification_level: TagUser.notification_levels[:watching]) + TagUser.create(tag: tag4, user: user, notification_level: TagUser.notification_levels[:watching_first_post]) + TagUser.create(tag: Fabricate(:tag), user: admin_user, notification_level: TagUser.notification_levels[:watching_first_post]) + + expect(json[:muted_tags]).to eq([tag1.name]) + expect(json[:tracked_tags]).to eq([tag2.name]) + expect(json[:watched_tags]).to eq([tag3.name]) + expect(json[:watching_first_post_tags]).to eq([tag4.name]) + end + end + context "with `enable_names` true" do before do SiteSetting.enable_names = true