]> git.openstreetmap.org Git - rails.git/commitdiff
Merge remote-tracking branch 'upstream/pull/2072'
authorTom Hughes <tom@compton.nu>
Wed, 28 Nov 2018 18:24:04 +0000 (18:24 +0000)
committerTom Hughes <tom@compton.nu>
Wed, 28 Nov 2018 18:24:04 +0000 (18:24 +0000)
12 files changed:
Gemfile
Gemfile.lock
app/abilities/ability.rb
app/abilities/capability.rb
app/controllers/application_controller.rb
app/controllers/changeset_comments_controller.rb
app/controllers/notes_controller.rb
config/locales/en.yml
test/abilities/abilities_test.rb
test/abilities/capability_test.rb
test/factories/access_tokens.rb [new file with mode: 0644]
test/integration/user_roles_test.rb [deleted file]

diff --git a/Gemfile b/Gemfile
index 35d893309e985ce590052413dc277368610df990..a251b657d5ef210a26149572b6633e7b2425b7a5 100644 (file)
--- a/Gemfile
+++ b/Gemfile
@@ -1,7 +1,7 @@
 source "https://rubygems.org"
 
 # Require rails
-gem "rails", "5.2.1"
+gem "rails", "5.2.1.1"
 
 # Require things which have moved to gems in ruby 1.9
 gem "bigdecimal", "~> 1.1.0", :platforms => :ruby_19
index 7d41c8758f977a8d912965245601435ec031961b..727d2a429af4f35aa2c9052cc9f62033fd69a990 100644 (file)
@@ -4,47 +4,47 @@ GEM
     SystemTimer (1.2.3)
     aasm (5.0.1)
       concurrent-ruby (~> 1.0)
-    actioncable (5.2.1)
-      actionpack (= 5.2.1)
+    actioncable (5.2.1.1)
+      actionpack (= 5.2.1.1)
       nio4r (~> 2.0)
       websocket-driver (>= 0.6.1)
-    actionmailer (5.2.1)
-      actionpack (= 5.2.1)
-      actionview (= 5.2.1)
-      activejob (= 5.2.1)
+    actionmailer (5.2.1.1)
+      actionpack (= 5.2.1.1)
+      actionview (= 5.2.1.1)
+      activejob (= 5.2.1.1)
       mail (~> 2.5, >= 2.5.4)
       rails-dom-testing (~> 2.0)
-    actionpack (5.2.1)
-      actionview (= 5.2.1)
-      activesupport (= 5.2.1)
+    actionpack (5.2.1.1)
+      actionview (= 5.2.1.1)
+      activesupport (= 5.2.1.1)
       rack (~> 2.0)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.0.2)
     actionpack-page_caching (1.1.1)
       actionpack (>= 4.0.0, < 6)
-    actionview (5.2.1)
-      activesupport (= 5.2.1)
+    actionview (5.2.1.1)
+      activesupport (= 5.2.1.1)
       builder (~> 3.1)
       erubi (~> 1.4)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.0.3)
     active_record_union (1.3.0)
       activerecord (>= 4.0)
-    activejob (5.2.1)
-      activesupport (= 5.2.1)
+    activejob (5.2.1.1)
+      activesupport (= 5.2.1.1)
       globalid (>= 0.3.6)
-    activemodel (5.2.1)
-      activesupport (= 5.2.1)
-    activerecord (5.2.1)
-      activemodel (= 5.2.1)
-      activesupport (= 5.2.1)
+    activemodel (5.2.1.1)
+      activesupport (= 5.2.1.1)
+    activerecord (5.2.1.1)
+      activemodel (= 5.2.1.1)
+      activesupport (= 5.2.1.1)
       arel (>= 9.0)
-    activestorage (5.2.1)
-      actionpack (= 5.2.1)
-      activerecord (= 5.2.1)
+    activestorage (5.2.1.1)
+      actionpack (= 5.2.1.1)
+      activerecord (= 5.2.1.1)
       marcel (~> 0.3.1)
-    activesupport (5.2.1)
+    activesupport (5.2.1.1)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (>= 0.7, < 2)
       minitest (~> 5.1)
@@ -111,13 +111,13 @@ GEM
     dynamic_form (1.1.4)
     erubi (1.7.1)
     execjs (2.7.0)
-    exifr (1.3.4)
+    exifr (1.3.5)
     factory_bot (4.11.1)
       activesupport (>= 3.0.0)
     factory_bot_rails (4.11.1)
       factory_bot (~> 4.11.1)
       railties (>= 3.0.0)
-    faraday (0.15.3)
+    faraday (0.15.4)
       multipart-post (>= 1.2, < 3)
     ffi (1.9.25)
     fspath (3.1.0)
@@ -130,8 +130,8 @@ GEM
     http_accept_language (2.0.5)
     i18n (0.9.5)
       concurrent-ruby (~> 1.0)
-    i18n-js (3.1.0)
-      i18n (>= 0.6.6, < 2)
+    i18n-js (3.2.0)
+      i18n (>= 0.8.0, < 2)
     image_optim (0.26.3)
       exifr (~> 1.2, >= 1.2.2)
       fspath (~> 3.0)
@@ -265,18 +265,18 @@ GEM
     rack-test (1.1.0)
       rack (>= 1.0, < 3)
     rack-uri_sanitizer (0.0.2)
-    rails (5.2.1)
-      actioncable (= 5.2.1)
-      actionmailer (= 5.2.1)
-      actionpack (= 5.2.1)
-      actionview (= 5.2.1)
-      activejob (= 5.2.1)
-      activemodel (= 5.2.1)
-      activerecord (= 5.2.1)
-      activestorage (= 5.2.1)
-      activesupport (= 5.2.1)
+    rails (5.2.1.1)
+      actioncable (= 5.2.1.1)
+      actionmailer (= 5.2.1.1)
+      actionpack (= 5.2.1.1)
+      actionview (= 5.2.1.1)
+      activejob (= 5.2.1.1)
+      activemodel (= 5.2.1.1)
+      activerecord (= 5.2.1.1)
+      activestorage (= 5.2.1.1)
+      activesupport (= 5.2.1.1)
       bundler (>= 1.3.0)
-      railties (= 5.2.1)
+      railties (= 5.2.1.1)
       sprockets-rails (>= 2.0.0)
     rails-controller-testing (1.0.2)
       actionpack (~> 5.x, >= 5.0.1)
@@ -290,9 +290,9 @@ GEM
     rails-i18n (4.0.2)
       i18n (~> 0.6)
       rails (>= 4.0)
-    railties (5.2.1)
-      actionpack (= 5.2.1)
-      activesupport (= 5.2.1)
+    railties (5.2.1.1)
+      actionpack (= 5.2.1.1)
+      activesupport (= 5.2.1.1)
       method_source
       rake (>= 0.8.7)
       thor (>= 0.19.0, < 2.0)
@@ -361,7 +361,7 @@ GEM
     tins (1.20.2)
     tzinfo (1.2.5)
       thread_safe (~> 0.1)
-    uglifier (4.1.19)
+    uglifier (4.1.20)
       execjs (>= 0.3.0, < 3)
     unicode-display_width (1.4.0)
     validates_email_format_of (1.6.3)
@@ -434,7 +434,7 @@ DEPENDENCIES
   r2 (~> 0.2.7)
   rack-cors
   rack-uri_sanitizer
-  rails (= 5.2.1)
+  rails (= 5.2.1.1)
   rails-controller-testing
   rails-i18n (~> 4.0.0)
   record_tag_helper
index d7a10005764ba78011b2e68df59089575fc180fe..f2486d2b4307b3551c22a5899e5844eecb643e29 100644 (file)
@@ -4,22 +4,28 @@ 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,
          :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
 
     if user
       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
 
       if user.moderator?
+        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
index 2a5c927748bbdb969b6d37e376a099374085bed4..8ede9bb51bafedf8c40fa1d86bfd146d8ea79ba9 100644 (file)
@@ -4,8 +4,15 @@ class Capability
   include CanCan::Ability
 
   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
 
   private
index 7f9ab6ead6ae889209c4b41fd8df271799ce1c0b..6c6a087b7d1b9dadd75a775ff8a688ad05da966f 100644 (file)
@@ -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?
@@ -477,7 +454,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 +482,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
index 8442a4f360f34882030929f0f037d319abdd859b..a3023af3eb3fd6e057781f4028e1d9c4bf28c6eb 100644 (file)
@@ -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]
index 9cdc38446ca5df7335528d34aeed3bbe5bbae4ad..62fd9d07830a322e1c70e40c125fbc19a2bd3a6a 100644 (file)
@@ -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
 
index ad825ef67162a42665620139595c7e7f8a9862c0..2777c1df30fba99b00fd8d3bfa5f6f67e7916586 100644 (file)
@@ -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."
index fc37b0e7df9ab034f731057d298a339f408e716a..9444a45f5ecee1f71eaecad118ea44670f970610 100644 (file)
@@ -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
index a25c670434677ffe098da203fd9b7a534fa8ba15..ed42ef01a029937339a0042b24c8d69c2f03bee8 100644 (file)
@@ -12,6 +12,90 @@ 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 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
diff --git a/test/factories/access_tokens.rb b/test/factories/access_tokens.rb
new file mode 100644 (file)
index 0000000..98eb910
--- /dev/null
@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :access_token do
+    user
+    client_application
+  end
+end
diff --git a/test/integration/user_roles_test.rb b/test/integration/user_roles_test.rb
deleted file mode 100644 (file)
index 9d36715..0000000
+++ /dev/null
@@ -1,58 +0,0 @@
-require "test_helper"
-
-class UserRolesTest < ActionDispatch::IntegrationTest
-  def setup
-    stub_hostip_requests
-  end
-
-  test "grant" do
-    check_fail(:grant, :user, :moderator)
-    check_fail(:grant, :moderator_user, :moderator)
-    check_success(:grant, :administrator_user, :moderator)
-  end
-
-  test "revoke" do
-    check_fail(:revoke, :user, :moderator)
-    check_fail(:revoke, :moderator_user, :moderator)
-    # this other user doesn't have moderator role, so this fails
-    check_fail(:revoke, :administrator_user, :moderator)
-  end
-
-  private
-
-  def check_fail(action, user, role)
-    get "/login"
-    assert_response :redirect
-    assert_redirected_to "controller" => "users", "action" => "login", "cookie_test" => "true"
-    follow_redirect!
-    assert_response :success
-    post "/login", :params => { "username" => create(user).email, "password" => "test", :referer => "/" }
-    assert_response :redirect
-    follow_redirect!
-    assert_response :success
-
-    target_user = create(:user)
-    post "/user/#{ERB::Util.u(target_user.display_name)}/role/#{role}/#{action}"
-    assert_redirected_to user_path(target_user)
-
-    reset!
-  end
-
-  def check_success(action, user, role)
-    get "/login"
-    assert_response :redirect
-    assert_redirected_to "controller" => "users", "action" => "login", "cookie_test" => "true"
-    follow_redirect!
-    assert_response :success
-    post "/login", :params => { "username" => create(user).email, "password" => "test", :referer => "/" }
-    assert_response :redirect
-    follow_redirect!
-    assert_response :success
-
-    target_user = create(:user)
-    post "/user/#{ERB::Util.u(target_user.display_name)}/role/#{role}/#{action}"
-    assert_redirected_to user_path(target_user)
-
-    reset!
-  end
-end