From f2bf26f5c03ae2f8e028b0f8530cb03d94a8dec8 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Wed, 10 May 2023 13:30:56 -0500 Subject: [PATCH 01/10] Separated 'trusted users can edit others' setting for trust level 3 & 4 --- config/locales/server.en.yml | 4 ++- config/site_settings.yml | 8 +++-- ...e_trusted_users_can_edit_others_setting.rb | 33 +++++++++++++++++++ lib/guardian/post_guardian.rb | 8 +++-- lib/guardian/topic_guardian.rb | 13 +++++--- plugins/discourse-presence/plugin.rb | 5 +-- .../spec/integration/presence_spec.rb | 13 ++++++-- spec/lib/guardian_spec.rb | 32 ++++++++++++++---- spec/requests/topics_controller_spec.rb | 6 ++-- 9 files changed, 99 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 68bb82d6b8a920..7d8559769c55cf 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1911,7 +1911,9 @@ en: tl3_requires_likes_received: "The minimum number of likes that must be received in the last (tl3 time period) days to qualify for promotion to trust level 3." tl3_links_no_follow: "Do not remove rel=nofollow from links posted by trust level 3 users." tl4_delete_posts_and_topics: "Allow TL4 users to delete posts and topics created by other users. TL4 users will also be able to see deleted topics and posts." - trusted_users_can_edit_others: "Allow users with high trust levels to edit content from other users" + # trusted_users_can_edit_others: "Allow users with high trust levels to edit content from other users" + edit_all_topic_groups: "Allow users in this group to edit other users' topic titles and tags" + edit_all_post_groups: "Allow users in this group to edit other users' posts" min_trust_to_create_topic: "The minimum trust level required to create a new topic." allow_flagging_staff: "If enabled, users can flag posts from staff accounts." diff --git a/config/site_settings.yml b/config/site_settings.yml index 4db12493453769..8a870fbb61016b 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1679,8 +1679,12 @@ trust: tl4_delete_posts_and_topics: default: false client: true - trusted_users_can_edit_others: - default: true + edit_all_topic_groups: + default: "13" + type: group_list + edit_all_post_groups: + default: "14" + type: group_list security: detailed_404: false diff --git a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb new file mode 100644 index 00000000000000..c2ae9281e5b571 --- /dev/null +++ b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class SeparateTrustedUsersCanEditOthersSetting < ActiveRecord::Migration[7.0] + def up + execute " + DO + $do$ + BEGIN + IF EXISTS (SELECT * FROM site_settings WHERE name = 'trusted_users_can_edit_others') THEN + DELETE FROM site_settings WHERE name = 'trusted_users_can_edit_others'; + INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_topic_groups', 20, '', now(), now()); + INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_post_groups', 20, '', now(), now()); + END IF; + END + $do$ + " + end + + def down + execute " + DO + $do$ + BEGIN + IF EXISTS (SELECT * FROM site_settings WHERE name IN ('edit_all_topic_groups','edit_all_post_groups')) THEN + DELETE FROM site_settings WHERE name = 'edit_all_topic_groups'; + DELETE FROM site_settings WHERE name = 'edit_all_post_groups'; + INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('trusted_users_can_edit_others', 5, 'f', now(), now()); + END IF; + END + $do$ + " + end +end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index a72b4befb45f64..420427ae917503 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -130,8 +130,7 @@ def can_edit_post?(post) return false if post.locked? && !is_staff? if ( - is_staff? || - (SiteSetting.trusted_users_can_edit_others? && @user.has_trust_level?(TrustLevel[4])) || + is_staff? || can_edit_all_regular_posts? || is_category_group_moderator?(post.topic&.category) ) return can_create_post?(post.topic) @@ -173,6 +172,11 @@ def can_edit_post?(post) false end + def can_edit_all_regular_posts? + SiteSetting.edit_all_post_groups.present? && + user.in_any_groups?(SiteSetting.edit_all_post_groups.split("|").map(&:to_i)) + end + def can_edit_hidden_post?(post) return false if post.nil? post.hidden_at.nil? || diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 9ba3a248c5d6cd..f177cf8ba7bf1b 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -121,16 +121,21 @@ def can_edit_topic?(topic) # TL4 users can edit archived topics, but can not edit private messages if ( - SiteSetting.trusted_users_can_edit_others? && topic.archived && !topic.private_message? && - user.has_trust_level?(TrustLevel[4]) && can_create_post?(topic) + can_edit_all_regular_posts? && topic.archived && !topic.private_message? && + can_create_post?(topic) ) return true end + def can_edit_all_regular_topics? + SiteSetting.edit_all_topic_groups.present? && + user.in_any_groups?(SiteSetting.edit_all_topic_groups.split("|").map(&:to_i)) + end + # TL3 users can not edit archived topics and private messages if ( - SiteSetting.trusted_users_can_edit_others? && !topic.archived && !topic.private_message? && - user.has_trust_level?(TrustLevel[3]) && can_create_post?(topic) + can_edit_all_regular_topics? && !topic.archived && !topic.private_message? && + can_create_post?(topic) ) return true end diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index ff8af8d663f8c7..16597327fc9292 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/plugins/discourse-presence/plugin.rb @@ -57,8 +57,9 @@ ] end - if !topic.private_message? && SiteSetting.trusted_users_can_edit_others? - config.allowed_group_ids << Group::AUTO_GROUPS[:trust_level_4] + # if !topic.private_message? && SiteSetting.trusted_users_can_edit_others? + if !topic.private_message? && SiteSetting.edit_all_post_groups.present? + config.allowed_group_ids << SiteSetting.edit_all_post_groups.to_i end if SiteSetting.enable_category_group_moderation? && diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index c3a024c45f68ef..2c81c1eedc4791 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -66,7 +66,9 @@ end it "handles category moderators for edit" do - SiteSetting.trusted_users_can_edit_others = false + # SiteSetting.trusted_users_can_edit_others = false + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil p = Fabricate(:post, topic: private_topic, user: private_topic.user) c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") @@ -136,7 +138,10 @@ end it "allows only author and staff when editing a public post with tl4 editing disabled" do - SiteSetting.trusted_users_can_edit_others = false + # SiteSetting.trusted_users_can_edit_others = false + + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil p = Fabricate(:post, topic: public_topic, user: user) c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") @@ -148,7 +153,9 @@ it "follows the wiki edit trust level site setting" do p = Fabricate(:post, topic: public_topic, user: user, wiki: true) SiteSetting.min_trust_to_edit_wiki_post = TrustLevel.levels[:basic] - SiteSetting.trusted_users_can_edit_others = false + # SiteSetting.trusted_users_can_edit_others = false + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") expect(c.config.public).to eq(false) diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 1d175a34f0375e..dc1672932078e0 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -1626,8 +1626,12 @@ expect(Guardian.new(trust_level_4).can_edit?(post)).to be_truthy end - it "returns false as a TL4 user if trusted_users_can_edit_others is false" do - SiteSetting.trusted_users_can_edit_others = false + # it "returns false as a TL4 user if trusted_users_can_edit_others is false" do + it "returns false as a TL4 user if edit_all_post_groups is false" do + # SiteSetting.trusted_users_can_edit_others = false + + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil expect(Guardian.new(trust_level_4).can_edit?(post)).to eq(false) end @@ -1876,9 +1880,16 @@ expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(true) end - it "is false at TL3, if `trusted_users_can_edit_others` is false" do - SiteSetting.trusted_users_can_edit_others = false - expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(false) + # it "is false at TL3, if `trusted_users_can_edit_others` is false" do + # SiteSetting.trusted_users_can_edit_others = false + # expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(false) + # end + + # it "returns false as a TL3 user if trusted_users_can_edit_others is false" do + it "returns false as a TL3 user if edit_all_topic_groups is false" do + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil + expect(Guardian.new(trust_level_3).can_edit?(post)).to eq(false) end it "returns false when the category is read only" do @@ -1930,8 +1941,15 @@ expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to be_truthy end - it "is false at TL4, if `trusted_users_can_edit_others` is false" do - SiteSetting.trusted_users_can_edit_others = false + # it "is false at TL4, if `trusted_users_can_edit_others` is false" do + # SiteSetting.trusted_users_can_edit_others = false + # expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(false) + # end + + # it "returns false as a TL4 user if trusted_users_can_edit_others is false" do + it "returns false as a TL4 user if edit_all_post_groups is false" do + SiteSetting.edit_all_topic_groups = nil + SiteSetting.edit_all_post_groups = nil expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(false) end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 4ad4830079bc2d..0450f3a2cefd06 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1611,8 +1611,10 @@ def topic_user_post_timings_count(user, topic) end describe "when first post is locked" do - it "blocks non-staff from editing even if 'trusted_users_can_edit_others' is true" do - SiteSetting.trusted_users_can_edit_others = true + # it "blocks non-staff from editing even if 'trusted_users_can_edit_others' is true" do + it "blocks non-staff from editing even if 'edit_all_topic_groups' and 'edit_all_post_groups' are true" do + SiteSetting.edit_all_topic_groups = Group::AUTO_GROUPS[:trust_level_3] + SiteSetting.edit_all_post_groups = Group::AUTO_GROUPS[:trust_level_4] user.update!(trust_level: 3) topic.first_post.update!(locked_by_id: admin.id) From ea9570f40db5d00638f947d9f24a95fb0f300e67 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Thu, 11 May 2023 12:33:12 -0500 Subject: [PATCH 02/10] Separated 'trusted users can edit others' setting for trust level 3 & 4 --- config/locales/server.en.yml | 1 - lib/guardian/post_guardian.rb | 1 + plugins/discourse-presence/plugin.rb | 3 +-- .../spec/integration/presence_spec.rb | 17 +++++++------ spec/lib/guardian_spec.rb | 24 +++---------------- spec/requests/topics_controller_spec.rb | 3 +-- 6 files changed, 16 insertions(+), 33 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7d8559769c55cf..9a861a671b8ca3 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1911,7 +1911,6 @@ en: tl3_requires_likes_received: "The minimum number of likes that must be received in the last (tl3 time period) days to qualify for promotion to trust level 3." tl3_links_no_follow: "Do not remove rel=nofollow from links posted by trust level 3 users." tl4_delete_posts_and_topics: "Allow TL4 users to delete posts and topics created by other users. TL4 users will also be able to see deleted topics and posts." - # trusted_users_can_edit_others: "Allow users with high trust levels to edit content from other users" edit_all_topic_groups: "Allow users in this group to edit other users' topic titles and tags" edit_all_post_groups: "Allow users in this group to edit other users' posts" diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 420427ae917503..10effca64970b1 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -173,6 +173,7 @@ def can_edit_post?(post) end def can_edit_all_regular_posts? + puts SiteSetting.edit_all_post_groups.inspect SiteSetting.edit_all_post_groups.present? && user.in_any_groups?(SiteSetting.edit_all_post_groups.split("|").map(&:to_i)) end diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index 16597327fc9292..42919c143a6294 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/plugins/discourse-presence/plugin.rb @@ -57,9 +57,8 @@ ] end - # if !topic.private_message? && SiteSetting.trusted_users_can_edit_others? if !topic.private_message? && SiteSetting.edit_all_post_groups.present? - config.allowed_group_ids << SiteSetting.edit_all_post_groups.to_i + config.allowed_group_ids += SiteSetting.edit_all_post_groups.split("|").map(&:to_i) end if SiteSetting.enable_category_group_moderation? && diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index 2c81c1eedc4791..d3f0d3afaed6ad 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -66,8 +66,6 @@ end it "handles category moderators for edit" do - # SiteSetting.trusted_users_can_edit_others = false - SiteSetting.edit_all_topic_groups = nil SiteSetting.edit_all_post_groups = nil p = Fabricate(:post, topic: private_topic, user: private_topic.user) @@ -81,6 +79,16 @@ expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff], group.id) end + it "adds edit_all_post_groups to the presence channel" do + p = Fabricate(:post, topic: public_topic, user: user) + g = Fabricate(:group) + + SiteSetting.edit_all_post_groups = "#{Group::AUTO_GROUPS[:trust_level_1]}|#{g.id}" + + c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") + expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff], Group::AUTO_GROUPS[:trust_level_1], g.id) + end + it "handles permissions for a public topic" do c = PresenceChannel.new("/discourse-presence/reply/#{public_topic.id}") expect(c.config.public).to eq(false) @@ -138,9 +146,6 @@ end it "allows only author and staff when editing a public post with tl4 editing disabled" do - # SiteSetting.trusted_users_can_edit_others = false - - SiteSetting.edit_all_topic_groups = nil SiteSetting.edit_all_post_groups = nil p = Fabricate(:post, topic: public_topic, user: user) @@ -153,8 +158,6 @@ it "follows the wiki edit trust level site setting" do p = Fabricate(:post, topic: public_topic, user: user, wiki: true) SiteSetting.min_trust_to_edit_wiki_post = TrustLevel.levels[:basic] - # SiteSetting.trusted_users_can_edit_others = false - SiteSetting.edit_all_topic_groups = nil SiteSetting.edit_all_post_groups = nil c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index dc1672932078e0..11b4e8d5c959cd 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -1626,11 +1626,7 @@ expect(Guardian.new(trust_level_4).can_edit?(post)).to be_truthy end - # it "returns false as a TL4 user if trusted_users_can_edit_others is false" do - it "returns false as a TL4 user if edit_all_post_groups is false" do - # SiteSetting.trusted_users_can_edit_others = false - - SiteSetting.edit_all_topic_groups = nil + it "returns false as a TL4 user if edit_all_post_groups is empty" do SiteSetting.edit_all_post_groups = nil expect(Guardian.new(trust_level_4).can_edit?(post)).to eq(false) end @@ -1880,15 +1876,8 @@ expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(true) end - # it "is false at TL3, if `trusted_users_can_edit_others` is false" do - # SiteSetting.trusted_users_can_edit_others = false - # expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(false) - # end - - # it "returns false as a TL3 user if trusted_users_can_edit_others is false" do - it "returns false as a TL3 user if edit_all_topic_groups is false" do + it "returns false as a TL3 user if edit_all_topic_groups is empty" do SiteSetting.edit_all_topic_groups = nil - SiteSetting.edit_all_post_groups = nil expect(Guardian.new(trust_level_3).can_edit?(post)).to eq(false) end @@ -1941,14 +1930,7 @@ expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to be_truthy end - # it "is false at TL4, if `trusted_users_can_edit_others` is false" do - # SiteSetting.trusted_users_can_edit_others = false - # expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(false) - # end - - # it "returns false as a TL4 user if trusted_users_can_edit_others is false" do - it "returns false as a TL4 user if edit_all_post_groups is false" do - SiteSetting.edit_all_topic_groups = nil + it "returns false as a TL4 user if edit_all_post_groups is empty" do SiteSetting.edit_all_post_groups = nil expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(false) end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 0450f3a2cefd06..4b6739892ac5fe 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1611,8 +1611,7 @@ def topic_user_post_timings_count(user, topic) end describe "when first post is locked" do - # it "blocks non-staff from editing even if 'trusted_users_can_edit_others' is true" do - it "blocks non-staff from editing even if 'edit_all_topic_groups' and 'edit_all_post_groups' are true" do + it "blocks non-staff from editing even if 'edit_all_topic_groups' and 'edit_all_post_groups' are set to default" do SiteSetting.edit_all_topic_groups = Group::AUTO_GROUPS[:trust_level_3] SiteSetting.edit_all_post_groups = Group::AUTO_GROUPS[:trust_level_4] user.update!(trust_level: 3) From 82f7001947ed3424fe25c66bbdaa0747bab9e24c Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Thu, 11 May 2023 12:47:56 -0500 Subject: [PATCH 03/10] Deleted unnecessary lines and comments, let allowed_group_ids to take integer, and added a test --- lib/guardian/post_guardian.rb | 1 - .../discourse-presence/spec/integration/presence_spec.rb | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 10effca64970b1..420427ae917503 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -173,7 +173,6 @@ def can_edit_post?(post) end def can_edit_all_regular_posts? - puts SiteSetting.edit_all_post_groups.inspect SiteSetting.edit_all_post_groups.present? && user.in_any_groups?(SiteSetting.edit_all_post_groups.split("|").map(&:to_i)) end diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index d3f0d3afaed6ad..46127c9c1ea5e8 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -86,7 +86,11 @@ SiteSetting.edit_all_post_groups = "#{Group::AUTO_GROUPS[:trust_level_1]}|#{g.id}" c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") - expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff], Group::AUTO_GROUPS[:trust_level_1], g.id) + expect(c.config.allowed_group_ids).to contain_exactly( + Group::AUTO_GROUPS[:staff], + Group::AUTO_GROUPS[:trust_level_1], + g.id, + ) end it "handles permissions for a public topic" do From 83b245c0ca4a16856587851eece066301fb3c101 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Wed, 17 May 2023 11:40:44 -0500 Subject: [PATCH 04/10] Fixed the description for the 'edit_all_topic_groups' --- config/locales/server.en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 74a2f36aa8beec..5d4c9a9bca4a05 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1910,7 +1910,7 @@ en: tl3_requires_likes_received: "The minimum number of likes that must be received in the last (tl3 time period) days to qualify for promotion to trust level 3." tl3_links_no_follow: "Do not remove rel=nofollow from links posted by trust level 3 users." tl4_delete_posts_and_topics: "Allow TL4 users to delete posts and topics created by other users. TL4 users will also be able to see deleted topics and posts." - edit_all_topic_groups: "Allow users in this group to edit other users' topic titles and tags" + edit_all_topic_groups: "Allow users in this group to edit other users' topic titles, tags, and categories" edit_all_post_groups: "Allow users in this group to edit other users' posts" min_trust_to_create_topic: "The minimum trust level required to create a new topic." From d7bec2f4de8655602d43e2cfa2d32681ad487443 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Tue, 20 Jun 2023 09:48:14 -0500 Subject: [PATCH 05/10] Deleted unnecessary tests and modified tests --- ...ted_users_can_edit_others_site_setting.rb} | 30 +++++++----------- lib/guardian/post_guardian.rb | 7 ++--- lib/guardian/topic_guardian.rb | 8 ++--- .../spec/integration/presence_spec.rb | 16 +--------- spec/lib/guardian_spec.rb | 31 +++++++++++-------- spec/requests/topics_controller_spec.rb | 2 +- 6 files changed, 36 insertions(+), 58 deletions(-) rename db/migrate/{20230509214723_separate_trusted_users_can_edit_others_setting.rb => 20230509214723_separate_trusted_users_can_edit_others_site_setting.rb} (61%) diff --git a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb similarity index 61% rename from db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb rename to db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb index c2ae9281e5b571..514442ea649bee 100644 --- a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_setting.rb +++ b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb @@ -1,33 +1,25 @@ # frozen_string_literal: true -class SeparateTrustedUsersCanEditOthersSetting < ActiveRecord::Migration[7.0] +class SeparateTrustedUsersCanEditOthersSiteSetting < ActiveRecord::Migration[7.0] def up - execute " - DO - $do$ - BEGIN - IF EXISTS (SELECT * FROM site_settings WHERE name = 'trusted_users_can_edit_others') THEN + if select_value("SELECT 1 FROM site_settings WHERE name = 'trusted_users_can_edit_others'") + execute <<~SQL DELETE FROM site_settings WHERE name = 'trusted_users_can_edit_others'; INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_topic_groups', 20, '', now(), now()); INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_post_groups', 20, '', now(), now()); - END IF; - END - $do$ - " + SQL + end end def down - execute " - DO - $do$ - BEGIN - IF EXISTS (SELECT * FROM site_settings WHERE name IN ('edit_all_topic_groups','edit_all_post_groups')) THEN + if select_value( + "SELECT 1 FROM site_settings WHERE name IN ('edit_all_topic_groups', 'edit_all_post_groups')", + ) + execute <<~SQL DELETE FROM site_settings WHERE name = 'edit_all_topic_groups'; DELETE FROM site_settings WHERE name = 'edit_all_post_groups'; INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('trusted_users_can_edit_others', 5, 'f', now(), now()); - END IF; - END - $do$ - " + SQL + end end end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 420427ae917503..18d6b502faed17 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -129,10 +129,7 @@ def can_edit_post?(post) # Must be staff to edit a locked post return false if post.locked? && !is_staff? - if ( - is_staff? || can_edit_all_regular_posts? || - is_category_group_moderator?(post.topic&.category) - ) + if (is_staff? || is_in_edit_post_groups? || is_category_group_moderator?(post.topic&.category)) return can_create_post?(post.topic) end @@ -172,7 +169,7 @@ def can_edit_post?(post) false end - def can_edit_all_regular_posts? + def is_in_edit_post_groups? SiteSetting.edit_all_post_groups.present? && user.in_any_groups?(SiteSetting.edit_all_post_groups.split("|").map(&:to_i)) end diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index f177cf8ba7bf1b..4937a20ea4b7b7 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -119,22 +119,20 @@ def can_edit_topic?(topic) return true end - # TL4 users can edit archived topics, but can not edit private messages if ( - can_edit_all_regular_posts? && topic.archived && !topic.private_message? && + is_in_edit_post_groups? && topic.archived && !topic.private_message? && can_create_post?(topic) ) return true end - def can_edit_all_regular_topics? + def is_in_edit_topic_groups? SiteSetting.edit_all_topic_groups.present? && user.in_any_groups?(SiteSetting.edit_all_topic_groups.split("|").map(&:to_i)) end - # TL3 users can not edit archived topics and private messages if ( - can_edit_all_regular_topics? && !topic.archived && !topic.private_message? && + is_in_edit_topic_groups? && !topic.archived && !topic.private_message? && can_create_post?(topic) ) return true diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index 46127c9c1ea5e8..2acf5b76ca630b 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -149,27 +149,13 @@ expect(c.config.allowed_user_ids).to contain_exactly(user.id) end - it "allows only author and staff when editing a public post with tl4 editing disabled" do - SiteSetting.edit_all_post_groups = nil - - p = Fabricate(:post, topic: public_topic, user: user) - c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") - expect(c.config.public).to eq(false) - expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) - expect(c.config.allowed_user_ids).to contain_exactly(user.id) - end - it "follows the wiki edit trust level site setting" do p = Fabricate(:post, topic: public_topic, user: user, wiki: true) SiteSetting.min_trust_to_edit_wiki_post = TrustLevel.levels[:basic] - SiteSetting.edit_all_post_groups = nil c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") expect(c.config.public).to eq(false) - expect(c.config.allowed_group_ids).to contain_exactly( - Group::AUTO_GROUPS[:staff], - Group::AUTO_GROUPS[:trust_level_1], - ) + expect(c.config.allowed_group_ids).to contain(Group::AUTO_GROUPS[:trust_level_1]) expect(c.config.allowed_user_ids).to contain_exactly(user.id) end diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 11b4e8d5c959cd..65d5e4b95dcd86 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -1626,11 +1626,6 @@ expect(Guardian.new(trust_level_4).can_edit?(post)).to be_truthy end - it "returns false as a TL4 user if edit_all_post_groups is empty" do - SiteSetting.edit_all_post_groups = nil - expect(Guardian.new(trust_level_4).can_edit?(post)).to eq(false) - end - it "returns false when trying to edit a topic with no trust" do SiteSetting.min_trust_to_edit_post = 2 post.user.trust_level = 1 @@ -1876,11 +1871,6 @@ expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(true) end - it "returns false as a TL3 user if edit_all_topic_groups is empty" do - SiteSetting.edit_all_topic_groups = nil - expect(Guardian.new(trust_level_3).can_edit?(post)).to eq(false) - end - it "returns false when the category is read only" do topic.category.set_permissions(everyone: :readonly) topic.category.save @@ -1930,9 +1920,24 @@ expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to be_truthy end - it "returns false as a TL4 user if edit_all_post_groups is empty" do - SiteSetting.edit_all_post_groups = nil - expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(false) + it "returns true if the user is in edit_all_post_groups" do + SiteSetting.edit_all_post_groups = 14 + expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(true) + end + + it "returns false if the user is not in edit_all_post_groups" do + SiteSetting.edit_all_post_groups = 14 + expect(Guardian.new(trust_level_3).can_edit?(archived_topic)).to eq(false) + end + + it "returns true if the user is in edit_all_topic_groups" do + SiteSetting.edit_all_topic_groups = 13 + expect(Guardian.new(trust_level_3).can_edit?(archived_topic)).to eq(true) + end + + it "returns false if the user is not in edit_all_topic_groups" do + SiteSetting.edit_all_topic_groups = 13 + expect(Guardian.new(trust_level_2).can_edit?(archived_topic)).to eq(false) end it "returns false at trust level 3" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 4b6739892ac5fe..d8f509a20de064 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1611,7 +1611,7 @@ def topic_user_post_timings_count(user, topic) end describe "when first post is locked" do - it "blocks non-staff from editing even if 'edit_all_topic_groups' and 'edit_all_post_groups' are set to default" do + it "blocks user from editing even if they are in 'edit_all_topic_groups' and 'edit_all_post_groups'" do SiteSetting.edit_all_topic_groups = Group::AUTO_GROUPS[:trust_level_3] SiteSetting.edit_all_post_groups = Group::AUTO_GROUPS[:trust_level_4] user.update!(trust_level: 3) From 8156163874192b6d12ed2636569c961611e3dd9b Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Tue, 20 Jun 2023 11:00:18 -0500 Subject: [PATCH 06/10] Fixed failing spec tests --- lib/guardian/topic_guardian.rb | 10 +++++----- spec/lib/guardian_spec.rb | 14 ++------------ 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 4937a20ea4b7b7..eee58e7aea1ce1 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -126,11 +126,6 @@ def can_edit_topic?(topic) return true end - def is_in_edit_topic_groups? - SiteSetting.edit_all_topic_groups.present? && - user.in_any_groups?(SiteSetting.edit_all_topic_groups.split("|").map(&:to_i)) - end - if ( is_in_edit_topic_groups? && !topic.archived && !topic.private_message? && can_create_post?(topic) @@ -144,6 +139,11 @@ def is_in_edit_topic_groups? (!first_post&.hidden? || can_edit_hidden_post?(first_post)) end + def is_in_edit_topic_groups? + SiteSetting.edit_all_topic_groups.present? && + user.in_any_groups?(SiteSetting.edit_all_topic_groups.split("|").map(&:to_i)) + end + def can_recover_topic?(topic) if is_staff? || (topic&.category && is_category_group_moderator?(topic.category)) || (SiteSetting.tl4_delete_posts_and_topics && user&.has_trust_level?(TrustLevel[4])) diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 65d5e4b95dcd86..1eea57f6fff79e 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -1921,25 +1921,15 @@ end it "returns true if the user is in edit_all_post_groups" do - SiteSetting.edit_all_post_groups = 14 + SiteSetting.edit_all_post_groups = "14" expect(Guardian.new(trust_level_4).can_edit?(archived_topic)).to eq(true) end it "returns false if the user is not in edit_all_post_groups" do - SiteSetting.edit_all_post_groups = 14 + SiteSetting.edit_all_post_groups = "14" expect(Guardian.new(trust_level_3).can_edit?(archived_topic)).to eq(false) end - it "returns true if the user is in edit_all_topic_groups" do - SiteSetting.edit_all_topic_groups = 13 - expect(Guardian.new(trust_level_3).can_edit?(archived_topic)).to eq(true) - end - - it "returns false if the user is not in edit_all_topic_groups" do - SiteSetting.edit_all_topic_groups = 13 - expect(Guardian.new(trust_level_2).can_edit?(archived_topic)).to eq(false) - end - it "returns false at trust level 3" do expect(Guardian.new(trust_level_3).can_edit?(archived_topic)).to be_falsey end From 951e35ea084ccbf26d7af6884a831f8cafbbcbad Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Mon, 26 Jun 2023 10:19:57 -0500 Subject: [PATCH 07/10] wrote test cases for when SiteSetting.edit_all_post/topic_groups is not empty --- ...sted_users_can_edit_others_site_setting.rb | 4 +++- lib/guardian/post_guardian.rb | 2 +- lib/guardian/topic_guardian.rb | 2 +- spec/lib/guardian/post_guardian_spec.rb | 22 +++++++++++++++++++ spec/lib/guardian/topic_guardian_spec.rb | 21 ++++++++++++++++++ 5 files changed, 48 insertions(+), 3 deletions(-) diff --git a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb index 514442ea649bee..3f0ec2ecc1bf67 100644 --- a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb +++ b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb @@ -2,7 +2,9 @@ class SeparateTrustedUsersCanEditOthersSiteSetting < ActiveRecord::Migration[7.0] def up - if select_value("SELECT 1 FROM site_settings WHERE name = 'trusted_users_can_edit_others'") + if select_value( + "SELECT 1 FROM site_settings WHERE name = 'trusted_users_can_edit_others' AND value = 'f'", + ) execute <<~SQL DELETE FROM site_settings WHERE name = 'trusted_users_can_edit_others'; INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_topic_groups', 20, '', now(), now()); diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 18d6b502faed17..1eec783c141bed 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -171,7 +171,7 @@ def can_edit_post?(post) def is_in_edit_post_groups? SiteSetting.edit_all_post_groups.present? && - user.in_any_groups?(SiteSetting.edit_all_post_groups.split("|").map(&:to_i)) + user.in_any_groups?(SiteSetting.edit_all_post_groups.to_s.split("|").map(&:to_i)) end def can_edit_hidden_post?(post) diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index eee58e7aea1ce1..0f8782f3bbf8ba 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -141,7 +141,7 @@ def can_edit_topic?(topic) def is_in_edit_topic_groups? SiteSetting.edit_all_topic_groups.present? && - user.in_any_groups?(SiteSetting.edit_all_topic_groups.split("|").map(&:to_i)) + user.in_any_groups?(SiteSetting.edit_all_topic_groups.to_s.split("|").map(&:to_i)) end def can_recover_topic?(topic) diff --git a/spec/lib/guardian/post_guardian_spec.rb b/spec/lib/guardian/post_guardian_spec.rb index 713886a6f3be29..64b52a830bb541 100644 --- a/spec/lib/guardian/post_guardian_spec.rb +++ b/spec/lib/guardian/post_guardian_spec.rb @@ -8,6 +8,7 @@ fab!(:tl4_user) { Fabricate(:trust_level_4) } fab!(:moderator) { Fabricate(:moderator) } fab!(:category) { Fabricate(:category) } + fab!(:group) { Fabricate(:group) } fab!(:topic) { Fabricate(:topic, category: category) } fab!(:hidden_post) { Fabricate(:post, topic: topic, hidden: true) } @@ -29,4 +30,25 @@ expect(Guardian.new(admin).can_see_hidden_post?(hidden_post)).to eq(true) end end + + describe "#is_in_edit_post_groups?" do + it "returns true if the user is in edit_all_post_groups" do + group.add(user) + SiteSetting.edit_all_post_groups = group.id.to_s + + expect(Guardian.new(user).is_in_edit_post_groups?).to eq(true) + end + + it "returns false if the user is not in edit_all_post_groups" do + SiteSetting.edit_all_post_groups = Group::AUTO_GROUPS[:trust_level_4] + + expect(Guardian.new(tl3_user).is_in_edit_post_groups?).to eq(false) + end + + it "returns false if the edit_all_post_groups is empty" do + SiteSetting.edit_all_post_groups = nil + + expect(Guardian.new(user).is_in_edit_post_groups?).to eq(false) + end + end end diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb index 7bddc0e9e469b3..5b070b167c70de 100644 --- a/spec/lib/guardian/topic_guardian_spec.rb +++ b/spec/lib/guardian/topic_guardian_spec.rb @@ -173,6 +173,27 @@ end end + describe "#is_in_edit_topic_groups?" do + it "returns true if the user is in edit_all_topic_groups" do + group.add(user) + SiteSetting.edit_all_topic_groups = group.id.to_s + + expect(Guardian.new(user).is_in_edit_topic_groups?).to eq(true) + end + + it "returns false if the user is not in edit_all_topic_groups" do + SiteSetting.edit_all_topic_groups = Group::AUTO_GROUPS[:trust_level_4] + + expect(Guardian.new(tl3_user).is_in_edit_topic_groups?).to eq(false) + end + + it "returns false if the edit_all_topic_groups is empty" do + SiteSetting.edit_all_topic_groups = nil + + expect(Guardian.new(user).is_in_edit_topic_groups?).to eq(false) + end + end + describe "#can_review_topic?" do it "returns false for TL4 users" do topic = Fabricate(:topic) From 965f557ff30167b3706f3e8645a959496dd04663 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Mon, 26 Jun 2023 10:42:20 -0500 Subject: [PATCH 08/10] Removed unnecessary lines --- ...e_trusted_users_can_edit_others_site_setting.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb index 3f0ec2ecc1bf67..91d2bd7d985d94 100644 --- a/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb +++ b/db/migrate/20230509214723_separate_trusted_users_can_edit_others_site_setting.rb @@ -6,7 +6,6 @@ def up "SELECT 1 FROM site_settings WHERE name = 'trusted_users_can_edit_others' AND value = 'f'", ) execute <<~SQL - DELETE FROM site_settings WHERE name = 'trusted_users_can_edit_others'; INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_topic_groups', 20, '', now(), now()); INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('edit_all_post_groups', 20, '', now(), now()); SQL @@ -14,14 +13,9 @@ def up end def down - if select_value( - "SELECT 1 FROM site_settings WHERE name IN ('edit_all_topic_groups', 'edit_all_post_groups')", - ) - execute <<~SQL - DELETE FROM site_settings WHERE name = 'edit_all_topic_groups'; - DELETE FROM site_settings WHERE name = 'edit_all_post_groups'; - INSERT INTO site_settings (name, data_type, value, created_at, updated_at) VALUES ('trusted_users_can_edit_others', 5, 'f', now(), now()); - SQL - end + execute <<~SQL + DELETE FROM site_settings WHERE name = 'edit_all_topic_groups'; + DELETE FROM site_settings WHERE name = 'edit_all_post_groups'; + SQL end end From 351628fb554537f6879e2cff75e42a72e146ff07 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Mon, 26 Jun 2023 15:46:13 -0500 Subject: [PATCH 09/10] Fixed spec issues --- spec/lib/guardian/post_guardian_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/lib/guardian/post_guardian_spec.rb b/spec/lib/guardian/post_guardian_spec.rb index 89b7013984219d..da2e385fd9d5b8 100644 --- a/spec/lib/guardian/post_guardian_spec.rb +++ b/spec/lib/guardian/post_guardian_spec.rb @@ -9,7 +9,6 @@ fab!(:group) { Fabricate(:group) } fab!(:group_user) { Fabricate(:group_user, group: group, user: user) } fab!(:category) { Fabricate(:category) } - fab!(:group) { Fabricate(:group) } fab!(:topic) { Fabricate(:topic, category: category) } fab!(:hidden_post) { Fabricate(:post, topic: topic, hidden: true) } @@ -60,7 +59,6 @@ describe "#is_in_edit_post_groups?" do it "returns true if the user is in edit_all_post_groups" do - group.add(user) SiteSetting.edit_all_post_groups = group.id.to_s expect(Guardian.new(user).is_in_edit_post_groups?).to eq(true) @@ -69,7 +67,7 @@ it "returns false if the user is not in edit_all_post_groups" do SiteSetting.edit_all_post_groups = Group::AUTO_GROUPS[:trust_level_4] - expect(Guardian.new(tl3_user).is_in_edit_post_groups?).to eq(false) + expect(Guardian.new(Fabricate(:trust_level_3)).is_in_edit_post_groups?).to eq(false) end it "returns false if the edit_all_post_groups is empty" do From 4832287e703a4f8df4cdc6718de8134e19a33542 Mon Sep 17 00:00:00 2001 From: Guhyoun Nam Date: Mon, 3 Jul 2023 10:45:09 -0500 Subject: [PATCH 10/10] FIxed backend error --- plugins/discourse-presence/spec/integration/presence_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index 2acf5b76ca630b..5a5acc1477560d 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -155,7 +155,7 @@ c = PresenceChannel.new("/discourse-presence/edit/#{p.id}") expect(c.config.public).to eq(false) - expect(c.config.allowed_group_ids).to contain(Group::AUTO_GROUPS[:trust_level_1]) + expect(c.config.allowed_group_ids).to include(Group::AUTO_GROUPS[:trust_level_1]) expect(c.config.allowed_user_ids).to contain_exactly(user.id) end