From 5f33656c8d6725969ac63dbfe038633ad0e4352f Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 20 Mar 2012 16:22:07 +0000 Subject: [PATCH] Get rid of custom CSRF protection for user role changes By restricting role changes to POST requests, which they should be anyway, we get all the rails CSRF protection for free. --- app/controllers/user_roles_controller.rb | 17 ----------------- app/views/user/view.html.erb | 4 ++-- app/views/user_roles/grant.html.erb | 7 ------- app/views/user_roles/revoke.html.erb | 7 ------- config/routes.rb | 4 ++-- db/structure.sql | 3 ++- test/functional/user_roles_controller_test.rb | 8 -------- test/integration/user_roles_test.rb | 10 ++++------ 8 files changed, 10 insertions(+), 50 deletions(-) delete mode 100644 app/views/user_roles/grant.html.erb delete mode 100644 app/views/user_roles/revoke.html.erb diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb index 54dc90dee..cb0425f32 100644 --- a/app/controllers/user_roles_controller.rb +++ b/app/controllers/user_roles_controller.rb @@ -8,7 +8,6 @@ class UserRolesController < ApplicationController before_filter :require_valid_role before_filter :not_in_role, :only => [:grant] before_filter :in_role, :only => [:revoke] - around_filter :setup_nonce def grant @this_user.roles.create({ @@ -38,22 +37,6 @@ class UserRolesController < ApplicationController redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] unless @this_user end - ## - # the random nonce here which isn't predictable, making an CSRF - # procedure much, much more difficult. setup the nonce. if the given - # nonce matches the session nonce then yield into the actual method. - # otherwise, just sets up the nonce for the form. - def setup_nonce - if params[:nonce] and params[:nonce] == session[:nonce] - @nonce = params[:nonce] - yield - else - @nonce = OAuth::Helper.generate_nonce - session[:nonce] = @nonce - render - end - end - ## # require that the given role is valid. the role is a URL # parameter, so should always be present. diff --git a/app/views/user/view.html.erb b/app/views/user/view.html.erb index d9a65b342..f661c9b85 100644 --- a/app/views/user/view.html.erb +++ b/app/views/user/view.html.erb @@ -5,9 +5,9 @@ <% UserRole::ALL_ROLES.each do |role| %> <% if @user and @user.administrator? %> <% if @this_user.has_role? role %> - <%= link_to(image_tag("roles/#{role}.png", :size => "20x20", :border => 0, :alt => t("user.view.role.revoke.#{role}"), :title => t("user.view.role.revoke.#{role}")), :controller => 'user_roles', :action => 'revoke', :display_name => @this_user.display_name, :role => role) %> + <%= link_to image_tag("roles/#{role}.png", :size => "20x20", :border => 0, :alt => t("user.view.role.revoke.#{role}"), :title => t("user.view.role.revoke.#{role}")), revoke_role_path(:display_name => @this_user.display_name, :role => role), :method => :post, :confirm => t('user_role.revoke.are_you_sure', :name => @this_user.display_name, :role => role) %> <% else %> - <%= link_to(image_tag("roles/blank_#{role}.png", :size => "20x20", :border => 0, :alt => t("user.view.role.grant.#{role}"), :title => t("user.view.role.grant.#{role}")), :controller => 'user_roles', :action => 'grant', :display_name => @this_user.display_name, :role => role) %> + <%= link_to image_tag("roles/blank_#{role}.png", :size => "20x20", :border => 0, :alt => t("user.view.role.grant.#{role}"), :title => t("user.view.role.grant.#{role}")), grant_role_path(:display_name => @this_user.display_name, :role => role), :method => :post, :confirm => t('user_role.grant.are_you_sure', :name => @this_user.display_name, :role => role) %> <% end %> <% elsif @this_user.has_role? role %> <%= image_tag("roles/#{role}.png", :size => "20x20", :border => 0, :alt => t("user.view.role.#{role}"), :title => t("user.view.role.#{role}")) %> diff --git a/app/views/user_roles/grant.html.erb b/app/views/user_roles/grant.html.erb deleted file mode 100644 index 13f81184c..000000000 --- a/app/views/user_roles/grant.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= form_tag request.fullpath do %> -<%= hidden_field_tag 'nonce', @nonce %> -<% @title = t('user_role.grant.heading') %> -

<%= t('user_role.grant.heading') %>

-

<%= t('user_role.grant.are_you_sure', :name => params[:display_name], :role => params[:role]) %>

-

<%= submit_tag t('user_role.grant.confirm') %>

-<% end %> diff --git a/app/views/user_roles/revoke.html.erb b/app/views/user_roles/revoke.html.erb deleted file mode 100644 index 240a91fff..000000000 --- a/app/views/user_roles/revoke.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= form_tag request.fullpath do %> -<%= hidden_field_tag 'nonce', @nonce %> -<% @title = t('user_role.revoke.heading') %> -

<%= t('user_role.revoke.heading') %>

-

<%= t('user_role.revoke.are_you_sure', :name => params[:display_name], :role => params[:role]) %>

-

<%= submit_tag t'user_role.revoke.confirm' %>

-<% end %> diff --git a/config/routes.rb b/config/routes.rb index fec224bb9..9550c84ff 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -219,8 +219,8 @@ OpenStreetMap::Application.routes.draw do match '/oauth/test_request' => 'oauth#test_request', :as => :test_request # roles and banning pages - match '/user/:display_name/role/:role/grant' => 'user_roles#grant', :via => [:get, :post] - match '/user/:display_name/role/:role/revoke' => 'user_roles#revoke', :via => [:get, :post] + match '/user/:display_name/role/:role/grant' => 'user_roles#grant', :via => :post, :as => "grant_role" + match '/user/:display_name/role/:role/revoke' => 'user_roles#revoke', :via => :post, :as => "revoke_role" match '/user/:display_name/blocks' => 'user_blocks#blocks_on', :via => :get match '/user/:display_name/blocks_by' => 'user_blocks#blocks_by', :via => :get match '/blocks/new/:display_name' => 'user_blocks#new', :via => :get, :as => "new_user_block" diff --git a/db/structure.sql b/db/structure.sql index 57a9b7211..eed9b053b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -44,7 +44,8 @@ SET search_path = public, pg_catalog; CREATE TYPE format_enum AS ENUM ( 'html', - 'markdown' + 'markdown', + 'text' ); diff --git a/test/functional/user_roles_controller_test.rb b/test/functional/user_roles_controller_test.rb index 1e2d29b45..e89230a7f 100644 --- a/test/functional/user_roles_controller_test.rb +++ b/test/functional/user_roles_controller_test.rb @@ -4,18 +4,10 @@ class UserRolesControllerTest < ActionController::TestCase ## # test all routes which lead to this controller def test_routes - assert_routing( - { :path => "/user/username/role/rolename/grant", :method => :get }, - { :controller => "user_roles", :action => "grant", :display_name => "username", :role => "rolename" } - ) assert_routing( { :path => "/user/username/role/rolename/grant", :method => :post }, { :controller => "user_roles", :action => "grant", :display_name => "username", :role => "rolename" } ) - assert_routing( - { :path => "/user/username/role/rolename/revoke", :method => :get }, - { :controller => "user_roles", :action => "revoke", :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" } diff --git a/test/integration/user_roles_test.rb b/test/integration/user_roles_test.rb index 17531ef3f..948bb895c 100644 --- a/test/integration/user_roles_test.rb +++ b/test/integration/user_roles_test.rb @@ -16,6 +16,8 @@ class UserRolesTest < ActionController::IntegrationTest check_fail(:revoke, :administrator_user, :moderator) end +private + def check_fail(action, user, role) get '/login' assert_response :redirect @@ -27,8 +29,7 @@ class UserRolesTest < ActionController::IntegrationTest follow_redirect! assert_response :success - get "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}" - assert_response :redirect + post "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}" assert_redirected_to :controller => 'user', :action => 'view', :display_name => users(:second_public_user).display_name reset! @@ -45,10 +46,7 @@ class UserRolesTest < ActionController::IntegrationTest follow_redirect! assert_response :success - get "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}" - assert_response :success - post "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}", {:confirm => "yes", :nonce => session[:nonce]} - assert_response :redirect + post "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}" assert_redirected_to :controller => 'user', :action => 'view', :display_name => users(:second_public_user).display_name reset! -- 2.39.5