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 d7a10005764ba78011b2e68df59089575fc180fe..2db2746b6213d4a85114c432f345146372a66968 100644 (file)
--- 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 2a5c927748bbdb969b6d37e376a099374085bed4..89d88a7605353578d80ad3eedc7bca1e71460053 100644 (file)
--- 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 7f9ab6ead6ae889209c4b41fd8df271799ce1c0b..b9cf449ea2f3ffce06bd9d219b33f8f31b6b11e5 100644 (file)
--- 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 8442a4f360f34882030929f0f037d319abdd859b..a3023af3eb3fd6e057781f4028e1d9c4bf28c6eb 100644 (file)
--- 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 a25c670434677ffe098da203fd9b7a534fa8ba15..8e77537a45c2f4eca02d22895d7cfa6b906f6cbc 100644 (file)
--- 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 (file)
index 0000000..98eb910
--- /dev/null
+++ b/test/factories/access_tokens.rb
@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :access_token do
+    user
+    client_application
+  end
+end