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

@@ -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))

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]

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|

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]

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

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

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

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])

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])

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])

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