-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support provider device_authorization_endpoint #365
Conversation
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.
Device code flow support, and the additional DeviceAuthURL
field in Endpoint
finally merged into golang.org/x/oauth2
@ v0.13.0
.
Perhaps this PR could be updated and made ready for review by the package maintainers?
Some mini comments:
The "coreos/cbodonnell" search/replace in README.md
, go.mod
, example/{idtoken,userinfo}/app.go
, and test
should't be part of the PR.
I'd be inclined to update go.mod
to depend golang.org/x/oauth2
v0.13.0
, and to
update oidc/oidc.go func (p *Provider) Endpoint() oauth2.Endpoint {
to have it initialize the DeviceAuthURL
field; that way it's auto-populated in the Endpoint for any existing code using Provider.Endpoint()
- though I'm not sure if the package maintainers would agree or not...
Thanks!
I don't see device_authorization_endpoint as a field provided by OpenID Connect discovery https://openid.net/specs/openid-connect-discovery-1_0.html Is there another spec I should be looking at? |
An to be clear, you can retrieve any additional discovery fields through https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#Provider.Claims So users can already access device_authorization_endpoint if a provider returns it today |
RFC-8628: OAuth 2.0 Device Authorization Grant -(section 4) defines So, whilst I also felt that because go-oidc's Hope that helps. |
Yes, thanks, I was aware that it could be extracted this way. I've tried to provide some references / perspective as to why I felt it should be filled out by |
Thanks @somersf - I see both sides here since it's technically not part of the OIDC spec, but I also see how it fits in given the change to |
@somersf the PR has been updated. @ericchiang Let us know if this is something reasonable for this project based on the thoughts above. If not, I'd suggest we add a small snippet to the linked issue with the recommended way to retrieve this information for reference. |
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.
Thanks for the updates - LGTM. It's over to the project maintainers for their say though!
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.
lgtm, can you please squash and resolve conflicts? Also do we need to update our go.mod to support newer versions of oauth2 with the added endpoint field?
@ericchiang done! the |
@ericchiang anything else we need to do for this one based on my last comment? |
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.
Sorry for the delay here! lgtm
DeviceAuthURL
was added tooauth2.Endpoint
ingolang.org/x/oauth2
v0.13.0
This PR adds support for parsing the provider
device_authorization_endpoint
from the OIDC discovery endpoint and populatesDeviceAuthURL
for theoauth2.Endpoint
returned byProvider.Endpoint()
.Related issue: #357