From: Matt Amos Date: Thu, 1 Oct 2009 17:15:34 +0000 (+0000) Subject: Refactored user_role and controller. Now much more DRY. X-Git-Tag: live~7155^2~29 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/1e3b3c1f10213c48a40083e8bbca1429d9f819d6?hp=77851bac7bf76473de33b7bbab00bc1b3d834cb1 Refactored user_role and controller. Now much more DRY. --- diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb index 9064b811d..51106866e 100644 --- a/app/controllers/user_roles_controller.rb +++ b/app/controllers/user_roles_controller.rb @@ -3,43 +3,81 @@ class UserRolesController < ApplicationController before_filter :authorize_web before_filter :require_user + before_filter :lookup_this_user before_filter :require_administrator + before_filter :require_valid_role + before_filter :not_in_role, :only => [:grant] + before_filter :in_role, :only => [:revoke] + around_filter :setup_nonce def grant - # 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], :granter_id => @user.id) - 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 - @nonce = OAuth::Helper.generate_nonce - session[:nonce] = @nonce - end + @this_user.roles.create(:role => @role, :granter_id => @user.id) + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name end def revoke - # added a random nonce here which isn't predictable, making an CSRF procedure much, much more difficult. + UserRole.delete_all({:user_id => @this_user.id, :role => @role}) + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name + end + + private + def require_administrator + unless @user.administrator? + flash[:notice] = t'user_role.filter.not_an_administrator' + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name + end + end + + ## + # ensure that there is a "this_user" instance variable + def lookup_this_user + @this_user = User.find_by_display_name(params[:display_name]) + rescue ActiveRecord::RecordNotFound + 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] - 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 + @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. + def require_valid_role + @role = params[:role] + unless UserRole::ALL_ROLES.include?(@role) + flash[:notice] = t('user_role.filter.not_a_role', :role => @role) + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name end end - private - def require_administrator - redirect_to "/403.html" unless @user.administrator? + ## + # checks that the user doesn't already have this role + def not_in_role + if @this_user.has_role? @role + flash[:notice] = t('user_role.filter.already_has_role', :role => @role) + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name + end end + ## + # checks that the user doesn't already have this role + def in_role + unless @this_user.has_role? @role + flash[:notice] = t('user_role.filter.doesnt_have_role', :role => @role) + redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name + end + end end diff --git a/app/models/user_role.rb b/app/models/user_role.rb index efa540fec..fb783281e 100644 --- a/app/models/user_role.rb +++ b/app/models/user_role.rb @@ -1,8 +1,8 @@ class UserRole < ActiveRecord::Base + belongs_to :user ALL_ROLES = ['administrator', 'moderator'] validates_inclusion_of :role, :in => ALL_ROLES - belongs_to :user - + validates_uniqueness_of :role, :scope => :user_id end diff --git a/config/locales/en.yml b/config/locales/en.yml index 241e0327f..95b53821f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1003,6 +1003,11 @@ en: success: "{{name}} was removed from your friends." not_a_friend: "{{name}} is not one of your friends." user_role: + filter: + not_an_administrator: "Only administrators can perform user role management, and you are not an administrator." + not_a_role: "The string `{{role}}' isn't a valid role." + already_has_role: "The user already has role {{role}}." + doesnt_have_role: "The user does not have role {{role}}." grant: are_you_sure: "Are you sure you want to grant the role `{{role}}' to the user `{{name}}'?" confirm: "Confirm" diff --git a/test/functional/user_roles_controller_test.rb b/test/functional/user_roles_controller_test.rb deleted file mode 100644 index 3bced12e4..000000000 --- a/test/functional/user_roles_controller_test.rb +++ /dev/null @@ -1,38 +0,0 @@ -require File.dirname(__FILE__) + '/../test_helper' - -class UserRolesControllerTest < ActionController::TestCase - fixtures :users, :user_roles - - test "grant" do - check_forbidden(:grant, :public_user) - check_forbidden(:grant, :moderator_user) - check_success(:grant, :administrator_user) - end - - test "revoke" do - check_forbidden(:revoke, :public_user) - check_forbidden(:revoke, :moderator_user) - check_success(:revoke, :administrator_user) - end - - 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 "/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 diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb index 822c923cc..f0b9070d6 100644 --- a/test/integration/user_blocks_test.rb +++ b/test/integration/user_blocks_test.rb @@ -1,7 +1,7 @@ require File.dirname(__FILE__) + '/../test_helper' class UserBlocksTest < ActionController::IntegrationTest - fixtures :users, :user_blocks + fixtures :users, :user_blocks, :user_roles def auth_header(user, pass) {"HTTP_AUTHORIZATION" => "Basic %s" % Base64.encode64("#{user}:#{pass}")} diff --git a/test/integration/user_roles_test.rb b/test/integration/user_roles_test.rb new file mode 100644 index 000000000..df8b1561f --- /dev/null +++ b/test/integration/user_roles_test.rb @@ -0,0 +1,46 @@ +require File.dirname(__FILE__) + '/../test_helper' + +class UserRolesControllerTest < ActionController::IntegrationTest + fixtures :users, :user_roles + + test "grant" do + check_fail(:grant, :public_user, :moderator) + check_fail(:grant, :moderator_user, :moderator) + check_success(:grant, :administrator_user, :moderator) + end + + test "revoke" do + check_fail(:revoke, :public_user, :moderator) + check_fail(:revoke, :moderator_user, :moderator) + # this other user doesn't have moderator role, so this fails + check_fail(:revoke, :administrator_user, :moderator) + end + + def check_fail(action, user, role) + post '/login', {'user[email]' => users(user).email, 'user[password]' => "test", :referer => "/"} + assert_response :redirect + follow_redirect! + assert_response :success + + get "/user/#{users(:second_public_user).display_name}/role/#{role}/#{action}" + assert_response :redirect + assert_redirected_to :controller => 'user', :action => 'view', :display_name => users(:second_public_user).display_name + + reset! + end + + def check_success(action, user, role) + post '/login', {'user[email]' => users(user).email, 'user[password]' => "test", :referer => "/"} + assert_response :redirect + 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 + assert_redirected_to :controller => 'user', :action => 'view', :display_name => users(:second_public_user).display_name + + reset! + end +end