From fb087b7ff6d168bbfcaa5f83d53ac122d6a18a1c Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Thu, 18 Jan 2024 14:08:40 +0800 Subject: [PATCH] DEV: Convert min_trust_to_post_links to groups (#25298) We're changing the implementation of trust levels to use groups. Part of this is to have site settings that reference trust levels use groups instead. It converts the min_trust_to_post_links site setting to post_links_allowed_groups. This isn't used by any of our plugins or themes, so very little fallout. --- config/locales/server.en.yml | 2 + config/site_settings.yml | 7 ++++ ...wed_groups_based_on_deprecated_settings.rb | 27 +++++++++++++ lib/guardian/post_guardian.rb | 2 +- lib/site_settings/deprecated_settings.rb | 2 + spec/fabricators/post_fabricator.rb | 4 +- spec/lib/cooked_post_processor_spec.rb | 4 +- spec/lib/guardian_spec.rb | 18 ++++----- spec/lib/post_destroyer_spec.rb | 2 +- spec/lib/post_revisor_spec.rb | 2 +- spec/models/post_mover_spec.rb | 6 +-- spec/models/post_spec.rb | 38 +++++++++++-------- spec/models/topic_link_click_spec.rb | 4 +- spec/models/topic_link_spec.rb | 8 ++-- spec/requests/admin/users_controller_spec.rb | 2 +- .../user_summary_serializer_spec.rb | 2 +- spec/services/user_destroyer_spec.rb | 2 +- 17 files changed, 89 insertions(+), 43 deletions(-) create mode 100644 db/migrate/20240117090801_fill_post_links_allowed_groups_based_on_deprecated_settings.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5f341a1a39e..ff8790949f7 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1984,6 +1984,7 @@ en: min_trust_to_flag_posts: "The minimum trust level required to flag posts" flag_post_allowed_groups: "Groups that are allowed to flag posts." min_trust_to_post_links: "The minimum trust level required to include links in posts" + post_links_allowed_groups: "Groups that are allowed to include links in posts." min_trust_to_post_embedded_media: "The minimum trust level required to embed media items in a post" min_trust_level_to_allow_profile_background: "The minimum trust level required to upload a profile background" min_trust_level_to_allow_user_card_background: "The minimum trust level required to upload a user card background" @@ -2586,6 +2587,7 @@ en: create_tag_allowed_groups: "min_trust_to_create_tag" send_email_messages_allowed_groups: "min_trust_to_send_email_messages" skip_review_media_groups: "review_media_unless_trust_level" + post_links_allowed_groups: "min_trust_to_post_links" placeholder: discourse_connect_provider_secrets: diff --git a/config/site_settings.yml b/config/site_settings.yml index 894755dc361..e781553261a 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1756,6 +1756,13 @@ trust: min_trust_to_post_links: default: 0 enum: "TrustLevelSetting" + hidden: true + post_links_allowed_groups: + default: "10" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" min_trust_to_post_embedded_media: default: 0 enum: "TrustLevelSetting" diff --git a/db/migrate/20240117090801_fill_post_links_allowed_groups_based_on_deprecated_settings.rb b/db/migrate/20240117090801_fill_post_links_allowed_groups_based_on_deprecated_settings.rb new file mode 100644 index 00000000000..99586ba79b4 --- /dev/null +++ b/db/migrate/20240117090801_fill_post_links_allowed_groups_based_on_deprecated_settings.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class FillPostLinksAllowedGroupsBasedOnDeprecatedSettings < ActiveRecord::Migration[7.0] + def up + configured_trust_level = + DB.query_single( + "SELECT value FROM site_settings WHERE name = 'min_trust_to_post_links' LIMIT 1", + ).first + + # Default for old setting is TL0, we only need to do anything if it's been changed in the DB. + if configured_trust_level.present? + # Matches Group::AUTO_GROUPS to the trust levels. + corresponding_group = "1#{configured_trust_level}" + + # Data_type 20 is group_list. + DB.exec( + "INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('post_links_allowed_groups', :setting, '20', NOW(), NOW())", + setting: corresponding_group, + ) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index b4884186e18..6edac13b171 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -3,7 +3,7 @@ # mixin for all guardian methods dealing with post permissions module PostGuardian def unrestricted_link_posting? - authenticated? && @user.has_trust_level?(TrustLevel[SiteSetting.min_trust_to_post_links]) + authenticated? && @user.in_any_groups?(SiteSetting.post_links_allowed_groups_map) end def link_posting_access diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb index 98f76cb3e6b..97df85f0530 100644 --- a/lib/site_settings/deprecated_settings.rb +++ b/lib/site_settings/deprecated_settings.rb @@ -37,6 +37,7 @@ module SiteSettings::DeprecatedSettings ["min_trust_to_create_tag", "create_tag_allowed_groups", false, "3.3"], ["min_trust_to_send_email_messages", "send_email_messages_allowed_groups", false, "3.3"], ["review_media_unless_trust_level", "skip_review_media_groups", false, "3.3"], + ["min_trust_to_post_links", "post_links_allowed_groups", false, "3.3"], ] OVERRIDE_TL_GROUP_SETTINGS = %w[ @@ -58,6 +59,7 @@ module SiteSettings::DeprecatedSettings min_trust_to_create_tag min_trust_to_send_email_messages review_media_unless_trust_level + min_trust_to_post_links ] def group_to_tl(old_setting, new_setting) diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index cbfbc1b35b3..30cb72e395c 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true Fabricator(:post) do - user + user { Fabricate(:user, refresh_auto_groups: true) } topic { |attrs| Fabricate(:topic, user: attrs[:user]) } raw "Hello world" post_type Post.types[:regular] @@ -124,7 +124,7 @@ Fabricator(:post_with_uploads_and_links, from: :post) { raw <<~MD } MD Fabricator(:post_with_external_links, from: :post) do - user + user { Fabricate(:user, refresh_auto_groups: true) } topic raw <<~MD Here's a link to twitter: http://twitter.com diff --git a/spec/lib/cooked_post_processor_spec.rb b/spec/lib/cooked_post_processor_spec.rb index 556e9ace6ee..40af8e110df 100644 --- a/spec/lib/cooked_post_processor_spec.rb +++ b/spec/lib/cooked_post_processor_spec.rb @@ -184,9 +184,11 @@ RSpec.describe CookedPostProcessor do - #{url_with_query_param} RAW - let(:staff_post) { Fabricate(:post, user: Fabricate(:admin), raw: <<~RAW) } + let(:staff_post) do + Fabricate(:post, user: Fabricate(:admin, refresh_auto_groups: true), raw: <<~RAW) This is a #{url_with_path} topic RAW + end before do urls.each do |url| diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index c28bf6b0e3d..ec44c85755f 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -50,15 +50,15 @@ RSpec.describe Guardian do end it "is none for a user of a low trust level" do - user.trust_level = 0 - SiteSetting.min_trust_to_post_links = 1 + user.change_trust_level!(TrustLevel[0]) + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_1] expect(Guardian.new(user).link_posting_access).to eq("none") end it "is limited for a user of a low trust level with a allowlist" do SiteSetting.allowed_link_domains = "example.com" - user.trust_level = 0 - SiteSetting.min_trust_to_post_links = 1 + user.change_trust_level!(TrustLevel[0]) + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_1] expect(Guardian.new(user).link_posting_access).to eq("limited") end end @@ -75,10 +75,10 @@ RSpec.describe Guardian do end it "supports customization by site setting" do - user.trust_level = 0 - SiteSetting.min_trust_to_post_links = 0 + user.change_trust_level!(TrustLevel[0]) + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_0] expect(Guardian.new(user).can_post_link?(host: host)).to eq(true) - SiteSetting.min_trust_to_post_links = 1 + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_1] expect(Guardian.new(user).can_post_link?(host: host)).to eq(false) end @@ -86,8 +86,8 @@ RSpec.describe Guardian do before { SiteSetting.allowed_link_domains = host } it "allows a new user to post the link to the host" do - user.trust_level = 0 - SiteSetting.min_trust_to_post_links = 1 + user.change_trust_level!(TrustLevel[0]) + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_1] expect(Guardian.new(user).can_post_link?(host: host)).to eq(true) expect(Guardian.new(user).can_post_link?(host: "another-host.com")).to eq(false) end diff --git a/spec/lib/post_destroyer_spec.rb b/spec/lib/post_destroyer_spec.rb index fd2c73f99b8..f33b7b6292e 100644 --- a/spec/lib/post_destroyer_spec.rb +++ b/spec/lib/post_destroyer_spec.rb @@ -1000,7 +1000,7 @@ RSpec.describe PostDestroyer do let!(:second_post) { Fabricate(:post, topic: topic) } fab!(:other_topic) { Fabricate(:topic) } let!(:other_post) { Fabricate(:post, topic: other_topic) } - fab!(:user) + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } let!(:base_url) { URI.parse(Discourse.base_url) } let!(:guardian) { Guardian.new } let!(:url) do diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 52e66d7da4f..302b115e35d 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -854,7 +854,7 @@ RSpec.describe PostRevisor do end describe "admin editing a new user's post" do - fab!(:changed_by) { Fabricate(:admin) } + fab!(:changed_by) { Fabricate(:admin, refresh_auto_groups: true) } before do SiteSetting.newuser_max_embedded_media = 0 diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index fca9f76d052..9418425a3c0 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -2,7 +2,7 @@ RSpec.describe PostMover do fab!(:admin) - fab!(:evil_trout) + fab!(:evil_trout) { Fabricate(:evil_trout, refresh_auto_groups: true) } describe "#move_types" do context "when verifying enum sequence" do @@ -532,8 +532,8 @@ RSpec.describe PostMover do end fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } - fab!(:user3) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user, refresh_auto_groups: true) } + fab!(:user3) { Fabricate(:user, refresh_auto_groups: true) } fab!(:admin1) { Fabricate(:admin) } fab!(:admin2) { Fabricate(:admin) } diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index b2cce0836ee..05fd815cb89 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -275,7 +275,7 @@ RSpec.describe Post do end describe "maximum media embeds" do - fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } + fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0], refresh_auto_groups: true) } let(:post_no_images) { Fabricate.build(:post, post_args.merge(user: newuser)) } let(:post_one_image) { post_with_body("![sherlock](http://bbc.co.uk/sherlock.jpg)", newuser) } let(:post_two_images) do @@ -421,7 +421,7 @@ RSpec.describe Post do end describe "maximum attachments" do - fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } + fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0], refresh_auto_groups: true) } let(:post_no_attachments) { Fabricate.build(:post, post_args.merge(user: newuser)) } let(:post_one_attachment) do post_with_body( @@ -474,7 +474,7 @@ RSpec.describe Post do end describe "links" do - fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } + fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0], refresh_auto_groups: true) } let(:no_links) { post_with_body("hello world my name is evil trout", newuser) } let(:one_link) { post_with_body("[jlawr](http://www.imdb.com/name/nm2225369)", newuser) } let(:two_links) do @@ -557,7 +557,7 @@ RSpec.describe Post do end describe "maximums" do - fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } + fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0], refresh_auto_groups: true) } let(:post_one_link) do post_with_body("[sherlock](http://www.bbc.co.uk/programmes/b018ttws)", newuser) end @@ -607,29 +607,29 @@ RSpec.describe Post do expect(post_two_links).to be_valid end - context "with min_trust_to_post_links" do + context "when posting links is limited to certain TL groups" do it "considers oneboxes links" do - SiteSetting.min_trust_to_post_links = 3 - post_onebox.user.trust_level = TrustLevel[2] + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_3] + post_onebox.user.change_trust_level!(TrustLevel[2]) expect(post_onebox).not_to be_valid end it "considers links within code" do - SiteSetting.min_trust_to_post_links = 3 - post_onebox.user.trust_level = TrustLevel[2] + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_3] + post_onebox.user.change_trust_level!(TrustLevel[2]) expect(post_code_link).not_to be_valid end - it "doesn't allow allow links if `min_trust_to_post_links` is not met" do - SiteSetting.min_trust_to_post_links = 2 - post_two_links.user.trust_level = TrustLevel[1] + it "doesn't allow allow links if user is not in allowed groups" do + SiteSetting.post_links_allowed_groups = Group::AUTO_GROUPS[:trust_level_2] + post_two_links.user.change_trust_level!(TrustLevel[1]) expect(post_one_link).not_to be_valid end it "will skip the check for allowlisted domains" do SiteSetting.allowed_link_domains = "www.bbc.co.uk" - SiteSetting.min_trust_to_post_links = 2 - post_two_links.user.trust_level = TrustLevel[1] + SiteSetting.post_links_allowed_groups = "12" + post_two_links.user.change_trust_level!(TrustLevel[1]) expect(post_one_link).to be_valid end end @@ -686,7 +686,7 @@ RSpec.describe Post do end context "with max mentions" do - fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } + fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0], refresh_auto_groups: true) } let(:post_with_one_mention) { post_with_body("@Jake is the person I'm mentioning", newuser) } let(:post_with_two_mentions) do post_with_body("@Jake @Finn are the people I'm mentioning", newuser) @@ -1127,7 +1127,13 @@ RSpec.describe Post do describe "cooking" do let(:post) do - Fabricate.build(:post, post_args.merge(raw: "please read my blog http://blog.example.com")) + Fabricate.build( + :post, + post_args.merge( + raw: "please read my blog http://blog.example.com", + user: Fabricate(:user, refresh_auto_groups: true), + ), + ) end it "should unconditionally follow links for staff" do diff --git a/spec/models/topic_link_click_spec.rb b/spec/models/topic_link_click_spec.rb index 53b2f71ba77..1217948f556 100644 --- a/spec/models/topic_link_click_spec.rb +++ b/spec/models/topic_link_click_spec.rb @@ -11,7 +11,7 @@ RSpec.describe TopicLinkClick do describe "topic_links" do before do - @topic = Fabricate(:topic) + @topic = Fabricate(:topic, user: Fabricate(:user, refresh_auto_groups: true)) @post = Fabricate(:post_with_external_links, user: @topic.user, topic: @topic) TopicLink.extract_from(@post) @topic_link = @topic.topic_links.first @@ -247,7 +247,7 @@ RSpec.describe TopicLinkClick do context "with a query param and google analytics" do before do - @topic = Fabricate(:topic) + @topic = Fabricate(:topic, user: Fabricate(:user, refresh_auto_groups: true)) @post = Fabricate( :post, diff --git a/spec/models/topic_link_spec.rb b/spec/models/topic_link_spec.rb index 773d34c8009..7b6ad05386b 100644 --- a/spec/models/topic_link_spec.rb +++ b/spec/models/topic_link_spec.rb @@ -2,8 +2,8 @@ RSpec.describe TopicLink do let(:test_uri) { URI.parse(Discourse.base_url) } - fab!(:topic) { Fabricate(:topic, title: "unique topic name") } - fab!(:user) { topic.user } + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } + fab!(:topic) { Fabricate(:topic, user: user, title: "unique topic name") } fab!(:post) it { is_expected.to validate_presence_of :url } @@ -443,7 +443,7 @@ RSpec.describe TopicLink do context "with data" do let(:post) do - topic = Fabricate(:topic) + topic = Fabricate(:topic, user: Fabricate(:user, refresh_auto_groups: true)) Fabricate(:post_with_external_links, user: topic.user, topic: topic) end @@ -536,7 +536,7 @@ RSpec.describe TopicLink do end describe ".duplicate_lookup" do - fab!(:user) { Fabricate(:user, username: "junkrat") } + fab!(:user) { Fabricate(:user, username: "junkrat", refresh_auto_groups: true) } let(:post_with_internal_link) do Fabricate(:post, user: user, raw: "Check out this topic #{post.topic.url}/122131") diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index f3bfce7a7f0..cf8f68c48c0 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -1181,7 +1181,7 @@ RSpec.describe Admin::UsersController do end describe "#destroy" do - fab!(:delete_me) { Fabricate(:user) } + fab!(:delete_me) { Fabricate(:user, refresh_auto_groups: true) } shared_examples "user deletion possible" do it "returns a 403 if the user doesn't exist" do diff --git a/spec/serializers/user_summary_serializer_spec.rb b/spec/serializers/user_summary_serializer_spec.rb index 2f4f4af4d55..7ba6bba7a02 100644 --- a/spec/serializers/user_summary_serializer_spec.rb +++ b/spec/serializers/user_summary_serializer_spec.rb @@ -29,7 +29,7 @@ RSpec.describe UserSummarySerializer do end it "returns correct links data ranking" do - topic = Fabricate(:topic) + topic = Fabricate(:topic, user: Fabricate(:user, refresh_auto_groups: true)) post = Fabricate(:post_with_external_links, user: topic.user, topic: topic) TopicLink.extract_from(post) TopicLink diff --git a/spec/services/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb index 7cf9d4cff02..eafc268cd03 100644 --- a/spec/services/user_destroyer_spec.rb +++ b/spec/services/user_destroyer_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe UserDestroyer do - fab!(:user) { Fabricate(:user_with_secondary_email) } + fab!(:user) { Fabricate(:user_with_secondary_email, refresh_auto_groups: true) } fab!(:admin) { Fabricate(:admin, refresh_auto_groups: true) } describe ".new" do