Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user block api call #4301

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions app/abilities/api_ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,20 @@ def initialize(token)
can [:inbox, :outbox, :read, :update, :destroy], Message if scope?(token, :consume_messages)
can :create, Message if scope?(token, :send_messages)

if user.terms_agreed?
can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scope?(token, :write_api)
can :create, ChangesetComment if scope?(token, :write_api)
can [:create, :update, :delete], [Node, Way, Relation] if scope?(token, :write_api)
if user.terms_agreed? && scope?(token, :write_api)
can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset
can :create, ChangesetComment
can [:create, :update, :delete], [Node, Way, Relation]
end

if user.moderator?
can [:destroy, :restore], ChangesetComment if scope?(token, :write_api)

can :destroy, Note if scope?(token, :write_notes)

can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scope?(token, :write_redactions)
can :redact, [OldNode, OldWay, OldRelation] if user.terms_agreed? && scope?(token, :write_redactions)

can :create, UserBlock if scope?(token, :write_blocks)
end
end
end
Expand Down
31 changes: 31 additions & 0 deletions app/controllers/api/user_blocks_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
module Api
class UserBlocksController < ApiController
before_action :check_api_writable, :only => :create
before_action :authorize, :only => :create

authorize_resource

before_action :set_request_formats
Expand All @@ -11,5 +14,33 @@ def show
rescue ActiveRecord::RecordNotFound
raise OSM::APINotFoundError
end

def create
raise OSM::APIBadUserInput, "No user was given" unless params[:user]

user = User.visible.find_by(:id => params[:user])
raise OSM::APINotFoundError unless user
raise OSM::APIBadUserInput, "No reason was given" unless params[:reason]
AntonKhorev marked this conversation as resolved.
Show resolved Hide resolved
raise OSM::APIBadUserInput, "No period was given" unless params[:period]

period = Integer(params[:period], :exception => false)
raise OSM::APIBadUserInput, "Period should be a number of hours" unless period
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the permitted values should be more in line with user_block_periods in settings.yml:

# Periods (in hours) which are allowed for user blocks
user_block_periods: [0, 1, 3, 6, 12, 24, 48, 96, 168, 336, 731, 4383, 8766, 87660]

Are values not explicitly mentioned there ok for the UI, or would they cause issues in dropdown lists?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block durations are not stored directly. Dropdowns show the closes value to the remaining time: #5011


max_period = UserBlock::PERIODS.max
raise OSM::APIBadUserInput, "Period must be between 0 and #{max_period}" if period.negative? || period > max_period
raise OSM::APIBadUserInput, "Needs_view must be true if provided" unless params[:needs_view].nil? || params[:needs_view] == "true"

ends_at = Time.now.utc + period.hours
needs_view = params[:needs_view] == "true"
@user_block = UserBlock.create(
AntonKhorev marked this conversation as resolved.
Show resolved Hide resolved
:user => user,
:creator => current_user,
:reason => params[:reason],
:ends_at => ends_at,
:deactivates_at => (ends_at unless needs_view),
:needs_view => needs_view
)
render :show
end
end
end
1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2663,6 +2663,7 @@ en:
write_gpx: Upload GPS traces
write_notes: Modify notes
write_redactions: Redact map data
write_blocks: Create and revoke user blocks
read_email: Read user email address
consume_messages: Read, update status and delete user messages
send_messages: Send private messages to other users
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@
resource :subscription, :only => [:create, :destroy], :controller => "note_subscriptions"
end

resources :user_blocks, :only => :show, :id => /\d+/, :controller => "user_blocks"
resources :user_blocks, :only => [:show, :create], :id => /\d+/, :controller => "user_blocks"
end

# Data browsing
Expand Down
4 changes: 2 additions & 2 deletions lib/oauth.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
module Oauth
SCOPES = %w[
read_prefs write_prefs write_diary
write_api read_gpx write_gpx write_notes write_redactions
write_api read_gpx write_gpx write_notes write_redactions write_blocks
consume_messages send_messages openid
].freeze
PRIVILEGED_SCOPES = %w[read_email skip_authorization].freeze
MODERATOR_SCOPES = %w[write_redactions].freeze
MODERATOR_SCOPES = %w[write_redactions write_blocks].freeze

class Scope
attr_reader :name
Expand Down
179 changes: 177 additions & 2 deletions test/controllers/api/user_blocks_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
module Api
class UserBlocksControllerTest < ActionDispatch::IntegrationTest
def test_routes
assert_routing(
{ :path => "/api/0.6/user_blocks", :method => :post },
{ :controller => "api/user_blocks", :action => "create" }
)
assert_routing(
{ :path => "/api/0.6/user_blocks/1", :method => :get },
{ :controller => "api/user_blocks", :action => "show", :id => "1" }
Expand All @@ -14,11 +18,22 @@ def test_routes
end

def test_show
block = create(:user_block)
blocked_user = create(:user)
creator_user = create(:moderator_user)
block = create(:user_block, :user => blocked_user, :creator => creator_user, :reason => "because running tests")

get api_user_block_path(block)
assert_response :success
assert_select "user_block[id='#{block.id}']", 1
assert_select "osm>user_block", 1 do
assert_select ">@id", block.id.to_s
assert_select ">user", 1
assert_select ">user>@uid", blocked_user.id.to_s
assert_select ">creator", 1
assert_select ">creator>@uid", creator_user.id.to_s
assert_select ">revoker", 0
assert_select ">reason", 1
assert_select ">reason", "because running tests"
end

get api_user_block_path(block, :format => "json")
assert_response :success
Expand All @@ -32,5 +47,165 @@ def test_show_not_found
assert_response :not_found
assert_equal "text/plain", @response.media_type
end

def test_create_no_permission
blocked_user = create(:user)
assert_empty blocked_user.blocks

post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1)
assert_response :unauthorized
assert_empty blocked_user.blocks

regular_creator_user = create(:user)
auth_header = bearer_authorization_header(regular_creator_user, :scopes => %w[read_prefs])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1), :headers => auth_header
assert_response :forbidden
assert_empty blocked_user.blocks

auth_header = bearer_authorization_header(regular_creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1), :headers => auth_header
assert_response :forbidden
assert_empty blocked_user.blocks

moderator_creator_user = create(:moderator_user)
auth_header = bearer_authorization_header(moderator_creator_user, :scopes => %w[read_prefs])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1), :headers => auth_header
assert_response :forbidden
assert_empty blocked_user.blocks
end

def test_create_invalid_because_no_user
blocked_user = create(:user, :deleted)
assert_empty blocked_user.blocks

creator_user = create(:moderator_user)
auth_header = bearer_authorization_header(creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:reason => "because", :period => 1), :headers => auth_header
assert_response :bad_request
assert_equal "text/plain", @response.media_type
assert_equal "No user was given", @response.body

assert_empty blocked_user.blocks
end

def test_create_invalid_because_user_is_unknown
creator_user = create(:moderator_user)
auth_header = bearer_authorization_header(creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:user => 0, :reason => "because", :period => 1), :headers => auth_header
assert_response :not_found
assert_equal "text/plain", @response.media_type
end

def test_create_invalid_because_user_is_deleted
blocked_user = create(:user, :deleted)
assert_empty blocked_user.blocks

creator_user = create(:moderator_user)
auth_header = bearer_authorization_header(creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1), :headers => auth_header
assert_response :not_found
assert_equal "text/plain", @response.media_type

assert_empty blocked_user.blocks
end

def test_create_invalid_because_missing_reason
create_with_params_and_assert_bad_request("No reason was given", :period => "10")
end

def test_create_invalid_because_missing_period
create_with_params_and_assert_bad_request("No period was given", :reason => "because")
end

def test_create_invalid_because_non_numeric_period
create_with_params_and_assert_bad_request("Period should be a number of hours", :reason => "because", :period => "one hour")
end

def test_create_invalid_because_negative_period
create_with_params_and_assert_bad_request("Period must be between 0 and #{UserBlock::PERIODS.max}", :reason => "go away", :period => "-1")
end

def test_create_invalid_because_excessive_period
create_with_params_and_assert_bad_request("Period must be between 0 and #{UserBlock::PERIODS.max}", :reason => "go away", :period => "10000000")
end

def test_create_invalid_because_unknown_needs_view
create_with_params_and_assert_bad_request("Needs_view must be true if provided", :reason => "because", :period => "1", :needs_view => "maybe")
end

def test_create_success
blocked_user = create(:user)
creator_user = create(:moderator_user)

assert_empty blocked_user.blocks
auth_header = bearer_authorization_header(creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => 1), :headers => auth_header
assert_response :success
assert_equal 1, blocked_user.blocks.length

block = blocked_user.blocks.take
assert_predicate block, :active?
assert_equal "because", block.reason
assert_equal creator_user, block.creator

assert_equal "application/xml", @response.media_type
assert_select "osm>user_block", 1 do
assert_select ">@id", block.id.to_s
assert_select ">@needs_view", "false"
assert_select ">user", 1
assert_select ">user>@uid", blocked_user.id.to_s
assert_select ">creator", 1
assert_select ">creator>@uid", creator_user.id.to_s
assert_select ">revoker", 0
assert_select ">reason", 1
assert_select ">reason", "because"
end
end
AntonKhorev marked this conversation as resolved.
Show resolved Hide resolved

def test_create_success_with_needs_view
blocked_user = create(:user)
creator_user = create(:moderator_user)

assert_empty blocked_user.blocks
auth_header = bearer_authorization_header(creator_user, :scopes => %w[read_prefs write_blocks])
post api_user_blocks_path(:user => blocked_user.id, :reason => "because", :period => "1", :needs_view => "true"), :headers => auth_header
assert_response :success
assert_equal 1, blocked_user.blocks.length

block = blocked_user.blocks.take
assert_predicate block, :active?
assert_equal "because", block.reason
assert_equal creator_user, block.creator

assert_equal "application/xml", @response.media_type
assert_select "osm>user_block", 1 do
assert_select ">@id", block.id.to_s
assert_select ">@needs_view", "true"
assert_select ">user", 1
assert_select ">user>@uid", blocked_user.id.to_s
assert_select ">creator", 1
assert_select ">creator>@uid", creator_user.id.to_s
assert_select ">revoker", 0
assert_select ">reason", 1
assert_select ">reason", "because"
end
end

private

def create_with_params_and_assert_bad_request(message, **params)
blocked_user = create(:user)
assert_empty blocked_user.blocks

moderator_creator_user = create(:moderator_user)
auth_header = bearer_authorization_header(moderator_creator_user, :scopes => %w[read_prefs write_blocks])

post api_user_blocks_path({ :user => blocked_user.id }.merge(params)), :headers => auth_header
assert_response :bad_request
assert_equal "text/plain", @response.media_type
assert_equal message, @response.body

assert_empty blocked_user.blocks
end
end
end
Loading