From 30d5e783e478789d2454e85c6d5b621b07a11081 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Mon, 28 Sep 2009 17:35:39 +0000 Subject: [PATCH] Added a confirmation step to the process of granting and revoking user roles. --- app/controllers/user_roles_controller.rb | 32 +++++++++++++------ app/views/user_roles/edit.html.erb | 20 ------------ app/views/user_roles/grant.html.erb | 5 +++ app/views/user_roles/index.html.erb | 22 ------------- app/views/user_roles/new.html.erb | 19 ----------- app/views/user_roles/revoke.html.erb | 5 +++ app/views/user_roles/show.html.erb | 13 -------- config/locales/en.yml | 4 +++ test/functional/user_roles_controller_test.rb | 26 ++++++++++----- 9 files changed, 54 insertions(+), 92 deletions(-) delete mode 100644 app/views/user_roles/edit.html.erb create mode 100644 app/views/user_roles/grant.html.erb delete mode 100644 app/views/user_roles/index.html.erb delete mode 100644 app/views/user_roles/new.html.erb create mode 100644 app/views/user_roles/revoke.html.erb delete mode 100644 app/views/user_roles/show.html.erb 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 @@ -

Editing user_role

- -<% form_for(@user_role) do |f| %> - <%= f.error_messages %> - -

- <%= f.label :user_id %>
- <%= f.text_field :user_id %> -

-

- <%= f.label :role %>
- <%= f.text_field :role %> -

-

- <%= f.submit 'Update' %> -

-<% 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 %> +

<%= 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/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 @@ -

Listing user_roles

- - - - - - - -<% @user_roles.each do |user_role| %> - - - - - - - -<% end %> -
UserRole
<%=h user_role.user_id %><%=h user_role.role %><%= link_to 'Show', user_role %><%= link_to 'Edit', edit_user_role_path(user_role) %><%= link_to 'Destroy', user_role, :confirm => 'Are you sure?', :method => :delete %>
- -
- -<%= 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 @@ -

New user_role

- -<% form_for(@user_role) do |f| %> - <%= f.error_messages %> - -

- <%= f.label :user_id %>
- <%= f.text_field :user_id %> -

-

- <%= f.label :role %>
- <%= f.text_field :role %> -

-

- <%= f.submit 'Create' %> -

-<% 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 %> +

<%= 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/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 @@ -

- User: - <%=h @user_role.user_id %> -

- -

- Role: - <%=h @user_role.role %> -

- - -<%= 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 -- 2.39.5