PagerDuty / go-pagerduty

go client library for PagerDuty v2 API
https://v2.developer.pagerduty.com/docs/rest-api
Apache License 2.0
287 stars 240 forks source link

Support for refresh_tokens? #407

Open rbren opened 2 years ago

rbren commented 2 years ago

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

theckman commented 2 years ago

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

rbren commented 2 years ago

HYG: https://docs.google.com/document/d/1GjyNU_rKAeX5ugFKDu8PsVY8IyDx-CxiTOJRoNRaEFU/edit

theckman commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.:

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.