From ae3e804ee6881c845a944f4825093f8cd8a3b636 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 22 Aug 2024 12:53:08 +0300 Subject: [PATCH 1/5] Add update block ability with conditions --- app/abilities/ability.rb | 5 +++- test/abilities/abilities_test.rb | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 907712328..6baa67be5 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -60,7 +60,10 @@ class Ability can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment can [:new, :create, :edit, :update, :destroy], Redaction - can [:new, :edit, :create, :update, :revoke, :revoke_all], UserBlock + can [:new, :create, :revoke, :revoke_all], UserBlock + can :update, UserBlock, :creator => user + can :update, UserBlock, :revoker => user + can :update, UserBlock, :active? => true end if user.administrator? diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb index cc981b792..59f5928d7 100644 --- a/test/abilities/abilities_test.rb +++ b/test/abilities/abilities_test.rb @@ -95,6 +95,53 @@ class ModeratorAbilityTest < AbilityTest assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment" end end + + test "Active block update permissions" do + creator_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + block = create(:user_block, :creator => creator_user) + + creator_ability = Ability.new creator_user + assert creator_ability.can?(:edit, block) + assert creator_ability.can?(:update, block) + + other_moderator_ability = Ability.new other_moderator_user + assert other_moderator_ability.can?(:edit, block) + assert other_moderator_ability.can?(:update, block) + end + + test "Expired block update permissions" do + creator_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + block = create(:user_block, :expired, :creator => creator_user) + + creator_ability = Ability.new creator_user + assert creator_ability.can?(:edit, block) + assert creator_ability.can?(:update, block) + + other_moderator_ability = Ability.new other_moderator_user + assert other_moderator_ability.cannot?(:edit, block) + assert other_moderator_ability.cannot?(:update, block) + end + + test "Revoked block update permissions" do + creator_user = create(:moderator_user) + revoker_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + block = create(:user_block, :revoked, :creator => creator_user, :revoker => revoker_user) + + creator_ability = Ability.new creator_user + assert creator_ability.can?(:edit, block) + assert creator_ability.can?(:update, block) + + revoker_ability = Ability.new revoker_user + assert revoker_ability.can?(:edit, block) + assert revoker_ability.can?(:update, block) + + other_moderator_ability = Ability.new other_moderator_user + assert other_moderator_ability.cannot?(:edit, block) + assert other_moderator_ability.cannot?(:update, block) + end end class AdministratorAbilityTest < AbilityTest From 67a5809c8a486b5e8cc92dabd08624afc1a12e6c Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 22 Aug 2024 12:56:15 +0300 Subject: [PATCH 2/5] Show edit buttons on active blocks to all moderators --- app/views/user_blocks/_block.html.erb | 3 +-- app/views/user_blocks/show.html.erb | 3 +-- test/controllers/user_blocks_controller_test.rb | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/views/user_blocks/_block.html.erb b/app/views/user_blocks/_block.html.erb index 68850c28d..b464b4ccd 100644 --- a/app/views/user_blocks/_block.html.erb +++ b/app/views/user_blocks/_block.html.erb @@ -15,8 +15,7 @@ <% end %> <%= link_to t(".show"), block %> - <% if current_user && (current_user.id == block.creator_id || - current_user.id == block.revoker_id) %><%= link_to t(".edit"), edit_user_block_path(block) %><% end %> + <% if can?(:edit, block) %><%= link_to t(".edit"), edit_user_block_path(block) %><% end %> <% if can?(:revoke, UserBlock) %> <% if block.active? %><%= link_to t(".revoke"), revoke_user_block_path(block) %><% end %> <% end %> diff --git a/app/views/user_blocks/show.html.erb b/app/views/user_blocks/show.html.erb index c36c043cf..06275c3f0 100644 --- a/app/views/user_blocks/show.html.erb +++ b/app/views/user_blocks/show.html.erb @@ -30,8 +30,7 @@ current_user.id == @user_block.revoker_id) || can?(:revoke, UserBlock) && @user_block.active? %>
- <% if current_user && (current_user.id == @user_block.creator_id || - current_user.id == @user_block.revoker_id) %> + <% if can?(:edit, @user_block) %> <%= link_to t(".edit"), edit_user_block_path(@user_block), :class => "btn btn-outline-primary" %> <% end %> <% if can?(:revoke, UserBlock) && @user_block.active? %> diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb index a3926cff0..68fc313c3 100644 --- a/test/controllers/user_blocks_controller_test.rb +++ b/test/controllers/user_blocks_controller_test.rb @@ -173,7 +173,7 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest block = create(:user_block, :creator => creator_user) session_for(other_moderator_user) - check_block_buttons block, :revoke => 1 + check_block_buttons block, :edit => 1, :revoke => 1 session_for(creator_user) check_block_buttons block, :edit => 1, :revoke => 1 From 5e7ab68721b2d7fe916690b8521bd94c60fbe583 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 21 Aug 2024 16:51:19 +0300 Subject: [PATCH 3/5] Let all moderators revoke blocks by editing --- app/controllers/user_blocks_controller.rb | 8 ++-- config/locales/en.yml | 1 + .../user_blocks_controller_test.rb | 46 +++++++++++++------ 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index e77f83999..7027359cc 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -70,8 +70,7 @@ class UserBlocksController < ApplicationController def update if @valid_params - if current_user != @user_block.creator && - current_user != @user_block.revoker + if cannot?(:update, @user_block) flash[:error] = t(@user_block.revoker ? ".only_creator_or_revoker_can_edit" : ".only_creator_can_edit") redirect_to :action => "edit" else @@ -81,7 +80,10 @@ class UserBlocksController < ApplicationController @user_block.ends_at = Time.now.utc + @block_period.hours @user_block.deactivates_at = (@user_block.ends_at unless @user_block.needs_view) @user_block.revoker = current_user if user_block_was_active && !@user_block.active? - if !user_block_was_active && @user_block.active? + if user_block_was_active && @user_block.active? && current_user != @user_block.creator + flash.now[:error] = t(".only_creator_can_edit_without_revoking") + render :action => "edit" + elsif !user_block_was_active && @user_block.active? flash.now[:error] = t(".inactive_block_cannot_be_reactivated") render :action => "edit" else diff --git a/config/locales/en.yml b/config/locales/en.yml index 4d5ce7f58..a1cf2cecf 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2949,6 +2949,7 @@ en: flash: "Created a block on user %{name}." update: only_creator_can_edit: "Only the moderator who created this block can edit it." + only_creator_can_edit_without_revoking: "Only the moderator who created this block can edit it without revoking." only_creator_or_revoker_can_edit: "Only the moderators who created or revoked this block can edit it." inactive_block_cannot_be_reactivated: "This block is inactive and cannot be reactivated." success: "Block updated." diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb index 68fc313c3..dbeb7569f 100644 --- a/test/controllers/user_blocks_controller_test.rb +++ b/test/controllers/user_blocks_controller_test.rb @@ -391,7 +391,6 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest # test the update action def test_update moderator_user = create(:moderator_user) - second_moderator_user = create(:moderator_user) active_block = create(:user_block, :creator => moderator_user) # Not logged in yet, so updating a block should fail @@ -405,19 +404,7 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest put user_block_path(:id => active_block) assert_redirected_to :controller => "errors", :action => "forbidden" - # Login as the wrong moderator - session_for(second_moderator_user) - - # Check that only the person who created a block can update it - assert_no_difference "UserBlock.count" do - put user_block_path(:id => active_block, - :user_block_period => "12", - :user_block => { :needs_view => true, :reason => "Vandalism" }) - end - assert_redirected_to edit_user_block_path(active_block) - assert_equal "Only the moderator who created this block can edit it.", flash[:error] - - # Login as the correct moderator + # Login as the moderator session_for(moderator_user) # A bogus block period should result in an error @@ -495,7 +482,7 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest ## # test the update action revoking the block - def test_revoke_using_update + def test_revoke_using_update_by_creator moderator_user = create(:moderator_user) block = create(:user_block, :creator => moderator_user) @@ -503,6 +490,8 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest put user_block_path(block, :user_block_period => "24", :user_block => { :needs_view => false, :reason => "Updated Reason" }) + assert_redirected_to user_block_path(block) + assert_equal "Block updated.", flash[:notice] block.reload assert_predicate block, :active? assert_nil block.revoker @@ -510,11 +499,38 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest put user_block_path(block, :user_block_period => "0", :user_block => { :needs_view => false, :reason => "Updated Reason" }) + assert_redirected_to user_block_path(block) + assert_equal "Block updated.", flash[:notice] block.reload assert_not_predicate block, :active? assert_equal moderator_user, block.revoker end + def test_revoke_using_update_by_other_moderator + creator_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + block = create(:user_block, :creator => creator_user) + + session_for(other_moderator_user) + put user_block_path(block, + :user_block_period => "24", + :user_block => { :needs_view => false, :reason => "Updated Reason" }) + assert_response :success + assert_equal "Only the moderator who created this block can edit it without revoking.", flash[:error] + block.reload + assert_predicate block, :active? + assert_nil block.revoker + + put user_block_path(block, + :user_block_period => "0", + :user_block => { :needs_view => false, :reason => "Updated Reason" }) + assert_redirected_to user_block_path(block) + assert_equal "Block updated.", flash[:notice] + block.reload + assert_not_predicate block, :active? + assert_equal other_moderator_user, block.revoker + end + ## # test the revoke action def test_revoke From 11a7bf7e0ecaecd3536a666ce3e798d349574903 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 21 Aug 2024 16:58:34 +0300 Subject: [PATCH 4/5] Remove duration controls for blocks that can only be revoked by editing --- app/views/user_blocks/edit.html.erb | 10 +++++--- .../user_blocks_controller_test.rb | 24 +++++++++++++++++-- test/factories/user_blocks.rb | 6 +++++ test/system/user_blocks_test.rb | 18 ++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/app/views/user_blocks/edit.html.erb b/app/views/user_blocks/edit.html.erb index 2d8956ffc..aaa2fcaa4 100644 --- a/app/views/user_blocks/edit.html.erb +++ b/app/views/user_blocks/edit.html.erb @@ -9,7 +9,7 @@ <%= bootstrap_form_for(@user_block) do |f| %> <%= f.richtext_field :reason, :cols => 80, :rows => 20, :format => @user_block.reason_format %> - <% if @user_block.active? %> + <% if @user_block.active? && @user_block.creator == current_user %> <%= f.form_group do %> <%= label_tag "user_block_period", t(".period"), :class => "form-label" %> <%= select_tag "user_block_period", @@ -23,11 +23,15 @@ <% end %> <% else %>
- <%= t "user_blocks.update.inactive_block_cannot_be_reactivated" %> + <% if @user_block.active? %> + <%= t "user_blocks.update.only_creator_can_edit_without_revoking" %> + <% else %> + <%= t "user_blocks.update.inactive_block_cannot_be_reactivated" %> + <% end %>
<%= hidden_field_tag "user_block_period", 0 %> - <%= f.hidden_field :needs_view %> + <%= hidden_field_tag "user_block[needs_view]", false %> <% end %> <%= f.primary %> diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb index dbeb7569f..2cbfb37b4 100644 --- a/test/controllers/user_blocks_controller_test.rb +++ b/test/controllers/user_blocks_controller_test.rb @@ -252,7 +252,9 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest ## # test the edit action def test_edit - active_block = create(:user_block) + creator_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + active_block = create(:user_block, :creator => creator_user) # Check that the block edit page requires us to login get edit_user_block_path(:id => active_block) @@ -266,16 +268,34 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest assert_redirected_to :controller => "errors", :action => "forbidden" # Login as a moderator - session_for(create(:moderator_user)) + session_for(other_moderator_user) # Check that the block edit page loads for moderators get edit_user_block_path(:id => active_block) assert_response :success assert_select "h1 a[href='#{user_path active_block.user}']", :text => active_block.user.display_name + assert_select "form#edit_user_block_#{active_block.id}", :count => 1 do + assert_select "textarea#user_block_reason", :count => 1 + assert_select "select#user_block_period", :count => 0 + assert_select "input#user_block_needs_view[type='checkbox']", :count => 0 + assert_select "input#user_block_period[type='hidden']", :count => 1 + assert_select "input#user_block_needs_view[type='hidden']", :count => 1 + assert_select "input[type='submit'][value='Update block']", :count => 1 + end + + # Login as the block creator + session_for(creator_user) + + # Check that the block edit page loads for the creator + get edit_user_block_path(:id => active_block) + assert_response :success + assert_select "h1 a[href='#{user_path active_block.user}']", :text => active_block.user.display_name assert_select "form#edit_user_block_#{active_block.id}", :count => 1 do assert_select "textarea#user_block_reason", :count => 1 assert_select "select#user_block_period", :count => 1 assert_select "input#user_block_needs_view[type='checkbox']", :count => 1 + assert_select "input#user_block_period[type='hidden']", :count => 0 + assert_select "input#user_block_needs_view[type='hidden']", :count => 0 assert_select "input[type='submit'][value='Update block']", :count => 1 end diff --git a/test/factories/user_blocks.rb b/test/factories/user_blocks.rb index 82c2fbe6e..8bdd6875e 100644 --- a/test/factories/user_blocks.rb +++ b/test/factories/user_blocks.rb @@ -7,6 +7,12 @@ FactoryBot.define do user creator :factory => :moderator_user + trait :zero_hour do + now = Time.now.utc + created_at { now } + ends_at { now } + end + trait :needs_view do needs_view { true } deactivates_at { nil } diff --git a/test/system/user_blocks_test.rb b/test/system/user_blocks_test.rb index 89f98d759..b4b7ada7a 100644 --- a/test/system/user_blocks_test.rb +++ b/test/system/user_blocks_test.rb @@ -98,4 +98,22 @@ class UserBlocksSystemTest < ApplicationSystemTestCase click_on "Update block" assert_text(/Reason for block:\s+Editing expired blocks works/) end + + test "other moderator can revoke 0-hour blocks" do + creator_user = create(:moderator_user) + other_moderator_user = create(:moderator_user) + block = create(:user_block, :zero_hour, :needs_view, :creator => creator_user, :reason => "Testing revoking 0-hour blocks") + sign_in_as(other_moderator_user) + + visit edit_user_block_path(block) + assert_field "Reason", :with => "Testing revoking 0-hour blocks" + assert_no_select "user_block_period" + assert_no_field "Needs view" + + fill_in "Reason", :with => "Revoking 0-hour blocks works" + click_on "Update block" + assert_text(/Revoker:\s+#{Regexp.escape other_moderator_user.display_name}/) + assert_text(/Status:\s+Ended/) + assert_text(/Reason for block:\s+Revoking 0-hour blocks works/) + end end From 76736bab4f4b5f24074bb0a147ea3dafb5cffa0e Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 21 Aug 2024 17:07:40 +0300 Subject: [PATCH 5/5] Change submit button on block edit page when can only revoke --- app/views/user_blocks/edit.html.erb | 10 ++++++++-- config/locales/en.yml | 1 + test/controllers/user_blocks_controller_test.rb | 6 ++++-- test/system/user_blocks_test.rb | 2 +- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/app/views/user_blocks/edit.html.erb b/app/views/user_blocks/edit.html.erb index aaa2fcaa4..14480eac8 100644 --- a/app/views/user_blocks/edit.html.erb +++ b/app/views/user_blocks/edit.html.erb @@ -21,6 +21,8 @@ <%= f.form_group :needs_view do %> <%= f.check_box :needs_view %> <% end %> + + <%= f.primary %> <% else %>
<% if @user_block.active? %> @@ -32,7 +34,11 @@ <%= hidden_field_tag "user_block_period", 0 %> <%= hidden_field_tag "user_block[needs_view]", false %> - <% end %> - <%= f.primary %> + <% if @user_block.active? %> + <%= f.submit t(".revoke"), :class => "btn btn-danger" %> + <% else %> + <%= f.primary %> + <% end %> + <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index a1cf2cecf..80811cd1f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2943,6 +2943,7 @@ en: title: "Editing block on %{name}" heading_html: "Editing block on %{name}" period: "How long, starting now, the user will be blocked from the API for." + revoke: "Revoke block" filter: block_period: "The blocking period must be one of the values selectable in the drop-down list." create: diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb index 2cbfb37b4..61440ae56 100644 --- a/test/controllers/user_blocks_controller_test.rb +++ b/test/controllers/user_blocks_controller_test.rb @@ -278,9 +278,10 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest assert_select "textarea#user_block_reason", :count => 1 assert_select "select#user_block_period", :count => 0 assert_select "input#user_block_needs_view[type='checkbox']", :count => 0 + assert_select "input[type='submit'][value='Update block']", :count => 0 assert_select "input#user_block_period[type='hidden']", :count => 1 assert_select "input#user_block_needs_view[type='hidden']", :count => 1 - assert_select "input[type='submit'][value='Update block']", :count => 1 + assert_select "input[type='submit'][value='Revoke block']", :count => 1 end # Login as the block creator @@ -294,9 +295,10 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest assert_select "textarea#user_block_reason", :count => 1 assert_select "select#user_block_period", :count => 1 assert_select "input#user_block_needs_view[type='checkbox']", :count => 1 + assert_select "input[type='submit'][value='Update block']", :count => 1 assert_select "input#user_block_period[type='hidden']", :count => 0 assert_select "input#user_block_needs_view[type='hidden']", :count => 0 - assert_select "input[type='submit'][value='Update block']", :count => 1 + assert_select "input[type='submit'][value='Revoke block']", :count => 0 end # We should get an error if the user doesn't exist diff --git a/test/system/user_blocks_test.rb b/test/system/user_blocks_test.rb index b4b7ada7a..e9fa4e7ec 100644 --- a/test/system/user_blocks_test.rb +++ b/test/system/user_blocks_test.rb @@ -111,7 +111,7 @@ class UserBlocksSystemTest < ApplicationSystemTestCase assert_no_field "Needs view" fill_in "Reason", :with => "Revoking 0-hour blocks works" - click_on "Update block" + click_on "Revoke block" assert_text(/Revoker:\s+#{Regexp.escape other_moderator_user.display_name}/) assert_text(/Status:\s+Ended/) assert_text(/Reason for block:\s+Revoking 0-hour blocks works/)