]> git.openstreetmap.org Git - rails.git/commitdiff
Merge branch 'master' into cancancan
authorAndy Allan <git@gravitystorm.co.uk>
Wed, 31 Oct 2018 10:16:47 +0000 (11:16 +0100)
committerAndy Allan <git@gravitystorm.co.uk>
Wed, 31 Oct 2018 10:16:47 +0000 (11:16 +0100)
17 files changed:
Gemfile
Gemfile.lock
app/controllers/application_controller.rb
app/controllers/diary_entry_controller.rb
app/controllers/issue_comments_controller.rb
app/controllers/issues_controller.rb
app/controllers/reports_controller.rb
app/controllers/site_controller.rb
app/controllers/user_preferences_controller.rb
app/models/ability.rb [new file with mode: 0644]
app/models/capability.rb [new file with mode: 0644]
app/views/layouts/_header.html.erb
config/locales/en.yml
test/controllers/user_preferences_controller_test.rb
test/models/abilities_test.rb [new file with mode: 0644]
test/models/capability_test.rb [new file with mode: 0644]
test/test_helper.rb

diff --git a/Gemfile b/Gemfile
index 9ba2703134d5d9b9bf8e5c47904c188396a762e7..b559027c2062fc1625604bc679417dca36e28831 100644 (file)
--- 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"
index 76a31e169e59a8c4aacd58ef738f2de056957302..cd94df5e102759d260642e7a06f226a667655a1d 100644 (file)
@@ -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)
index 17658231f110c6684a11ece3284a7c9a9edd914a..1df6dd7d14cf41e1fad3ac04eb4512fe231eec13 100644 (file)
@@ -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
index 54fe4dd1dc44a425d98d17ac22959e9264b4f44d..cff57920b10cdc7e1c0a4335b0004f5ab4c40496 100644 (file)
@@ -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
index 8d1acec759699d511058061152f5fcd966dad15c..0e4a7079e0e4c5e27608bf344767d529a4349e1f 100644 (file)
@@ -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
 
index ad38454f0650ba62ebd2dae40c1ab383c462c12a..8943f2d4aca695bd0ec2c6766586a70ffae95de1 100644 (file)
@@ -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
index ef87a8699fe807884ec29d09d65a1064bf49a2fa..8087268193d9159932826415b33f9e31a05b131c 100644 (file)
@@ -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?
index efb77e2f52aad92574600d3d87082e3e1376bbe1..4b960e4e2b9682fd6e49036a25fff89093988bf0 100644 (file)
@@ -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
index 0aa2e8d523240c5f1eb3add8f6c978204bcab86f..915c847de4f221fc8a45662efa712d1ab79a5b3a 100644 (file)
@@ -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/models/ability.rb b/app/models/ability.rb
new file mode 100644 (file)
index 0000000..f55f19e
--- /dev/null
@@ -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/models/capability.rb b/app/models/capability.rb
new file mode 100644 (file)
index 0000000..2a5c927
--- /dev/null
@@ -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
index e17c6a77be4cdfff11fc6444cdae7292b53515a4..946f95febf22f23aee9ba1391a70c2ab247be514 100644 (file)
@@ -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 %>
index f6d396f761ffc6d3a490bf1c399413a61bdef261..7b13a894bc860b3e09cddf1deee13f26e34b8a5a 100644 (file)
@@ -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:
index 3e5cbb36998b3b3f03b40037f4756adfd122361f..1a51779ed867a1653ad43f4e8f9b82866d672c69 100644 (file)
@@ -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 (file)
index 0000000..2bae4e8
--- /dev/null
@@ -0,0 +1,57 @@
+# 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 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)
+
+    [: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 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)
+    [: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/models/capability_test.rb b/test/models/capability_test.rb
new file mode 100644 (file)
index 0000000..a25c670
--- /dev/null
@@ -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
index 83cf909dd94f208c5b89c20e024f8e6941e040f4..b7b93455206e1fb5d01090cdec53b5bca27f94f9 100644 (file)
@@ -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)