From: Andy Allan Date: Wed, 14 Nov 2018 14:45:30 +0000 (+0100) Subject: Use CanCanCan for changeset comments X-Git-Tag: live~3363^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/8f70fb21146b1e8b90125e02d54e70bceea1d5fd Use CanCanCan for changeset comments This introduces different deny_access handlers for web and api requests, since we want to avoid sending redirects as API responses. See #2064 for discussion. --- diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index d7a100057..2db2746b6 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -4,6 +4,7 @@ class Ability include CanCan::Ability def initialize(user) + can :index, ChangesetComment 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, @@ -13,11 +14,13 @@ class Ability if user can :welcome, :site + can :create, ChangesetComment 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 [:destroy, :restore], ChangesetComment can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment can [:new, :create, :edit, :update, :destroy], Redaction diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb index 2a5c92774..89d88a760 100644 --- a/app/abilities/capability.rb +++ b/app/abilities/capability.rb @@ -4,8 +4,13 @@ class Capability include CanCan::Ability def initialize(token) + can :create, ChangesetComment if capability?(token, :allow_write_api) can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) + + if token&.user&.moderator? + can [:destroy, :restore], ChangesetComment if capability?(token, :allow_write_api) + end end private diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7f9ab6ead..b9cf449ea 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -477,7 +477,15 @@ class ApplicationController < ActionController::Base end end - def deny_access(_exception) + def deny_access(exception) + if @api_deny_access_handling + api_deny_access(exception) + else + web_deny_access(exception) + end + end + + def web_deny_access(_exception) if current_token set_locale report_error t("oauth.permissions.missing"), :forbidden @@ -497,6 +505,26 @@ class ApplicationController < ActionController::Base end end + def api_deny_access(_exception) + if current_token + set_locale + report_error t("oauth.permissions.missing"), :forbidden + elsif current_user + head :forbidden + else + realm = "Web Password" + errormessage = "Couldn't authenticate you" + response.headers["WWW-Authenticate"] = "Basic realm=\"#{realm}\"" + render :plain => errormessage, :status => :unauthorized + end + end + + attr_accessor :api_access_handling + + def api_deny_access_handler + @api_deny_access_handling = true + end + private # extract authorisation credentials from headers, returns user = nil if none diff --git a/app/controllers/changeset_comments_controller.rb b/app/controllers/changeset_comments_controller.rb index 8442a4f36..a3023af3e 100644 --- a/app/controllers/changeset_comments_controller.rb +++ b/app/controllers/changeset_comments_controller.rb @@ -3,8 +3,10 @@ class ChangesetCommentsController < ApplicationController before_action :authorize_web, :only => [:index] before_action :set_locale, :only => [:index] before_action :authorize, :only => [:create, :destroy, :restore] - before_action :require_moderator, :only => [:destroy, :restore] - before_action :require_allow_write_api, :only => [:create, :destroy, :restore] + before_action :api_deny_access_handler, :only => [:create, :destroy, :restore] + + authorize_resource + before_action :require_public_data, :only => [:create] before_action :check_api_writable, :only => [:create, :destroy, :restore] before_action :check_api_readable, :except => [:create, :index] diff --git a/test/abilities/capability_test.rb b/test/abilities/capability_test.rb index a25c67043..8e77537a4 100644 --- a/test/abilities/capability_test.rb +++ b/test/abilities/capability_test.rb @@ -12,6 +12,48 @@ class CapabilityTest < ActiveSupport::TestCase end end +class ChangesetCommentCapabilityTest < CapabilityTest + test "as a normal user with permissionless token" do + token = create(:access_token) + capability = Capability.new token + + [:create, :destroy, :restore].each do |action| + assert capability.cannot? action, ChangesetComment + end + end + + test "as a normal user with allow_write_api token" do + token = create(:access_token, :allow_write_api => true) + capability = Capability.new token + + [:destroy, :restore].each do |action| + assert capability.cannot? action, ChangesetComment + end + + [:create].each do |action| + assert capability.can? action, ChangesetComment + end + end + + test "as a moderator with permissionless token" do + token = create(:access_token, :user => create(:moderator_user)) + capability = Capability.new token + + [:create, :destroy, :restore].each do |action| + assert capability.cannot? action, ChangesetComment + end + end + + test "as a moderator with allow_write_api token" do + token = create(:access_token, :user => create(:moderator_user), :allow_write_api => true) + capability = Capability.new token + + [:create, :destroy, :restore].each do |action| + assert capability.can? action, ChangesetComment + end + end +end + class UserCapabilityTest < CapabilityTest test "user preferences" do # a user with no tokens diff --git a/test/factories/access_tokens.rb b/test/factories/access_tokens.rb new file mode 100644 index 000000000..98eb91066 --- /dev/null +++ b/test/factories/access_tokens.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :access_token do + user + client_application + end +end