badgateway / oauth2-client

OAuth2 client for Node and browsers
https://www.npmjs.com/package/@badgateway/oauth2-client
MIT License
269 stars 31 forks source link

Allow Refresh Logic Execution Without Refresh Token (fetch-wrapper.ts) #115

Closed koenr-bc closed 1 year ago

koenr-bc commented 1 year ago

I would like the option to execute the refresh timer without a refresh token.

In fetch-wrapper.ts https://github.com/badgateway/oauth2-client/blob/main/src/fetch-wrapper.ts#L255

The conditional if (!this.token?.expiresAt || !this.token.refreshToken) Causes it to not set a refreshTimer when no refreshToken is supplied. I would like to have to option to still execute the refresh logic even if no refresh token is supplied.

Since refreshToken will be called anyway when https://github.com/badgateway/oauth2-client/blob/main/src/fetch-wrapper.ts#L148 getToken is called.

Possible fix: Replace the conditional with: if (!this.token?.expiresAt)

Or maybe add an additional option like was done with this.options.scheduleRefresh e.g. this.options.forceScheduleRefresh ?

Thanks in advance! Greetings, Koen

koenr-bc commented 1 year ago

Another solution would be to increase the expiresAt with some kind of offset in fetch-wrapper.ts getToken. E.g;

Existing code:

  async getToken(): Promise<OAuth2Token> {

    if (this.token && (this.token.expiresAt === null || this.token.expiresAt > Date.now())) {

      // The current token is still valid
      return this.token;

    }

    return this.refreshToken();
  }

Possible solution:

  async getToken(): Promise<OAuth2Token> {

    if (this.token && (this.token.expiresAt === null || (this.token.expiresAt - this.options.expiresAtOffset) > Date.now())) {

      // The current token is still valid
      return this.token;

    }

    return this.refreshToken();
  }
koenr-bc commented 1 year ago

Created a PR at https://github.com/badgateway/oauth2-client/pull/117

evert commented 1 year ago

Hi Koen,

One of the goals of this library is to stay very lean. This is not a bad feature, but it's also not difficult to implement this as a user of the library, without modifying the internals.

The simplest version would be something like:

setInterval(
  () => fetchWrapper.refreshToken(),
 600_000, // every minute
);

So therefore I don't think it's that helpful to add this as additional configuration. The default behavior (refresh if we got a 401) works for most people.

koenr-bc commented 1 year ago

I could see your point regarding the scheduledRefresh, but the solution provided in the PR at https://github.com/badgateway/oauth2-client/pull/117 I think is still a good idea.

The problem is that the refresh token is not expired when accessing getToken, but when doing the actual http call, even though it might be a short period, the token can expire in the mean time.

Introducing an expiresAtOffset mitigates this and this isn't easily done outside of the internal of the library.

evert commented 1 year ago

Hi @koenr-bc , it's a good idea and good implementation. But my goal with this library is to not try to do everyone for everything with a configuration flag for every case, especially if the use-case can be solved outside the library.

Anyway, I did make another PR that should make it easier to do this. See #119. After this change, all you would have to override is this new function. You could simply set your own expiresAt if it's null. This is a fairly low-key change, so I hope you like that better.

koenr-bc commented 1 year ago

@evert agreed that is a fine solution also, thanks for the change!