FIX: Wrong scope used for notification levels user serializer (#13039)

FIX: Wrong scope used for notification levels user serializer (#13039)

This is a recent regression introduced by FIX: Base topic details message on current category and tag tracking state by martin-brennan · Pull Request #12937 · discourse/discourse · GitHub 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.

diff --git a/app/models/category_list.rb b/app/models/category_list.rb
index babb151..4144f79 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 8a664c7..84d3744 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 de8cfe0..c7c2d7a 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 e44c458..6870ffb 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 b6daa48..26249bb 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 053a805..6b6f015 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 d1bb28d..1c9afec 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 dc05ab1..58be748 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)

[... diff too long, it was truncated ...]

GitHub sha: 38742bc2

This commit appears in #13039 which was approved by tgxworld. It was merged by martin.