From 981e4a34b5d5ea1c1e3da1518697e2cf5e6ab0b3 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 12 Dec 2018 13:58:38 +0100 Subject: [PATCH 1/2] Use only token capabilities when a token is provided The Authenticate#allow? method (from oauth-plugin) sets current_user as a side effect of checking the token. But this allows a valid token to access all actions that are available to that user, beyond the capabilities for that token. --- app/controllers/application_controller.rb | 4 +-- .../user_preferences_controller_test.rb | 31 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6c6a087b7d..0411f75c42 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -446,9 +446,9 @@ def better_errors_allow_inline end def current_ability - # Add in capabilities from the oauth token if it exists and is a valid access token + # Use capabilities from the oauth token if it exists and is a valid access token if Authenticator.new(self, [:token]).allow? - Ability.new(current_user).merge(Capability.new(current_token)) + Capability.new(current_token) else Ability.new(current_user) end diff --git a/test/controllers/user_preferences_controller_test.rb b/test/controllers/user_preferences_controller_test.rb index ddd7e2a401..99b40d5f07 100644 --- a/test/controllers/user_preferences_controller_test.rb +++ b/test/controllers/user_preferences_controller_test.rb @@ -210,4 +210,35 @@ def test_delete_one UserPreference.find([user.id, "key"]) end end + + # Ensure that a valid access token with correct capabilities can be used to + # read preferences + def test_read_one_using_token + user = create(:user) + token = create(:access_token, :user => user, :allow_read_prefs => true) + create(:user_preference, :user => user, :k => "key", :v => "value") + + # Hack together an oauth request - an alternative would be to sign the request properly + @request.env["oauth.version"] = 1 + @request.env["oauth.strategies"] = [:token] + @request.env["oauth.token"] = token + + get :read_one, :params => { :preference_key => "key" } + assert_response :success + end + + # Ensure that a valid access token with incorrect capabilities can't be used + # to read preferences even, though the owner of that token could read them + # by other methods. + def test_read_one_using_token_fail + user = create(:user) + token = create(:access_token, :user => user, :allow_read_prefs => false) + create(:user_preference, :user => user, :k => "key", :v => "value") + @request.env["oauth.version"] = 1 + @request.env["oauth.strategies"] = [:token] + @request.env["oauth.token"] = token + + get :read_one, :params => { :preference_key => "key" } + assert_response :forbidden + end end From ca596106f5e2a42940db5bbdf45a677b328794d6 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 12 Dec 2018 16:01:54 +0100 Subject: [PATCH 2/2] 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 707272182e..d0e0b65d7a 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -11,6 +11,7 @@ def initialize(user) :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 @@ def initialize(user) 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 @@ def initialize(user) 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 8ede9bb51b..6aa1c418ca 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 016d7c87dd..01eee61df4 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 @@ def update_user(user, params) 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 92d56919cd..e87e8f8ee2 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 deb736a7b2..a1fda55913 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -1469,7 +1469,7 @@ def test_set_status # 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 @@ def test_delete # 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 @@ def test_index_get # 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 @@ def test_index_post_confirm 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 @@ def test_index_post_confirm 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 @@ def test_index_post_confirm 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 @@ def test_index_post_hide 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 @@ def test_index_post_hide 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 @@ def test_index_post_hide 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