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 14684560..87e27f2e 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 ec82b469..3107537e 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 5425b1da..b8c4d41e 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 ea0de425..79e9422c 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 39318d1a..7ae43e25 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 d579e878..2fd160de 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 4547336a..c947b67d 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 f7b7c8c4..d3985067 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