FIX: Wrong scope used for notification levels user serializer (PR #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).

My preferences:

The same preferences seen through another user’s profile:

The other user’s real preferences:

After this fix, me looking at the other user’s preferences and it working correctly:

GitHub

object&.id seems odd to me because the object can never be nil

    @user_scope ||= begin
      if user_is_current_user
        scope
      else
        Guardian.new(user)
      end
     end

Good point, the rest of the user serializer assumes it is there. Maybe some time in the past it wasn’t? Will change :slight_smile:

@tgxworld made those fixes.

    @user_scope ||= 

This worries me a bit because BasicUserSerializer is a superclass to so many things. It would be so easy to accidentally leak information for a user who can’t see it.

I don’t think we should muck with the guardian here, but instead change notification_levels_for(scope) to accept a guardian (scope) and a target user.

@eviltrout I am not sure why I am following how this would leak information? The notification_levels_for methods just use scope.user and scope.anonymous. Passing in a scope and a user seems unnecessary when I could just pass in the scope for the correct user.

Also all of the notification level attributes of the serializer are private, and as such are only sent back to the client if scope.can_edit?(object) so the risk of seeing the wrong user’s stuff is low IMO:

To be clear I agree your current implementation is safe. What concerns me is other developers coming across this API in the future and accidentally misusing it. To my knowledge, none of our core code relies on creating a Guardian instance for the thing you are looking at. A Guardian should always represent “the user performing the action.”

By changing the API to something like this:

notification_levels_for(guardian, user)

Both parameters would be required and it would be very difficult for someone in the future to mess it up. Also, you could remove all that logic in the BasicUserSerializer for user_scope.

In the same vein, (but not nearly as important to me), it might be cleaner to do scope.can_edit?(user) in your include_ methods rather than checking is_current_user || admin?

Ah I see what you are saying now, it is more about keeping consistent. Okay I will change the notification_levels_for and remove the user_scope from the serializer, however I am going to leave is_current_user as is – I merely refactored an existing check out into a method, and I do not want to change the meaning of it. I think it is a valid check and just replaces doing (object.id && object.id == scope.user.try(:id)) repeatedly.

@eviltrout I ended up just changing notification_levels_for to accept a user. It was entirely unnecessary for it 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?. I ripped out the other user serializer changes too. Merging today then we can make further tweaks if necessary.

Awesome, I prefer this quite a bit. Thanks for doing the work.