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

Support for refresh_tokens? #407

Open
rbren opened this issue Dec 8, 2021 · 5 comments · May be fixed by #473
Open

Support for refresh_tokens? #407

rbren opened this issue Dec 8, 2021 · 5 comments · May be fixed by #473

Comments

@rbren
Copy link

rbren commented Dec 8, 2021

We recently learned that all access_tokens are now expiring after 1 year. Are there plans to support refresh_tokens in this library?

@theckman
Copy link
Collaborator

theckman commented Dec 8, 2021

I'm not entirely familiar with what you're referring to. Could you share a link?

@rbren
Copy link
Author

rbren commented Dec 8, 2021

@theckman
Copy link
Collaborator

theckman commented Dec 9, 2021

@rbren this is the first I'm hearing of this change, so I appreciate you forwarding that over. Reading that document makes me realize that it seems disorganized on the PagerDuty side, where in that document they talk about adding refresh_token support but it's not mentioned anywhere in their API docs. 🤦‍♂️

Anywho, to your question...

As of today this package is not involved with the OAuth token negotiation process, and instead expects consumers to do that and pass the token to the client. So I think the initial answer to your question is that there are no plans for that right now, and consumers would need to do that going forward.

I think it would be a pretty substantial change to the client to support that, and so I think we'd need to be thoughtful/intentional about it if we do plan to support it.

Edit: What I mean by this last paragraph, is it then opens questions about how to manage the lifecycle of things like the refresh_token. So if we do negotiate a new bearer token in the background, and are provided a new refresh_token as well, how do we make sure to hand that new refresh_token back to the consumer of this package so that it's persisted for the next time the application starts?

@rbren
Copy link
Author

rbren commented Dec 9, 2021

All good questions! Thanks @theckman.

Even just having a function like refresh_token, err := refreshAccessToken(access_token, refresh_token) would be nice. There are also probably interesting ways to do an automatic refresh on 401, e.g. pass in a refreshTokenUpdate channel to the client. I wonder if there are other go client libs out there handling oauth

@dobs
Copy link
Contributor

dobs commented Dec 14, 2021

Clients doing their own token negotiation is often the "correct" way given that coordinating token lifecycle is normally outside the scope of the client itself.

Maybe providing documented examples leveraging oauth2 is a good starting point, either swapping pagerduty.Client's HTTPClient with config.Client's and/or using config.TokenSource? I can contribute some if they'd be useful.

We could also potentially provide some conveniences, e.g.:

  • An oauth2.Config helper function.
  • An oauth2.Endpoint value for folks doing their own config.
  • A new variation of pagerduty.NewOAuthClient (NewManagedOAuthClient? NewAutoOAuthClient?) that automatically sets up and leverages an oauth2.Config, exposes it on pagerduty.Client.

Again some things I'd be happy to contribute if it's a direction we think is worth pursuing.

One minor detail is that we'll also be rolling out more OIDC functionality so a useful alternative to oauth2 might be go-oidc, but they mostly play nice together.

rubensf added a commit to rubensf/go-pagerduty that referenced this issue Feb 15, 2023
PagerDuty currently uses Oauth2 tokens that need to be acquired and
refreshed with some frequency. The current setup only supports static
string auth tokens.

While uses could do the Oauth2 handling themselves, it'd lead to a quite
challenging maneuvering to re-create the PagerDuty client with the new
token once the previous expires, which is unnecessary when there are
official canned libraries to do that for us.

Another option could almost be to just use the oauth2 created Client
[1], but the way `prepRequest` works would interfere with it.

Fixes PagerDuty#407

1: https://pkg.go.dev/golang.org/x/oauth2#Config.Client
@rubensf rubensf linked a pull request Feb 15, 2023 that will close this issue
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 a pull request may close this issue.

3 participants