From: Tom Hughes <tom@compton.nu>
Date: Sat, 3 Nov 2018 14:34:18 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/2023'
X-Git-Tag: live~2816
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/16bef0c8ecad24ac0ca93963196bc844adbb57de?hp=391fb933f59075e36e9cea5ea35f5a5efdfc257e

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

diff --git a/Gemfile b/Gemfile
index d60f1ba94..05bfc6cbd 100644
--- a/Gemfile
+++ b/Gemfile
@@ -45,6 +45,7 @@ gem "image_optim_rails"
 
 # Load rails plugins
 gem "actionpack-page_caching"
+gem "cancancan"
 gem "composite_primary_keys", "~> 11.0.0"
 gem "delayed_job_active_record"
 gem "dynamic_form"
diff --git a/Gemfile.lock b/Gemfile.lock
index 5ddadc3e0..72f769929 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -66,6 +66,7 @@ GEM
     bootsnap (1.3.2)
       msgpack (~> 1.0)
     builder (3.2.3)
+    cancancan (2.1.3)
     canonical-rails (0.2.4)
       rails (>= 4.1, < 5.3)
     capybara (2.18.0)
@@ -387,6 +388,7 @@ DEPENDENCIES
   bigdecimal (~> 1.1.0)
   binding_of_caller
   bootsnap (>= 1.1.0)
+  cancancan
   canonical-rails
   capybara (~> 2.13)
   coffee-rails (~> 4.2)
diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
new file mode 100644
index 000000000..f55f19e4e
--- /dev/null
+++ b/app/abilities/ability.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class Ability
+  include CanCan::Ability
+
+  def initialize(user)
+    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
+
+    if user
+      can :welcome, :site
+      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 [:index, :show, :resolve, :ignore, :reopen], Issue
+        can :create, IssueComment
+      end
+
+      if user.administrator?
+        can [:hide, :hidecomment], [DiaryEntry, DiaryComment]
+        can [:index, :show, :resolve, :ignore, :reopen], Issue
+        can :create, IssueComment
+      end
+    end
+
+    # Define abilities for the passed in user here. For example:
+    #
+    #   user ||= User.new # guest user (not logged in)
+    #   if user.admin?
+    #     can :manage, :all
+    #   else
+    #     can :read, :all
+    #   end
+    #
+    # The first argument to `can` is the action you are giving the user
+    # permission to do.
+    # If you pass :manage it will apply to every action. Other common actions
+    # here are :read, :create, :update and :destroy.
+    #
+    # The second argument is the resource the user can perform the action on.
+    # If you pass :all it will apply to every resource. Otherwise pass a Ruby
+    # class of the resource.
+    #
+    # The third argument is an optional hash of conditions to further filter the
+    # objects.
+    # For example, here the user can only update published articles.
+    #
+    #   can :update, Article, :published => true
+    #
+    # See the wiki for details:
+    # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities
+  end
+end
diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb
new file mode 100644
index 000000000..2a5c92774
--- /dev/null
+++ b/app/abilities/capability.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class Capability
+  include CanCan::Ability
+
+  def initialize(token)
+    can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
+    can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
+  end
+
+  private
+
+  def capability?(token, cap)
+    token&.read_attribute(cap)
+  end
+end
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 17658231f..1df6dd7d1 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -3,6 +3,8 @@ class ApplicationController < ActionController::Base
 
   protect_from_forgery :with => :exception
 
+  rescue_from CanCan::AccessDenied, :with => :deny_access
+
   before_action :fetch_body
   around_action :better_errors_allow_inline, :if => proc { Rails.env.development? }
 
@@ -466,6 +468,29 @@ class ApplicationController < ActionController::Base
     raise
   end
 
+  def current_ability
+    # Add in capabilities from the oauth token if it exists and is a valid access token
+    if Authenticator.new(self, [:token]).allow?
+      Ability.new(current_user).merge(Capability.new(current_token))
+    else
+      Ability.new(current_user)
+    end
+  end
+
+  def deny_access(_exception)
+    if current_token
+      set_locale
+      report_error t("oauth.permissions.missing"), :forbidden
+    elsif current_user
+      set_locale
+      report_error t("application.permission_denied"), :forbidden
+    elsif request.get?
+      redirect_to :controller => "users", :action => "login", :referer => request.fullpath
+    else
+      head :forbidden
+    end
+  end
+
   private
 
   # extract authorisation credentials from headers, returns user = nil if none
diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb
index 6a3ec6755..70cb1654d 100644
--- a/app/controllers/diary_entry_controller.rb
+++ b/app/controllers/diary_entry_controller.rb
@@ -3,11 +3,12 @@ class DiaryEntryController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
+
+  authorize_resource
+
   before_action :lookup_user, :only => [:show, :comments]
   before_action :check_database_readable
   before_action :check_database_writable, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
-  before_action :require_administrator, :only => [:hide, :hidecomment]
   before_action :allow_thirdparty_images, :only => [:new, :edit, :index, :show, :comments]
 
   def new
@@ -215,6 +216,22 @@ class DiaryEntryController < ApplicationController
 
   private
 
+  # This is required because, being a default-deny system, cancancan
+  # _cannot_ tell you the reason you were denied access; and so
+  # the "nice" feedback presenting next steps can't be gleaned from
+  # the exception
+  ##
+  # for the hide actions, require that the user is a administrator, or fill out
+  # a helpful error message and return them to the user page.
+  def deny_access(exception)
+    if current_user && exception.action.in?([:hide, :hidecomment])
+      flash[:error] = t("users.filter.not_an_administrator")
+      redirect_to :action => "show"
+    else
+      super
+    end
+  end
+
   ##
   # return permitted diary entry parameters
   def entry_params
@@ -229,16 +246,6 @@ class DiaryEntryController < ApplicationController
     params.require(:diary_comment).permit(:body)
   end
 
-  ##
-  # require that the user is a administrator, or fill out a helpful error message
-  # and return them to the user page.
-  def require_administrator
-    unless current_user.administrator?
-      flash[:error] = t("users.filter.not_an_administrator")
-      redirect_to :action => "show"
-    end
-  end
-
   ##
   # decide on a location for the diary entry map
   def set_map_location
diff --git a/app/controllers/issue_comments_controller.rb b/app/controllers/issue_comments_controller.rb
index 8d1acec75..0e4a7079e 100644
--- a/app/controllers/issue_comments_controller.rb
+++ b/app/controllers/issue_comments_controller.rb
@@ -3,8 +3,8 @@ class IssueCommentsController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user
-  before_action :check_permission
+
+  authorize_resource
 
   def create
     @issue = Issue.find(params[:issue_id])
@@ -22,10 +22,12 @@ class IssueCommentsController < ApplicationController
     params.require(:issue_comment).permit(:body)
   end
 
-  def check_permission
-    unless current_user.administrator? || current_user.moderator?
+  def deny_access(_exception)
+    if current_user
       flash[:error] = t("application.require_moderator_or_admin.not_a_moderator_or_admin")
       redirect_to root_path
+    else
+      super
     end
   end
 
diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index ad38454f0..8943f2d4a 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -3,8 +3,9 @@ class IssuesController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user
-  before_action :check_permission
+
+  authorize_resource
+
   before_action :find_issue, :only => [:show, :resolve, :reopen, :ignore]
 
   def index
@@ -82,10 +83,12 @@ class IssuesController < ApplicationController
     @issue = Issue.find(params[:id])
   end
 
-  def check_permission
-    unless current_user.administrator? || current_user.moderator?
+  def deny_access(_exception)
+    if current_user
       flash[:error] = t("application.require_moderator_or_admin.not_a_moderator_or_admin")
       redirect_to root_path
+    else
+      super
     end
   end
 end
diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb
index ef87a8699..808726819 100644
--- a/app/controllers/reports_controller.rb
+++ b/app/controllers/reports_controller.rb
@@ -3,7 +3,8 @@ class ReportsController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user
+
+  authorize_resource
 
   def new
     if required_new_report_params_present?
diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb
index efb77e2f5..4b960e4e2 100644
--- a/app/controllers/site_controller.rb
+++ b/app/controllers/site_controller.rb
@@ -6,10 +6,11 @@ class SiteController < ApplicationController
   before_action :set_locale
   before_action :redirect_browse_params, :only => :index
   before_action :redirect_map_params, :only => [:index, :edit, :export]
-  before_action :require_user, :only => [:welcome]
   before_action :require_oauth, :only => [:index]
   before_action :update_totp, :only => [:index]
 
+  authorize_resource :class => false
+
   def index
     session[:location] ||= OSM.ip_location(request.env["REMOTE_ADDR"]) unless STATUS == :database_readonly || STATUS == :database_offline
   end
diff --git a/app/controllers/user_preferences_controller.rb b/app/controllers/user_preferences_controller.rb
index 0aa2e8d52..915c847de 100644
--- a/app/controllers/user_preferences_controller.rb
+++ b/app/controllers/user_preferences_controller.rb
@@ -2,8 +2,9 @@
 class UserPreferencesController < ApplicationController
   skip_before_action :verify_authenticity_token
   before_action :authorize
-  before_action :require_allow_read_prefs, :only => [:read_one, :read]
-  before_action :require_allow_write_prefs, :except => [:read_one, :read]
+
+  authorize_resource
+
   around_action :api_call_handle_error
 
   ##
diff --git a/app/views/layouts/_header.html.erb b/app/views/layouts/_header.html.erb
index e17c6a77b..946f95feb 100644
--- a/app/views/layouts/_header.html.erb
+++ b/app/views/layouts/_header.html.erb
@@ -54,7 +54,7 @@
       <li id="compact-secondary-nav" class="dropdown">
         <a class="dropdown-toggle" data-toggle="dropdown" href="#"><%= t 'layouts.more' %> <b class="caret"></b></a>
         <ul class="dropdown-menu">
-          <% if current_user and ( current_user.administrator? or current_user.moderator? ) %>
+          <% if can? :index, Issue %>
             <li class="<%= current_page_class(issues_path) %>">
               <%= link_to issues_path(:status => 'open') do %>
                 <%= open_issues_count %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f6d396f76..7b13a894b 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1798,6 +1798,7 @@ en:
         other: "GPX file with %{count} points from %{user}"
       description_without_count: "GPX file from %{user}"
   application:
+    permission_denied: You do not have permission to access that action
     require_cookies:
       cookies_needed: "You appear to have cookies disabled - please enable cookies in your browser before continuing."
     require_admin:
diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb
new file mode 100644
index 000000000..fc37b0e7d
--- /dev/null
+++ b/test/abilities/abilities_test.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class AbilityTest < ActiveSupport::TestCase
+end
+
+class GuestAbilityTest < AbilityTest
+  test "geocoder permission for a guest" do
+    ability = Ability.new nil
+
+    [:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim,
+     :search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse].each do |action|
+      assert ability.can?(action, :geocoder), "should be able to #{action} geocoder"
+    end
+  end
+
+  test "diary permissions for a guest" do
+    ability = Ability.new nil
+    [:index, :rss, :show, :comments].each do |action|
+      assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries"
+    end
+
+    [:create, :edit, :comment, :subscribe, :unsubscribe, :hide, :hidecomment].each do |action|
+      assert ability.cannot?(action, DiaryEntry), "should not be able to #{action} DiaryEntries"
+      assert ability.cannot?(action, DiaryComment), "should not be able to #{action} DiaryEntries"
+    end
+  end
+end
+
+class UserAbilityTest < AbilityTest
+  test "Diary permissions" do
+    ability = Ability.new create(:user)
+
+    [:index, :rss, :show, :comments, :create, :edit, :comment, :subscribe, :unsubscribe].each do |action|
+      assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries"
+    end
+
+    [:hide, :hidecomment].each do |action|
+      assert ability.cannot?(action, DiaryEntry), "should not be able to #{action} DiaryEntries"
+      assert ability.cannot?(action, DiaryComment), "should not be able to #{action} DiaryEntries"
+    end
+
+    [:index, :show, :resolve, :ignore, :reopen].each do |action|
+      assert ability.cannot?(action, Issue), "should not be able to #{action} Issues"
+    end
+  end
+end
+
+class ModeratorAbilityTest < AbilityTest
+  test "Issue permissions" do
+    ability = Ability.new create(:moderator_user)
+
+    [:index, :show, :resolve, :ignore, :reopen].each do |action|
+      assert ability.can?(action, Issue), "should be able to #{action} Issues"
+    end
+  end
+end
+
+class AdministratorAbilityTest < AbilityTest
+  test "Diary for an administrator" do
+    ability = Ability.new create(:administrator_user)
+    [:index, :rss, :show, :comments, :create, :edit, :comment, :subscribe, :unsubscribe, :hide, :hidecomment].each do |action|
+      assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries"
+    end
+
+    [:hide, :hidecomment].each do |action|
+      assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment"
+    end
+  end
+end
diff --git a/test/abilities/capability_test.rb b/test/abilities/capability_test.rb
new file mode 100644
index 000000000..a25c67043
--- /dev/null
+++ b/test/abilities/capability_test.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class CapabilityTest < ActiveSupport::TestCase
+  def tokens(*toks)
+    AccessToken.new do |token|
+      toks.each do |t|
+        token.public_send("#{t}=", true)
+      end
+    end
+  end
+end
+
+class UserCapabilityTest < CapabilityTest
+  test "user preferences" do
+    # a user with no tokens
+    capability = Capability.new nil
+    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
+      assert capability.cannot? act, UserPreference
+    end
+
+    # A user with empty tokens
+    capability = Capability.new tokens
+
+    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
+      assert capability.cannot? act, UserPreference
+    end
+
+    capability = Capability.new tokens(:allow_read_prefs)
+
+    [:update, :update_one, :delete_one].each do |act|
+      assert capability.cannot? act, UserPreference
+    end
+
+    [:read, :read_one].each do |act|
+      assert capability.can? act, UserPreference
+    end
+
+    capability = Capability.new tokens(:allow_write_prefs)
+    [:read, :read_one].each do |act|
+      assert capability.cannot? act, UserPreference
+    end
+
+    [:update, :update_one, :delete_one].each do |act|
+      assert capability.can? act, UserPreference
+    end
+  end
+end