From: Andy Allan Date: Wed, 31 Oct 2018 10:16:47 +0000 (+0100) Subject: Merge branch 'master' into cancancan X-Git-Tag: live~3336^2~4 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/f11221f05bcdd05edd7a9f97d6d57e7baaeb4921?hp=ad85a03e21fdeaa050d5b8ca075ab2ffe36d5a34 Merge branch 'master' into cancancan --- diff --git a/Gemfile b/Gemfile index 9ba270313..b559027c2 100644 --- a/Gemfile +++ b/Gemfile @@ -45,6 +45,7 @@ gem "image_optim_rails" # Load rails plugins gem "actionpack-page_caching" +gem "cancancan" gem "composite_primary_keys", "~> 11.0.0" gem "dynamic_form" gem "http_accept_language", "~> 2.0.0" diff --git a/Gemfile.lock b/Gemfile.lock index 76a31e169..cd94df5e1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -66,6 +66,7 @@ GEM bootsnap (1.3.2) msgpack (~> 1.0) builder (3.2.3) + cancancan (2.1.3) canonical-rails (0.2.4) rails (>= 4.1, < 5.3) capybara (2.18.0) @@ -387,6 +388,7 @@ DEPENDENCIES bigdecimal (~> 1.1.0) binding_of_caller bootsnap (>= 1.1.0) + cancancan canonical-rails capybara (~> 2.13) coffee-rails (~> 4.2) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 17658231f..1df6dd7d1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,6 +3,8 @@ class ApplicationController < ActionController::Base protect_from_forgery :with => :exception + rescue_from CanCan::AccessDenied, :with => :deny_access + before_action :fetch_body around_action :better_errors_allow_inline, :if => proc { Rails.env.development? } @@ -466,6 +468,29 @@ class ApplicationController < ActionController::Base raise end + def current_ability + # Add in 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)) + else + Ability.new(current_user) + end + end + + def deny_access(_exception) + if current_token + set_locale + report_error t("oauth.permissions.missing"), :forbidden + elsif current_user + set_locale + report_error t("application.permission_denied"), :forbidden + elsif request.get? + redirect_to :controller => "users", :action => "login", :referer => request.fullpath + else + head :forbidden + end + end + private # extract authorisation credentials from headers, returns user = nil if none diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index 54fe4dd1d..cff57920b 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -3,11 +3,12 @@ class DiaryEntryController < ApplicationController before_action :authorize_web before_action :set_locale - before_action :require_user, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe] + + authorize_resource + before_action :lookup_user, :only => [:show, :comments] before_action :check_database_readable before_action :check_database_writable, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe] - before_action :require_administrator, :only => [:hide, :hidecomment] before_action :allow_thirdparty_images, :only => [:new, :edit, :index, :show, :comments] def new @@ -215,6 +216,22 @@ class DiaryEntryController < ApplicationController private + # This is required because, being a default-deny system, cancancan + # _cannot_ tell you the reason you were denied access; and so + # the "nice" feedback presenting next steps can't be gleaned from + # the exception + ## + # for the hide actions, require that the user is a administrator, or fill out + # a helpful error message and return them to the user page. + def deny_access(exception) + if current_user && exception.action.in?([:hide, :hidecomment]) + flash[:error] = t("users.filter.not_an_administrator") + redirect_to :action => "show" + else + super + end + end + ## # return permitted diary entry parameters def entry_params @@ -229,16 +246,6 @@ class DiaryEntryController < ApplicationController params.require(:diary_comment).permit(:body) 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 - unless current_user.administrator? - flash[:error] = t("users.filter.not_an_administrator") - redirect_to :action => "show" - end - end - ## # decide on a location for the diary entry map def set_map_location diff --git a/app/controllers/issue_comments_controller.rb b/app/controllers/issue_comments_controller.rb index 8d1acec75..0e4a7079e 100644 --- a/app/controllers/issue_comments_controller.rb +++ b/app/controllers/issue_comments_controller.rb @@ -3,8 +3,8 @@ class IssueCommentsController < ApplicationController before_action :authorize_web before_action :set_locale - before_action :require_user - before_action :check_permission + + authorize_resource def create @issue = Issue.find(params[:issue_id]) @@ -22,10 +22,12 @@ class IssueCommentsController < ApplicationController params.require(:issue_comment).permit(:body) end - def check_permission - unless current_user.administrator? || current_user.moderator? + def deny_access(_exception) + if current_user flash[:error] = t("application.require_moderator_or_admin.not_a_moderator_or_admin") redirect_to root_path + else + super end end diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index ad38454f0..8943f2d4a 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -3,8 +3,9 @@ class IssuesController < ApplicationController before_action :authorize_web before_action :set_locale - before_action :require_user - before_action :check_permission + + authorize_resource + before_action :find_issue, :only => [:show, :resolve, :reopen, :ignore] def index @@ -82,10 +83,12 @@ class IssuesController < ApplicationController @issue = Issue.find(params[:id]) end - def check_permission - unless current_user.administrator? || current_user.moderator? + def deny_access(_exception) + if current_user flash[:error] = t("application.require_moderator_or_admin.not_a_moderator_or_admin") redirect_to root_path + else + super end end end diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index ef87a8699..808726819 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -3,7 +3,8 @@ class ReportsController < ApplicationController before_action :authorize_web before_action :set_locale - before_action :require_user + + authorize_resource def new if required_new_report_params_present? diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb index efb77e2f5..4b960e4e2 100644 --- a/app/controllers/site_controller.rb +++ b/app/controllers/site_controller.rb @@ -6,10 +6,11 @@ class SiteController < ApplicationController before_action :set_locale before_action :redirect_browse_params, :only => :index before_action :redirect_map_params, :only => [:index, :edit, :export] - before_action :require_user, :only => [:welcome] before_action :require_oauth, :only => [:index] before_action :update_totp, :only => [:index] + authorize_resource :class => false + def index session[:location] ||= OSM.ip_location(request.env["REMOTE_ADDR"]) unless STATUS == :database_readonly || STATUS == :database_offline end diff --git a/app/controllers/user_preferences_controller.rb b/app/controllers/user_preferences_controller.rb index 0aa2e8d52..915c847de 100644 --- a/app/controllers/user_preferences_controller.rb +++ b/app/controllers/user_preferences_controller.rb @@ -2,8 +2,9 @@ class UserPreferencesController < ApplicationController skip_before_action :verify_authenticity_token before_action :authorize - before_action :require_allow_read_prefs, :only => [:read_one, :read] - before_action :require_allow_write_prefs, :except => [:read_one, :read] + + authorize_resource + around_action :api_call_handle_error ## diff --git a/app/models/ability.rb b/app/models/ability.rb new file mode 100644 index 000000000..f55f19e4e --- /dev/null +++ b/app/models/ability.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class Ability + include CanCan::Ability + + def initialize(user) + can [:index, :permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id], :site + can [:index, :rss, :show, :comments], DiaryEntry + can [:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim, + :search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse], :geocoder + + if user + can :welcome, :site + can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry + can [:new, :create], Report + can [:read, :read_one, :update, :update_one, :delete_one], UserPreference + + if user.moderator? + can [:index, :show, :resolve, :ignore, :reopen], Issue + can :create, IssueComment + end + + if user.administrator? + can [:hide, :hidecomment], [DiaryEntry, DiaryComment] + can [:index, :show, :resolve, :ignore, :reopen], Issue + can :create, IssueComment + end + end + + # Define abilities for the passed in user here. For example: + # + # user ||= User.new # guest user (not logged in) + # if user.admin? + # can :manage, :all + # else + # can :read, :all + # end + # + # The first argument to `can` is the action you are giving the user + # permission to do. + # If you pass :manage it will apply to every action. Other common actions + # here are :read, :create, :update and :destroy. + # + # The second argument is the resource the user can perform the action on. + # If you pass :all it will apply to every resource. Otherwise pass a Ruby + # class of the resource. + # + # The third argument is an optional hash of conditions to further filter the + # objects. + # For example, here the user can only update published articles. + # + # can :update, Article, :published => true + # + # See the wiki for details: + # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities + end +end diff --git a/app/models/capability.rb b/app/models/capability.rb new file mode 100644 index 000000000..2a5c92774 --- /dev/null +++ b/app/models/capability.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class Capability + include CanCan::Ability + + def initialize(token) + can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) + can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) + end + + private + + def capability?(token, cap) + token&.read_attribute(cap) + end +end diff --git a/app/views/layouts/_header.html.erb b/app/views/layouts/_header.html.erb index e17c6a77b..946f95feb 100644 --- a/app/views/layouts/_header.html.erb +++ b/app/views/layouts/_header.html.erb @@ -54,7 +54,7 @@