code review 4543050: http: add Request.AddBasicAuth method

http: add Request.SetBasicAuth method

[bradfitz, 2011-05-13 19:16:58]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[dsymonds, 2011-05-13 19:33:57]

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:486: func (r *Request) AddBasicAuth(username, password
string) {
Maybe it should just take the string. It's easy enough to compose username +
":"+ password, and it would provide the flexibility to use other auth schemes. I
think I've seen some used elsewhere.

[bradfitz, 2011-05-13 19:36:47]

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:486: func (r *Request) AddBasicAuth(username, password
string) {
On 2011/05/13 19:33:57, dsymonds wrote:
> Maybe it should just take the string. It's easy enough to compose username +
> ":"+ password, and it would provide the flexibility to use other auth schemes.
I
> think I've seen some used elsewhere.

That feels weird.  I don't want to document "oh and hey for no reason you have
to concat your two logical parameters with a colon".

If we want to add another (e.g. Digest auth) we'd add a method for that, with
just the things Digest would need.  (including the Response from the previous
request with the Nonce)

[dsymonds, 2011-05-13 19:44:32]

LGTM

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:486: func (r *Request) AddBasicAuth(username, password
string) {
On 2011/05/13 19:36:47, bradfitz wrote:
> On 2011/05/13 19:33:57, dsymonds wrote:
> > Maybe it should just take the string. It's easy enough to compose username +
> > ":"+ password, and it would provide the flexibility to use other auth
schemes.
> I
> > think I've seen some used elsewhere.
> 
> That feels weird.  I don't want to document "oh and hey for no reason you have
> to concat your two logical parameters with a colon".
> 
> If we want to add another (e.g. Digest auth) we'd add a method for that, with
> just the things Digest would need.  (including the Response from the previous
> request with the Nonce) 

Well, Digest was something different again, but given that I can't find an
example, I'll assume my memory is faulty.

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:490: r.Header.Set("Authorization", "Basic "+string(buf))
Should this be Add instead of Set? It disagrees with the doc comment.

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request_test.go
File src/pkg/http/request_test.go (right):

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:180: t.Errorf("got header %q; want %q", g, e)
s/;/,/

[bradfitz, 2011-05-13 19:47:55]

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:486: func (r *Request) AddBasicAuth(username, password
string) {
On 2011/05/13 19:44:32, dsymonds wrote:
> On 2011/05/13 19:36:47, bradfitz wrote:
> > On 2011/05/13 19:33:57, dsymonds wrote:
> > > Maybe it should just take the string. It's easy enough to compose username
+
> > > ":"+ password, and it would provide the flexibility to use other auth
> schemes.
> > I
> > > think I've seen some used elsewhere.
> > 
> > That feels weird.  I don't want to document "oh and hey for no reason you
have
> > to concat your two logical parameters with a colon".
> > 
> > If we want to add another (e.g. Digest auth) we'd add a method for that,
with
> > just the things Digest would need.  (including the Response from the
previous
> > request with the Nonce) 
> 
> Well, Digest was something different again, but given that I can't find an
> example, I'll assume my memory is faulty.

There's Amazon s3 which uses a Base64 Authorization header:

http://camlistore.org/code/?p=camlistore.git;a=blob;f=lib/go/camli/misc/amazo...

But it needs a whole *Request, not just a username/password.

http://codereview.appspot.com/4543050/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:490: r.Header.Set("Authorization", "Basic "+string(buf))
On 2011/05/13 19:44:32, dsymonds wrote:
> Should this be Add instead of Set? It disagrees with the doc comment.

Oh, yes.

[rsc1, 2011-05-13 19:48:33]

s/AddBasicAuth/BasicAuth/

You are using Header.Set, not Header.Add, anyway.

[bradfitz, 2011-05-13 19:50:04]

I'm worried that sounds like an accessor, which would also be reasonable
helper method.

SetBasicAuth?

On Fri, May 13, 2011 at 12:48 PM, Russ Cox <rsc@google.com> wrote:

> s/AddBasicAuth/BasicAuth/
>
> You are using Header.Set, not Header.Add, anyway.
>

[rsc1, 2011-05-13 19:52:29]

Sure, SetBasicAuth.

[bradfitz, 2011-05-13 22:43:49]

*** Submitted as http://code.google.com/p/go/source/detail?r=aac9be4fefa1 ***

http: add Request.SetBasicAuth method

R=golang-dev, dsymonds, rsc
CC=golang-dev
http://codereview.appspot.com/4543050