From: Matt Amos <zerebubuth@gmail.com>
Date: Wed, 30 Sep 2009 17:39:42 +0000 (+0000)
Subject: Added better messages and error handling in a couple of places. Added integration... 
X-Git-Tag: live~6552^2~30
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/77851bac7bf76473de33b7bbab00bc1b3d834cb1?hp=95c3d640a44857b0b92ec3be6dad606e40057c5c

Added better messages and error handling in a couple of places. Added integration checks to ensure that the blocking is actually working. Tests FTW.
---

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 012ba2446..c701d8add 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -80,9 +80,9 @@ class ApplicationController < ActionController::Base
     end
 
     # check if the user has been banned
-    unless @user.nil? or @user.blocks.empty?
+    unless @user.nil? or @user.active_blocks.empty?
       # NOTE: need slightly more helpful message than this.
-      render :text => "You got banned!", :status => :forbidden
+      render :text => t('application.setup_user_auth.blocked'), :status => :forbidden
     end
   end
 
diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb
index 7d3830c25..f6bca4bce 100644
--- a/app/controllers/user_blocks_controller.rb
+++ b/app/controllers/user_blocks_controller.rb
@@ -122,12 +122,16 @@ class UserBlocksController < ApplicationController
   # ensure that there is a "this_user" instance variable
   def lookup_this_user
     @this_user = User.find_by_display_name(params[:display_name])
+  rescue ActiveRecord::RecordNotFound
+    redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] unless @this_user
   end
 
   ##
   # ensure that there is a "user_block" instance variable
   def lookup_user_block
     @user_block = UserBlock.find(params[:id])
+  rescue ActiveRecord::RecordNotFound
+    render :action => "not_found", :status => :not_found
   end
 
   ##
diff --git a/app/models/user.rb b/app/models/user.rb
index 526844672..95f0e3986 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -14,7 +14,7 @@ class User < ActiveRecord::Base
   has_many :client_applications
   has_many :oauth_tokens, :class_name => "OauthToken", :order => "authorized_at desc", :include => [:client_application]
 
-  has_many :blocks, :class_name => "UserBlock", :conditions => ["user_blocks.ends_at > now() or user_blocks.needs_view"]
+  has_many :active_blocks, :class_name => "UserBlock", :conditions => ['user_blocks.ends_at > \'#{Time.now.getutc.xmlschema(5)}\' or user_blocks.needs_view']
   has_many :roles, :class_name => "UserRole"
 
   validates_presence_of :email, :display_name
@@ -150,7 +150,7 @@ class User < ActiveRecord::Base
   # returns the first active block which would require users to view 
   # a message, or nil if there are none.
   def blocked_on_view
-    blocks.inject(nil) { |s,x| s || (x.needs_view? ? x : nil) }
+    active_blocks.inject(nil) { |s,x| s || (x.needs_view? ? x : nil) }
   end
 
   def delete
diff --git a/app/views/user_blocks/not_found.html.erb b/app/views/user_blocks/not_found.html.erb
new file mode 100644
index 000000000..3b5323d72
--- /dev/null
+++ b/app/views/user_blocks/not_found.html.erb
@@ -0,0 +1,3 @@
+<p><%= t'user_block.not_found.sorry', :id => params[:id] %></p>
+
+<%= link_to t('user_block.not_found.back'), user_blocks_path %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 363f620f4..241e0327f 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -800,6 +800,9 @@ en:
       scheduled_for_deletion: "Track scheduled for deletion"
     make_public:
       made_public: "Track made public"
+  application:
+    setup_user_auth:
+      blocked: "Your access to the API has been blocked. Please log-in to the web interface to find out more."
   oauth:
     oauthorize:
       request_access: "The application {{app_name}} is requesting access to your account. Please check whether you would like the application to have the following capabilities. You may choose as many or as few as you like."
@@ -1012,6 +1015,9 @@ en:
     model:
       non_moderator_update: "Must be a moderator to create or update a block."
       non_moderator_revoke: "Must be a moderator to revoke a block."
+    not_found:
+      sorry: "Sorry, the user block with ID {{id}} could not be found."
+      back: "Back to index"
     new:
       reason: "The reason why {{name}} is being blocked. Please be as calm and as reasonable as possible, giving as much detail as you can about the situation, remembering that the message will be publicly visible. Bear in mind that not all users understand the community jargon, so please try to use laymans terms."
       period: "How long, starting now, the user will be blocked from the API for."
diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb
new file mode 100644
index 000000000..822c923cc
--- /dev/null
+++ b/test/integration/user_blocks_test.rb
@@ -0,0 +1,56 @@
+require File.dirname(__FILE__) + '/../test_helper'
+
+class UserBlocksTest < ActionController::IntegrationTest
+  fixtures :users, :user_blocks
+
+  def auth_header(user, pass)
+    {"HTTP_AUTHORIZATION" => "Basic %s" % Base64.encode64("#{user}:#{pass}")}
+  end
+
+  def test_api_blocked
+    blocked_user = users(:public_user)
+
+    get "/api/#{API_VERSION}/user/details"
+    assert_response :unauthorized
+
+    get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
+    assert_response :success
+
+    # now block the user
+    UserBlock.create(:user_id => blocked_user.id,
+                     :creator_id => users(:moderator_user).id,
+                     :reason => "testing",
+                     :ends_at => Time.now.getutc + 5.minutes)
+    get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
+    assert_response :forbidden
+  end
+
+  def test_api_revoke
+    blocked_user = users(:public_user)
+    moderator = users(:moderator_user)
+
+    block = UserBlock.create(:user_id => blocked_user.id,
+                             :creator_id => moderator.id,
+                             :reason => "testing",
+                             :ends_at => Time.now.getutc + 5.minutes)
+    get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
+    assert_response :forbidden
+
+    # revoke the ban
+    post '/login', {'user[email]' => moderator.email, 'user[password]' => "test", :referer => "/blocks/#{block.id}/revoke"}
+    assert_response :redirect
+    follow_redirect!
+    assert_response :success
+    assert_template 'user_blocks/revoke'
+    post "/blocks/#{block.id}/revoke", {'confirm' => "yes"}
+    assert_response :redirect
+    follow_redirect!
+    assert_response :success
+    assert_template 'user_blocks/show'
+    reset!
+
+    # access the API again. this time it should work
+    get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
+    assert_response :success
+  end
+end