From c4eb195cc1c4061f7ac9b233297a2abae8cc8cb1 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Tue, 16 Jun 2015 18:57:47 -0700 Subject: [PATCH] Move challenge http status code logic See: https://github.com/docker/distribution/blob/3ea67df37383dd6941908a9949ab55c7073ece5b/registry/handlers/app.go#L498 Per the comment on line 498, this moves the logic of setting the http status code into the serveJSON func, leaving the auth.Challenge.ServeHTTP() func to just set the auth challenge header. Signed-off-by: Doug Davis --- registry/api/v2/errors.go | 2 +- registry/auth/auth.go | 8 ++++---- registry/auth/htpasswd/access.go | 1 - registry/auth/htpasswd/access_test.go | 1 + registry/auth/silly/access.go | 1 - registry/auth/silly/access_test.go | 1 + registry/auth/token/accesscontroller.go | 3 +-- registry/handlers/app.go | 11 +---------- 8 files changed, 9 insertions(+), 19 deletions(-) diff --git a/registry/api/v2/errors.go b/registry/api/v2/errors.go index 14684560a..87e27f2e4 100644 --- a/registry/api/v2/errors.go +++ b/registry/api/v2/errors.go @@ -24,7 +24,7 @@ var ( Description: `The access controller denied access for the operation on a resource. Often this will be accompanied by a 401 Unauthorized response status.`, - HTTPStatusCode: http.StatusForbidden, + HTTPStatusCode: http.StatusUnauthorized, }) // ErrorCodeDigestInvalid is returned when uploading a blob if the diff --git a/registry/auth/auth.go b/registry/auth/auth.go index ec82b4697..3107537e3 100644 --- a/registry/auth/auth.go +++ b/registry/auth/auth.go @@ -62,10 +62,10 @@ type Access struct { type Challenge interface { error // ServeHTTP prepares the request to conduct the appropriate challenge - // response. For most implementations, simply calling ServeHTTP should be - // sufficient. Because no body is written, users may write a custom body after - // calling ServeHTTP, but any headers must be written before the call and may - // be overwritten. + // response by adding the appropriate HTTP challenge header on the response + // message. Callers are expected to set the appropriate HTTP status code + // (e.g. 401) themselves. Because no body is written, users may write a + // custom body after calling ServeHTTP. ServeHTTP(w http.ResponseWriter, r *http.Request) } diff --git a/registry/auth/htpasswd/access.go b/registry/auth/htpasswd/access.go index 5425b1dab..b8c4d41e4 100644 --- a/registry/auth/htpasswd/access.go +++ b/registry/auth/htpasswd/access.go @@ -90,7 +90,6 @@ type challenge struct { func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { header := fmt.Sprintf("Basic realm=%q", ch.realm) w.Header().Set("WWW-Authenticate", header) - w.WriteHeader(http.StatusUnauthorized) } func (ch *challenge) Error() string { diff --git a/registry/auth/htpasswd/access_test.go b/registry/auth/htpasswd/access_test.go index ea0de425b..79e9422ca 100644 --- a/registry/auth/htpasswd/access_test.go +++ b/registry/auth/htpasswd/access_test.go @@ -49,6 +49,7 @@ func TestBasicAccessController(t *testing.T) { switch err := err.(type) { case auth.Challenge: err.ServeHTTP(w, r) + w.WriteHeader(http.StatusUnauthorized) return default: t.Fatalf("unexpected error authorizing request: %v", err) diff --git a/registry/auth/silly/access.go b/registry/auth/silly/access.go index 39318d1a3..7ae43e25d 100644 --- a/registry/auth/silly/access.go +++ b/registry/auth/silly/access.go @@ -83,7 +83,6 @@ func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { } w.Header().Set("WWW-Authenticate", header) - w.WriteHeader(http.StatusUnauthorized) } func (ch *challenge) Error() string { diff --git a/registry/auth/silly/access_test.go b/registry/auth/silly/access_test.go index d579e8780..2fd160de9 100644 --- a/registry/auth/silly/access_test.go +++ b/registry/auth/silly/access_test.go @@ -22,6 +22,7 @@ func TestSillyAccessController(t *testing.T) { switch err := err.(type) { case auth.Challenge: err.ServeHTTP(w, r) + w.WriteHeader(http.StatusUnauthorized) return default: t.Fatalf("unexpected error authorizing request: %v", err) diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go index 4547336a4..c947b67df 100644 --- a/registry/auth/token/accesscontroller.go +++ b/registry/auth/token/accesscontroller.go @@ -117,10 +117,9 @@ func (ac *authChallenge) SetHeader(header http.Header) { } // ServeHttp handles writing the challenge response -// by setting the challenge header and status code. +// by setting the challenge header. func (ac *authChallenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { ac.SetHeader(w.Header()) - w.WriteHeader(ac.Status()) } // accessController implements the auth.AccessController interface. diff --git a/registry/handlers/app.go b/registry/handlers/app.go index f7b7c8c4b..d39850670 100644 --- a/registry/handlers/app.go +++ b/registry/handlers/app.go @@ -495,16 +495,7 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont if err != nil { switch err := err.(type) { case auth.Challenge: - // NOTE(duglin): - // Since err.ServeHTTP will set the HTTP status code for us - // we need to set the content-type here. The serveJSON - // func will try to do it but it'll be too late at that point. - // I would have have preferred to just have the auth.Challenge - // ServerHTTP func just add the WWW-Authenticate header and let - // serveJSON set the HTTP status code and content-type but I wasn't - // sure if that's an ok design change. STEVVOOE ? - w.Header().Set("Content-Type", "application/json; charset=utf-8") - + // Add the appropriate WWW-Auth header err.ServeHTTP(w, r) var errs errcode.Errors