From ca596106f5e2a42940db5bbdf45a677b328794d6 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 12 Dec 2018 16:01:54 +0100 Subject: [PATCH] Refactor users_controller to use CanCanCan for authorisation --- app/abilities/ability.rb | 3 +++ app/abilities/capability.rb | 2 ++ app/controllers/users_controller.rb | 25 ++++------------------- config/locales/en.yml | 2 -- test/controllers/users_controller_test.rb | 24 +++++++++++----------- 5 files changed, 21 insertions(+), 35 deletions(-) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 707272182..d0e0b65d7 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -11,6 +11,7 @@ class Ability :search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse], :geocoder can [:index, :create, :comment, :feed, :show, :search, :mine], Note can [:index, :show], Redaction + can [:terms, :api_users, :login, :logout, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :api_read, :auth_success, :auth_failure], User can [:index, :show, :blocks_on, :blocks_by], UserBlock if user @@ -19,6 +20,7 @@ class Ability can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry can [:close, :reopen], Note can [:new, :create], Report + can [:account, :go_public, :make_friend, :remove_friend, :api_details, :api_gpx_files], User can [:read, :read_one, :update, :update_one, :delete_one], UserPreference if user.moderator? @@ -34,6 +36,7 @@ class Ability can [:hide, :hidecomment], [DiaryEntry, DiaryComment] can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment + can [:set_status, :delete, :index], User can [:grant, :revoke], UserRole end end diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb index 8ede9bb51..6aa1c418c 100644 --- a/app/abilities/capability.rb +++ b/app/abilities/capability.rb @@ -6,6 +6,8 @@ class Capability def initialize(token) can :create, ChangesetComment if capability?(token, :allow_write_api) can [:create, :comment, :close, :reopen], Note if capability?(token, :allow_write_notes) + can [:api_details], User if capability?(token, :allow_read_prefs) + can [:api_gpx_files], User if capability?(token, :allow_read_gpx) can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 016d7c87d..01eee61df 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -6,15 +6,15 @@ class UsersController < ApplicationController before_action :authorize, :only => [:api_details, :api_gpx_files] before_action :authorize_web, :except => [:api_read, :api_users, :api_details, :api_gpx_files] before_action :set_locale, :except => [:api_read, :api_users, :api_details, :api_gpx_files] - before_action :require_user, :only => [:account, :go_public, :make_friend, :remove_friend] + before_action :api_deny_access_handler, :only => [:api_read, :api_users, :api_details, :api_gpx_files] + + authorize_resource + before_action :require_self, :only => [:account] before_action :check_database_readable, :except => [:login, :api_read, :api_users, :api_details, :api_gpx_files] before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public, :make_friend, :remove_friend] before_action :check_api_readable, :only => [:api_read, :api_users, :api_details, :api_gpx_files] - before_action :require_allow_read_prefs, :only => [:api_details] - before_action :require_allow_read_gpx, :only => [:api_gpx_files] before_action :require_cookies, :only => [:new, :login, :confirm] - before_action :require_administrator, :only => [:set_status, :delete, :index] around_action :api_call_handle_error, :only => [:api_read, :api_users, :api_details, :api_gpx_files] before_action :lookup_user_by_id, :only => [:api_read] before_action :lookup_user_by_name, :only => [:set_status, :delete] @@ -749,23 +749,6 @@ class UsersController < ApplicationController end end - ## - # require that the user is a administrator, or fill out a helpful error message - # and return them to the user page. - def require_administrator - if current_user && !current_user.administrator? - flash[:error] = t("users.filter.not_an_administrator") - - if params[:display_name] - redirect_to user_path(:display_name => params[:display_name]) - else - redirect_to :action => "login", :referer => request.fullpath - end - elsif !current_user - redirect_to :action => "login", :referer => request.fullpath - end - end - ## # require that the user in the URL is the logged in user def require_self diff --git a/config/locales/en.yml b/config/locales/en.yml index 92d56919c..e87e8f8ee 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2175,8 +2175,6 @@ en: button: "Unfriend" success: "%{name} was removed from your friends." not_a_friend: "%{name} is not one of your friends." - filter: - not_an_administrator: "You need to be an administrator to perform that action." index: title: Users heading: Users diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index deb736a7b..a1fda5591 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -1469,7 +1469,7 @@ class UsersControllerTest < ActionController::TestCase # Now try as a normal user get :set_status, :params => { :display_name => user.display_name, :status => "suspended" }, :session => { :user => user } assert_response :redirect - assert_redirected_to :action => :show, :display_name => user.display_name + assert_redirected_to :controller => :errors, :action => :forbidden # Finally try as an administrator get :set_status, :params => { :display_name => user.display_name, :status => "suspended" }, :session => { :user => create(:administrator_user) } @@ -1489,7 +1489,7 @@ class UsersControllerTest < ActionController::TestCase # Now try as a normal user get :delete, :params => { :display_name => user.display_name, :status => "suspended" }, :session => { :user => user } assert_response :redirect - assert_redirected_to :action => :show, :display_name => user.display_name + assert_redirected_to :controller => :errors, :action => :forbidden # Finally try as an administrator get :delete, :params => { :display_name => user.display_name, :status => "suspended" }, :session => { :user => create(:administrator_user) } @@ -1531,14 +1531,14 @@ class UsersControllerTest < ActionController::TestCase # Shouldn't work when logged in as a normal user get :index assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden session[:user] = moderator_user.id # Shouldn't work when logged in as a moderator get :index assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden session[:user] = administrator_user.id @@ -1598,8 +1598,8 @@ class UsersControllerTest < ActionController::TestCase assert_no_difference "User.active.count" do post :index, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } end - assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_response :forbidden + assert_equal "pending", inactive_user.reload.status assert_equal "suspended", suspended_user.reload.status @@ -1610,7 +1610,7 @@ class UsersControllerTest < ActionController::TestCase post :index, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } end assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden assert_equal "pending", inactive_user.reload.status assert_equal "suspended", suspended_user.reload.status @@ -1621,7 +1621,7 @@ class UsersControllerTest < ActionController::TestCase post :index, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } end assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden assert_equal "pending", inactive_user.reload.status assert_equal "suspended", suspended_user.reload.status @@ -1645,8 +1645,8 @@ class UsersControllerTest < ActionController::TestCase assert_no_difference "User.active.count" do post :index, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } end - assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_response :forbidden + assert_equal "active", normal_user.reload.status assert_equal "confirmed", confirmed_user.reload.status @@ -1657,7 +1657,7 @@ class UsersControllerTest < ActionController::TestCase post :index, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } end assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden assert_equal "active", normal_user.reload.status assert_equal "confirmed", confirmed_user.reload.status @@ -1668,7 +1668,7 @@ class UsersControllerTest < ActionController::TestCase post :index, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } end assert_response :redirect - assert_redirected_to :action => :login, :referer => users_path + assert_redirected_to :controller => :errors, :action => :forbidden assert_equal "active", normal_user.reload.status assert_equal "confirmed", confirmed_user.reload.status -- 2.43.2