From 3b34703db30a859c0f667be219900c282f3578fe Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 5 Nov 2025 16:37:43 +0100 Subject: [PATCH] Make sure that only admins or card creators can delete cards --- app/controllers/cards_controller.rb | 5 +++ app/models/user/role.rb | 4 ++ app/views/cards/_messages.html.erb | 10 +++-- test/controllers/boards_controller_test.rb | 6 +-- test/controllers/cards_controller_test.rb | 50 ++++++++++++++++++++++ test/models/user/role_test.rb | 19 ++++++++ test/test_helpers/session_test_helper.rb | 5 +++ 7 files changed, 91 insertions(+), 8 deletions(-) diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index 56bd94da1..3b9f0fcde 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -3,6 +3,7 @@ class CardsController < ApplicationController before_action :set_board, only: %i[ create ] before_action :set_card, only: %i[ show edit update destroy ] + before_action :ensure_permission_to_administer_card, only: %i[ destroy ] def index set_page_and_extract_portion_from @filter.cards @@ -42,6 +43,10 @@ class CardsController < ApplicationController @card = Current.user.accessible_cards.find params[:id] end + def ensure_permission_to_administer_card + head :forbidden unless Current.user.can_administer_card?(@card) + end + def suppressing_broadcasts_unless_published(card, &block) if card.published? yield diff --git a/app/models/user/role.rb b/app/models/user/role.rb index 029e2740d..a327ffcf6 100644 --- a/app/models/user/role.rb +++ b/app/models/user/role.rb @@ -25,4 +25,8 @@ module User::Role def can_administer_board?(board) admin? || board.creator == self end + + def can_administer_card?(card) + admin? || card.creator == self + end end diff --git a/app/views/cards/_messages.html.erb b/app/views/cards/_messages.html.erb index 144be94cc..c92c94f7c 100644 --- a/app/views/cards/_messages.html.erb +++ b/app/views/cards/_messages.html.erb @@ -6,8 +6,10 @@ <%= render "cards/comments/watchers", card: card %> <% end %> - + <% if Current.user.can_administer_card?(card) %> + + <% end %> <% end %> diff --git a/test/controllers/boards_controller_test.rb b/test/controllers/boards_controller_test.rb index cba9c1d0c..14f573780 100644 --- a/test/controllers/boards_controller_test.rb +++ b/test/controllers/boards_controller_test.rb @@ -97,8 +97,7 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest end test "non-admin cannot change all_access on board they don't own" do - Session.destroy_all - sign_in_as :jz + logout_and_sign_in_as :jz board = boards(:writebook) original_all_access = board.all_access @@ -110,8 +109,7 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest end test "non-admin cannot change individual user accesses on board they don't own" do - Session.destroy_all - sign_in_as :jz + logout_and_sign_in_as :jz board = boards(:writebook) original_users = board.users.sort diff --git a/test/controllers/cards_controller_test.rb b/test/controllers/cards_controller_test.rb index 959dcef65..bc1a82e4b 100644 --- a/test/controllers/cards_controller_test.rb +++ b/test/controllers/cards_controller_test.rb @@ -70,4 +70,54 @@ class CardsControllerTest < ActionDispatch::IntegrationTest get card_path(cards(:logo)) assert_response :not_found end + + test "admins can see delete button on any card" do + get card_path(cards(:logo)) + assert_response :success + assert_match "Delete this card", response.body + end + + test "card creators can see delete button on their own cards" do + logout_and_sign_in_as :david + + get card_path(cards(:logo)) + assert_response :success + assert_match "Delete this card", response.body + end + + test "non-admins cannot see delete button on cards they did not create" do + logout_and_sign_in_as :jz + + get card_path(cards(:logo)) + assert_response :success + assert_no_match "Delete this card", response.body + end + + test "non-admins cannot delete cards they did not create" do + logout_and_sign_in_as :jz + + assert_no_difference -> { Card.count } do + delete card_path(cards(:logo)) + end + + assert_response :forbidden + end + + test "card creators can delete their own cards" do + logout_and_sign_in_as :david + + assert_difference -> { Card.count }, -1 do + delete card_path(cards(:logo)) + end + + assert_redirected_to boards(:writebook) + end + + test "admins can delete any card" do + assert_difference -> { Card.count }, -1 do + delete card_path(cards(:logo)) + end + + assert_redirected_to boards(:writebook) + end end diff --git a/test/models/user/role_test.rb b/test/models/user/role_test.rb index 4ae9bdf04..9f0889f9e 100644 --- a/test/models/user/role_test.rb +++ b/test/models/user/role_test.rb @@ -26,4 +26,23 @@ class User::RoleTest < ActiveSupport::TestCase # Creator cannot administer other people's boards assert_not users(:david).can_administer_board?(private_board) end + + test "can administer card?" do + logo_card = cards(:logo) + text_card = cards(:text) + + # Admin can administer any card + assert users(:kevin).can_administer_card?(logo_card) + assert users(:kevin).can_administer_card?(text_card) + + # Creator can administer their own card + assert users(:david).can_administer_card?(logo_card) + + # Regular user cannot administer cards they didn't create + assert_not users(:jz).can_administer_card?(logo_card) + assert_not users(:jz).can_administer_card?(text_card) + + # Creator cannot administer other people's cards + assert_not users(:david).can_administer_card?(text_card) + end end diff --git a/test/test_helpers/session_test_helper.rb b/test/test_helpers/session_test_helper.rb index dc8f89d55..8936b3548 100644 --- a/test/test_helpers/session_test_helper.rb +++ b/test/test_helpers/session_test_helper.rb @@ -27,6 +27,11 @@ module SessionTestHelper assert_not_nil cookie, "Expected session_token cookie to be set after sign in" end + def logout_and_sign_in_as(identity) + Session.delete_all + sign_in_as identity + end + def sign_out untenanted do delete session_path