radarphp / Radar.Adr

The Action-Domain-Responder core for Radar.
MIT License
55 stars 7 forks source link

Responder::notAuthenticated() returning an incorrect status code #13

Closed jonjomckay closed 9 years ago

jonjomckay commented 9 years ago

Hello! After using Radar to start building my application, I've noticed that using Payload::NOT_AUTHENTICATED gives me back a 400 status code. I think this should be a 401 instead? I could be totally wrong, but Im going by this StackOverflow question and my (hazy) understanding of the HTTP spec :)

pmjones commented 9 years ago

You're right, it should be 401. However, HTTP spec indicates a 401 "MUST" be accompanied by a WWW-Authenticate header, with an appropriate scheme and challenge. So we can do one of three things here:

  1. Leave it as 400 and note that users must override it for correctness, providing a WWW-Authenticate appropriate to their situation.
  2. Change it to 401 and and note that users must still provide a WWW-Authenticate appropriate to their situation.
  3. Change it to 401 and find some good, generic, all-purpose, but secure (or at least secure-ish) WWW-Authenticate header to pass along by default.

The option I dislike least is number 2.

Thoughts?

jonjomckay commented 9 years ago

I like number 2 as well - it returns the right status code, and leaves it up to the developer which WWW-Authenticate challenge to return instead of a one-case-fits-all solution!

pmjones commented 9 years ago

@ramsey Thoughts on this thread? The code in question is at https://github.com/radarphp/Radar.Adr/blob/1.x/src/Responder/Responder.php#L97-L101

ramsey commented 9 years ago

I've been reading through RFC 7235. It seems the default authentication scheme defined by HTTP is "Basic," but that's not what you want here, since that would cause the browser to prompt the user for a username and password. What we want most likely for a web application is to prompt the user with a web form that they should fill out, and it's a form that's part of and created by our application (not created by the browser).

I looked through the registry of authentication schemes and could find nothing that's suitable for a web form.

I mentioned to you last night that I remembered dealing with this problem in the past and trying to remember how I solved it. I recall doing something like this:

WWW-Authenticate: WebForm id="formId"

The WebForm isn't a standard authentication scheme, but the browser ignores it. The id could indicate to the browser the ID of the form on the current page that will be used to authenticate, but it's not required.

At this point, I can't remember if I saw this presented somewhere or if I made it up myself, but I do remember using something like this once upon a time.

It might be interesting to draw up an RFC to propose that such a scheme be added to the IANA registry and see how the IETF community responds to it. Seeing as how we've had over 15 years of the WWW-Authenticate header with no auth scheme added to denote using a web form, I wonder whether this has already been discussed at that level and decided against.

ramsey commented 9 years ago

It should be noted that, when the web form is submitted, the browser doesn't send an Authorization header with the WebForm scheme and credentials, and that's probably why nothing like this has been ratified yet.

pmjones commented 9 years ago

It seems the default authentication scheme defined by HTTP is "Basic," but that's not what you want here, since that would cause the browser to prompt the user for a username and password.

Yes, that's exactly what I'm trying to avoid.

Would it be sufficient, for purposes of (1) providing a properly-formatted response but (2) indicating that the implementor still has some work to do, to send WWW-Authenticate: Unknown (or some other syntactically-correct but otherwise useless value ;-) as the default response?

ramsey commented 9 years ago

I'm going to do a bit more research on this.

pmjones commented 9 years ago

@ramsey I appreciate that; I'd like for it to be "as right as possible" without also requiring too much from people just getting started.

pmjones commented 9 years ago

@ramsey Further thoughts on this?

pmjones commented 9 years ago

@ramsey et al: pending further information, it seems best to go with sending a 401, and tell implementors that it's up to them to send a properly-formed WWW-Authenticate header as one of their application-specific tasks. Any objections?

ramsey commented 9 years ago

Sorry I haven't been able to make any progress on this. Short of getting a new authentication type of "Web Form" passed through the IETF or W3C and into the browsers, I think what you've settled on is the best approach.

cxj commented 9 years ago

Ugh. Who knew HTTP 401 would be such a can of worms? Clearly the standards are behind the times on this one.

Another writer's point of view, and I happen to agree, is that the spec allows for any token to be sent as the auth-scheme, and hence, you'd not be breaking the spec by making something up. How a non-conformant browser might react to said string is perhaps undefined.

Therefore, I'm in favor of option 3 or 2. I suggest there be an obvious hook in the Responder for the user-selected lesser of two evils, and maybe even a default value of something like:

WWW-Authenticate: PickYourPoison

Note that however bad that is, I think it's much better than sending HTTP 400, as that causes real problems with a client that actually understands what a 400 is: "The request could not be understood by the server due to malformed syntax."

A bunch of (OUTDATED) references, just in case they're useful at some point in the future.

pmjones commented 9 years ago

OK, 401 it is, with the user having to provide a relevant WWW-Authenticate header.

ramsey commented 9 years ago

FYI, RFC 2616 has been obsoleted and updated by RFCs 7230-7235. One of the updates is the meaning of the 400 status code, which now means:

the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

See https://tools.ietf.org/html/rfc7231#section-6.5.1

cxj commented 9 years ago

Thanks. RFC7231 appears to have the status "proposed standard." Does that mean the older RFC has already been, or only will be superseded? It also appears to drop the 401 status code completely, which rather confuses me.

ramsey commented 9 years ago

"Proposed standard" is their designation for "final." It was ratified last year.

Status code 401 is defined in RFC 7235.

cxj commented 9 years ago

Thanks again for the further clarification!

cxj commented 9 years ago

Wow. After spending the last 45 minutes reading the updated standards, all I can say is no wonder standards committees frequently have the well-deserved reputation for being gangs of idiots.

The 401 documentation is vague, lacking examples, lacking details and lacking links to relevant, related standards documents, when compared to IETF's own writings in other standards documents for other status codes. But once you get past that, find the other documents and read them, you discover that the standards bodies spent all kinds of time detailing authentication schemes that virtually nobody uses, while completely ignoring the one most applications use, form-based login.

They detail things like two kinds of OAuth (in 2 completely separate RFCs), HTTP Origin-Bound Authentication (HOBA), and a Microsoft Kerberos scheme they readily admit violates the standards.

But not a word about form-based login.

pmjones commented 9 years ago

In fairness, building something like this in the first place is a massive undertaking, and that it works as well as it does is a testament to their foresight. That they were insufficiently prescient should probably not be regarded as "just plain dumb." ;-)

cxj commented 9 years ago

True, but I'm belly-aching about the standards that were written 2014. Those shouldn't have required any prescience since form-based auth had been around for a dozen years or so. Ah well.

At least syntactically, inventing a new scheme doesn't break the standard. It just means it won't be listed amongst those registered at IANA:

http://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml

pmjones commented 9 years ago

standards that were written 2014 ... shouldn't have required any prescience

Fair enough ...

At least syntactically, inventing a new scheme doesn't break the standard.

... and at least there's that.

Let a thousand new schemes bloom! ;-)

cxj commented 9 years ago

Should Responder::notAuthenticated() then look something like this?

protected function notAuthenticated()
{
    $this->response = $this->response->withHeader($this->userDefinedScheme());  // new
    $this->response = $this->response->withStatus(401);
    $this->jsonBody($this->payload->getInput());
}

Where the userDefinedScheme() is something we need to invent, and the user of the library needs to config, or set, prior to use. It would return the WWW-Authenticate header along with the user provided scheme token.

pmjones commented 9 years ago

/me ponders

If we set up a method for that, it should probably be getWwwAuthenticate(), and return a dummy value. Then the notAuthenticated() method can have the line $this->response->withHeader('WWW-Authenticate', $this->getWwwAuthenticate()) after the status-setting line.

cxj commented 9 years ago

Yes! That's what I was trying to get at -- clumsily. :-)

pmjones commented 9 years ago

PR!

dzuelke commented 9 years ago

Should that default method maybe log a notice or warning?

cxj commented 9 years ago

@dzuelke Good question. Personally, I'd add it to documentation (I'll even create a PR for that), but will defer the question on adding a log to Paul.