Configurable Nonces #45
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package oauth1 | ||
|
|
||
| import ( | ||
| "crypto/rand" | ||
| "encoding/base64" | ||
| "encoding/hex" | ||
| ) | ||
|
|
||
| // Noncer provides random nonce strings. | ||
| type Noncer interface { | ||
| Nonce() string | ||
| } | ||
|
|
||
| // NoncerFunc is an adapter to allow the use of | ||
| // 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). |
||
|
|
||
| // Nonce calls f(). | ||
| func (f NoncerFunc) Nonce() string { | ||
| return f() | ||
| } | ||
|
|
||
| var ( | ||
| // 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. |
||
| Length: 32, | ||
| } | ||
| ) | ||
|
|
||
| // Base64Noncer reads Length bytes from crypto/rand and | ||
| // 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. |
||
| } | ||
|
|
||
| // Nonce provides a random nonce string. | ||
| func (n Base64Noncer) Nonce() string { | ||
| length := n.Length | ||
| if length == 0 { | ||
| length = 32 | ||
| } | ||
| b := make([]byte, length) | ||
| rand.Read(b) | ||
| return base64.StdEncoding.EncodeToString(b) | ||
| } | ||
|
|
||
| // 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. |
||
| Length int | ||
| } | ||
|
|
||
| // Nonce provides a random nonce string. | ||
| func (n HexNoncer) Nonce() string { | ||
| length := n.Length | ||
| if length == 0 { | ||
| length = 32 | ||
| } | ||
| b := make([]byte, length) | ||
| rand.Read(b) | ||
| return hex.EncodeToString(b) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.