From: Tom Hughes Date: Wed, 28 Nov 2018 18:24:04 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/2072' X-Git-Tag: live~4143 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/a790c47923ae5722743ba168eb79f3beabe8a71e?hp=74d2c4336bab2efe44c6bdadf688d26b18e320b3 Merge remote-tracking branch 'upstream/pull/2072' --- diff --git a/Gemfile b/Gemfile index 35d893309..a251b657d 100644 --- 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 diff --git a/Gemfile.lock b/Gemfile.lock index 7d41c8758..727d2a429 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -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 diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index d7a100057..f2486d2b4 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -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 diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb index 2a5c92774..8ede9bb51 100644 --- a/app/abilities/capability.rb +++ b/app/abilities/capability.rb @@ -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 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7f9ab6ead..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? @@ -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 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/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 a25c67043..ed42ef01a 100644 --- a/test/abilities/capability_test.rb +++ b/test/abilities/capability_test.rb @@ -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 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 diff --git a/test/integration/user_roles_test.rb b/test/integration/user_roles_test.rb deleted file mode 100644 index 9d367150d..000000000 --- a/test/integration/user_roles_test.rb +++ /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