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

Extend OAuth provider configuration object to include a token refresh handler #4902

Closed
essential-randomness opened this issue Jul 11, 2022 · 6 comments
Labels
adapters Changes related to the core code concerning database adapters core Refers to `@auth/core` enhancement New feature or request help needed The maintainer needs help due to time constraint/missing knowledge providers stale Did not receive any activity for 60 days

Comments

@essential-randomness
Copy link

essential-randomness commented Jul 11, 2022

Description 📓

The OAuth provider configuration object includes options (e.g. authorization, token, and userinfo) to configure how each provider handles the various stages of the OAuth flow.

While a mechanism for token rotation is explained in the documentation, there is no way to contribute the token refresh code to the set of built-in OAuth providers. I believe allowing this configuration to be shared with other developers through a built-in mechanism would be incredibly beneficial to the users of this library, even if the token rotation is not automatically executed.

In short, my proposal would be to add the following option to the OAuth provider configuration object :

{
  // ...
  /**
   * Endpoint that refreshes OAuth tokens and returns a new `access_token` and `refresh_token`.
   */
  refresh: EndpointHandler<
      RefreshParams,
      {
        // unsure about what goes here
      },
      { tokens: TokenSet }
  >;
  // ...
}

where RefreshParams would be something like:

interface RefreshParams {
  request_uri?: string;
  grant_type?: string;
  // Possibly more
}

This would allow developers to share the code for OAuth token refresh with the community. I don't foresee specific issues with such an extension.

Note that while I likely won't have time to implement a full automatic refresh flow, I'd be willing to contribute this specific extension (and a couple configurations for existing providers) as long as someone can offer help figuring out the right method signature.

How to reproduce ☕️

N/A

Contributing 🙌🏽

Yes, I am willing to help implement this feature in a PR

@essential-randomness essential-randomness added enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Jul 11, 2022
@balazsorban44
Copy link
Member

Thank you! Handling this is definitely on our list, but doing it consistently across different providers and strategies (jwt, database) is what makes it hard (I.e. not all providers have the ability to even refresh tokens, should we warn/swallow/throw error then?)

@essential-randomness
Copy link
Author

I understand this is more complicated than it might seem at first glance. What I'm proposing is not to tackle this generically, but to start with a more manageable intermediate step on the road towards doing so.

Of course, my proposal could also be more complicated than I personally see, but since I prototyped it for my own use case yesterday, let me show you what I have in mind.

1 — Extend the definition of OAuthConfig to have an additional (optional) refresh endpoint handler.

declare module "next-auth/providers" {
  // TODO: These types aren't right but it's beyond my ability to fix them alone.
  type RefreshEndpointHandler = EndpointHandler<
    Record<string, unknown>,
    {
      tokens: TokenSet;
    },
    TokenSet
  >;
  interface OAuthConfig<P> {
    /**
     * Needs following data in Context:
     * - context.provider.refresh.url
     * - context.tokens.refresh_token
     * - context.provider.clientId
     * - context.provider.clientSecret
     */
    refresh?: RefreshEndpointHandler;
  }
}

Unrelated, but I'm really struggling with your TS types for generics in both OAuthConfig and EndpointHandlers. I'd appreciate some help understanding them since I couldn't find the right place to read more about them, maybe in another discussion if there's no documentation anywhere.

2 — Implement the refresh handler on Providers that are known to have refresh tokens

Here is sample implementations from two providers I've been working on:

GitHubApp Provider

Note: this is not the same as the GitHub OAuth provider, which does not include the refresh token

{
  // Other configuration methods, including authorization and token.
  refresh: {
    url: `https://github.com/login/oauth/access_token`,
    request: async (context) => {
      if (!context.tokens.refresh_token) {
        throw new Error("Tried to refresh provider without refresh token.");
      }
      const response = await fetch(context.provider.refresh!.url!, {
        method: "POST",
        headers: {
          "Content-Type": "application/json",
          Accept: "application/json",
        },
        body: JSON.stringify({
          grant_type: "refresh_token",
          client_id: context.provider.clientId!,
          client_secret: context.provider.clientSecret!,
          refresh_token: context.tokens.refresh_token,
        }),
      });
      const newTokens = await response.json();
      const tokenSet: TokenSet = {
        access_token: newTokens.access_token,
        refresh_token: newTokens.refresh_token,
        expires_at: Math.round(
          Date.now() / 1000 + parseInt(newTokens.expires_in)
        ),
      };
      return tokenSet;
    },
  },
}

TumblrAuthProvider

{
  // Other configuration methods, including authorization and token.
  refresh: {
    url: "https://api.tumblr.com/v2/oauth2/token",
    request: async (context) => {
      if (!context.tokens.refresh_token) {
        throw new Error("Tried to refresh provider without refresh token.");
      }
      const response = await fetch(context.provider.refresh!.url!, {
        method: "POST",
        headers: {
          "Content-Type": "application/json",
        },
        body: JSON.stringify({
          grant_type: "refresh_token",
          client_id: context.provider.clientId!,
          client_secret: context.provider.clientSecret!,
          refresh_token: context.tokens.refresh_token,
        }),
      });
      const tokens = await response.json();
      return {
        access_token: tokens.access_token,
        refresh_token: tokens.refresh_token,
        expires_at: Math.round(Date.now() / 1000 + parseInt(tokens.expires_in)),
      };
    },
}

Note that these are similar enough that there could be some kind of extensible configuration provided out of the box, similarly to authorization, token and userinfo. I didn't provide it because I don't know where that code should go.

This is all I'm proposing to standardize in the library: an option to extend the currently-existing providers with a refresh method. This way, assuming that the specific provider is known to have a refresh token, people can start contributing refresh mechanism that everyone using the library can start relying on, even if that requires them to write custom code. One day, when you figure out how to do refresh automatically, you'll already find yourself having some of the work done.

3 — Update the "how to refresh tokens" documentation to include instructions for how to use refresh

This step is not about changing the library, but about documenting how users can write custom code to refresh their tokens, taking advantage of the built-in refresh mechanisms other users have provided. Here is the code I've written, expanded from the "how to refresh tokens" documentation.

3a — Create a method to check whether a given provider has a refresh mechanism

/**
 * Returns the given provider's configuration options, IFF they
 * contain the right set of options for token refresh.
 */
const getRefreshableConfiguration = ({
  providerName,
  authOptions,
}: {
  providerName: string;
  authOptions: NextAuthOptions;
}) => {
  const providerConfig = authOptions.providers.find(
    (provider) => provider.id == providerName
  )?.options as OAuthConfig<unknown>;

  if (
    providerConfig &&
    "refresh" in providerConfig &&
    providerConfig.refresh?.url &&
    providerConfig.refresh?.request
  ) {
    return null;
  }

  return providerConfig;
};

3b — Add the appropriate checks to the jwt callback

  callbacks: {
    // Expanded from: https://next-auth.js.org/tutorials/refresh-token-rotation#server-side
    async jwt({ token, user, account }) {
      // Initial sign in
      if (account && user) {
        return {
          // ADDED THIS
          provider: account.provider,
          accessToken: account.access_token,
          accessTokenExpires: account.expires_at
            ? Date.now() + account.expires_at * 1000
            : undefined,
          refreshToken: account.refresh_token,
          user: {
            ...user,
            // ADDED THIS
            providerAccountId: account.providerAccountId,
          },
        };
      }

      // Return previous token if the access token has not expired yet
      if (Date.now() < token.accessTokenExpires) {
        return token;
      }
      
      const providerConfig = getRefreshableConfiguration({
        providerName: token.provider as string,
        authOptions,
      });

      if (!providerConfig) {
        return {
          ...token,
          error: "RefreshAccessTokenError",
          error_message: "Access token is expired, but provider has no refresh configuration available."
        };
      }

      if (!token.refreshToken) {
        return {
          ...token,
          error: "RefreshAccessTokenError",
          error_message: "No refresh token available for provider with expired authentication and refresh mechanism."
        }
      }

      try {
        const tokens = await providerConfig.refresh!.request!({
          tokens: {
            refresh_token: refreshToken,
          },
          provider: providerOptions,
          user,
        });
        // TODO: the refreshed token values should be persisted according to the adapter in use
       return tokens;
      } catch (e) {
        return {
          ...token,
          error: "RefreshAccessTokenError",
          error_message: "Couldn't refresh token."
        }
      }
    },

...and this is all! I'm sure that my code has a bunch of things "wrong" with it, and many things that could be done better. As I mentioned, I'd love to work with you on better defining how something similar could work. I understand that the work for a full "automatic refresh" solution has a lot of complexity, but I believe this intermediate step can help users of your library now, and help you in the future once you decide to tackle the larger issue.

@balazsorban44
Copy link
Member

Luckily, I think we could just defer to the underlying library for some of the logic: https://github.com/panva/node-openid-client/blob/main/docs/README.md#clientrefreshrefreshtoken-extras

https://github.com/panva/oauth4webapi/blob/main/docs/functions/refreshTokenGrantRequest.md (in #4299)

Maybe we could make this an opt-in flag either on NextAuthOptions or per-provider, and call it experimental. 🤔

One issue that needs to be solved is when multiple requests hit the backend and all want to update at the same time. See one discussion here: #3940

We will also want to make sure that refreshing tokens update the database's Account, so the Adapter API needs to be extended as well.

@balazsorban44 balazsorban44 added help needed The maintainer needs help due to time constraint/missing knowledge providers adapters Changes related to the core code concerning database adapters core Refers to `@auth/core` and removed triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Jul 13, 2022
@essential-randomness
Copy link
Author

It's great that some of the logic can be deferred to the library underneath! node-openid-client is a great tool.

For the "multiple requests at the same time" problem and the "update the Account in the database" problem, those are indeed blockers for officially supporting token refresh as a whole. But since the documentation is already suggesting custom solutions for token refresh that run into those issues, it should be ok to start improving the overall flow without solving them upfront. As long the documentation is clear about the limitations of the current mechanism (information that should probably be added to that page regardless), people can make informed choices about opting in.

Of course, having a feature with known bugs /limitation doesn't feel great even if "experimental". But in this case, the status quo situation for the users of this library is that once their tokens have stopped working they have no easy way to refresh them. Personally, I'd rather opt-in into a (temporarily) buggy solution to the issue, since the alternative is that I must write that buggy solution myself anyway, as I've been doing :P

FYI, updating the database's Account tokens should be done even when the user goes through the OAuth flow again and new tokens are issued, not just when the refresh token is used. Currently the tokens are not updated unless I first delete the account from the database with the Prisma adapter. So that's a related but separate issue that should still be solved :)

@stale
Copy link

stale bot commented Sep 16, 2022

It looks like this issue did not receive any activity for 60 days. It will be closed in 7 days if no further activity occurs. If you think your issue is still relevant, commenting will keep it open. Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Sep 16, 2022
@stale
Copy link

stale bot commented Sep 24, 2022

To keep things tidy, we are closing this issue for now. If you think your issue is still relevant, leave a comment and we might reopen it. Thanks!

@stale stale bot closed this as completed Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters Changes related to the core code concerning database adapters core Refers to `@auth/core` enhancement New feature or request help needed The maintainer needs help due to time constraint/missing knowledge providers stale Did not receive any activity for 60 days
Projects
None yet
Development

No branches or pull requests

2 participants