Add support for authorization of HTTP client requests

Add support for authorization of HTTP client requests

This implements the infrastructure for authorization of HTTP client
requests. Currently only basic authentication is fully implemented,
but the infrastructure to support digest is there.

Please focus on the developer facing API with the authenticate callback and
addCredentials method.

R=ager@google.com,ajohnsen@google.com

BUG=dart:2788
TEST=tests/standalone/io/http_auth_test.dart

Committed: https://code.google.com/p/dart/source/detail?r=14189

[Mads Ager (google), 2012-10-17 11:26:43]

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart#newcode721
runtime/bin/http.dart:721: * the updated credentials. Otherwise response
procession will
procession -> processing?

How about returning an HttpClientCredentials object from authenticate and 
getting rid of the 'addCredentials' method? A [null] could indicate not having
credentials to add. That seems like a simplification for the user.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:359: if (done()) throw new HttpException("Failed to
parse header value $expected [$s]");
Long line.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:361: throw new HttpException("Failed to parse header
value x $expected [$s]");
Ditto.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:390: throw new HttpException("Failed to parse header
value y [$s]");
Accidental edit?

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:1746: _Credentials cr =
What if we authorize once and the credentials are added to the credentials
store. Then there is a password change and another request with the same client.
That could get us in here again with another challenge? However, we will use the
cached credentials without calling authenticate again? Shouldn't we add a
mechanism to remove the credentials and reauthorize if they did not give access?

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2356: if (prev != null) return prev;
I would use a two-space indent for these three lines.

[Anders Johnsen, 2012-10-22 14:01:22]

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart#newcode721
runtime/bin/http.dart:721: * the updated credentials. Otherwise response
procession will
On 2012/10/17 11:26:43, Mads Ager wrote:
> procession -> processing?
> 
> How about returning an HttpClientCredentials object from authenticate and 
> getting rid of the 'addCredentials' method? A [null] could indicate not having
> credentials to add. That seems like a simplification for the user.
>  

I agree. The HttpClient should not have any credentials stored internally.

[Søren Gjesse, 2012-10-26 09:43:00]

PTAL

Addressed comments and updated test (and fixed checked mode issues).

We could still discuss the API around this, but I think the HttpClient need to
cache the credentials as the credentials are needed for each request into an
authenticated realm. Also when implementing digest the credentials need to be
paired by state data.

Of cause this provides three ways of providing username and password:

1) Set user info on the URL (these credentials will not be cached)
2) Use HttpClient.addCredentials before making and requests
3) Use HttpClient.addCredentials in a callback from the provided
HttpClient.authenticate function.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart#newcode721
runtime/bin/http.dart:721: * the updated credentials. Otherwise response
procession will
On 2012/10/17 11:26:43, Mads Ager wrote:
> procession -> processing?
> 
> How about returning an HttpClientCredentials object from authenticate and 
> getting rid of the 'addCredentials' method? A [null] could indicate not having
> credentials to add. That seems like a simplification for the user.
>  

The reason for having the addCredentials method is to make it possible to add
credentials up-front and not have to deal with using this authenticate callback.
We could have the authenticate callback return the HttpClientCredentials object
to use instead of a boolean.

Maybe a HttpClientCredentials returned here should then be transient and not
added to the credentials store?

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http.dart#newcode721
runtime/bin/http.dart:721: * the updated credentials. Otherwise response
procession will
On 2012/10/22 14:01:22, ajohnsen wrote:
> On 2012/10/17 11:26:43, Mads Ager wrote:
> > procession -> processing?
> > 
> > How about returning an HttpClientCredentials object from authenticate and 
> > getting rid of the 'addCredentials' method? A [null] could indicate not
having
> > credentials to add. That seems like a simplification for the user.
> >  
> 
> I agree. The HttpClient should not have any credentials stored internally.

I don't agree here. The HttpClient should have a credentials store, as each
request made needs to have the "Authorization:" header set. If the credentials
are not stored all requests requiring authorization will require an additional
round trip with a status code 401 responses.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:359: if (done()) throw new HttpException("Failed to
parse header value $expected [$s]");
On 2012/10/17 11:26:43, Mads Ager wrote:
> Long line.

Done.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:361: throw new HttpException("Failed to parse header
value x $expected [$s]");
On 2012/10/17 11:26:43, Mads Ager wrote:
> Ditto.

Done.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:390: throw new HttpException("Failed to parse header
value y [$s]");
On 2012/10/17 11:26:43, Mads Ager wrote:
> Accidental edit?

Done.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:1746: _Credentials cr =
On 2012/10/17 11:26:43, Mads Ager wrote:
> What if we authorize once and the credentials are added to the credentials
> store. Then there is a password change and another request with the same
client.
> That could get us in here again with another challenge? However, we will use
the
> cached credentials without calling authenticate again? Shouldn't we add a
> mechanism to remove the credentials and reauthorize if they did not give
access?
> 

Added a used flag on credentials in the cache so that is credentials which have
been send, but come back with 401 will be removed and re-authorization will
happen.

https://codereview.chromium.org/11118021/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2356: if (prev != null) return prev;
On 2012/10/17 11:26:43, Mads Ager wrote:
> I would use a two-space indent for these three lines.

Done.

[Mads Ager (google), 2012-10-26 11:07:02]

LGTM!

My main concern here was the reauthentication support which is there now. Good
point about the credentials store to avoid extra round trips for every
authenticated request to the same realm.

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1750: // already been used. If is has already been
used it must now be
is -> it

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1773: // TODO(sgesse): Support digest.
Should we throw an Unsupported exception for now or something similar.

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2501: // TODO(sgjesse): Implement!!!
Throw unsupported exception?

[Søren Gjesse, 2012-10-26 12:59:01]

PTAL

Changes HttpClient.authenticate to be async.

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1750: // already been used. If is has already been
used it must now be
On 2012/10/26 11:07:02, Mads Ager wrote:
> is -> it

Done.

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1773: // TODO(sgesse): Support digest.
On 2012/10/26 11:07:02, Mads Ager wrote:
> Should we throw an Unsupported exception for now or something similar.

As it is it will just fall through and pass the status code 401 to the client,
which I think is fine.

https://codereview.chromium.org/11118021/diff/6001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2501: // TODO(sgjesse): Implement!!!
On 2012/10/26 11:07:02, Mads Ager wrote:
> Throw unsupported exception?

Done.

[Anders Johnsen, 2012-10-26 13:04:50]

LGTM

https://codereview.chromium.org/11118021/diff/11001/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/11001/runtime/bin/http.dart#new...
runtime/bin/http.dart:719: * provided the [Futire] should complete with [false].
If
Futire->Future

[Søren Gjesse, 2012-10-26 13:41:06]

https://codereview.chromium.org/11118021/diff/11001/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/11001/runtime/bin/http.dart#new...
runtime/bin/http.dart:719: * provided the [Futire] should complete with [false].
If
On 2012/10/26 13:04:50, ajohnsen wrote:
> Futire->Future

Done.

[Mads Ager (google), 2012-10-26 13:53:20]

LGTM

https://codereview.chromium.org/11118021/diff/16001/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://codereview.chromium.org/11118021/diff/16001/runtime/bin/http.dart#new...
runtime/bin/http.dart:757: * authentication have been resolved. If credentials
cannot be
have -> has