Add UserInfo endpoint #1473
Add UserInfo endpoint #1473
Conversation
Co-authored-by: Yuxing Li <[email protected]> Co-authored-by: Francisco Santiago <[email protected]>
alindeman
force-pushed
the
add-user-endpoint
branch
from
7592623 to
7970903
Compare
|
I'll review this next week. Thanks for contributing! 😃 🎉 |
alindeman
force-pushed
the
add-user-endpoint
branch
from
7970903 to
46f5726
Compare
| } | ||
| rawIDToken := auth[len(prefix):] | ||
|
|
||
| verifier := oidc.NewVerifier(s.issuerURL.String(), &storageKeySet{s.storage}, &oidc.Config{SkipClientIDCheck: true}) |
There was a problem hiding this comment.
Is this a costly operation? (I.e., do we want to store and re-use oidc.NewVerifier? (I have no idea, genuine question.)
There was a problem hiding this comment.
It's not, as far as I can tell. The implementation is currently:
func NewVerifier(issuerURL string, keySet KeySet, config *Config) *IDTokenVerifier {
return &IDTokenVerifier{keySet: keySet, config: config, issuer: issuerURL}
}Somewhat notably, constructing an oidc.Provider is a relatively expensive operation, but we don't have a need to do that in non-test code.
server/server_test.go
Outdated
| { | ||
| name: "fetch userinfo", | ||
| handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token) error { | ||
| _, err := p.UserInfo(ctx, config.TokenSource(ctx, token)) |
There was a problem hiding this comment.
❓ Would it be worthwhile to assert something on the response?
There was a problem hiding this comment.
I went back and forth on it. Not many of the other tests do, but since it ended up being easy enough, I've added something in 840065f. Let me know what you think.
We have a story around user info now
@srenatus Thanks for the review. Let me know if you'd like to see anything else addressed. |
There was a problem hiding this comment.
😃 LGTM, one question plus another nitpick. @JoelSpeed @ericchiang What do you think?
| @@ -6,7 +6,7 @@ require ( | |||
| github.com/boltdb/bolt v1.3.1 // indirect | |||
| github.com/cockroachdb/cmux v0.0.0-20170110192607-30d10be49292 // indirect | |||
| github.com/coreos/etcd v3.2.9+incompatible | |||
| github.com/coreos/go-oidc v0.0.0-20170307191026-be73733bb8cc | |||
| github.com/coreos/go-oidc v2.0.0+incompatible | |||
There was a problem hiding this comment.
❓Is this a problem somehow?
There was a problem hiding this comment.
No, it mostly means this repository uses a v2 tag, but not in the native go mod way. In fact, it's not been converted to a go module yet at all (therefore incompatible).
I'm personally not concerned about it: it's pretty standard fare while the Go community converts things slowly to modules over time. If/when go-oidc becomes a module, we might have to update some imports, but otherwise shouldn't be a big deal.
| @@ -151,6 +152,7 @@ type discovery struct { | |||
| Auth string `json:"authorization_endpoint"` | |||
| Token string `json:"token_endpoint"` | |||
| Keys string `json:"jwks_uri"` | |||
| UserInfo string `json:"userinfo_endpoint"` | |||
|
Any reviewers have any additional feedback or requests to change things? |
|
If no one beats me to it, I'll merge this tomorrow and push a release tag |
Co-authored-by: Andy Lindeman <[email protected]>
Co-authored-by: Andy Lindeman <[email protected]>
|
I've considered another round-trip with the @alindeman for squashing commits, but... I don't think it matters that much. I'm going to push the big green button. 🚀 |
|
@srenatus @alindeman @jackielii @JoelSpeed thanks for all the work getting this implemented! |
Add UserInfo endpoint
This PR takes the great work in #1454, reworks it a bit to lean on
oidc.Verifierfor verification, and adds tests.Fixes #376
Closes #1133
Closes #1201
Closes #1453
Closes #1454