From b8247478f4f772df8d09a29f3acd383fc6d32cf0 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Mon, 28 Oct 2024 03:44:10 +0300 Subject: [PATCH 1/1] Use resourceful routes for granting/revoking user roles --- app/abilities/ability.rb | 2 +- app/controllers/concerns/user_methods.rb | 5 +- app/controllers/user_roles_controller.rb | 8 ++-- app/helpers/user_roles_helper.rb | 6 +-- config/routes.rb | 8 ++-- test/abilities/abilities_test.rb | 6 +-- .../controllers/user_roles_controller_test.rb | 46 +++++++++---------- test/helpers/user_roles_helper_test.rb | 36 +++++++-------- 8 files changed, 59 insertions(+), 58 deletions(-) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 9a8f193a1..725ceccb2 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -68,7 +68,7 @@ class Ability can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment can [:set_status, :destroy, :index], User - can [:grant, :revoke], UserRole + can [:create, :destroy], UserRole end end end diff --git a/app/controllers/concerns/user_methods.rb b/app/controllers/concerns/user_methods.rb index 28305b5c3..d79ed48d2 100644 --- a/app/controllers/concerns/user_methods.rb +++ b/app/controllers/concerns/user_methods.rb @@ -6,9 +6,10 @@ module UserMethods ## # ensure that there is a "user" instance variable def lookup_user - @user = User.active.find_by!(:display_name => params[:display_name]) + display_name = params[:display_name] || params[:user_display_name] + @user = User.active.find_by!(:display_name => display_name) rescue ActiveRecord::RecordNotFound - render_unknown_user params[:display_name] + render_unknown_user display_name end ## diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb index 469b2c40b..912453be8 100644 --- a/app/controllers/user_roles_controller.rb +++ b/app/controllers/user_roles_controller.rb @@ -9,15 +9,15 @@ class UserRolesController < ApplicationController before_action :lookup_user before_action :require_valid_role - before_action :not_in_role, :only => [:grant] - before_action :in_role, :only => [:revoke] + before_action :not_in_role, :only => :create + before_action :in_role, :only => :destroy - def grant + def create @user.roles.create(:role => @role, :granter => current_user) redirect_to user_path(@user) end - def revoke + def destroy # checks that administrator role is not revoked from current user if current_user == @user && @role == "administrator" flash[:error] = t("user_role.filter.not_revoke_admin_current_user") diff --git a/app/helpers/user_roles_helper.rb b/app/helpers/user_roles_helper.rb index e839c0ae6..02017bdb9 100644 --- a/app/helpers/user_roles_helper.rb +++ b/app/helpers/user_roles_helper.rb @@ -7,12 +7,12 @@ module UserRolesHelper if current_user&.administrator? if user.role?(role) link_to role_icon_svg_tag(role, false, t("users.show.role.revoke.#{role}")), - revoke_role_path(user, role), - :method => :post, + user_role_path(user, role), + :method => :delete, :data => { :confirm => t("user_role.revoke.are_you_sure", :name => user.display_name, :role => role) } else link_to role_icon_svg_tag(role, true, t("users.show.role.grant.#{role}")), - grant_role_path(user, role), + user_role_path(user, role), :method => :post, :data => { :confirm => t("user_role.grant.are_you_sure", :name => user.display_name, :role => role) } end diff --git a/config/routes.rb b/config/routes.rb index 22c4ad642..af72c457d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -266,7 +266,9 @@ OpenStreetMap::Application.routes.draw do post "/diary_comments/:comment/unhide" => "diary_comments#unhide", :comment => /\d+/, :as => :unhide_diary_comment # user pages - resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy] + resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy] do + resource :role, :controller => "user_roles", :path => "roles/:role", :only => [:create, :destroy] + end get "/user/:display_name/account", :to => redirect(:path => "/account/edit") post "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user @@ -323,9 +325,7 @@ OpenStreetMap::Application.routes.draw do end resources :user_mutes, :only => [:index] - # roles and banning pages - post "/user/:display_name/role/:role/grant" => "user_roles#grant", :as => "grant_role" - post "/user/:display_name/role/:role/revoke" => "user_roles#revoke", :as => "revoke_role" + # banning pages get "/user/:display_name/blocks" => "user_blocks#blocks_on", :as => "user_blocks_on" get "/user/:display_name/blocks_by" => "user_blocks#blocks_by", :as => "user_blocks_by" get "/blocks/new/:display_name" => "user_blocks#new", :as => "new_user_block" diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb index 59f5928d7..99168375a 100644 --- a/test/abilities/abilities_test.rb +++ b/test/abilities/abilities_test.rb @@ -45,7 +45,7 @@ class GuestAbilityTest < AbilityTest test "user roles permissions for a guest" do ability = Ability.new nil - [:grant, :revoke].each do |action| + [:create, :destroy].each do |action| assert ability.cannot?(action, UserRole), "should not be able to #{action} UserRoles" end end @@ -86,7 +86,7 @@ class ModeratorAbilityTest < AbilityTest test "User Roles permissions" do ability = Ability.new create(:moderator_user) - [:grant, :revoke].each do |action| + [:create, :destroy].each do |action| assert ability.cannot?(action, UserRole), "should not be able to #{action} UserRoles" end @@ -159,7 +159,7 @@ class AdministratorAbilityTest < AbilityTest test "User Roles permissions for an administrator" do ability = Ability.new create(:administrator_user) - [:grant, :revoke].each do |action| + [:create, :destroy].each do |action| assert ability.can?(action, UserRole), "should be able to #{action} UserRoles" end end diff --git a/test/controllers/user_roles_controller_test.rb b/test/controllers/user_roles_controller_test.rb index 66735ccf5..301a89f56 100644 --- a/test/controllers/user_roles_controller_test.rb +++ b/test/controllers/user_roles_controller_test.rb @@ -5,32 +5,32 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest # test all routes which lead to this controller def test_routes assert_routing( - { :path => "/user/username/role/rolename/grant", :method => :post }, - { :controller => "user_roles", :action => "grant", :display_name => "username", :role => "rolename" } + { :path => "/user/username/roles/rolename", :method => :post }, + { :controller => "user_roles", :action => "create", :user_display_name => "username", :role => "rolename" } ) assert_routing( - { :path => "/user/username/role/rolename/revoke", :method => :post }, - { :controller => "user_roles", :action => "revoke", :display_name => "username", :role => "rolename" } + { :path => "/user/username/roles/rolename", :method => :delete }, + { :controller => "user_roles", :action => "destroy", :user_display_name => "username", :role => "rolename" } ) end ## - # test the grant action - def test_grant + # test the grant role action + def test_update target_user = create(:user) normal_user = create(:user) administrator_user = create(:administrator_user) super_user = create(:super_user) # Granting should fail when not logged in - post grant_role_path(target_user, "moderator") + post user_role_path(target_user, "moderator") assert_response :forbidden # Login as an unprivileged user session_for(normal_user) # Granting should still fail - post grant_role_path(target_user, "moderator") + post user_role_path(target_user, "moderator") assert_redirected_to :controller => :errors, :action => :forbidden # Login as an administrator @@ -39,7 +39,7 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest UserRole::ALL_ROLES.each do |role| # Granting a role to a non-existent user should fail assert_difference "UserRole.count", 0 do - post grant_role_path("non_existent_user", role) + post user_role_path("non_existent_user", role) end assert_response :not_found assert_template "users/no_such_user" @@ -47,20 +47,20 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest # Granting a role to a user that already has it should fail assert_no_difference "UserRole.count" do - post grant_role_path(super_user, role) + post user_role_path(super_user, role) end assert_redirected_to user_path(super_user) assert_equal "The user already has role #{role}.", flash[:error] # Granting a role to a user that doesn't have it should work... assert_difference "UserRole.count", 1 do - post grant_role_path(target_user, role) + post user_role_path(target_user, role) end assert_redirected_to user_path(target_user) # ...but trying a second time should fail assert_no_difference "UserRole.count" do - post grant_role_path(target_user, role) + post user_role_path(target_user, role) end assert_redirected_to user_path(target_user) assert_equal "The user already has role #{role}.", flash[:error] @@ -68,29 +68,29 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest # Granting a non-existent role should fail assert_difference "UserRole.count", 0 do - post grant_role_path(target_user, "no_such_role") + post user_role_path(target_user, "no_such_role") end assert_redirected_to user_path(target_user) assert_equal "The string 'no_such_role' is not a valid role.", flash[:error] end ## - # test the revoke action - def test_revoke + # test the revoke role action + def test_destroy target_user = create(:user) normal_user = create(:user) administrator_user = create(:administrator_user) super_user = create(:super_user) # Revoking should fail when not logged in - post revoke_role_path(target_user, "moderator") + delete user_role_path(target_user, "moderator") assert_response :forbidden # Login as an unprivileged user session_for(normal_user) # Revoking should still fail - post revoke_role_path(target_user, "moderator") + delete user_role_path(target_user, "moderator") assert_redirected_to :controller => :errors, :action => :forbidden # Login as an administrator @@ -99,7 +99,7 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest UserRole::ALL_ROLES.each do |role| # Removing a role from a non-existent user should fail assert_difference "UserRole.count", 0 do - post revoke_role_path("non_existent_user", role) + delete user_role_path("non_existent_user", role) end assert_response :not_found assert_template "users/no_such_user" @@ -107,20 +107,20 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest # Removing a role from a user that doesn't have it should fail assert_no_difference "UserRole.count" do - post revoke_role_path(target_user, role) + delete user_role_path(target_user, role) end assert_redirected_to user_path(target_user) assert_equal "The user does not have role #{role}.", flash[:error] # Removing a role from a user that has it should work... assert_difference "UserRole.count", -1 do - post revoke_role_path(super_user, role) + delete user_role_path(super_user, role) end assert_redirected_to user_path(super_user) # ...but trying a second time should fail assert_no_difference "UserRole.count" do - post revoke_role_path(super_user, role) + delete user_role_path(super_user, role) end assert_redirected_to user_path(super_user) assert_equal "The user does not have role #{role}.", flash[:error] @@ -128,13 +128,13 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest # Revoking a non-existent role should fail assert_difference "UserRole.count", 0 do - post revoke_role_path(target_user, "no_such_role") + delete user_role_path(target_user, "no_such_role") end assert_redirected_to user_path(target_user) assert_equal "The string 'no_such_role' is not a valid role.", flash[:error] # Revoking administrator role from current user should fail - post revoke_role_path(administrator_user, "administrator") + delete user_role_path(administrator_user, "administrator") assert_redirected_to user_path(administrator_user) assert_equal "Cannot revoke administrator role from current user.", flash[:error] end diff --git a/test/helpers/user_roles_helper_test.rb b/test/helpers/user_roles_helper_test.rb index 41366199c..3b63889a3 100644 --- a/test/helpers/user_roles_helper_test.rb +++ b/test/helpers/user_roles_helper_test.rb @@ -31,7 +31,7 @@ class UserRolesHelperTest < ActionView::TestCase create(:user) do |user| icon = role_icon(user, "moderator") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Grant moderator access" end @@ -39,7 +39,7 @@ class UserRolesHelperTest < ActionView::TestCase icon = role_icon(user, "importer") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'importer')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Grant importer access" end @@ -49,7 +49,7 @@ class UserRolesHelperTest < ActionView::TestCase create(:moderator_user) do |user| icon = role_icon(user, "moderator") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Revoke moderator access" end @@ -57,7 +57,7 @@ class UserRolesHelperTest < ActionView::TestCase icon = role_icon(user, "importer") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'importer')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Grant importer access" end @@ -67,7 +67,7 @@ class UserRolesHelperTest < ActionView::TestCase create(:importer_user) do |user| icon = role_icon(user, "moderator") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Grant moderator access" end @@ -75,7 +75,7 @@ class UserRolesHelperTest < ActionView::TestCase icon = role_icon(user, "importer") icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon) - assert_dom icon_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']", :count => 1 do + assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']", :count => 1 do assert_dom "> svg", :count => 1 do assert_dom "> title", :text => "Revoke importer access" end @@ -113,13 +113,13 @@ class UserRolesHelperTest < ActionView::TestCase icons = role_icons(user) icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons) assert_dom icons_dom, "a:root", :count => 3 - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant administrator access" end - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant moderator access" end - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'importer')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant importer access" end end @@ -128,13 +128,13 @@ class UserRolesHelperTest < ActionView::TestCase icons = role_icons(user) icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons) assert_dom icons_dom, "a:root", :count => 3 - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant administrator access" end - assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']" do assert_dom "> svg > title", :text => "Revoke moderator access" end - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'importer')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant importer access" end end @@ -143,13 +143,13 @@ class UserRolesHelperTest < ActionView::TestCase icons = role_icons(user) icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons) assert_dom icons_dom, "a:root", :count => 3 - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant administrator access" end - assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']" do assert_dom "> svg > title", :text => "Grant moderator access" end - assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']" do assert_dom "> svg > title", :text => "Revoke importer access" end end @@ -158,13 +158,13 @@ class UserRolesHelperTest < ActionView::TestCase icons = role_icons(user) icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons) assert_dom icons_dom, "a:root", :count => 3 - assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'administrator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='delete']" do assert_dom "> svg > title", :text => "Revoke administrator access" end - assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']" do assert_dom "> svg > title", :text => "Revoke moderator access" end - assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']" do + assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']" do assert_dom "> svg > title", :text => "Revoke importer access" end end -- 2.39.5