Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

http basic auth failures respect request.accept for json/xml #6394

Closed
wants to merge 2 commits into from

5 participants

@darragh

I noticed that rails http authentication currently always returns a string like "HTTP Basic: Access denied." regardless of the request Accept header.

We got reports of this causing some clients problems (I guess they are expecting json, and always attempting to parse the body as json).

I thought I'd experiment with modifying rails to respond with some valid json when accept is for json, and likewise for xml.

Is this a good idea?

for json it will now return something like:

{"error":"HTTP Basic: Access denied."}

or xml

<error>HTTP Basic: Access denied.</error>
darragh added some commits
@darragh darragh Extract out common method for rendering a 401 Access Denied response.
Looks after setting the WWW-Authenticate header, the status code and the response body.
cdafc1f
@darragh darragh http basic auth 401 body encoded as json or xml when requested
We got reports of problems caused by our rails app returning text/html content to api requests with an accept header of application/json. This change alters the http basic auth failure response to match the request accept format when xml or json
8791793
@steveklabnik
Collaborator

:+1: conceptually, I need to check out the code yet though. Yay checking commits during a pomodoro break!

@steveklabnik
Collaborator

So, after some more thought, I'm not sure how I feel about this. Really, if you get a 401, you shouldn't even really be bothering to look at the body. That said, I don't think this is bad either it just shouldn't really be necessary.

@guilleiguaran

As @steveklabnik the clients shouldn't bother about parsing the body when they get a 401

Thanks for your pull request but I think this can live better as a custom responder instead of be a standard in rails

@foton

If

clients shouldn't bother about parsing the body

why is in the body returned "HTTP Basic: Access denied.\n"?

@dmathieu
Collaborator

@foton: what @guilleiguaran said is that in a json or xml API, when there is a 401, it is the client's job to handle not parsing the body.

This is actually very questionable. However, really solving this problem would also mean rendering automatically 500 errors in json and xml. All errors defaultly need to behave the same way.

And since there is no clear standard on how APIs should render errors, and every app does it it's own way, let them do so. A responder can easily allow you to manage that.

@foton

OK, I understand.

@steveklabnik
Collaborator

since there is no clear standard on how APIs should render errors

http://tools.ietf.org/html/draft-nottingham-http-problem-04 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 19, 2012
  1. @darragh

    Extract out common method for rendering a 401 Access Denied response.

    darragh authored
    Looks after setting the WWW-Authenticate header, the status code and the response body.
  2. @darragh

    http basic auth 401 body encoded as json or xml when requested

    darragh authored
    We got reports of problems caused by our rails app returning text/html content to api requests with an accept header of application/json. This change alters the http basic auth failure response to match the request accept format when xml or json
This page is out of date. Refresh to see the latest.
View
29 actionpack/lib/action_controller/metal/http_authentication.rb
@@ -4,6 +4,19 @@
module ActionController
# Makes it dead easy to do HTTP Basic, Digest and Token authentication.
module HttpAuthentication
+ module AccessDeniedResponder
+ def respond_with_access_denied(controller, type, header, message_override=nil)
+ controller.headers["WWW-Authenticate"] = header
+ controller.status = 401
+ error_message = message_override || "HTTP #{type}: Access denied.\n"
+ controller.__send__(:respond_to) do |format|
+ format.json { controller.__send__(:render, :json => {:error => error_message.strip}) }
+ format.xml { controller.__send__(:render, :xml => "<error>#{error_message.strip}</error>") }
+ format.any { controller.__send__(:render, :text => error_message) }
+ end
+ end
+ end
+
# Makes it dead easy to do HTTP \Basic authentication.
#
# === Simple \Basic example
@@ -63,6 +76,7 @@ module HttpAuthentication
# end
module Basic
extend self
+ extend AccessDeniedResponder
module ControllerMethods
extend ActiveSupport::Concern
@@ -109,9 +123,7 @@ def encode_credentials(user_name, password)
end
def authentication_request(controller, realm)
- controller.headers["WWW-Authenticate"] = %(Basic realm="#{realm.gsub(/"/, "")}")
- controller.response_body = "HTTP Basic: Access denied.\n"
- controller.status = 401
+ respond_with_access_denied(controller, "Basic", %(Basic realm="#{realm.gsub(/"/, "")}"))
end
end
@@ -159,6 +171,7 @@ def authentication_request(controller, realm)
# variables, and check for HTTP_AUTHORIZATION, amongst others.
module Digest
extend self
+ extend AccessDeniedResponder
module ControllerMethods
def authenticate_or_request_with_http_digest(realm = "Application", &password_procedure)
@@ -239,14 +252,12 @@ def authentication_header(controller, realm)
secret_key = secret_token(controller.request)
nonce = self.nonce(secret_key)
opaque = opaque(secret_key)
- controller.headers["WWW-Authenticate"] = %(Digest realm="#{realm}", qop="auth", algorithm=MD5, nonce="#{nonce}", opaque="#{opaque}")
+ %(Digest realm="#{realm}", qop="auth", algorithm=MD5, nonce="#{nonce}", opaque="#{opaque}")
end
def authentication_request(controller, realm, message = nil)
message ||= "HTTP Digest: Access denied.\n"
- authentication_header(controller, realm)
- controller.response_body = message
- controller.status = 401
+ respond_with_access_denied(controller, "Digest", authentication_header(controller, realm), message)
end
def secret_token(request)
@@ -386,6 +397,7 @@ def opaque(secret_key)
# RewriteRule ^(.*)$ dispatch.fcgi [E=X-HTTP_AUTHORIZATION:%{HTTP:Authorization},QSA,L]
module Token
extend self
+ extend AccessDeniedResponder
module ControllerMethods
def authenticate_or_request_with_http_token(realm = "Application", &login_procedure)
@@ -460,8 +472,7 @@ def encode_credentials(token, options = {})
#
# Returns nothing.
def authentication_request(controller, realm)
- controller.headers["WWW-Authenticate"] = %(Token realm="#{realm.gsub(/"/, "")}")
- controller.__send__ :render, :text => "HTTP Token: Access denied.\n", :status => :unauthorized
+ respond_with_access_denied(controller, "Token", %(Token realm="#{realm.gsub(/"/, "")}"))
end
end
end
View
27 actionpack/test/controller/http_basic_authentication_test.rb
@@ -93,6 +93,33 @@ def test_encode_credentials_has_no_newline
assert_no_match(/\n/, result)
end
+ test "unsuccessful authentication request with application/json Accept header" do
+ @request.env["HTTP_ACCEPT"] = "application/json"
+ get :index
+
+ assert_response :unauthorized
+ assert_equal '{"error":"HTTP Basic: Access denied."}', @response.body
+ assert @response.headers['Content-Type'].start_with? 'application/json'
+ end
+
+ test "unsuccessful authentication request with text/xml Accept header" do
+ @request.env["HTTP_ACCEPT"] = "application/xml"
+ get :index
+
+ assert_response :unauthorized
+ assert_equal '<error>HTTP Basic: Access denied.</error>', @response.body
+ assert @response.headers['Content-Type'].start_with? 'application/xml'
+ end
+
+ test "unsuccessful authentication request defaults to plain text/html output with typical accept header" do
+ @request.env["HTTP_ACCEPT"] = "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"
+ get :index
+
+ assert_response :unauthorized
+ assert_equal "HTTP Basic: Access denied.\n", @response.body
+ assert @response.headers['Content-Type'].start_with? 'text/html'
+ end
+
test "authentication request without credential" do
get :display
Something went wrong with that request. Please try again.