Skip to content

Commit

Permalink
Use CanCanCan for changeset comments
Browse files Browse the repository at this point in the history
This introduces different deny_access handlers for web and api requests, since we want to avoid sending redirects as API responses. See openstreetmap#2064 for discussion.
  • Loading branch information
gravitystorm committed Nov 28, 2018
1 parent b29c173 commit 8f70fb2
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 3 deletions.
3 changes: 3 additions & 0 deletions app/abilities/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ 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,
Expand All @@ -13,11 +14,13 @@ def initialize(user)

if user
can :welcome, :site
can :create, ChangesetComment
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 [:destroy, :restore], ChangesetComment
can [:index, :show, :resolve, :ignore, :reopen], Issue
can :create, IssueComment
can [:new, :create, :edit, :update, :destroy], Redaction
Expand Down
5 changes: 5 additions & 0 deletions app/abilities/capability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ class Capability
include CanCan::Ability

def initialize(token)
can :create, ChangesetComment if capability?(token, :allow_write_api)
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)
end
end

private
Expand Down
30 changes: 29 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,15 @@ def current_ability
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
Expand All @@ -497,6 +505,26 @@ def deny_access(_exception)
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
Expand Down
6 changes: 4 additions & 2 deletions app/controllers/changeset_comments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
42 changes: 42 additions & 0 deletions test/abilities/capability_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,48 @@ def tokens(*toks)
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 UserCapabilityTest < CapabilityTest
test "user preferences" do
# a user with no tokens
Expand Down
6 changes: 6 additions & 0 deletions test/factories/access_tokens.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
FactoryBot.define do
factory :access_token do
user
client_application
end
end

0 comments on commit 8f70fb2

Please sign in to comment.