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

@@ -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.

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}")) %>

app/views/user_roles/grant.html.erb

@@ -1,7 +0,0 @@
-<%= form_tag request.fullpath do %>
-<%= hidden_field_tag 'nonce', @nonce %>
-<% @title = t('user_role.grant.heading') %>
-<h1><%= t('user_role.grant.heading') %></h1>
-<p><%= t('user_role.grant.are_you_sure', :name => params[:display_name], :role => params[:role]) %></p>
-<p><%= submit_tag t('user_role.grant.confirm') %></p>
-<% end %>

app/views/user_roles/revoke.html.erb

@@ -1,7 +0,0 @@
-<%= form_tag request.fullpath do %>
-<%= hidden_field_tag 'nonce', @nonce %>
-<% @title = t('user_role.revoke.heading') %>
-<h1><%= t('user_role.revoke.heading') %></h1>
-<p><%= t('user_role.revoke.are_you_sure', :name => params[:display_name], :role => params[:role]) %></p>
-<p><%= submit_tag t'user_role.revoke.confirm' %></p>
-<% end %>

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"

db/structure.sql

@@ -44,7 +44,8 @@ SET search_path = public, pg_catalog;
 
 CREATE TYPE format_enum AS ENUM (
     'html',
-    'markdown'
+    'markdown',
+    'text'
 );
 
 

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" }

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!