From: Matt Amos <zerebubuth@gmail.com>
Date: Mon, 28 Sep 2009 17:35:39 +0000 (+0000)
Subject: Added a confirmation step to the process of granting and revoking user roles.
X-Git-Tag: live~7116^2~34
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/30d5e783e478789d2454e85c6d5b621b07a11081?hp=ab1444f338a4e1a94f30c50dbdff4c51a604f135

Added a confirmation step to the process of granting and revoking user roles.
---

diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb
index b1f24c275..7e56693df 100644
--- a/app/controllers/user_roles_controller.rb
+++ b/app/controllers/user_roles_controller.rb
@@ -6,23 +6,35 @@ class UserRolesController < ApplicationController
   before_filter :require_administrator
 
   def grant
-    this_user = User.find_by_display_name(params[:display_name], :conditions => {:visible => true})
-    if this_user and UserRole::ALL_ROLES.include? params[:role]
-      this_user.roles.create(:role => params[:role])
+    # added a random nonce here which isn't predictable, making an CSRF procedure much, much more difficult.
+    if params[:nonce] and params[:nonce] == session[:nonce]
+      this_user = User.find_by_display_name(params[:display_name], :conditions => {:visible => true})
+      if this_user and UserRole::ALL_ROLES.include? params[:role]
+        this_user.roles.create(:role => params[:role])
+        redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name]
+      else
+        flash[:notice] = t('user_role.grant.fail', :role => params[:role], :name => params[:display_name])
+      end
     else
-      flash[:notice] = t('user_role.grant.fail', :role => params[:role], :name => params[:display_name])
+      @nonce = OAuth::Helper.generate_nonce
+      session[:nonce] = @nonce
     end
-    redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name]
   end
 
   def revoke
-    this_user = User.find_by_display_name(params[:display_name], :conditions => {:visible => true})
-    if this_user and UserRole::ALL_ROLES.include? params[:role]
-      UserRole.delete_all({:user_id => this_user.id, :role => params[:role]})
+    # added a random nonce here which isn't predictable, making an CSRF procedure much, much more difficult.
+    if params[:nonce] and params[:nonce] == session[:nonce]
+      this_user = User.find_by_display_name(params[:display_name], :conditions => {:visible => true})
+      if this_user and UserRole::ALL_ROLES.include? params[:role]
+        UserRole.delete_all({:user_id => this_user.id, :role => params[:role]})
+        redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name]
+      else
+        flash[:notice] = t('user_role.revoke.fail', :role => params[:role], :name => params[:display_name])
+      end
     else
-      flash[:notice] = t('user_role.revoke.fail', :role => params[:role], :name => params[:display_name])
+      @nonce = OAuth::Helper.generate_nonce
+      session[:nonce] = @nonce
     end
-    redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name]
   end
 
   private
diff --git a/app/views/user_roles/edit.html.erb b/app/views/user_roles/edit.html.erb
deleted file mode 100644
index 609b42651..000000000
--- a/app/views/user_roles/edit.html.erb
+++ /dev/null
@@ -1,20 +0,0 @@
-<h1>Editing user_role</h1>
-
-<% form_for(@user_role) do |f| %>
-  <%= f.error_messages %>
-
-  <p>
-    <%= f.label :user_id %><br />
-    <%= f.text_field :user_id %>
-  </p>
-  <p>
-    <%= f.label :role %><br />
-    <%= f.text_field :role %>
-  </p>
-  <p>
-    <%= f.submit 'Update' %>
-  </p>
-<% end %>
-
-<%= link_to 'Show', @user_role %> |
-<%= link_to 'Back', user_roles_path %>
\ No newline at end of file
diff --git a/app/views/user_roles/grant.html.erb b/app/views/user_roles/grant.html.erb
new file mode 100644
index 000000000..ca45266d6
--- /dev/null
+++ b/app/views/user_roles/grant.html.erb
@@ -0,0 +1,5 @@
+<% form_tag request.request_uri do %>
+<%= hidden_field_tag 'nonce', @nonce %>
+<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/index.html.erb b/app/views/user_roles/index.html.erb
deleted file mode 100644
index e245d68fc..000000000
--- a/app/views/user_roles/index.html.erb
+++ /dev/null
@@ -1,22 +0,0 @@
-<h1>Listing user_roles</h1>
-
-<table>
-  <tr>
-    <th>User</th>
-    <th>Role</th>
-  </tr>
-
-<% @user_roles.each do |user_role| %>
-  <tr>
-    <td><%=h user_role.user_id %></td>
-    <td><%=h user_role.role %></td>
-    <td><%= link_to 'Show', user_role %></td>
-    <td><%= link_to 'Edit', edit_user_role_path(user_role) %></td>
-    <td><%= link_to 'Destroy', user_role, :confirm => 'Are you sure?', :method => :delete %></td>
-  </tr>
-<% end %>
-</table>
-
-<br />
-
-<%= link_to 'New user_role', new_user_role_path %>
\ No newline at end of file
diff --git a/app/views/user_roles/new.html.erb b/app/views/user_roles/new.html.erb
deleted file mode 100644
index 0791993f9..000000000
--- a/app/views/user_roles/new.html.erb
+++ /dev/null
@@ -1,19 +0,0 @@
-<h1>New user_role</h1>
-
-<% form_for(@user_role) do |f| %>
-  <%= f.error_messages %>
-
-  <p>
-    <%= f.label :user_id %><br />
-    <%= f.text_field :user_id %>
-  </p>
-  <p>
-    <%= f.label :role %><br />
-    <%= f.text_field :role %>
-  </p>
-  <p>
-    <%= f.submit 'Create' %>
-  </p>
-<% end %>
-
-<%= link_to 'Back', user_roles_path %>
\ No newline at end of file
diff --git a/app/views/user_roles/revoke.html.erb b/app/views/user_roles/revoke.html.erb
new file mode 100644
index 000000000..17219d9c7
--- /dev/null
+++ b/app/views/user_roles/revoke.html.erb
@@ -0,0 +1,5 @@
+<% form_tag request.request_uri do %>
+<%= hidden_field_tag 'nonce', @nonce %>
+<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/app/views/user_roles/show.html.erb b/app/views/user_roles/show.html.erb
deleted file mode 100644
index 3db11afa8..000000000
--- a/app/views/user_roles/show.html.erb
+++ /dev/null
@@ -1,13 +0,0 @@
-<p>
-  <b>User:</b>
-  <%=h @user_role.user_id %>
-</p>
-
-<p>
-  <b>Role:</b>
-  <%=h @user_role.role %>
-</p>
-
-
-<%= link_to 'Edit', edit_user_role_path(@user_role) %> |
-<%= link_to 'Back', user_roles_path %>
\ No newline at end of file
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 8220a91e9..5459cd1da 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1001,8 +1001,12 @@ en:
       not_a_friend: "{{name}} is not one of your friends."
   user_role:
     grant:
+      are_you_sure: "Are you sure you want to grant the role `{{role}}' to the user `{{name}}'?"
+      confirm: "Confirm"
       fail: "Couldn't grant role `{{role}}' to user `{{name}}'. Please check that the user and role are both valid."
     revoke:
+      are_you_sure: "Are you sure you want to revoke the role `{{role}}' from the user `{{name}}'?"
+      confirm: "Confirm"
       fail: "Couldn't revoke role `{{role}}' from user `{{name}}'. Please check that the user and role are both valid."
   user_block:
     new:
diff --git a/test/functional/user_roles_controller_test.rb b/test/functional/user_roles_controller_test.rb
index c2de53934..3bced12e4 100644
--- a/test/functional/user_roles_controller_test.rb
+++ b/test/functional/user_roles_controller_test.rb
@@ -4,25 +4,35 @@ class UserRolesControllerTest < ActionController::TestCase
   fixtures :users, :user_roles
 
   test "grant" do
-    check_redirect(:grant, :public_user, "/403.html")
-    check_redirect(:grant, :moderator_user, "/403.html")
-    check_redirect(:grant, :administrator_user, {:controller => :user, :action => :view})
+    check_forbidden(:grant, :public_user)
+    check_forbidden(:grant, :moderator_user)
+    check_success(:grant, :administrator_user)
   end
 
   test "revoke" do
-    check_redirect(:revoke, :public_user, "/403.html")
-    check_redirect(:revoke, :moderator_user, "/403.html")
-    check_redirect(:revoke, :administrator_user, {:controller => :user, :action => :view})
+    check_forbidden(:revoke, :public_user)
+    check_forbidden(:revoke, :moderator_user)
+    check_success(:revoke, :administrator_user)
   end
 
-  def check_redirect(action, user, redirect)
+  def check_forbidden(action, user)
     UserRole::ALL_ROLES.each do |role|
       u = users(user)
       basic_authorization(u.email, "test")
       
       get(action, {:display_name => users(:second_public_user).display_name, :role => role}, {'user' => u.id})
       assert_response :redirect
-      assert_redirected_to redirect
+      assert_redirected_to "/403.html"
+    end
+  end
+
+  def check_success(action, user)
+    UserRole::ALL_ROLES.each do |role|
+      u = users(user)
+      basic_authorization(u.email, "test")
+      
+      get(action, {:display_name => users(:second_public_user).display_name, :role => role}, {'user' => u.id})
+      assert_response :success
     end
   end
 end