From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 10 Oct 2018 09:26:30 +0000 (+0200)
Subject: Merge branch 'authz' of https://github.com/rubyforgood/openstreetmap-website into... 
X-Git-Tag: live~2834^2~15
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/420a7289a0b08eee091f6650c2e83166df3fbe69?hp=4530f6e3e4a327f5adf348f1cb69fd1ef0bd90f9

Merge branch 'authz' of https://github.com/rubyforgood/openstreetmap-website into rubyforgood-authz
---

diff --git a/Gemfile b/Gemfile
index 9ba270313..b559027c2 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 "dynamic_form"
 gem "http_accept_language", "~> 2.0.0"
diff --git a/Gemfile.lock b/Gemfile.lock
index 76a31e169..cd94df5e1 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/controllers/application_controller.rb b/app/controllers/application_controller.rb
index bd1995014..2c5fbe51d 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -1,8 +1,11 @@
 class ApplicationController < ActionController::Base
   include SessionPersistence
+  # check_authorization
 
   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 +469,23 @@ class ApplicationController < ActionController::Base
     raise
   end
 
+  def current_ability
+    Ability.new(current_user).merge(granted_capabily)
+  end
+
+  def granted_capabily
+    Capability.new(current_user, current_token)
+  end
+
+  def deny_access(_exception)
+    if current_user
+      set_locale
+      report_error t("oauth.permissions.missing"), :forbidden
+    else
+      require_user
+    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 723fff17e..d3d7f6a7c 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("user.filter.not_an_administrator")
+      redirect_to :action => "view"
+    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("user.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/site_controller.rb b/app/controllers/site_controller.rb
index efb77e2f5..2fa91256e 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
@@ -102,7 +103,9 @@ class SiteController < ApplicationController
     @locale = params[:copyright_locale] || I18n.locale
   end
 
-  def welcome; end
+  def welcome
+    require_user
+  end
 
   def help; 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/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d18cf188c..09bdd6d3e 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1,6 +1,8 @@
 class UsersController < ApplicationController
   layout "site", :except => [:api_details]
 
+  skip_authorization_check :only => [:login, :logout]
+
   skip_before_action :verify_authenticity_token, :only => [:api_read, :api_users, :api_details, :api_gpx_files, :auth_success]
   before_action :disable_terms_redirect, :only => [:terms, :save, :logout, :api_details]
   before_action :authorize, :only => [:api_details, :api_gpx_files]
diff --git a/app/models/ability.rb b/app/models/ability.rb
new file mode 100644
index 000000000..5be0b37e5
--- /dev/null
+++ b/app/models/ability.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+class Ability
+  include CanCan::Ability
+
+  def initialize(user)
+    can :index, :site
+    can [:permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id, :welcome], :site
+
+    can [:list, :rss, :view, :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 :weclome, :site
+
+      can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry
+
+      can [:hide, :hidecomment], [DiaryEntry, DiaryComment] if user.administrator?
+    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/models/capability.rb b/app/models/capability.rb
new file mode 100644
index 000000000..db2d71711
--- /dev/null
+++ b/app/models/capability.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class Capability
+  include CanCan::Ability
+
+  def initialize(user, token)
+    if user
+      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
+  end
+
+  private
+
+  # If a user provides no tokens, they've authenticated via a non-oauth method
+  # and permission to access to all capabilities is assumed.
+  def capability?(token, cap)
+    token.nil? || token.read_attribute(cap)
+  end
+end
diff --git a/test/controllers/user_preferences_controller_test.rb b/test/controllers/user_preferences_controller_test.rb
index 3e5cbb369..1a51779ed 100644
--- a/test/controllers/user_preferences_controller_test.rb
+++ b/test/controllers/user_preferences_controller_test.rb
@@ -35,6 +35,7 @@ class UserPreferencesControllerTest < ActionController::TestCase
 
     # authenticate as a user with no preferences
     basic_authorization create(:user).email, "test"
+    grant_oauth_token :allow_read_prefs
 
     # try the read again
     get :read
@@ -75,6 +76,7 @@ class UserPreferencesControllerTest < ActionController::TestCase
 
     # authenticate as a user with preferences
     basic_authorization user.email, "test"
+    grant_oauth_token :allow_read_prefs
 
     # try the read again
     get :read_one, :params => { :preference_key => "key" }
@@ -108,6 +110,7 @@ class UserPreferencesControllerTest < ActionController::TestCase
 
     # authenticate as a user with preferences
     basic_authorization user.email, "test"
+    grant_oauth_token :allow_write_prefs
 
     # try the put again
     assert_no_difference "UserPreference.count" do
@@ -159,6 +162,7 @@ class UserPreferencesControllerTest < ActionController::TestCase
 
     # authenticate as a user with preferences
     basic_authorization user.email, "test"
+    grant_oauth_token :allow_write_prefs
 
     # try adding a new preference
     assert_difference "UserPreference.count", 1 do
@@ -196,6 +200,7 @@ class UserPreferencesControllerTest < ActionController::TestCase
 
     # authenticate as a user with preferences
     basic_authorization user.email, "test"
+    grant_oauth_token :allow_write_prefs
 
     # try the delete again
     assert_difference "UserPreference.count", -1 do
diff --git a/test/models/abilities_test.rb b/test/models/abilities_test.rb
new file mode 100644
index 000000000..28a5c7fd9
--- /dev/null
+++ b/test/models/abilities_test.rb
@@ -0,0 +1,65 @@
+# 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
+    [:list, :rss, :view, :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 be able to #{action} DiaryEntries"
+      assert ability.cannot?(action, DiaryComment), "should be able to #{action} DiaryEntries"
+    end
+  end
+end
+
+class UserAbilityTest < AbilityTest
+  test "Diary permissions" do
+    ability = Ability.new create(:user)
+
+    [:list, :rss, :view, :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 be able to #{action} DiaryEntries"
+      assert ability.cannot?(action, DiaryComment), "should be able to #{action} DiaryEntries"
+    end
+  end
+end
+
+class AdministratorAbilityTest < AbilityTest
+  test "Diary for an administrator" do
+    ability = Ability.new create(:administrator_user)
+    [:list, :rss, :view, :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
+
+  test "administrator does not auto-grant user preferences" do
+    ability = Ability.new create(:administrator_user)
+
+    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
+      assert ability.cannot? act, UserPreference
+    end
+  end
+end
diff --git a/test/models/capability_test.rb b/test/models/capability_test.rb
new file mode 100644
index 000000000..2d5d65002
--- /dev/null
+++ b/test/models/capability_test.rb
@@ -0,0 +1,51 @@
+# 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
+    user = create(:user)
+
+    # a user with no tokens
+    capability = Capability.new create(:user), nil
+    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
+      assert capability.can? act, UserPreference
+    end
+
+    # A user with empty tokens
+    capability = Capability.new create(:user), tokens
+
+    [:read, :read_one, :update, :update_one, :delete_one].each do |act|
+      assert capability.cannot? act, UserPreference
+    end
+
+    capability = Capability.new user, 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 user, 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
diff --git a/test/test_helper.rb b/test/test_helper.rb
index 83cf909dd..b7b934552 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -87,6 +87,16 @@ module ActiveSupport
       @request.env["HTTP_AUTHORIZATION"] = format("Basic %{auth}", :auth => Base64.encode64("#{user}:#{pass}"))
     end
 
+    ##
+    # set oauth token permissions
+    def grant_oauth_token(*tokens)
+      request.env["oauth.token"] = AccessToken.new do |token|
+        tokens.each do |t|
+          token.public_send("#{t}=", true)
+        end
+      end
+    end
+
     ##
     # set request readers to ask for a particular error format
     def error_format(format)