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.

diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb
index 54dc90dee911e7f62d9a36d48f0250380c4f0673..cb0425f3237b04d582e10fbc2e302aed51e01ae9 100644 (file)
--- 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]
   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({
 
   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
 
     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.
   ##
   # 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 d9a65b3429ea5cc022df69413663b917bdc8923b..f661c9b85b5b378a71cb631b962224cfd8062b9e 100644 (file)
--- 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 %>
 <% 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 %>
     <% 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}")) %>
     <% 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 (file)
index 13f8118..0000000
--- 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') %>
-<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 %>

diff --git a/app/views/user_roles/revoke.html.erb b/app/views/user_roles/revoke.html.erb
deleted file mode 100644 (file)
index 240a91f..0000000
--- 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') %>
-<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 %>

diff --git a/config/routes.rb b/config/routes.rb
index fec224bb90dcbe48f89bb9c6ca057435de151a08..9550c84ffebdcc0899b20ae479017f0e37417dbb 100644 (file)
--- 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 '/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"
   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 57a9b7211a4eafca66444e04e94f049f33c1dfdf..eed9b053b7e2c38906e7c9da8695a8ab32d4f216 100644 (file)
--- 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',
 
 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 1e2d29b450eca82019e2ff225ac0f3357f8339fb..e89230a7f58e86b5483df28b9074f2b9d58bbd6b 100644 (file)
--- 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
   ##
   # 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/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" }
     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 17531ef3fb8b6354f5d9c683760c9be6fe76c49a..948bb895cd10a7c3110ea6f81d85ca5fe3f8713a 100644 (file)
--- 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
 
     check_fail(:revoke, :administrator_user, :moderator)
   end
 

+private
+

   def check_fail(action, user, role)
     get '/login'
     assert_response :redirect
   def check_fail(action, user, role)
     get '/login'
     assert_response :redirect


@@ -27,8 +29,7 @@ class UserRolesTest < ActionController::IntegrationTest
     follow_redirect!
     assert_response :success
 
     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!
     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
 
     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!
     assert_redirected_to :controller => 'user', :action => 'view', :display_name => users(:second_public_user).display_name
 
     reset!