From 600812f6ad23ab7e1cb3ab25c4b09844ba6daa55 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 10 Jun 2020 11:49:18 +0200 Subject: [PATCH] Rework user#set_status and #delete to avoid GET requests This renames the delete action to destroy, and starts using resourceful routing for user actions. --- app/abilities/ability.rb | 2 +- app/controllers/users_controller.rb | 4 ++-- app/views/users/show.html.erb | 12 +++++------ config/routes.rb | 5 ++--- test/controllers/users_controller_test.rb | 26 +++++++++++------------ 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 5b1d7df68..e72b4e85b 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -56,7 +56,7 @@ class Ability can [:hide, :unhide, :hidecomment, :unhidecomment], DiaryEntry can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment - can [:set_status, :delete, :index], User + can [:set_status, :destroy, :index], User can [:grant, :revoke], UserRole end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4b949fe07..676f8d8f9 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -12,7 +12,7 @@ class UsersController < ApplicationController before_action :require_self, :only => [:account] before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public] before_action :require_cookies, :only => [:new, :login, :confirm] - before_action :lookup_user_by_name, :only => [:set_status, :delete] + before_action :lookup_user_by_name, :only => [:set_status, :destroy] before_action :allow_thirdparty_images, :only => [:show, :account] def terms @@ -393,7 +393,7 @@ class UsersController < ApplicationController ## # delete a user, marking them as deleted and removing personal data - def delete + def destroy @user.delete redirect_to user_path(:display_name => params[:display_name]) end diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 011226109..3b330d294 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -117,30 +117,30 @@ <% if can? :set_status, User %> <% if ["active", "confirmed"].include? @user.status %>
  • - <%= link_to t(".deactivate_user"), set_status_user_path(:status => "pending", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".deactivate_user"), set_status_user_path(:status => "pending", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
    • <% elsif ["pending"].include? @user.status %>
  • - <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
    • <% end %> <% if ["active", "suspended"].include? @user.status %>
  • - <%= link_to t(".confirm_user"), set_status_user_path(:status => "confirmed", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".confirm_user"), set_status_user_path(:status => "confirmed", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
    • <% end %>
  • <% if ["pending", "active", "confirmed", "suspended"].include? @user.status %> - <%= link_to t(".hide_user"), set_status_user_path(:status => "deleted", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".hide_user"), set_status_user_path(:status => "deleted", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %> <% else %> - <%= link_to t(".unhide_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".unhide_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
    • <% end %> <% end %> <% if can? :delete, User %>
  • - <%= link_to t(".delete_user"), delete_user_path(:display_name => @user.display_name), :data => { :confirm => t(".confirm") } %> + <%= link_to t(".delete_user"), user_path(:display_name => @user.display_name), :method => :delete, :data => { :confirm => t(".confirm") } %>
    • <% end %> diff --git a/config/routes.rb b/config/routes.rb index 2744b1323..a98a11430 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -235,10 +235,9 @@ OpenStreetMap::Application.routes.draw do post "/user/:display_name/diary/:id/unsubscribe" => "diary_entries#unsubscribe", :as => :diary_entry_unsubscribe, :id => /\d+/ # user pages - get "/user/:display_name" => "users#show", :as => "user" + resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy] match "/user/:display_name/account" => "users#account", :via => [:get, :post], :as => "user_account" - get "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user - get "/user/:display_name/delete" => "users#delete", :as => :delete_user + post "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user # friendships match "/user/:display_name/make_friend" => "friendships#make_friend", :via => [:get, :post], :as => "make_friend" diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 94e22f6c3..91ef7222f 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -122,12 +122,12 @@ class UsersControllerTest < ActionDispatch::IntegrationTest ) assert_routing( - { :path => "/user/username/set_status", :method => :get }, + { :path => "/user/username/set_status", :method => :post }, { :controller => "users", :action => "set_status", :display_name => "username" } ) assert_routing( - { :path => "/user/username/delete", :method => :get }, - { :controller => "users", :action => "delete", :display_name => "username" } + { :path => "/user/username", :method => :delete }, + { :controller => "users", :action => "destroy", :display_name => "username" } ) assert_routing( @@ -1223,41 +1223,39 @@ class UsersControllerTest < ActionDispatch::IntegrationTest user = create(:user) # Try without logging in - get set_status_user_path(user), :params => { :status => "suspended" } - assert_response :redirect - assert_redirected_to :action => :login, :referer => set_status_user_path(:status => "suspended") + post set_status_user_path(user), :params => { :status => "suspended" } + assert_response :forbidden # Now try as a normal user session_for(user) - get set_status_user_path(user), :params => { :status => "suspended" } + post set_status_user_path(user), :params => { :status => "suspended" } assert_response :redirect assert_redirected_to :controller => :errors, :action => :forbidden # Finally try as an administrator session_for(create(:administrator_user)) - get set_status_user_path(user), :params => { :status => "suspended" } + post set_status_user_path(user), :params => { :status => "suspended" } assert_response :redirect assert_redirected_to :action => :show, :display_name => user.display_name assert_equal "suspended", User.find(user.id).status end - def test_delete + def test_destroy user = create(:user, :home_lat => 12.1, :home_lon => 12.1, :description => "test") # Try without logging in - get delete_user_path(user), :params => { :status => "suspended" } - assert_response :redirect - assert_redirected_to :action => :login, :referer => delete_user_path(:status => "suspended") + delete user_path(user), :params => { :status => "suspended" } + assert_response :forbidden # Now try as a normal user session_for(user) - get delete_user_path(user), :params => { :status => "suspended" } + delete user_path(user), :params => { :status => "suspended" } assert_response :redirect assert_redirected_to :controller => :errors, :action => :forbidden # Finally try as an administrator session_for(create(:administrator_user)) - get delete_user_path(user), :params => { :status => "suspended" } + delete user_path(user), :params => { :status => "suspended" } assert_response :redirect assert_redirected_to :action => :show, :display_name => user.display_name -- 2.43.2