diff --git a/basculehttp/challenge.go b/basculehttp/challenge.go index 35fc336..0cd30a7 100644 --- a/basculehttp/challenge.go +++ b/basculehttp/challenge.go @@ -15,6 +15,16 @@ const ( // // This value is used by default when no header is supplied to Challenges.WriteHeader. WWWAuthenticateHeader = "WWW-Authenticate" + + // RealmParameter is the name of the reserved parameter for realm. + RealmParameter = "realm" + + // CharsetParameter is the name of the reserved parameter for charset. + CharsetParameter = "charset" + + // Token68Parameter is the name of the reserved attribute for token68 encoding. + // This package does not support token68 encoding. + Token68Parameter = "token68" ) var ( @@ -29,78 +39,153 @@ var ( // ErrReservedChallengeParameter indicates that an attempt was made to add a // challenge auth parameter that was reserved by the RFC. + // + // Since this package explicitly does not support token68, trying to set a token68 + // parameter results in this error. ErrReservedChallengeParameter = errors.New("Reserved challenge auth parameter") ) -// reservedChallengeParameterNames holds the names of reserved challenge auth parameters -// that cannot be added to a ChallengeParameters. -var reservedChallengeParameterNames = map[string]bool{ - "realm": true, - "token68": true, +// blankOrWhitespace tests if v is blank or has any whitespace. These +// are disallowed by RFC 7235. +func blankOrWhitespace(v string) bool { + switch { + case len(v) == 0: + return true + + case fastContainsSpace(v): + return true + + default: + return false + } } // ChallengeParameters holds the set of parameters. The zero value of this // type is ready to use. This type handles writing parameters as well as // provides commonly used parameter names for convenience. +// +// It is not required by spec, but any realm parameter is always placed first. +// Additionally, the output of parameters is consistently ordered and will always +// follow the order in which the parameters were set, realm being the exception. +// +// Token68 is not supported. Any attempt to set that parameter will result +// in an error. type ChallengeParameters struct { + // realm is a reserved parameter. the spec doesn't require it to be + // first, but this package always renders it first if supplied. so it's + // called out as a separate struct field. + realm string + names, values []string - byName map[string]int // the parameter index + byName map[string]int // the parameter indices } // Len returns the number of name/value pairs contained in these parameters. -func (cp *ChallengeParameters) Len() int { - return len(cp.names) +func (cp *ChallengeParameters) Len() (c int) { + c = len(cp.names) + if len(cp.realm) > 0 { + c++ + } + + return +} + +// empty is a faster check for emptiness than Len() == 0. +func (cp *ChallengeParameters) empty() bool { + return len(cp.realm) == 0 && len(cp.names) == 0 +} + +// unsafeSet performs no validation on the name or value. This method must +// be called after validation checks or in a context where the name and +// value are known to be safe. This method also doesn't handle special +// parameters, like the realm. +func (cp *ChallengeParameters) unsafeSet(name, value string) { + if i, exists := cp.byName[name]; exists { + cp.values[i] = value + } else if len(value) > 0 { + if cp.byName == nil { + cp.byName = make(map[string]int) + } + + cp.byName[name] = len(cp.names) + cp.names = append(cp.names, name) + cp.values = append(cp.values, value) + } } // Set sets the value of a parameter. If a parameter was already set, it is -// ovewritten. +// ovewritten. The realm may be set via this method, but token68 will be +// rejected as invalid. // -// If the parameter name is invalid, this method raises an error. +// This method returns ErrInvalidChallengeParameter if passed a name or a value +// that is blank or contains whitespace. func (cp *ChallengeParameters) Set(name, value string) (err error) { switch { - case len(name) == 0: + case blankOrWhitespace(name): err = ErrInvalidChallengeParameter - case fastContainsSpace(name): + case blankOrWhitespace(value): err = ErrInvalidChallengeParameter - case reservedChallengeParameterNames[name]: + case Token68Parameter == strings.ToLower(name): err = ErrReservedChallengeParameter + case RealmParameter == strings.ToLower(name): + cp.realm = value + default: - if i, exists := cp.byName[name]; exists { - cp.values[i] = value - } else { - if cp.byName == nil { - cp.byName = make(map[string]int) - } - - cp.byName[name] = len(cp.names) - cp.names = append(cp.names, name) - cp.values = append(cp.values, value) - } + cp.unsafeSet(name, value) + } + + return +} + +// SetRealm sets a realm auth parameter. The value cannot be blank or +// contain any whitespace. +func (cp *ChallengeParameters) SetRealm(value string) (err error) { + if blankOrWhitespace(value) { + err = ErrInvalidChallengeParameter + } else { + cp.realm = value + } + + return +} + +// SetCharset sets a charset auth parameter. Basic auth is the main scheme +// that uses this. The value cannot be blank or contain any whitespace. +func (cp *ChallengeParameters) SetCharset(value string) (err error) { + if blankOrWhitespace(value) { + err = ErrInvalidChallengeParameter + } else { + cp.unsafeSet(CharsetParameter, value) } return } -// Charset sets a charset auth parameter. Basic auth is the main scheme -// that uses this. -func (cp *ChallengeParameters) Charset(value string) error { - return cp.Set("charset", value) +func writeParameter(dst *strings.Builder, name, value string) { + dst.WriteString(name) + dst.WriteString(`="`) + dst.WriteString(value) + dst.WriteRune('"') } // Write formats this challenge to the given builder. -func (cp *ChallengeParameters) Write(o *strings.Builder) { +func (cp *ChallengeParameters) Write(dst *strings.Builder) { + first := true + if len(cp.realm) > 0 { + writeParameter(dst, RealmParameter, cp.realm) + first = false + } + for i := 0; i < len(cp.names); i++ { - if i > 0 { - o.WriteString(", ") + if !first { + dst.WriteString(", ") } - o.WriteString(cp.names[i]) - o.WriteString(`="`) - o.WriteString(cp.values[i]) - o.WriteRune('"') + writeParameter(dst, cp.names[i], cp.values[i]) + first = false } } @@ -112,18 +197,19 @@ func (cp *ChallengeParameters) String() string { } // NewChallengeParameters creates a ChallengeParameters from a sequence of name/value pairs. -// The strings are expected to be in name, value, name, value, ... sequence. If the number -// of strings is odd, then the last parameter will have a blank value. +// The strings are expected to be in name1, value1, name2, value2, ..., nameN, valueN sequence. +// If the number of strings is odd, this method returns an error. If any duplicate names +// occur, only the last name/value pair is used. // // If any error occurs while setting parameters, execution is halted and that // error is returned. func NewChallengeParameters(s ...string) (cp ChallengeParameters, err error) { + if len(s)%2 != 0 { + err = errors.New("Odd number of challenge parameters") + } + for i, j := 0, 1; err == nil && i < len(s); i, j = i+2, j+2 { - if j < len(s) { - err = cp.Set(s[i], s[j]) - } else { - err = cp.Set(s[i], "") - } + err = cp.Set(s[i], s[j]) } return @@ -134,14 +220,6 @@ type Challenge struct { // Scheme is the name of scheme supplied in the challenge. This field is required. Scheme Scheme - // Realm is the name of the realm for the challenge. This field is - // optional, but it is HIGHLY recommended to set it to something useful - // to a client. - Realm string - - // Token68 controls whether the token68 flag is written in the challenge. - Token68 bool - // Parameters are the optional auth parameters. Parameters ChallengeParameters } @@ -158,18 +236,8 @@ func (c Challenge) Write(o *strings.Builder) (err error) { err = ErrInvalidChallengeScheme default: - o.WriteString(s) - if len(c.Realm) > 0 { - o.WriteString(` realm="`) - o.WriteString(c.Realm) - o.WriteRune('"') - } - - if c.Token68 { - o.WriteString(" token68") - } - - if c.Parameters.Len() > 0 { + o.WriteString(string(c.Scheme)) + if !c.Parameters.empty() { o.WriteRune(' ') c.Parameters.Write(o) } @@ -182,14 +250,15 @@ func (c Challenge) Write(o *strings.Builder) (err error) { // // Although realm is optional, it is HIGHLY recommended to set it to something // recognizable for a client. -func NewBasicChallenge(realm string, UTF8 bool) (c Challenge, err error) { +func NewBasicChallenge(realm string, UTF8 bool) (c Challenge) { c = Challenge{ Scheme: SchemeBasic, - Realm: realm, } + // ignore errors, as this function allows realm to be empty. + c.Parameters.SetRealm(realm) if UTF8 { - err = c.Parameters.Charset("UTF-8") + c.Parameters.SetCharset("UTF-8") } return @@ -205,20 +274,25 @@ func (chs Challenges) Append(ch ...Challenge) Challenges { return append(chs, ch...) } -// WriteHeader inserts one Http authenticate header per challenge in this set. +// WriteHeader write one WWWAuthenticateHeader for each challenge in this +// set. +// +// If any challenge returns an error during formatting, execution is +// halted and that error is returned. +func (chs Challenges) WriteHeader(dst http.Header) error { + return chs.WriteHeaderCustom(dst, WWWAuthenticateHeader) +} + +// WriteHeaderCustom inserts one HTTP authenticate header per challenge in this set. // If this set is empty, the given http.Header is not modified. // // The name is used as the header name for each header this method writes. -// Typically, this will be WWW-Authenticate or Proxy-Authenticate. If name -// is blank, WWWAuthenticateHeaderName is used. +// Typically, this will be WWW-Authenticate or Proxy-Authenticate. The name +// parameter is required. // // If any challenge returns an error during formatting, execution is // halted and that error is returned. -func (chs Challenges) WriteHeader(name string, h http.Header) error { - if len(name) == 0 { - name = WWWAuthenticateHeader - } - +func (chs Challenges) WriteHeaderCustom(dst http.Header, name string) error { var o strings.Builder for _, ch := range chs { err := ch.Write(&o) @@ -226,7 +300,7 @@ func (chs Challenges) WriteHeader(name string, h http.Header) error { return err } - h.Add(name, o.String()) + dst.Add(name, o.String()) o.Reset() } diff --git a/basculehttp/challenge_test.go b/basculehttp/challenge_test.go index 93bb231..4898a2b 100644 --- a/basculehttp/challenge_test.go +++ b/basculehttp/challenge_test.go @@ -24,45 +24,154 @@ func (suite *ChallengeTestSuite) newValidParameters(s ...string) ChallengeParame return cp } -// newValidBasic uses NewBasicChallenge to create a Challenge and asserts -// that no error occurred. -func (suite *ChallengeTestSuite) newValidBasic(realm string, UTF8 bool) Challenge { - c, err := NewBasicChallenge(realm, UTF8) - suite.Require().NoError(err) - return c +func (suite *ChallengeTestSuite) testChallengeParametersInvalid() { + testCases := []struct { + name, value string + }{ + {}, // both blank + {"valid", ""}, + {"", "valid"}, + {"token68", "value"}, // reserved + {"embedded whitespace", "value"}, + {"name", "embedded whitespace"}, + } + + for i, testCase := range testCases { + suite.Run(strconv.Itoa(i), func() { + var cp ChallengeParameters + suite.Error(cp.Set(testCase.name, testCase.value)) + }) + } } -func (suite *ChallengeTestSuite) TestChallengeParameters() { +func (suite *ChallengeTestSuite) testChallengeParametersEmpty() { + var cp ChallengeParameters + suite.Zero(cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Empty(o.String()) + suite.Empty(cp.String()) +} + +func (suite *ChallengeTestSuite) testChallengeParametersValid() { + testCases := []struct { + namesAndValues []string + expectedFormat string + }{ + { + namesAndValues: []string{"realm", "test"}, + expectedFormat: `realm="test"`, + }, + { + namesAndValues: []string{"nonce", "this_is_a_nonce", "qos", "a,b,c"}, + expectedFormat: `nonce="this_is_a_nonce", qos="a,b,c"`, + }, + { + namesAndValues: []string{"nonce", "this_is_a_nonce", "realm", "test@example.com", "qos", "a,b,c"}, + expectedFormat: `realm="test@example.com", nonce="this_is_a_nonce", qos="a,b,c"`, // realm is always first + }, + } + + for i, testCase := range testCases { + suite.Run(strconv.Itoa(i), func() { + cp, err := NewChallengeParameters(testCase.namesAndValues...) + suite.Require().NoError(err) + suite.Equal(len(testCase.namesAndValues)/2, cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Equal(testCase.expectedFormat, o.String()) + suite.Equal(testCase.expectedFormat, cp.String()) + }) + } +} + +func (suite *ChallengeTestSuite) testChallengeParametersDuplicate() { + var cp ChallengeParameters + suite.NoError(cp.Set("name", "value1")) + suite.NoError(cp.Set("another", "somevalue")) + suite.NoError(cp.Set("name", "value2")) + + var o strings.Builder + cp.Write(&o) + suite.Equal( + `name="value2", another="somevalue"`, + o.String(), + ) + + suite.Equal( + `name="value2", another="somevalue"`, + cp.String(), + ) + + suite.Equal(2, cp.Len()) +} + +func (suite *ChallengeTestSuite) testChallengeParametersSetRealm() { suite.Run("Invalid", func() { - badParameterNames := []string{ - "", - " ", - "this is not ok", - "neither\tis\bthis", - "token68", // reserved - "realm", // reserved - } - - for i, bad := range badParameterNames { - suite.Run(strconv.Itoa(i), func() { - var cp ChallengeParameters - suite.Error(cp.Set(bad, "value")) - }) - } + var cp ChallengeParameters + suite.Error(cp.SetRealm("embedded whitespace")) + suite.Zero(cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Empty(o.String()) + suite.Empty(cp.String()) + }) + + suite.Run("Valid", func() { + var cp ChallengeParameters + suite.NoError(cp.SetRealm("myrealm")) + suite.Equal(1, cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Equal(`realm="myrealm"`, o.String()) + suite.Equal(`realm="myrealm"`, cp.String()) + }) +} + +func (suite *ChallengeTestSuite) testChallengeParametersSetCharset() { + suite.Run("Invalid", func() { + var cp ChallengeParameters + suite.Error(cp.SetCharset("embedded whitespace")) + suite.Zero(cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Empty(o.String()) + suite.Empty(cp.String()) }) - suite.Run("Duplicate", func() { + suite.Run("Valid", func() { var cp ChallengeParameters - suite.NoError(cp.Set("name", "value1")) - suite.NoError(cp.Set("another", "somevalue")) - suite.NoError(cp.Set("name", "value2")) - suite.Equal( - `name="value2", another="somevalue"`, - cp.String(), - ) + suite.NoError(cp.SetCharset("UTF-8")) + suite.Equal(1, cp.Len()) + + var o strings.Builder + cp.Write(&o) + suite.Equal(`charset="UTF-8"`, o.String()) + suite.Equal(`charset="UTF-8"`, cp.String()) }) } +func (suite *ChallengeTestSuite) testChallengeParametersOddParameterCount() { + cp, err := NewChallengeParameters("1", "2", "3") + suite.Error(err) + suite.Zero(cp.Len()) +} + +func (suite *ChallengeTestSuite) TestChallengeParameters() { + suite.Run("Invalid", suite.testChallengeParametersInvalid) + suite.Run("Empty", suite.testChallengeParametersEmpty) + suite.Run("Valid", suite.testChallengeParametersValid) + suite.Run("Duplicate", suite.testChallengeParametersDuplicate) + suite.Run("SetRealm", suite.testChallengeParametersSetRealm) + suite.Run("SetCharset", suite.testChallengeParametersSetCharset) + suite.Run("OddParameterCount", suite.testChallengeParametersOddParameterCount) +} + func (suite *ChallengeTestSuite) testChallengeValid() { testCases := []struct { challenge Challenge @@ -76,46 +185,34 @@ func (suite *ChallengeTestSuite) testChallengeValid() { }, { challenge: Challenge{ - Scheme: SchemeBasic, - Realm: "test", + Scheme: SchemeBasic, + Parameters: suite.newValidParameters(RealmParameter, "test"), }, expectedFormat: `Basic realm="test"`, }, { - challenge: suite.newValidBasic("", false), + challenge: NewBasicChallenge("", false), expectedFormat: `Basic`, }, { - challenge: suite.newValidBasic("test", false), + challenge: NewBasicChallenge("test", false), expectedFormat: `Basic realm="test"`, }, { - challenge: suite.newValidBasic("test@example.com", true), - expectedFormat: `Basic realm="test@example.com" charset="UTF-8"`, + challenge: NewBasicChallenge("test@example.com", true), + expectedFormat: `Basic realm="test@example.com", charset="UTF-8"`, }, { challenge: Challenge{ Scheme: Scheme("Custom"), - Realm: "test@example.com", Parameters: suite.newValidParameters( - "nonce", "this is a nonce", - "qop", "a, b, c", + "nonce", "this_is_a_nonce", + "qop", "a,b,c", + RealmParameter, "test@example.com", // this will get placed first "custom", "1234", ), }, - expectedFormat: `Custom realm="test@example.com" nonce="this is a nonce", qop="a, b, c", custom="1234"`, - }, - { - challenge: Challenge{ - Scheme: Scheme("Bearer"), - Realm: "my realm", - Token68: true, - Parameters: suite.newValidParameters( - "nonce", "this is a nonce", - "blank", - ), - }, - expectedFormat: `Bearer realm="my realm" token68 nonce="this is a nonce", blank=""`, + expectedFormat: `Custom realm="test@example.com", nonce="this_is_a_nonce", qop="a,b,c", custom="1234"`, }, } @@ -161,28 +258,32 @@ func (suite *ChallengeTestSuite) testChallengesValid() { { challenges: Challenges{}. Append( - suite.newValidBasic("test@server.com", true), + NewBasicChallenge("test@server.com", true), ), expected: []string{ - `Basic realm="test@server.com" charset="UTF-8"`, + `Basic realm="test@server.com", charset="UTF-8"`, }, }, { challenges: Challenges{}. Append(Challenge{ - Scheme: Scheme("Bearer"), - Realm: "my realm", - Parameters: suite.newValidParameters("foo", "bar"), + Scheme: Scheme("Bearer"), + Parameters: suite.newValidParameters( + RealmParameter, "myrealm", + "foo", "bar", + ), }). Append(Challenge{ - Scheme: Scheme("Custom"), - Realm: "another realm@somewhere.net", - Token68: true, - Parameters: suite.newValidParameters("nonce", "this is a nonce", "age", "123"), + Scheme: Scheme("Custom"), + Parameters: suite.newValidParameters( + "nonce", "this_is_a_nonce", + RealmParameter, "anotherrealm@somewhere.net", + "age", "123", + ), }), expected: []string{ - `Bearer realm="my realm" foo="bar"`, - `Custom realm="another realm@somewhere.net" token68 nonce="this is a nonce", age="123"`, + `Bearer realm="myrealm", foo="bar"`, + `Custom realm="anotherrealm@somewhere.net", nonce="this_is_a_nonce", age="123"`, }, }, } @@ -191,13 +292,13 @@ func (suite *ChallengeTestSuite) testChallengesValid() { suite.Run(strconv.Itoa(i), func() { suite.Run("DefaultHeader", func() { header := make(http.Header) - suite.NoError(testCase.challenges.WriteHeader("", header)) + suite.NoError(testCase.challenges.WriteHeader(header)) suite.ElementsMatch(testCase.expected, header.Values(WWWAuthenticateHeader)) }) suite.Run("CustomHeader", func() { header := make(http.Header) - suite.NoError(testCase.challenges.WriteHeader("Custom", header)) + suite.NoError(testCase.challenges.WriteHeaderCustom(header, "Custom")) suite.ElementsMatch(testCase.expected, header.Values("Custom")) }) }) @@ -220,7 +321,8 @@ func (suite *ChallengeTestSuite) testChallengesInvalid() { for i, bad := range badChallenges { suite.Run(strconv.Itoa(i), func() { header := make(http.Header) - suite.Error(bad.WriteHeader("", header)) + suite.Error(bad.WriteHeader(header)) + suite.Error(bad.WriteHeaderCustom(header, "Custom")) }) } } diff --git a/basculehttp/middleware.go b/basculehttp/middleware.go index de5de72..cca0aa7 100644 --- a/basculehttp/middleware.go +++ b/basculehttp/middleware.go @@ -226,7 +226,7 @@ func (m *Middleware) writeWorkflowError(response http.ResponseWriter, request *h ) if statusCode == http.StatusUnauthorized { - writeErr = m.challenges.WriteHeader("", response.Header()) + writeErr = m.challenges.WriteHeader(response.Header()) } if writeErr == nil { diff --git a/basculehttp/middleware_test.go b/basculehttp/middleware_test.go index 3758362..531f0d2 100644 --- a/basculehttp/middleware_test.go +++ b/basculehttp/middleware_test.go @@ -47,11 +47,6 @@ func (suite *MiddlewareTestSuite) serveHTTPNoCall(http.ResponseWriter, *http.Req suite.Fail("The handler should not have been called") } -func (suite *MiddlewareTestSuite) assertChallenge(c Challenge, err error) Challenge { - suite.Require().NoError(err) - return c -} - func (suite *MiddlewareTestSuite) TestUseAuthenticatorError() { m, err := NewMiddleware( UseAuthenticator( @@ -305,7 +300,7 @@ func (suite *MiddlewareTestSuite) testBasicAuthChallenge() { ), ), WithChallenges( - suite.assertChallenge(NewBasicChallenge("test", true)), + NewBasicChallenge("test", true), ), ) @@ -319,7 +314,7 @@ func (suite *MiddlewareTestSuite) testBasicAuthChallenge() { suite.Equal(http.StatusUnauthorized, response.Code) suite.Equal( - `Basic realm="test" charset="UTF-8"`, + `Basic realm="test", charset="UTF-8"`, response.Result().Header.Get(WWWAuthenticateHeader), )