diff --git a/app/models/user.rb b/app/models/user.rb index 228f536ac58..f4f7ef3f067 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -137,13 +137,6 @@ class User < ActiveRecord::Base belongs_to :uploaded_avatar, class_name: "Upload" has_many :sidebar_section_links, dependent: :delete_all - has_many :category_sidebar_section_links, - -> { where(linkable_type: "Category") }, - class_name: "SidebarSectionLink" - has_many :custom_sidebar_tags, - through: :sidebar_section_links, - source: :linkable, - source_type: "Tag" delegate :last_sent_email_address, to: :email_logs @@ -170,11 +163,8 @@ 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 :add_default_sidebar_section_links - - 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_create :set_default_sidebar_section_links + after_update :set_default_sidebar_section_links, if: Proc.new { self.saved_change_to_staged? } after_update :trigger_user_updated_event, if: Proc.new { self.human? && self.saved_change_to_uploaded_avatar_id? } @@ -360,9 +350,22 @@ class User < ActiveRecord::Base ) end + def secured_sidebar_category_ids(user_guardian = nil) + user_guardian ||= guardian + + SidebarSectionLink.where(user_id: self.id, linkable_type: "Category").pluck(:linkable_id) & + user_guardian.allowed_category_ids + end + def visible_sidebar_tags(user_guardian = nil) user_guardian ||= guardian - DiscourseTagging.filter_visible(custom_sidebar_tags, user_guardian) + + DiscourseTagging.filter_visible( + Tag.where( + id: SidebarSectionLink.where(user_id: self.id, linkable_type: "Tag").select(:linkable_id), + ), + user_guardian, + ) end def self.max_password_length @@ -2057,16 +2060,6 @@ class User < ActiveRecord::Base if SiteSetting.default_navigation_menu_categories.present? categories_to_update = SiteSetting.default_navigation_menu_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: categories_to_update, @@ -2074,39 +2067,13 @@ class User < ActiveRecord::Base end if SiteSetting.tagging_enabled && SiteSetting.default_navigation_menu_tags.present? - tags_to_update = SiteSetting.default_navigation_menu_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: tags_to_update) + SidebarSectionLinksUpdater.update_tag_section_links( + self, + tag_ids: Tag.where(name: SiteSetting.default_navigation_menu_tags.split("|")).pluck(:id), + ) 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/app/serializers/concerns/user_sidebar_mixin.rb b/app/serializers/concerns/user_sidebar_mixin.rb index bfeddacee46..549ca54c695 100644 --- a/app/serializers/concerns/user_sidebar_mixin.rb +++ b/app/serializers/concerns/user_sidebar_mixin.rb @@ -20,7 +20,7 @@ module UserSidebarMixin end def sidebar_category_ids - object.category_sidebar_section_links.pluck(:linkable_id) & scope.allowed_category_ids + object.secured_sidebar_category_ids(scope) end def include_sidebar_category_ids? diff --git a/app/services/sidebar_section_links_updater.rb b/app/services/sidebar_section_links_updater.rb index b3faacb4f08..682372aafb3 100644 --- a/app/services/sidebar_section_links_updater.rb +++ b/app/services/sidebar_section_links_updater.rb @@ -5,18 +5,15 @@ class SidebarSectionLinksUpdater if category_ids.blank? delete_section_links(user: user, linkable_type: "Category") else - category_ids = Category.secured(Guardian.new(user)).where(id: category_ids).pluck(:id) + category_ids = Category.where(id: category_ids).pluck(:id) update_section_links(user: user, linkable_type: "Category", new_linkable_ids: category_ids) end end - def self.update_tag_section_links(user, tag_names:) - if tag_names.blank? + def self.update_tag_section_links(user, tag_ids:) + if tag_ids.blank? delete_section_links(user: user, linkable_type: "Tag") else - tag_ids = - DiscourseTagging.filter_visible(Tag, Guardian.new(user)).where(name: tag_names).pluck(:id) - update_section_links(user: user, linkable_type: "Tag", new_linkable_ids: tag_ids) end end @@ -46,6 +43,7 @@ class SidebarSectionLinksUpdater linkable_id: to_delete, ).delete_all end + SidebarSectionLink.insert_all(to_insert_attributes) if to_insert_attributes.present? end end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 4ceee214d22..4b6e4bc6781 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -65,6 +65,7 @@ class UserUpdater def initialize(actor, user) @user = user + @user_guardian = Guardian.new(user) @guardian = Guardian.new(actor) @actor = actor end @@ -153,10 +154,10 @@ class UserUpdater # special handling for theme_id cause we need to bump a sequence number if attributes.key?(:theme_ids) - user_guardian = Guardian.new(user) attributes[:theme_ids].reject!(&:blank?) attributes[:theme_ids].map!(&:to_i) - if user_guardian.allow_themes?(attributes[:theme_ids]) + + if @user_guardian.allow_themes?(attributes[:theme_ids]) user.user_option.theme_key_seq += 1 if user.user_option.theme_ids != attributes[:theme_ids] else attributes.delete(:theme_ids) @@ -211,14 +212,22 @@ class UserUpdater if attributes.key?(:sidebar_category_ids) SidebarSectionLinksUpdater.update_category_section_links( user, - category_ids: attributes[:sidebar_category_ids], + category_ids: + Category + .secured(@user_guardian) + .where(id: attributes[:sidebar_category_ids]) + .pluck(:id), ) end if attributes.key?(:sidebar_tag_names) && SiteSetting.tagging_enabled SidebarSectionLinksUpdater.update_tag_section_links( user, - tag_names: attributes[:sidebar_tag_names], + tag_ids: + DiscourseTagging + .filter_visible(Tag, @user_guardian) + .where(name: attributes[:sidebar_tag_names]) + .pluck(:id), ) end diff --git a/spec/lib/sidebar_section_links_updater_spec.rb b/spec/lib/sidebar_section_links_updater_spec.rb index e9c54d0e250..a0be7e5c3de 100644 --- a/spec/lib/sidebar_section_links_updater_spec.rb +++ b/spec/lib/sidebar_section_links_updater_spec.rb @@ -7,12 +7,11 @@ RSpec.describe SidebarSectionLinksUpdater do describe ".update_category_section_links" do fab!(:public_category) { Fabricate(:category) } fab!(:public_category2) { Fabricate(:category) } - fab!(:group) { Fabricate(:group) } - fab!(:secured_category) { Fabricate(:private_category, group: group) } fab!(:user_category_section_link) do Fabricate(:category_sidebar_section_link, linkable: public_category, user: user) end + fab!(:user2_category_section_link) do Fabricate(:category_sidebar_section_link, linkable: public_category, user: user2) end @@ -24,71 +23,49 @@ RSpec.describe SidebarSectionLinksUpdater do expect(SidebarSectionLink.exists?(linkable: public_category, user: user2)).to eq(true) end - it "updates user's sidebar category section link records to given category ids except for category restricted to user" do + it "updates user's sidebar category section link records to given category ids" do expect( SidebarSectionLink.where(linkable_type: "Category", user: user).pluck(:linkable_id), ).to contain_exactly(public_category.id) described_class.update_category_section_links( user, - category_ids: [public_category2.id, secured_category.id], + category_ids: [public_category.id, public_category2.id], ) expect( SidebarSectionLink.where(linkable_type: "Category", user: user).pluck(:linkable_id), - ).to contain_exactly(public_category2.id) - - group.add(user) - - described_class.update_category_section_links( - user, - category_ids: [public_category2.id, secured_category.id], - ) - - expect( - SidebarSectionLink.where(linkable_type: "Category", user: user).pluck(:linkable_id), - ).to contain_exactly(public_category2.id, secured_category.id) + ).to contain_exactly(public_category.id, public_category2.id) end end describe ".update_tag_section_links" do fab!(:tag) { Fabricate(:tag) } fab!(:tag2) { Fabricate(:tag) } - fab!(:hidden_tag) { Fabricate(:tag) } - fab!(:staff_tag_group) do - Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) - end fab!(:user_tag_section_link) { Fabricate(:tag_sidebar_section_link, linkable: tag, user: user) } + fab!(:user2_tag_section_link) do Fabricate(:tag_sidebar_section_link, linkable: tag, user: user2) end it "deletes all sidebar tag section links when tag names provided is blank" do - described_class.update_tag_section_links(user, tag_names: []) + described_class.update_tag_section_links(user, tag_ids: []) expect(SidebarSectionLink.exists?(linkable: tag, user: user)).to eq(false) expect(SidebarSectionLink.exists?(linkable: tag, user: user2)).to eq(true) end - it "updates user's sidebar tag section link records to given tag names except for tags not visible to user" do + it "updates user's sidebar tag section link records to given tag names" do expect( SidebarSectionLink.where(linkable_type: "Tag", user: user).pluck(:linkable_id), ).to contain_exactly(tag.id) - described_class.update_tag_section_links(user, tag_names: [tag2.name, hidden_tag.name]) + described_class.update_tag_section_links(user, tag_ids: [tag.id, tag2.id]) expect( SidebarSectionLink.where(linkable_type: "Tag", user: user).pluck(:linkable_id), - ).to contain_exactly(tag2.id) - - user.update!(admin: true) - - described_class.update_tag_section_links(user, tag_names: [tag2.name, hidden_tag.name]) - - expect( - SidebarSectionLink.where(linkable_type: "Tag", user: user).pluck(:linkable_id), - ).to contain_exactly(tag2.id, hidden_tag.id) + ).to contain_exactly(tag.id, tag2.id) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f05ad465f56..acdd0904f19 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -52,76 +52,18 @@ RSpec.describe User do SiteSetting.default_navigation_menu_tags = "#{tag.name}|#{hidden_tag.name}" end - it "creates the right sidebar section link records for categories and tags that a user can see" do + it "creates sidebar section link records for categories and tags that have been configured as defaults" do user = Fabricate(:user) expect( SidebarSectionLink.where(linkable_type: "Category", user_id: user.id).pluck(:linkable_id), - ).to contain_exactly(category.id) - - expect( - SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id), - ).to contain_exactly(tag.id) - - admin = Fabricate(:admin) - - expect( - SidebarSectionLink.where(linkable_type: "Category", user_id: admin.id).pluck( - :linkable_id, - ), ).to contain_exactly(category.id, secured_category.id) expect( - SidebarSectionLink.where(linkable_type: "Tag", user_id: admin.id).pluck(:linkable_id), + SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id), ).to contain_exactly(tag.id, hidden_tag.id) end - 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(another_category.id, secured_category.id) - expect( - SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id), - ).to contain_exactly(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 navigation_menu site setting is still legacy" do SiteSetting.navigation_menu = "legacy" @@ -3372,6 +3314,29 @@ RSpec.describe User do end end + describe "#secured_sidebar_category_ids" do + fab!(:user) { Fabricate(:user) } + fab!(:category) { Fabricate(:category) } + fab!(:group) { Fabricate(:group) } + fab!(:secured_category) { Fabricate(:private_category, group: group) } + + fab!(:category_sidebar_section_link) do + Fabricate(:category_sidebar_section_link, user: user, linkable: category) + end + + fab!(:secured_category_sidebar_section_link) do + Fabricate(:category_sidebar_section_link, user: user, linkable: secured_category) + end + + it "should only return the category ids of category sidebar section link records that the user is allowed to see" do + expect(user.secured_sidebar_category_ids).to contain_exactly(category.id) + + user.update!(admin: true) + + expect(user.secured_sidebar_category_ids).to contain_exactly(category.id, secured_category.id) + end + end + describe "#visible_sidebar_tags" do fab!(:user) { Fabricate(:user) } fab!(:tag) { Fabricate(:tag) } diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a8afab4fd0a..1c9729df0a9 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2581,9 +2581,10 @@ RSpec.describe UsersController do end.to change { user.sidebar_section_links.count }.from(1).to(0) end - it "should allow user to modify category sidebar section links" do + it "should allow user to only modify category sidebar section links for categories they have access to" do category = Fabricate(:category) - restricted_category = Fabricate(:category, read_restricted: true) + group = Fabricate(:group) + restricted_category = Fabricate(:private_category, group: group) category_sidebar_section_link = Fabricate(:category_sidebar_section_link, user: user) put "/u/#{user.username}.json", @@ -2598,6 +2599,21 @@ RSpec.describe UsersController do sidebar_section_link = user.sidebar_section_links.first expect(sidebar_section_link.linkable).to eq(category) + + group.add(user) + + expect do + put "/u/#{user.username}.json", + params: { + sidebar_category_ids: [category.id, restricted_category.id], + } + + expect(response.status).to eq(200) + end.to change { user.sidebar_section_links.count }.from(1).to(2) + + expect(SidebarSectionLink.exists?(user: user, linkable: restricted_category)).to eq( + true, + ) end it "should allow user to remove all tag sidebar section links" do @@ -2623,15 +2639,18 @@ RSpec.describe UsersController do expect(user.reload.sidebar_section_links.count).to eq(0) end - it "should allow user to add tag sidebar section links" do + it "should allow user to add tag sidebar section links only for tags that are visible to the user" do SiteSetting.tagging_enabled = true tag = Fabricate(:tag) tag_sidebar_section_link = Fabricate(:tag_sidebar_section_link, user: user) + hidden_tag = Fabricate(:tag) + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) + put "/u/#{user.username}.json", params: { - sidebar_tag_names: [tag.name, "somerandomtag"], + sidebar_tag_names: [tag.name, "somerandomtag", hidden_tag.name], } expect(response.status).to eq(200) @@ -2641,6 +2660,19 @@ RSpec.describe UsersController do sidebar_section_link = user.sidebar_section_links.first expect(sidebar_section_link.linkable).to eq(tag) + + user.update!(admin: true) + + expect do + put "/u/#{user.username}.json", + params: { + sidebar_tag_names: [tag.name, "somerandomtag", hidden_tag.name], + } + + expect(response.status).to eq(200) + end.to change { user.sidebar_section_links.count }.from(1).to(2) + + expect(SidebarSectionLink.exists?(user: user, linkable: hidden_tag)).to eq(true) end end end diff --git a/spec/support/user_sidebar_serializer_attributes.rb b/spec/support/user_sidebar_serializer_attributes.rb index 6424947d8cf..3e4b0647beb 100644 --- a/spec/support/user_sidebar_serializer_attributes.rb +++ b/spec/support/user_sidebar_serializer_attributes.rb @@ -12,12 +12,15 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| fab!(:category) { Fabricate(:category) } fab!(:category_2) { Fabricate(:category) } fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:category_sidebar_section_link) do Fabricate(:category_sidebar_section_link, user: user, linkable: category) end + fab!(:category_sidebar_section_link_2) do Fabricate(:category_sidebar_section_link, user: user, linkable: category_2) end + fab!(:category_sidebar_section_link_3) do Fabricate(:category_sidebar_section_link, user: user, linkable: private_category) end @@ -47,19 +50,25 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| describe "#sidebar_tags" do fab!(:tag) { Fabricate(:tag, name: "foo", description: "foo tag") } + fab!(:pm_tag) do Fabricate(:tag, name: "bar", pm_topic_count: 5, staff_topic_count: 0, public_topic_count: 0) end + fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } + fab!(:staff_tag_group) do Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"]) end + fab!(:tag_sidebar_section_link) do Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) end + fab!(:tag_sidebar_section_link_2) do Fabricate(:tag_sidebar_section_link, user: user, linkable: pm_tag) end + fab!(:tag_sidebar_section_link_3) do Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag) end