From 738f1958d815ec27729e4c913e1e0f1b03874277 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Mon, 5 Dec 2022 11:39:10 -0700 Subject: [PATCH] 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: fb2507c6ce80a1f4fc3a68a23342e5eca765a13f * remove empty line --- app/models/user.rb | 47 ++++++++++++++++++++++++++++++++++------ spec/models/user_spec.rb | 38 +++++++++++++++++++++++++++----- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 79a6fc22d62..766ef7bf8d0 100644 --- a/app/models/user.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 38540d40173..b7276d5caa0 100644 --- a/spec/models/user_spec.rb +++ b/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