From a0fdfb9d4dd6e84703d155d9c1f3c72d52324814 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Thu, 23 Jul 2015 19:39:56 -0700 Subject: [PATCH 1/2] Simplify auth.Challenge interface to SetHeaders This removes the erroneous http.Handler interface in favor a simple SetHeaders method that only operattes on the response. Several unnecessary uses of pointer types were also fixed up. Signed-off-by: Stephen J Day --- registry/auth/auth.go | 12 ++++++------ registry/auth/htpasswd/access.go | 10 ++++++---- registry/auth/htpasswd/access_test.go | 2 +- registry/auth/silly/access.go | 7 +++++-- registry/auth/silly/access_test.go | 2 +- registry/auth/token/accesscontroller.go | 20 ++++++++------------ registry/handlers/app.go | 2 +- 7 files changed, 28 insertions(+), 27 deletions(-) diff --git a/registry/auth/auth.go b/registry/auth/auth.go index 3107537e3..7ae2a157d 100644 --- a/registry/auth/auth.go +++ b/registry/auth/auth.go @@ -61,12 +61,12 @@ type Access struct { // header values based on the error. type Challenge interface { error - // ServeHTTP prepares the request to conduct the appropriate challenge - // 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) + + // SetHeaders prepares the request to conduct a challenge response by + // adding the an HTTP challenge header on the response message. Callers + // are expected to set the appropriate HTTP status code (e.g. 401) + // themselves. + SetHeaders(w http.ResponseWriter) } // AccessController controls access to registry resources based on a request diff --git a/registry/auth/htpasswd/access.go b/registry/auth/htpasswd/access.go index b8c4d41e4..bb153f4b5 100644 --- a/registry/auth/htpasswd/access.go +++ b/registry/auth/htpasswd/access.go @@ -87,12 +87,14 @@ type challenge struct { err error } -func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { - header := fmt.Sprintf("Basic realm=%q", ch.realm) - w.Header().Set("WWW-Authenticate", header) +var _ auth.Challenge = challenge{} + +// SetHeaders sets the basic challenge header on the response. +func (ch challenge) SetHeaders(w http.ResponseWriter) { + w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q", ch.realm)) } -func (ch *challenge) Error() string { +func (ch challenge) Error() string { return fmt.Sprintf("basic authentication challenge: %#v", ch) } diff --git a/registry/auth/htpasswd/access_test.go b/registry/auth/htpasswd/access_test.go index 79e9422ca..db0405475 100644 --- a/registry/auth/htpasswd/access_test.go +++ b/registry/auth/htpasswd/access_test.go @@ -48,7 +48,7 @@ func TestBasicAccessController(t *testing.T) { if err != nil { switch err := err.(type) { case auth.Challenge: - err.ServeHTTP(w, r) + err.SetHeaders(w) w.WriteHeader(http.StatusUnauthorized) return default: diff --git a/registry/auth/silly/access.go b/registry/auth/silly/access.go index 7ae43e25d..7d6efb079 100644 --- a/registry/auth/silly/access.go +++ b/registry/auth/silly/access.go @@ -75,7 +75,10 @@ type challenge struct { scope string } -func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { +var _ auth.Challenge = challenge{} + +// SetHeaders sets a simple bearer challenge on the response. +func (ch challenge) SetHeaders(w http.ResponseWriter) { header := fmt.Sprintf("Bearer realm=%q,service=%q", ch.realm, ch.service) if ch.scope != "" { @@ -85,7 +88,7 @@ func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("WWW-Authenticate", header) } -func (ch *challenge) Error() string { +func (ch challenge) Error() string { return fmt.Sprintf("silly authentication challenge: %#v", ch) } diff --git a/registry/auth/silly/access_test.go b/registry/auth/silly/access_test.go index 2fd160de9..8b5ecb801 100644 --- a/registry/auth/silly/access_test.go +++ b/registry/auth/silly/access_test.go @@ -21,7 +21,7 @@ func TestSillyAccessController(t *testing.T) { if err != nil { switch err := err.(type) { case auth.Challenge: - err.ServeHTTP(w, r) + err.SetHeaders(w) w.WriteHeader(http.StatusUnauthorized) return default: diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go index c947b67df..0549f8eff 100644 --- a/registry/auth/token/accesscontroller.go +++ b/registry/auth/token/accesscontroller.go @@ -82,20 +82,22 @@ type authChallenge struct { accessSet accessSet } +var _ auth.Challenge = authChallenge{} + // Error returns the internal error string for this authChallenge. -func (ac *authChallenge) Error() string { +func (ac authChallenge) Error() string { return ac.err.Error() } // Status returns the HTTP Response Status Code for this authChallenge. -func (ac *authChallenge) Status() int { +func (ac authChallenge) Status() int { return http.StatusUnauthorized } // challengeParams constructs the value to be used in // the WWW-Authenticate response challenge header. // See https://tools.ietf.org/html/rfc6750#section-3 -func (ac *authChallenge) challengeParams() string { +func (ac authChallenge) challengeParams() string { str := fmt.Sprintf("Bearer realm=%q,service=%q", ac.realm, ac.service) if scope := ac.accessSet.scopeParam(); scope != "" { @@ -111,15 +113,9 @@ func (ac *authChallenge) challengeParams() string { return str } -// SetHeader sets the WWW-Authenticate value for the given header. -func (ac *authChallenge) SetHeader(header http.Header) { - header.Add("WWW-Authenticate", ac.challengeParams()) -} - -// ServeHttp handles writing the challenge response -// by setting the challenge header. -func (ac *authChallenge) ServeHTTP(w http.ResponseWriter, r *http.Request) { - ac.SetHeader(w.Header()) +// SetChallenge sets the WWW-Authenticate value for the response. +func (ac authChallenge) SetHeaders(w http.ResponseWriter) { + w.Header().Add("WWW-Authenticate", ac.challengeParams()) } // accessController implements the auth.AccessController interface. diff --git a/registry/handlers/app.go b/registry/handlers/app.go index f61b2c1e0..8395ea656 100644 --- a/registry/handlers/app.go +++ b/registry/handlers/app.go @@ -502,7 +502,7 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont switch err := err.(type) { case auth.Challenge: // Add the appropriate WWW-Auth header - err.ServeHTTP(w, r) + err.SetHeaders(w) if err := errcode.ServeJSON(w, v2.ErrorCodeUnauthorized.WithDetail(accessRecords)); err != nil { ctxu.GetLogger(context).Errorf("error serving error json: %v (from %v)", err, context.Errors) From d31f9fd5b1abb7c5462230db163fb12e9c770e37 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Thu, 23 Jul 2015 19:48:47 -0700 Subject: [PATCH 2/2] auth.AccessController interface now uses distribution/context Signed-off-by: Stephen J Day --- registry/auth/auth.go | 2 +- registry/auth/htpasswd/access.go | 7 +++---- registry/auth/silly/access.go | 5 ++--- registry/auth/token/accesscontroller.go | 5 ++--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/registry/auth/auth.go b/registry/auth/auth.go index 7ae2a157d..862c8d28c 100644 --- a/registry/auth/auth.go +++ b/registry/auth/auth.go @@ -34,7 +34,7 @@ import ( "fmt" "net/http" - "golang.org/x/net/context" + "github.com/docker/distribution/context" ) // UserInfo carries information about diff --git a/registry/auth/htpasswd/access.go b/registry/auth/htpasswd/access.go index bb153f4b5..5ac3d84a7 100644 --- a/registry/auth/htpasswd/access.go +++ b/registry/auth/htpasswd/access.go @@ -11,9 +11,8 @@ import ( "net/http" "os" - ctxu "github.com/docker/distribution/context" + "github.com/docker/distribution/context" "github.com/docker/distribution/registry/auth" - "golang.org/x/net/context" ) var ( @@ -57,7 +56,7 @@ func newAccessController(options map[string]interface{}) (auth.AccessController, } func (ac *accessController) Authorized(ctx context.Context, accessRecords ...auth.Access) (context.Context, error) { - req, err := ctxu.GetRequest(ctx) + req, err := context.GetRequest(ctx) if err != nil { return nil, err } @@ -71,7 +70,7 @@ func (ac *accessController) Authorized(ctx context.Context, accessRecords ...aut } if err := ac.htpasswd.authenticateUser(username, password); err != nil { - ctxu.GetLogger(ctx).Errorf("error authenticating user %q: %v", username, err) + context.GetLogger(ctx).Errorf("error authenticating user %q: %v", username, err) return nil, &challenge{ realm: ac.realm, err: ErrAuthenticationFailure, diff --git a/registry/auth/silly/access.go b/registry/auth/silly/access.go index 7d6efb079..2b801d946 100644 --- a/registry/auth/silly/access.go +++ b/registry/auth/silly/access.go @@ -12,9 +12,8 @@ import ( "net/http" "strings" - ctxu "github.com/docker/distribution/context" + "github.com/docker/distribution/context" "github.com/docker/distribution/registry/auth" - "golang.org/x/net/context" ) // accessController provides a simple implementation of auth.AccessController @@ -44,7 +43,7 @@ func newAccessController(options map[string]interface{}) (auth.AccessController, // Authorized simply checks for the existence of the authorization header, // responding with a bearer challenge if it doesn't exist. func (ac *accessController) Authorized(ctx context.Context, accessRecords ...auth.Access) (context.Context, error) { - req, err := ctxu.GetRequest(ctx) + req, err := context.GetRequest(ctx) if err != nil { return nil, err } diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go index 0549f8eff..5b1ff7caa 100644 --- a/registry/auth/token/accesscontroller.go +++ b/registry/auth/token/accesscontroller.go @@ -11,10 +11,9 @@ import ( "os" "strings" - ctxu "github.com/docker/distribution/context" + "github.com/docker/distribution/context" "github.com/docker/distribution/registry/auth" "github.com/docker/libtrust" - "golang.org/x/net/context" ) // accessSet maps a typed, named resource to @@ -220,7 +219,7 @@ func (ac *accessController) Authorized(ctx context.Context, accessItems ...auth. accessSet: newAccessSet(accessItems...), } - req, err := ctxu.GetRequest(ctx) + req, err := context.GetRequest(ctx) if err != nil { return nil, err }