From 36057638cad9d391d8f67d5553cecab0553279a3 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Wed, 13 Dec 2023 13:25:13 +0800 Subject: [PATCH] DEV: Convert min_trust_to_edit_post to groups (#24840) 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_edit_post site setting to edit_post_allowed_groups. The old implementation will co-exist for a short period while I update any references in plugins and themes. --- app/models/concerns/limited_edit.rb | 11 +++++++- config/locales/server.en.yml | 2 ++ config/site_settings.yml | 7 +++++ ...wed_groups_based_on_deprecated_settings.rb | 27 ++++++++++++++++++ lib/guardian/post_guardian.rb | 7 ++++- lib/site_settings/deprecated_settings.rb | 1 + spec/lib/guardian_spec.rb | 16 ++++++----- spec/lib/validators/post_validator_spec.rb | 2 +- spec/requests/posts_controller_spec.rb | 28 +++++-------------- spec/requests/topics_controller_spec.rb | 20 ++----------- spec/system/table_builder_spec.rb | 2 +- 11 files changed, 74 insertions(+), 49 deletions(-) create mode 100644 db/migrate/20231212044856_fill_edit_post_allowed_groups_based_on_deprecated_settings.rb diff --git a/app/models/concerns/limited_edit.rb b/app/models/concerns/limited_edit.rb index f73e8630359..d2bbe65743d 100644 --- a/app/models/concerns/limited_edit.rb +++ b/app/models/concerns/limited_edit.rb @@ -4,13 +4,22 @@ module LimitedEdit extend ActiveSupport::Concern def edit_time_limit_expired?(user) - return true if user.trust_level < SiteSetting.min_trust_to_edit_post + return true if !trusted_with_edits?(user) time_limit = user_time_limit(user) created_at && (time_limit > 0) && (created_at < time_limit.minutes.ago) end private + # TODO: This duplicates some functionality from PostGuardian. This should + # probably be checked there instead, because this has nothing to do + # with time limits. + # + def trusted_with_edits?(user) + user.trust_level >= SiteSetting.min_trust_to_edit_post || + user.in_any_groups?(SiteSetting.edit_post_allowed_groups_map) + end + def user_time_limit(user) if user.trust_level < 2 SiteSetting.post_edit_time_limit.to_i diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2cb9c374a1f..5089baa5292 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1965,6 +1965,7 @@ en: edit_wiki_post_allowed_groups: "Groups that are allowed to edit posts marked as wiki." min_trust_to_edit_post: "The minimum trust level required to edit posts." + edit_post_allowed_groups: "Groups that are allowed to edit posts." min_trust_to_allow_self_wiki: "The minimum trust level required to make user's own post wiki." @@ -2557,6 +2558,7 @@ en: edit_wiki_post_allowed_groups: "minmin_trust_to_edit_wiki_post" uploaded_avatars_allowed_groups: "allow_uploaded_avatars" create_topic_allowed_groups: "min_trust_to_create_topic" + edit_post_allowed_groups: "min_trust_to_edit_post" placeholder: discourse_connect_provider_secrets: diff --git a/config/site_settings.yml b/config/site_settings.yml index f3af4251368..9b1288699c1 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1699,6 +1699,13 @@ trust: min_trust_to_edit_post: default: 0 enum: "TrustLevelSetting" + hidden: true + edit_post_allowed_groups: + default: "10" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" min_trust_to_allow_self_wiki: default: 3 enum: "TrustLevelSetting" diff --git a/db/migrate/20231212044856_fill_edit_post_allowed_groups_based_on_deprecated_settings.rb b/db/migrate/20231212044856_fill_edit_post_allowed_groups_based_on_deprecated_settings.rb new file mode 100644 index 00000000000..ff688613548 --- /dev/null +++ b/db/migrate/20231212044856_fill_edit_post_allowed_groups_based_on_deprecated_settings.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class FillEditPostAllowedGroupsBasedOnDeprecatedSettings < ActiveRecord::Migration[7.0] + def up + configured_trust_level = + DB.query_single( + "SELECT value FROM site_settings WHERE name = 'min_trust_to_edit_post' 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('edit_post_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 0584e0b1c3a..c2435c62e65 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -163,7 +163,7 @@ module PostGuardian return can_create_post?(post.topic) end - return false if @user.trust_level < SiteSetting.min_trust_to_edit_post + return false if !trusted_with_edits? if is_my_own?(post) return false if @user.silenced? @@ -371,6 +371,11 @@ module PostGuardian private + def trusted_with_edits? + @user.trust_level >= SiteSetting.min_trust_to_edit_post || + @user.in_any_groups?(SiteSetting.edit_post_allowed_groups_map) + end + def can_create_post_in_topic?(topic) if !SiteSetting.enable_system_message_replies? && topic.try(:subtype) == "system_message" return false diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb index cf461af0ac9..b747488e9e5 100644 --- a/lib/site_settings/deprecated_settings.rb +++ b/lib/site_settings/deprecated_settings.rb @@ -22,6 +22,7 @@ module SiteSettings::DeprecatedSettings ["min_trust_to_edit_wiki_post", "edit_wiki_post_allowed_groups", false, "3.3"], ["allow_uploaded_avatars", "uploaded_avatars_allowed_groups", false, "3.3"], ["min_trust_to_create_topic", "create_topic_allowed_groups", false, "3.3"], + ["min_trust_to_edit_post", "edit_post_allowed_groups", false, "3.3"], ] def setup_deprecated_methods diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 7c6c7666e6b..3c01f2515dd 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -14,11 +14,11 @@ RSpec.describe Guardian do fab!(:automatic_group) { Fabricate(:group, automatic: true) } fab!(:plain_category) { Fabricate(:category) } - fab!(:trust_level_0) { Fabricate(:user, trust_level: 0) } - fab!(:trust_level_1) { Fabricate(:user, trust_level: 1) } - fab!(:trust_level_2) { Fabricate(:user, trust_level: 2) } - fab!(:trust_level_3) { Fabricate(:user, trust_level: 3) } - fab!(:trust_level_4) { Fabricate(:user, trust_level: 4) } + fab!(:trust_level_0) { Fabricate(:user, trust_level: 0, refresh_auto_groups: true) } + fab!(:trust_level_1) { Fabricate(:user, trust_level: 1, refresh_auto_groups: true) } + fab!(:trust_level_2) { Fabricate(:user, trust_level: 2, refresh_auto_groups: true) } + fab!(:trust_level_3) { Fabricate(:user, trust_level: 3, refresh_auto_groups: true) } + fab!(:trust_level_4) { Fabricate(:user, trust_level: 4, refresh_auto_groups: true) } fab!(:another_admin) { Fabricate(:admin) } fab!(:coding_horror) @@ -1678,6 +1678,7 @@ RSpec.describe Guardian do it "returns false when trying to edit a topic with no trust" do SiteSetting.min_trust_to_edit_post = 2 + SiteSetting.edit_post_allowed_groups = 12 post.user.trust_level = 1 expect(Guardian.new(topic.user).can_edit?(topic)).to be_falsey @@ -1685,6 +1686,7 @@ RSpec.describe Guardian do it "returns false when trying to edit a post with no trust" do SiteSetting.min_trust_to_edit_post = 2 + SiteSetting.edit_post_allowed_groups = 12 post.user.trust_level = 1 expect(Guardian.new(post.user).can_edit?(post)).to be_falsey @@ -1731,7 +1733,6 @@ RSpec.describe Guardian do SiteSetting.shared_drafts_category = category.id SiteSetting.shared_drafts_allowed_groups = Group::AUTO_GROUPS[:trust_level_2] Fabricate(:shared_draft, topic: topic) - Group.refresh_automatic_groups! end it "returns true if a shared draft exists" do @@ -1769,7 +1770,8 @@ RSpec.describe Guardian do describe "post edit time limits" do context "when post is older than post_edit_time_limit" do - let(:topic) { Fabricate(:topic) } + let(:user) { Fabricate(:user, refresh_auto_groups: true) } + let(:topic) { Fabricate(:topic, user: user) } let(:old_post) do Fabricate(:post, topic: topic, user: topic.user, created_at: 6.minutes.ago) end diff --git a/spec/lib/validators/post_validator_spec.rb b/spec/lib/validators/post_validator_spec.rb index 8bb6bb8f0a7..a13411ac65b 100644 --- a/spec/lib/validators/post_validator_spec.rb +++ b/spec/lib/validators/post_validator_spec.rb @@ -318,7 +318,7 @@ RSpec.describe PostValidator do end describe "force_edit_last_validator" do - fab!(:user) + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } fab!(:other_user) { Fabricate(:user) } fab!(:topic) diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index bd45714ef4b..4090af446e1 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -82,9 +82,9 @@ end RSpec.describe PostsController do fab!(:admin) fab!(:moderator) - fab!(:user) - fab!(:user_trust_level_0) { Fabricate(:trust_level_0) } - fab!(:user_trust_level_1) { Fabricate(:trust_level_1) } + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } + fab!(:user_trust_level_0) { Fabricate(:trust_level_0, refresh_auto_groups: true) } + fab!(:user_trust_level_1) { Fabricate(:trust_level_1, refresh_auto_groups: true) } fab!(:category) fab!(:topic) fab!(:post_by_user) { Fabricate(:post, user: user) } @@ -827,7 +827,6 @@ RSpec.describe PostsController do before do SiteSetting.min_first_post_typing_time = 0 SiteSetting.whispers_allowed_groups = "#{Group::AUTO_GROUPS[:staff]}" - Group.refresh_automatic_groups! end context "with api" do @@ -1078,7 +1077,7 @@ RSpec.describe PostsController do end describe "when logged in" do - fab!(:user) + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } before { sign_in(user) } @@ -1232,7 +1231,6 @@ RSpec.describe PostsController do end it "can send a message to a group" do - Group.refresh_automatic_groups! group = Group.create(name: "test_group", messageable_level: Group::ALIAS_LEVELS[:nobody]) user1 = user group.add(user1) @@ -1268,7 +1266,6 @@ RSpec.describe PostsController do end it "can send a message to a group with caps" do - Group.refresh_automatic_groups! group = Group.create(name: "Test_group", messageable_level: Group::ALIAS_LEVELS[:nobody]) user1 = user group.add(user1) @@ -1489,11 +1486,6 @@ RSpec.describe PostsController do xit "should add custom fields to topic that is permitted for a non-staff user via the deprecated `meta_data` param" do sign_in(user) - Discourse.expects(:deprecate).with( - "the :meta_data param is deprecated, use the :topic_custom_fields param instead", - anything, - ) - post "/posts.json", params: { raw: "this is the test content", @@ -1586,7 +1578,6 @@ RSpec.describe PostsController do user_4 = Fabricate(:user, username: "Iyi_Iyi") user_4.update_attribute(:username, "İyi_İyi") user_4.update_attribute(:username_lower, "İyi_İyi".downcase) - Group.refresh_automatic_groups! post "/posts.json", params: { @@ -1652,8 +1643,7 @@ RSpec.describe PostsController do it "it triggers flag_linked_posts_as_spam when the post creator returns spam" do SiteSetting.newuser_spam_host_threshold = 1 - sign_in(Fabricate(:user, trust_level: 0)) - Group.refresh_automatic_groups! + sign_in(Fabricate(:user, trust_level: 0, refresh_auto_groups: true)) post "/posts.json", params: { @@ -1844,9 +1834,7 @@ RSpec.describe PostsController do end describe "warnings" do - fab!(:user_2) { Fabricate(:user) } - - before { Group.refresh_automatic_groups! } + fab!(:user_2) { Fabricate(:user, refresh_auto_groups: true) } context "as a staff user" do before { sign_in(admin) } @@ -1948,7 +1936,7 @@ RSpec.describe PostsController do end context "with TL4 users" do - fab!(:trust_level_4) + fab!(:trust_level_4) { Fabricate(:trust_level_4, refresh_auto_groups: true) } before { sign_in(trust_level_4) } @@ -2341,8 +2329,6 @@ RSpec.describe PostsController do include_examples "action requires login", :get, "/posts/system/deleted.json" describe "when logged in" do - before { Group.refresh_automatic_groups! } - it "raises an error if the user doesn't have permission to see the deleted posts" do sign_in(user) get "/posts/system/deleted.json" diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index a54492663f7..8b1876a7e6b 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1630,7 +1630,7 @@ RSpec.describe TopicsController do end end - describe "with permission" do + context "with permission" do fab!(:post_hook) { Fabricate(:post_web_hook) } fab!(:topic_hook) { Fabricate(:topic_web_hook) } @@ -1887,8 +1887,7 @@ RSpec.describe TopicsController do end.not_to change { topic.reload.first_post.revisions.count } expect(response.status).to eq(403) - user_2.update!(trust_level: 2) - Group.refresh_automatic_groups! + user_2.groups << Group.find_by(name: "trust_level_2") expect do put "/t/#{topic.id}/tags.json", params: { tags: [tag.name] } end.to change { topic.reload.first_post.revisions.count @@ -3600,8 +3599,6 @@ RSpec.describe TopicsController do end context "with private message" do - before_all { Group.refresh_automatic_groups! } - fab!(:group) do Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]).tap do |g| g.add(user_2) @@ -4529,8 +4526,6 @@ RSpec.describe TopicsController do fab!(:topic) { Fabricate(:topic, user: user) } fab!(:post) { Fabricate(:post, user: user, topic: topic) } - before { Group.refresh_automatic_groups! } - it "raises an error when the user doesn't have permission to convert topic" do sign_in(user) put "/t/#{topic.id}/convert-topic/private.json" @@ -4557,8 +4552,6 @@ RSpec.describe TopicsController do fab!(:topic) { Fabricate(:private_message_topic, user: user) } fab!(:post) { Fabricate(:post, user: post_author1, topic: topic) } - before { Group.refresh_automatic_groups! } - it "raises an error when the user doesn't have permission to convert topic" do sign_in(user) put "/t/#{topic.id}/convert-topic/public.json" @@ -4961,10 +4954,7 @@ RSpec.describe TopicsController do fab!(:moderator_pm) { Fabricate(:private_message_topic, user: moderator) } - before do - SiteSetting.max_allowed_message_recipients = 2 - Group.refresh_automatic_groups! - end + before { SiteSetting.max_allowed_message_recipients = 2 } it "doesn't allow normal users to invite" do post "/t/#{pm.id}/invite.json", params: { user: user_2.username } @@ -5292,8 +5282,6 @@ RSpec.describe TopicsController do end describe "#private_message_reset_new" do - before_all { Group.refresh_automatic_groups! } - fab!(:group) do Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]).tap { |g| g.add(user_2) } end @@ -5412,8 +5400,6 @@ RSpec.describe TopicsController do end describe "#archive_message" do - before_all { Group.refresh_automatic_groups! } - fab!(:group) do Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]).tap { |g| g.add(user) } end diff --git a/spec/system/table_builder_spec.rb b/spec/system/table_builder_spec.rb index d103455d61e..43a68974ebb 100644 --- a/spec/system/table_builder_spec.rb +++ b/spec/system/table_builder_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe "Table Builder", type: :system do - fab!(:user) + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } let(:composer) { PageObjects::Components::Composer.new } let(:insert_table_modal) { PageObjects::Modals::InsertTable.new } fab!(:topic) { Fabricate(:topic, user: user) }