FIX: Only modify secured sidebar links on user promotion/demotion (#19141)

* FIX: Only modify secured sidebar links on user promotion/demotion

If a user is created populate their sidebar with the default
categories/tags that they have access to.

If a user is promoted to admin populate any new categories/tags that
they now have access to.

If an admin is demoted remove any categories/tags that they no longer
have access to.

This will only apply for "secured" categories. For example if these are
the default sitebar categories:

- general
- site feedback
- staff

and a user only has these sidebar categories:

- general

when they are promoted to admin they will only receive the "staff"
category. As this is a default category they didn't previously have
access to.

* Add spec, remove tag logic on update

Change it so that if a user becomes unstaged it used the "add" method
instead of the "update" method because it is essentially following the
on_create path.

On admin promotion/demotion remove the logic for updating sidebar tags because
we don't currently have the tag equivalent like we do for User.secure_categories.

Added the test case for when a user is promoted to admin it should
receive *only* the new sidebar categories they didn't previously have
access to. Same for admin demotion.

* Add spec for suppress_secured_categories_from_admin site setting

* Update tags as well on admin promotion/demotion

* only update tags when they are enabled

* Use new SidebarSectionLinkUpdater

We now have a SidebarSectionLinkUpdater
that was introduced in: fb2507c6ce

* remove empty line

app/models/user.rb

@@ -85,7 +85,7 @@ class User < ActiveRecord::Base
 
   has_many :topics_allowed, through: :topic_allowed_users, source: :topic
   has_many :groups, through: :group_users
-  has_many :secure_categories, through: :groups, source: :categories
+  has_many :secure_categories, -> { distinct }, through: :groups, source: :categories
   has_many :associated_groups, through: :user_associated_groups, dependent: :destroy
 
   # deleted in user_second_factors relationship
@@ -135,10 +135,14 @@ class User < ActiveRecord::Base
   after_create :ensure_in_trust_level_group
   after_create :set_default_categories_preferences
   after_create :set_default_tags_preferences
-  after_create :set_default_sidebar_section_links
+  after_create :add_default_sidebar_section_links
 
-  after_update :set_default_sidebar_section_links, if: Proc.new  {
-    self.saved_change_to_staged? || self.saved_change_to_admin?
+  after_update :update_default_sidebar_section_links, if: Proc.new  {
+    self.saved_change_to_admin?
+  }
+
+  after_update :add_default_sidebar_section_links, if: Proc.new {
+    self.saved_change_to_staged?
   }
 
   after_update :trigger_user_updated_event, if: Proc.new {
@@ -1946,25 +1950,54 @@ class User < ActiveRecord::Base
 
   private
 
-  def set_default_sidebar_section_links
+  def set_default_sidebar_section_links(update: false)
     return if !SiteSetting.enable_experimental_sidebar_hamburger
     return if staged? || bot?
 
     if SiteSetting.default_sidebar_categories.present?
+      categories_to_update = SiteSetting.default_sidebar_categories.split("|")
+
+      if update
+        filtered_default_category_ids = Category.secured(self.guardian).where(id: categories_to_update).pluck(:id)
+        existing_category_ids = SidebarSectionLink.where(user: self, linkable_type: 'Category').pluck(:linkable_id)
+
+        categories_to_update = existing_category_ids + (filtered_default_category_ids & self.secure_category_ids)
+      end
+
       SidebarSectionLinksUpdater.update_category_section_links(
         self,
-        category_ids: SiteSetting.default_sidebar_categories.split("|")
+        category_ids: categories_to_update
       )
     end
 
     if SiteSetting.tagging_enabled && SiteSetting.default_sidebar_tags.present?
+      tags_to_update = SiteSetting.default_sidebar_tags.split("|")
+
+      if update
+        default_tag_ids = Tag.where(name: tags_to_update).pluck(:id)
+        filtered_default_tags = DiscourseTagging.filter_visible(Tag, self.guardian).where(id: default_tag_ids).pluck(:name)
+
+        existing_tag_ids = SidebarSectionLink.where(user: self, linkable_type: 'Tag').pluck(:linkable_id)
+        existing_tags = DiscourseTagging.filter_visible(Tag, self.guardian).where(id: existing_tag_ids).pluck(:name)
+
+        tags_to_update = existing_tags + (filtered_default_tags & DiscourseTagging.hidden_tag_names)
+      end
+
       SidebarSectionLinksUpdater.update_tag_section_links(
         self,
-        tag_names: SiteSetting.default_sidebar_tags.split("|")
+        tag_names: tags_to_update
       )
     end
   end
 
+  def add_default_sidebar_section_links
+    set_default_sidebar_section_links
+  end
+
+  def update_default_sidebar_section_links
+    set_default_sidebar_section_links(update: true)
+  end
+
   def stat
     user_stat || create_user_stat
   end

spec/models/user_spec.rb

@@ -64,19 +64,47 @@ RSpec.describe User do
           tag.id,
           hidden_tag.id
         )
+      end
 
-        # A user promoted to admin should get secured sidebar records
+      it 'should create and remove the right sidebar section link records when user is promoted/demoted as an admin' do
+        user = Fabricate(:user)
+        another_category = Fabricate(:category)
+        another_tag = Fabricate(:tag)
+
+        # User has customized their sidebar categories and tags
+        SidebarSectionLink.where(user: user).delete_all
+        SidebarSectionLinksUpdater.update_category_section_links(user, category_ids: [another_category.id])
+        SidebarSectionLinksUpdater.update_tag_section_links(user, tag_names: [another_tag.name])
+
+        # A user promoted to admin now has any default categories/tags they didn't previously have access to
         user.update(admin: true)
-
         expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to contain_exactly(
-          category.id,
+          another_category.id,
           secured_category.id
         )
-
         expect(SidebarSectionLink.where(linkable_type: 'Tag', user_id: user.id).pluck(:linkable_id)).to contain_exactly(
-          tag.id,
+          another_tag.id,
           hidden_tag.id
         )
+
+        # User still has their customized sidebar categories and tags after demotion
+        user.update(admin: false)
+        expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to contain_exactly(
+          another_category.id
+        )
+        expect(SidebarSectionLink.where(linkable_type: 'Tag', user_id: user.id).pluck(:linkable_id)).to contain_exactly(
+          another_tag.id
+        )
+      end
+
+      it 'should not receive any new categories w/ suppress secured categories from admin enabled' do
+        SiteSetting.suppress_secured_categories_from_admin = true
+        user = Fabricate(:user)
+        SidebarSectionLink.where(user: user).delete_all # User has customized their sidebar categories
+        user.update(admin: true)
+        expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to be_empty
+        user.update(admin: false)
+        expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to be_empty
       end
 
       it 'should not create any sidebar section link records when experimental sidebar is disabled' do