Skip to content
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

Add Endpoint() api call to oidc.Provider #139

Closed
wants to merge 1 commit into from

Conversation

DrDaveD
Copy link

@DrDaveD DrDaveD commented Aug 13, 2024

This adds an Endpoint() api call to oidc.Provider which simply invokes the underlying coreos/go-oidc Endpoint() api. That returns an oauth2.Endpoint object which since coreos/go-oidc#365 includes the DeviceAuthURL in addition to the AuthURL and TokenURL values read by oidc discovery. This enables simplifying applications that need to use those URLs and avoids them having to separately discover that information, for example hashicorp/vault-plugin-auth-jwt#131 which adds the device flow to jwt/oidc authentication.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty for the PR. Next, perhaps consider opening an issue first where we can discuss the proposed feature/change and get alignment on how to proceed?

With that said, adding this seems reasonable as long as the contribution covers all the necessary changes and not just a one-off additional provider func to dig out the oauth2.Endpoint if the provider returned it in its discovery reponse. To that end I have a few comments/questions/suggestions:

  • How would this work if the provider doesn't support discovery and is configured via the WithProviderConfig(...) option?
  • I think, we need a solution that supports both discovery and the WithProviderConfig option. If we do that we can likely return a copy of the provider's oidc.ProviderConfig here which makes it consistent with the current implementation which wraps the underlying oauth2.Endpoint with it's own type.
  • We need unit tests for all changes, so we need to add support for the device AuthURL to the oidc.TestProvider

@DrDaveD
Copy link
Author

DrDaveD commented Aug 13, 2024

Thanks for the quick response. I had started to add a DeviceAuthURL field to ProviderConfig and to DiscoveryInfo but it appeared to be opening a big can of worms so I thought I would try first just this most simple of changes, especially since that was exactly what I needed and I don't have any idea if anyone else would use the other changes. Next I will try to do what you ask.

@DrDaveD
Copy link
Author

DrDaveD commented Aug 14, 2024

Replaced by #140

@DrDaveD DrDaveD closed this Aug 14, 2024
@DrDaveD DrDaveD deleted the add-oidc-endpoint branch August 14, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants