Skip to content

Commit

Permalink
replaced ActionController::Forbidden with a user-friendly flash message.
Browse files Browse the repository at this point in the history
Setting the 403 status code turned out to be a bad user experience in some browsers
such as Chrome on Windows machines.
  • Loading branch information
Dan Croak committed Jan 23, 2011
1 parent 9903447 commit b004f19
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 29 deletions.
20 changes: 14 additions & 6 deletions app/controllers/clearance/passwords_controller.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -45,17 +45,25 @@ def update


def forbid_missing_token def forbid_missing_token
if params[:token].blank? if params[:token].blank?
raise ActionController::Forbidden, "missing token" flash_failure_when_forbidden
render :template => 'passwords/new'
end end
end end


def forbid_non_existent_user def forbid_non_existent_user
unless ::User.find_by_id_and_confirmation_token( unless ::User.find_by_id_and_confirmation_token(
params[:user_id], params[:token]) params[:user_id], params[:token])
raise ActionController::Forbidden, "non-existent user" flash_failure_when_forbidden
render :template => 'passwords/new'
end end
end end


def flash_failure_when_forbidden
flash.now[:failure] = translate(:forbidden,
:scope => [:clearance, :controllers, :passwords],
:default => "Please double check the URL or try submitting the form again.")
end

def flash_notice_after_create def flash_notice_after_create
flash[:notice] = translate(:deliver_change_password, flash[:notice] = translate(:deliver_change_password,
:scope => [:clearance, :controllers, :passwords], :scope => [:clearance, :controllers, :passwords],
Expand All @@ -69,14 +77,14 @@ def flash_failure_after_create
:default => "Unknown email.") :default => "Unknown email.")
end end


def url_after_create
sign_in_url
end

def flash_success_after_update def flash_success_after_update
flash[:success] = translate(:signed_in, :default => "Signed in.") flash[:success] = translate(:signed_in, :default => "Signed in.")
end end


def url_after_create
sign_in_url
end

def url_after_update def url_after_update
'/' '/'
end end
Expand Down
3 changes: 0 additions & 3 deletions lib/clearance.rb
Original file line number Original file line Diff line number Diff line change
@@ -1,6 +1,3 @@
require 'clearance/extensions/errors'
require 'clearance/extensions/rescue'

require 'clearance/configuration' require 'clearance/configuration'
require 'clearance/authentication' require 'clearance/authentication'
require 'clearance/user' require 'clearance/user'
Expand Down
6 changes: 0 additions & 6 deletions lib/clearance/extensions/errors.rb

This file was deleted.

5 changes: 0 additions & 5 deletions lib/clearance/extensions/rescue.rb

This file was deleted.

6 changes: 1 addition & 5 deletions lib/clearance/shoulda_macros.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@ def should_deny_access(opts = {})
# HTTP FLUENCY # HTTP FLUENCY


def should_forbid(description, &block) def should_forbid(description, &block)
should "forbid #{description}" do warn "[DEPRECATION] should_forbid and Clearance's ActionController::Forbidden have been removed. Setting the 403 status code turned out to be an awful user experience in some browsers such as Chrome on Windows machines."
assert_raises ActionController::Forbidden do
instance_eval(&block)
end
end
end end


# RENDERING # RENDERING
Expand Down
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@
visit edit_user_password_path(:user_id => user) visit edit_user_password_path(:user_id => user)
end end


Then /^I should be forbidden$/ do
assert_response :forbidden
end

# Actions # Actions


When /^I sign in as "(.*)\/(.*)"$/ do |email, password| When /^I sign in as "(.*)\/(.*)"$/ do |email, password|
Expand Down
19 changes: 19 additions & 0 deletions test/controllers/passwords_controller_test.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -85,14 +85,33 @@ class PasswordsControllerTest < ActionController::TestCase
should render_template(:edit) should render_template(:edit)
end end


# here to see deprecation warning
should_forbid "on GET to #edit with correct id but blank token" do should_forbid "on GET to #edit with correct id but blank token" do
get :edit, :user_id => @user.to_param, :token => "" get :edit, :user_id => @user.to_param, :token => ""
end end


context "on GET to #edit with correct id but blank token" do
setup do
get :edit, :user_id => @user.to_param, :token => ""
end

should set_the_flash.to(/double check the URL/i)
should render_template(:new)
end

should_forbid "on GET to #edit with correct id but no token" do should_forbid "on GET to #edit with correct id but no token" do
get :edit, :user_id => @user.to_param get :edit, :user_id => @user.to_param
end end


context "on GET to #edit with correct id but no token" do
setup do
get :edit, :user_id => @user.to_param
end

should set_the_flash.to(/double check the URL/i)
should render_template(:new)
end

context "on PUT to #update with matching password and password confirmation" do context "on PUT to #update with matching password and password confirmation" do
setup do setup do
new_password = "new_password" new_password = "new_password"
Expand Down

2 comments on commit b004f19

@habibalamin
Copy link

Choose a reason for hiding this comment

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

Does anyone know what the bad UX is on Chrome for Windows that this commit is referring to?

@croaky
Copy link
Contributor

@croaky croaky commented on b004f19 Sep 30, 2020

Choose a reason for hiding this comment

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

I don't remember exactly. My apologies for not including a better description in the commit message.

Some Googling from around the time period turns up https://stackoverflow.com/questions/1674659/browser-behavior-on-403-forbidden-error

Please sign in to comment.