|
|
|
Package oauth2 should be agnostic about the source of the
the private key contents.
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total messages: 8
|
jbd
|
11 years, 4 months ago (2014年08月17日 01:13:14 UTC) #1 | ||||||||||||||||||||||||||||||||
https://codereview.appspot.com/129310043/diff/1/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/129310043/diff/1/google/example_test.go#newcode56 google/example_test.go:56: // $ openssl pkcs12 -in key.p12 -out key.pem -nodes how does this look in godoc HTML? I think you need more spaces before to make it render as code. https://codereview.appspot.com/129310043/diff/1/google/example_test.go#newcode57 google/example_test.go:57: // Supports only PEM containers without a pass phrase. This is a sentence fragment. Use a complete sentence with a subject. Also, passphrase is apparently one word without a space. https://codereview.appspot.com/129310043/diff/1/jwt.go File jwt.go (right): https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode37 jwt.go:37: // Use openssl pkcs12 -in key.p12 -out key.pem -nodes to convert put the command on its own line outside of the flow of text. verify with godoc that it looks nice. You can probably move all the English first so the command isn't in the middle of a sentence. https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode147 jwt.go:147: // to a rsa.PrivateKey It detects whether the private key is in a to an * period after Key https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode149 jwt.go:149: // from PEM container before conversion. Supports PEM containers It only supports https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode153 jwt.go:153: if block != nil { I might also check err == nil and change _ to err
https://codereview.appspot.com/129310043/diff/1/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/129310043/diff/1/google/example_test.go#newcode56 google/example_test.go:56: // $ openssl pkcs12 -in key.p12 -out key.pem -nodes On 2014年08月17日 03:46:48, bradfitz wrote: > how does this look in godoc HTML? I think you need more spaces before to make it > render as code. The example is already escaped entirely. https://codereview.appspot.com/129310043/diff/1/google/example_test.go#newcode57 google/example_test.go:57: // Supports only PEM containers without a pass phrase. On 2014年08月17日 03:46:48, bradfitz wrote: > This is a sentence fragment. Use a complete sentence with a subject. > > Also, passphrase is apparently one word without a space. Done. https://codereview.appspot.com/129310043/diff/1/jwt.go File jwt.go (right): https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode37 jwt.go:37: // Use openssl pkcs12 -in key.p12 -out key.pem -nodes to convert On 2014年08月17日 03:46:49, bradfitz wrote: > put the command on its own line outside of the flow of text. verify with godoc > that it looks nice. > > You can probably move all the English first so the command isn't in the middle > of a sentence. Field documentation is already escaped too. But the command has its own line now, looks much better. Done. https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode147 jwt.go:147: // to a rsa.PrivateKey It detects whether the private key is in a On 2014年08月17日 03:46:49, bradfitz wrote: > to an * > > period after Key Done. https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode149 jwt.go:149: // from PEM container before conversion. Supports PEM containers On 2014年08月17日 03:46:49, bradfitz wrote: > It only supports Done. https://codereview.appspot.com/129310043/diff/1/jwt.go#newcode153 jwt.go:153: if block != nil { On 2014年08月17日 03:46:49, bradfitz wrote: > I might also check err == nil and change _ to err Not an error. pem.Decode doesn't return an error. http://golang.org/src/pkg/encoding/pem/pem.go?s=2182:2230#L66
PTAL
https://codereview.appspot.com/129310043/diff/20001/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/129310043/diff/20001/google/example_test.go#ne... google/example_test.go:56: // $ openssl pkcs12 -in key.p12 -out key.pem -nodes the formatting of this differs from the other example_test.go file. which is prettier in HTML? https://codereview.appspot.com/129310043/diff/20001/google/example_test.go#ne... google/example_test.go:57: // Supports only PEM containers without a pass phrase. passphrase one word https://codereview.appspot.com/129310043/diff/20001/jwt.go File jwt.go (right): https://codereview.appspot.com/129310043/diff/20001/jwt.go#newcode36 jwt.go:36: // PEM containers with a pass phrase are not supported. passphrase one word https://codereview.appspot.com/129310043/diff/20001/jwt.go#newcode44 jwt.go:44: Scopes []string `json:"scopes"` why do these have json annotations at all? seems weird to omit PrivateKey. how are people expected to use this with json?
https://codereview.appspot.com/129310043/diff/20001/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/129310043/diff/20001/google/example_test.go#ne... google/example_test.go:56: // $ openssl pkcs12 -in key.p12 -out key.pem -nodes On 2014年08月18日 17:20:37, bradfitz wrote: > the formatting of this differs from the other example_test.go file. which is > prettier in HTML? Fixed for consistency. https://codereview.appspot.com/129310043/diff/20001/google/example_test.go#ne... google/example_test.go:57: // Supports only PEM containers without a pass phrase. On 2014年08月18日 17:20:37, bradfitz wrote: > passphrase one word Many duplicate examples :( Done. https://codereview.appspot.com/129310043/diff/20001/jwt.go File jwt.go (right): https://codereview.appspot.com/129310043/diff/20001/jwt.go#newcode36 jwt.go:36: // PEM containers with a pass phrase are not supported. On 2014年08月18日 17:20:37, bradfitz wrote: > passphrase one word Done. https://codereview.appspot.com/129310043/diff/20001/jwt.go#newcode44 jwt.go:44: Scopes []string `json:"scopes"` On 2014年08月18日 17:20:37, bradfitz wrote: > why do these have json annotations at all? Most users keep their credentials somewhere else, are not embedding them. Likely that they want to marshal/unmarshal. > seems weird to omit PrivateKey. how > are people expected to use this with json? Persist the options object without a private key, set private key before using. I wouldn't like to write my private key here and there. This should be the expected behaviour.