]> git.openstreetmap.org Git - rails.git/commitdiff
Get rid of custom CSRF protection for user role changes
authorTom Hughes <tom@compton.nu>
Tue, 20 Mar 2012 16:22:07 +0000 (16:22 +0000)
committerTom Hughes <tom@compton.nu>
Tue, 20 Mar 2012 17:21:13 +0000 (17:21 +0000)
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
app/views/user/view.html.erb
app/views/user_roles/grant.html.erb [deleted file]
app/views/user_roles/revoke.html.erb [deleted file]
config/routes.rb
db/structure.sql
test/functional/user_roles_controller_test.rb
test/integration/user_roles_test.rb

index 54dc90dee911e7f62d9a36d48f0250380c4f0673..cb0425f3237b04d582e10fbc2e302aed51e01ae9 100644 (file)
@@ -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.
index d9a65b3429ea5cc022df69413663b917bdc8923b..f661c9b85b5b378a71cb631b962224cfd8062b9e 100644 (file)
@@ -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
+++ /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
+++ /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 %>
index fec224bb90dcbe48f89bb9c6ca057435de151a08..9550c84ffebdcc0899b20ae479017f0e37417dbb 100644 (file)
@@ -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"
index 57a9b7211a4eafca66444e04e94f049f33c1dfdf..eed9b053b7e2c38906e7c9da8695a8ab32d4f216 100644 (file)
@@ -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'
 );
 
 
 );
 
 
index 1e2d29b450eca82019e2ff225ac0f3357f8339fb..e89230a7f58e86b5483df28b9074f2b9d58bbd6b 100644 (file)
@@ -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" }
index 17531ef3fb8b6354f5d9c683760c9be6fe76c49a..948bb895cd10a7c3110ea6f81d85ca5fe3f8713a 100644 (file)
@@ -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!