Configurable Nonces #45
Conversation
Export the Noncer interface, and add a Noncer field to Config to provide custom nonce creation. Create a DefaultNoncer Noncer that formalizes the existing functionality, which is used by default. Add a new HexNoncer as an alternative option.
noncer.go
Outdated
| // DefaultNoncer is the default Noncer. It reads 32 | ||
| // bytes from crypto/rand and returns those bytes as a | ||
| // base64 encoded string. | ||
| DefaultNoncer Noncer = Base64Noncer{ |
There was a problem hiding this comment.
Not sure what another exported type for this really gains. DefaultNoncer isn't something abstracting the real noncer in a way we'd be able to change it in future, your use case shows servers are sensitive to the details.
There was a problem hiding this comment.
That sounds good to me. I added this to have a designated default for package consumers who don't want to customize have a handy option, but it's redundant.
noncer.go
Outdated
| // returns those bytes as a base64 encoded string. If | ||
| // Length is 0, 32 bytes are read. | ||
| type Base64Noncer struct { | ||
| Length int |
There was a problem hiding this comment.
Is there a true need for altering the length? Every exported type and field is a library commitment.
There was a problem hiding this comment.
Or if someone sets it below 32 there may be some concern about lack of uniqueness among nonces. The spec doesn't provide a hard requirement.
There was a problem hiding this comment.
That sounds good to me. I'm not invested either way.
noncer.go
Outdated
| // HexNoncer reads Length bytes from crypto/rand and | ||
| // returns those bytes as a base64 encoded string. If | ||
| // Length is 0, 32 bytes are read. | ||
| type HexNoncer struct { |
There was a problem hiding this comment.
Does this need to be here? Those users who have special nonce needs can implement their own. Not sure hex is special or just your preferred variant.
There was a problem hiding this comment.
I suppose, go ahead and keep it. If we're going to export an interface, there should be two concrete types that implement it.
There was a problem hiding this comment.
Doesn't need to be there, and I'm good either way. I know what I need to use and I tossed it in here as an alternative option. I'll keep it in per your second comment, but I'm fine either way.
auther.go
Outdated
| @@ -137,12 +129,11 @@ func (a *auther) commonOAuthParams() map[string]string { | |||
|
|
|||
| // Returns a base64 encoded random 32 byte string. | |||
| func (a *auther) nonce() string { | |||
| if a.noncer != nil { | |||
| return a.noncer.Nonce() | |||
| noncer := DefaultNoncer | |||
There was a problem hiding this comment.
Seems like this defaulting logic would more commonly belong in newAuther, retaining the field that was there?
The test changes in this PR have to do with this choice, and not testing the actual noncer details it seems.
There was a problem hiding this comment.
That makes sense. I was thinking about the pattern I use when the struct is exported and you're not sure whether it could be nil, but that doesn't apply here.
noncer.go
Outdated
| // ordinary functions as Noncers. If f is a function | ||
| // with the appropriate signature, NoncerFunc(f) is a | ||
| // Noncer that calls f. | ||
| type NoncerFunc func() string |
There was a problem hiding this comment.
Probably not needed, folks can write a struct. And most should never need to alter the noncer.
There was a problem hiding this comment.
This is in there because it's how I originally implemented the Noncers (I didn't bother with separate types and just used NoncerFuncs).
Remove redundant "DefaultNoncer" var (just refer to Base64Noncer). Remove NoncerFunc. Remove Length field from two provided Noncers and instead set length to 32 bytes.
|
Awesome, I needed exactly this, for similar reasons (ended up doing dirty hacks on the module to just test my code). |
* Add a Noncer interface to allow custom noncers * Default to Base64Noncer (no change, recommended) * Add HexNoncer for providers that don't accept non-alphanumeric nonces
We're working with a service that uses OAuth 1.0 for authentication, but does not accept non-alphanumeric characters in the nonce. That's peculiar to this service, but by exporting the existing Noncer interface and adding a Noncer field to the Config, it's easy to enable configuration for this situation. Because this is a general change, other peculiar cases can now also be covered.
The existing behavior is part of an exported DefaultNoncer, which is the default used when no custom Noncer is specified. DefaultNoncer is a Base64Noncer with Length 32.
If anyone else needs a noncer that excludes non-alphanumeric characters, a new HexNoncer is also included. HexNoncer works just like Base64Noncer, but encodes in hexadecimal instead of base64.