From: Tom Hughes <tom@compton.nu>
Date: Wed, 28 Nov 2018 18:09:20 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/2075'
X-Git-Tag: live~2863
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/b99b1926974a92935a14ecf046915e83421e3cd1?hp=ed8e15c8f0f663a199f68b5f3b71c36529b2b51e

Merge remote-tracking branch 'upstream/pull/2075'
---

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index 2db2746b6..f2486d2b4 100644
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -9,6 +9,7 @@ class Ability
     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
+    can [:index, :create, :comment, :feed, :show, :search, :mine], Note
     can [:index, :show], Redaction
     can [:index, :show, :blocks_on, :blocks_by], UserBlock
 
@@ -16,6 +17,7 @@ class Ability
       can :welcome, :site
       can :create, ChangesetComment
       can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry
+      can [:close, :reopen], Note
       can [:new, :create], Report
       can [:read, :read_one, :update, :update_one, :delete_one], UserPreference
 
@@ -23,6 +25,7 @@ class Ability
         can [:destroy, :restore], ChangesetComment
         can [:index, :show, :resolve, :ignore, :reopen], Issue
         can :create, IssueComment
+        can :destroy, Note
         can [:new, :create, :edit, :update, :destroy], Redaction
         can [:new, :edit, :create, :update, :revoke], UserBlock
       end
diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb
index 89d88a760..8ede9bb51 100644
--- a/app/abilities/capability.rb
+++ b/app/abilities/capability.rb
@@ -5,11 +5,13 @@ 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 [: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)
+      can :destroy, Note if capability?(token, :allow_write_notes)
     end
   end
 
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index b9cf449ea..6c6a087b7 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -118,24 +118,6 @@ class ApplicationController < ActionController::Base
     require_capability(:allow_write_gpx)
   end
 
-  def require_allow_write_notes
-    require_capability(:allow_write_notes)
-  end
-
-  ##
-  # require that the user is a moderator, or fill out a helpful error message
-  # and return them to the index for the controller this is wrapped from.
-  def require_moderator
-    unless current_user.moderator?
-      if request.get?
-        flash[:error] = t("application.require_moderator.not_a_moderator")
-        redirect_to :action => "index"
-      else
-        head :forbidden
-      end
-    end
-  end
-
   ##
   # sets up the current_user for use by other methods. this is mostly called
   # from the authorize method, but can be called elsewhere if authorisation
@@ -193,11 +175,6 @@ class ApplicationController < ActionController::Base
   ##
   # to be used as a before_filter *after* authorize. this checks that
   # the user is a moderator and, if not, returns a forbidden error.
-  #
-  # NOTE: this isn't a very good way of doing it - it duplicates logic
-  # from require_moderator - but what we really need to do is a fairly
-  # drastic refactoring based on :format and respond_to? but not a
-  # good idea to do that in this branch.
   def authorize_moderator(errormessage = "Access restricted to moderators")
     # check user is a moderator
     unless current_user.moderator?
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 9cdc38446..62fd9d078 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -6,9 +6,11 @@ class NotesController < ApplicationController
   before_action :authorize_web, :only => [:mine]
   before_action :setup_user_auth, :only => [:create, :comment, :show]
   before_action :authorize, :only => [:close, :reopen, :destroy]
-  before_action :require_moderator, :only => [:destroy]
+  before_action :api_deny_access_handler, :except => [:mine]
+
+  authorize_resource
+
   before_action :check_api_writable, :only => [:create, :comment, :close, :reopen, :destroy]
-  before_action :require_allow_write_notes, :only => [:create, :comment, :close, :reopen, :destroy]
   before_action :set_locale
   around_action :api_call_handle_error, :api_call_timeout
 
diff --git a/config/locales/en.yml b/config/locales/en.yml
index ad825ef67..2777c1df3 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1811,10 +1811,6 @@ en:
       cookies_needed: "You appear to have cookies disabled - please enable cookies in your browser before continuing."
     require_admin:
       not_an_admin: You need to be an admin to perform that action.
-    require_moderator:
-      not_a_moderator: "You need to be a moderator to perform that action."
-    require_moderator_or_admin:
-      not_a_moderator_or_admin: You need to be a moderator or an admin to perform that action
     setup_user_auth:
       blocked_zero_hour: "You have an urgent message on the OpenStreetMap web site. You need to read the message before you will be able to save your edits."
       blocked: "Your access to the API has been blocked. Please log-in to the web interface to find out more."
diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb
index fc37b0e7d..9444a45f5 100644
--- a/test/abilities/abilities_test.rb
+++ b/test/abilities/abilities_test.rb
@@ -26,6 +26,18 @@ class GuestAbilityTest < AbilityTest
       assert ability.cannot?(action, DiaryComment), "should not be able to #{action} DiaryEntries"
     end
   end
+
+  test "note permissions for a guest" do
+    ability = Ability.new nil
+
+    [:index, :create, :comment, :feed, :show, :search, :mine].each do |action|
+      assert ability.can?(action, Note), "should be able to #{action} Notes"
+    end
+
+    [:close, :reopen, :destroy].each do |action|
+      assert ability.cannot?(action, Note), "should not be able to #{action} Notes"
+    end
+  end
 end
 
 class UserAbilityTest < AbilityTest
@@ -45,6 +57,18 @@ class UserAbilityTest < AbilityTest
       assert ability.cannot?(action, Issue), "should not be able to #{action} Issues"
     end
   end
+
+  test "Note permissions" do
+    ability = Ability.new create(:user)
+
+    [:index, :create, :comment, :feed, :show, :search, :mine, :close, :reopen].each do |action|
+      assert ability.can?(action, Note), "should be able to #{action} Notes"
+    end
+
+    [:destroy].each do |action|
+      assert ability.cannot?(action, Note), "should not be able to #{action} Notes"
+    end
+  end
 end
 
 class ModeratorAbilityTest < AbilityTest
@@ -55,6 +79,14 @@ class ModeratorAbilityTest < AbilityTest
       assert ability.can?(action, Issue), "should be able to #{action} Issues"
     end
   end
+
+  test "Note permissions" do
+    ability = Ability.new create(:moderator_user)
+
+    [:index, :create, :comment, :feed, :show, :search, :mine, :close, :reopen, :destroy].each do |action|
+      assert ability.can?(action, Note), "should be able to #{action} Notes"
+    end
+  end
 end
 
 class AdministratorAbilityTest < AbilityTest
diff --git a/test/abilities/capability_test.rb b/test/abilities/capability_test.rb
index 8e77537a4..ed42ef01a 100644
--- a/test/abilities/capability_test.rb
+++ b/test/abilities/capability_test.rb
@@ -54,6 +54,48 @@ class ChangesetCommentCapabilityTest < CapabilityTest
   end
 end
 
+class NoteCapabilityTest < CapabilityTest
+  test "as a normal user with permissionless token" do
+    token = create(:access_token)
+    capability = Capability.new token
+
+    [:create, :comment, :close, :reopen, :destroy].each do |action|
+      assert capability.cannot? action, Note
+    end
+  end
+
+  test "as a normal user with allow_write_notes token" do
+    token = create(:access_token, :allow_write_notes => true)
+    capability = Capability.new token
+
+    [:destroy].each do |action|
+      assert capability.cannot? action, Note
+    end
+
+    [:create, :comment, :close, :reopen].each do |action|
+      assert capability.can? action, Note
+    end
+  end
+
+  test "as a moderator with permissionless token" do
+    token = create(:access_token, :user => create(:moderator_user))
+    capability = Capability.new token
+
+    [:destroy].each do |action|
+      assert capability.cannot? action, Note
+    end
+  end
+
+  test "as a moderator with allow_write_notes token" do
+    token = create(:access_token, :user => create(:moderator_user), :allow_write_notes => true)
+    capability = Capability.new token
+
+    [:destroy].each do |action|
+      assert capability.can? action, Note
+    end
+  end
+end
+
 class UserCapabilityTest < CapabilityTest
   test "user preferences" do
     # a user with no tokens