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

RFC: Improve OAuth provider configuration #1846

Closed
balazsorban44 opened this issue Apr 25, 2021 · 9 comments
Closed

RFC: Improve OAuth provider configuration #1846

balazsorban44 opened this issue Apr 25, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@balazsorban44
Copy link
Member

balazsorban44 commented Apr 25, 2021

Summary of proposed feature
Introducing a new way of configuring OAuth providers. With this proposal, we can provide granular control from basic (with the best defaults we can think of) to completely controlled hooks into the login flow. (The user would be able to do the actual fetch calls themselves. Hopefully, this will almost never would be necessary)

Here is an example:

export default function Auth0(options) {
  return {
    id: "auth0",
    name: "Auth0",
    authorization: `https://${options.domain}/authorize?grant_type=authorization_code&response_type=code&scope=openid%20email%20profile`,
    // Alternatively, you could pass an url and params
    // which will be combined for you for convinience.
    // authorization: {
    //   url: `https://${options.domain}/authorize`,
    //   params: {
    //     grant_type: "authorization_code",
    //     response_type: "code",
    //     scope: "openid email profile",
    //   },
    // },
    // Skips fetching user data. In some rare situations, you may not need this. See #1065
    userinfo: null,
    token: {
      url: `https://${options.domain}/oauth/token`,
      method: "POST",
      via: "header",
      // Complete control over how to get the userinfo.
      async request({ tokens }) {
        const response = await fetch("custom/endpoint/token", {
          headers: {
            Authorization: `Bearer ${tokens.access_token}`,
          },
        })
        return await response.json()
      },
    },
    profile(profile) {
      return {
        id: profile.sub,
        name: profile.nickname,
        email: profile.email,
        image: profile.picture,
      }
    },
    ...options,
  }
}

Purpose of proposed feature
To simplify the configuration by reducing the number of required fields, but give full control at the same time, for advanced use cases.

Detail about proposed feature

So to sum it up, 3 new methods could be introduced, using a similar shape:

/** Any contextual information that makes the request useful, eg.: tokens in case of the token endpoint */
type RequestContext = any

type OAuthEndpoint = string | null | {
  url?: string
  method?: "POST" | "GET"
  /** How to pass the access token */
  via?: "header" | "body" | "query"
  /** Will be used as URL params if method is GET, or the body as x-url-form-encoded if POST*/
  params?: URLSearchParams | Record<string, unknown>
  request(context: RequestContext)?: Promise<any>
}

Potential problems
As there are no conflicting configuration options, this could be implemented in a non-breaking way, meaning in theory we don't have to wait until the next major version.

Describe any alternatives you've considered
The current configuration is fragile since the OAuth spec can be interpreted differently, which often ends up with slightly too specific implementations, which has been hard to handle until now. The best solution was one-off configuration flags for certain providers, but in the long term, it introduced bloat in the code.

Additional context

Please indicate if you are willing and able to help implement the proposed feature.
Could fit neatly into #1698, or a follow-up PR.

Related: #1065, #1642, #1605, #1607, #1756, #950

Please share your opinions!

@balazsorban44 balazsorban44 added the enhancement New feature or request label Apr 25, 2021
@balazsorban44 balazsorban44 changed the title RFC: Improved OAuth provider configuration [WIP] RFC: Improve OAuth provider configuration Apr 25, 2021
@Fronix
Copy link

Fronix commented Apr 25, 2021

I feel this would make next-auth a lot more flexible and allow support for pretty much any provider. It is usually the get token part that differs between providers with some using POST other GET, some using pkce and so on. This would make it much easier to let users customize every provider!

If I'm understanding correct parts like this wouldn't be needed for providers that haven't been added to next-auth?
src/server/lib/oauth/client.js

  if (provider.id === 'reddit') {
    headers.Authorization = 'Basic ' + Buffer.from((provider.clientId + ':' + provider.clientSecret)).toString('base64')
  }


  if (provider.id === 'identity-server4' && !headers.Authorization) {
    headers.Authorization = `Bearer ${code}`
  }


  if (provider.protection.includes('pkce')) {
    params.code_verifier = codeVerifier
  }

@balazsorban44
Copy link
Member Author

Yes, we could remove a lot of custom handling for providers as well.

@balazsorban44
Copy link
Member Author

Hmm. 🤔 I was thinking that the profile callback could even be the request method of the new userinfo, further simplifying things...

@Bedol
Copy link

Bedol commented Apr 30, 2021

Looks like this kind of configuration will allow the implementation of other OAuth grant types.

@jackHedaya
Copy link

I'm digging this proposal. I turned away from using NextAuth simply because the OAuth provider I was using did not fit the library but with fine tune control like this I would be interested in giving it another shot.

@valstu
Copy link

valstu commented May 30, 2021

This could be solution to my problem as well. I'm using identity provider which requires that all authentication and token requests are signed with private key. Atm this is impossible without forking next-auth.

@valstu
Copy link

valstu commented May 31, 2021

Actually ended up using patch-package to monkey patch next-auth same way @Fronix mentioned on #950. But in conclusion I feel like the features mentions in this RFC would make it possible to integrate with many new providers without bloating the next-auth core with if (provider.id === 'mycustomprovider') {} blocks. I ended up monkey patching three separate files:

  • getAuthorizationUrl function on server/lib/signing/oauth.js
  • getOAuth2AccessToken function on server/lib/oauth/client.js
  • getProfile function on server/lib/oauth/callback.js

In the first two I needed to sign the requests and send data as JWS instead of typical query string. In the getProfile function I needed to decrypt the response with providers public key.

In my case the provider is quite specific (so called strong authentication for Finnish audience only) and requires these extra security measures. I could of course make a PR and add this provider to next-auth but I feel like adding this logic in next-auth core seems a bit overkill and would just bloat the code since the usage would be quite marginal. So thumbs up for this RFC.

@balazsorban44
Copy link
Member Author

For anyone following, I started working on this over at #2411. It is very early, so please be patient, but if you have anything constructive, please add it here, or on the PR!

balazsorban44 added a commit that referenced this issue Aug 4, 2021
> This touches on all OAuth providers, so there is a big potential for breaking by default. We have let new providers be added for contributors' specific needs, but from now on, we will require a more strict default on all new providers, so the basic behavior is predictable for everyone.
⚠ Unfortunately, we will not have the capacity to test each and every provider that has been added to the default providers, but we will do our best to test the most popular ones. (@ndom91 has worked on setting up the infrastructure for this). If you wish to make sure that the provider you are using will stay working, please reach out with your concerns and tell us how can you help us test that particular provider in the future. 🙏

That said, I will try my best to not break ANY of the currently built-in providers, or at least make the migration super easy. So hopefully, you won't have to change anything. It will most probably affect you if you defined a custom provider though.

We will monitor the default configuration much more closely, so the behavior will be more consistent across providers by default.

Closes #1846, Closes #1605, Closes #1607

BREAKING CHANGES:

Basecamp provider is removed. See the explanation [here](https://github.com/basecamp/api/blob/master/sections/authentication.md#on-authenticating-users-via-oauth)

**ALL** OAuth providers' `profile` callback is expected to only return these fields by default from now on: `id`, `name`, `email`, and `image` at most. Any of these missing values should be set to `null`.

The following new options are available:
1. `authorization` (replaces `authorizationUrl`, `authorizationParams`, `scope`)
2. `token` replaces (`accessTokenUrl`, `headers`, `params`)
3. `userinfo` (replaces `profileUrl`)

These three options map nicely to the OAuth spec's three endpoints for
1. initiating the login flow
2. retrieve OAuth tokens
3. retrieve user information

They all take the form of `EndpointHandler`:
```ts
type EndpointRequest<C, R> = (
  context: C & {
    /** `openid-client` Client */
    client: Client
    /** Provider is passed for convenience, ans also contains the `callbackUrl`. */
    provider: OAuthConfig & {
      signinUrl: string
      callbackUrl: string
    }
  }
) => Awaitable<R>

/** Gives granular control of the request to the given endpoint */
type AdvancedEndpointHandler<P extends UrlParams, C, R> = {
  /** Endpoint URL. Can contain parameters. Optionally, you can use `params`*/
  url?: string
  /** These will be prepended to the `url` */
  params?: P
  /**
   * Control the corresponding OAuth endpoint request completely.
   * Useful if your provider relies on some custom behavior
   * or it diverges from the OAuth spec.
   *
   * - ⚠ **This is an advanced option.**
   * You should **try to avoid using advanced options** unless you are very comfortable using them.
   */
  request?: EndpointRequest<C, R>
}

/** Either an URL (containing all the parameters) or an object with more granular control. */
type EndpointHandler<P extends UrlParams, C = any, R = any> =
  | string
  | AdvancedEndpointHandler<P, C, R>
```

In case of `authorization`, the `EndpointHandler` can define the `params` as [`AuthorizationParameters`](https://github.com/panva/node-openid-client/blob/51dc47d9ac619b71cd1c983b0be750a12bbae008/types/index.d.ts#L108-L143)

> Note: `authorization` does not implement `request` yet. We will have to see if there is demand for it.

From now on, instead of using the `...` spread operator when adding a new built-in provider, the user is expected to add `options` as a property at the end of the default config. This way, we can deep merge the user config with the default one. This is needed  to let the user do something like this:

```js
MyProvider({
  clientId: "",
  clientSecret: "",
  authorization: { params: {scope: ""} }
})
```
So even if the default config defines anything in `authorization`, only the user-defined parts will be overridden.
@balazsorban44
Copy link
Member Author

This has landed in the next-auth@4.0.0-next.20, please give this a try! Update to the documentation is coming in a subsequent PR, for now, see #2411 (comment) for an explanation. I hope it is helpful enough.

I kindly ask anyone who has one of the built-in providers dear to their heart to test it out and open an issue if it broke! I would like to make the migration to v4 smooth for anyone, and I will need help from the community, as we have 50+ built-in providers, and @ndom91 could not get an account for all of those! Please spread the word.

mnphpexpert added a commit to mnphpexpert/next-auth that referenced this issue Sep 2, 2024
> This touches on all OAuth providers, so there is a big potential for breaking by default. We have let new providers be added for contributors' specific needs, but from now on, we will require a more strict default on all new providers, so the basic behavior is predictable for everyone.
⚠ Unfortunately, we will not have the capacity to test each and every provider that has been added to the default providers, but we will do our best to test the most popular ones. (@ndom91 has worked on setting up the infrastructure for this). If you wish to make sure that the provider you are using will stay working, please reach out with your concerns and tell us how can you help us test that particular provider in the future. 🙏

That said, I will try my best to not break ANY of the currently built-in providers, or at least make the migration super easy. So hopefully, you won't have to change anything. It will most probably affect you if you defined a custom provider though.

We will monitor the default configuration much more closely, so the behavior will be more consistent across providers by default.

Closes nextauthjs#1846, Closes nextauthjs#1605, Closes nextauthjs#1607

BREAKING CHANGES:

Basecamp provider is removed. See the explanation [here](https://github.com/basecamp/api/blob/master/sections/authentication.md#on-authenticating-users-via-oauth)

**ALL** OAuth providers' `profile` callback is expected to only return these fields by default from now on: `id`, `name`, `email`, and `image` at most. Any of these missing values should be set to `null`.

The following new options are available:
1. `authorization` (replaces `authorizationUrl`, `authorizationParams`, `scope`)
2. `token` replaces (`accessTokenUrl`, `headers`, `params`)
3. `userinfo` (replaces `profileUrl`)

These three options map nicely to the OAuth spec's three endpoints for
1. initiating the login flow
2. retrieve OAuth tokens
3. retrieve user information

They all take the form of `EndpointHandler`:
```ts
type EndpointRequest<C, R> = (
  context: C & {
    /** `openid-client` Client */
    client: Client
    /** Provider is passed for convenience, ans also contains the `callbackUrl`. */
    provider: OAuthConfig & {
      signinUrl: string
      callbackUrl: string
    }
  }
) => Awaitable<R>

/** Gives granular control of the request to the given endpoint */
type AdvancedEndpointHandler<P extends UrlParams, C, R> = {
  /** Endpoint URL. Can contain parameters. Optionally, you can use `params`*/
  url?: string
  /** These will be prepended to the `url` */
  params?: P
  /**
   * Control the corresponding OAuth endpoint request completely.
   * Useful if your provider relies on some custom behavior
   * or it diverges from the OAuth spec.
   *
   * - ⚠ **This is an advanced option.**
   * You should **try to avoid using advanced options** unless you are very comfortable using them.
   */
  request?: EndpointRequest<C, R>
}

/** Either an URL (containing all the parameters) or an object with more granular control. */
type EndpointHandler<P extends UrlParams, C = any, R = any> =
  | string
  | AdvancedEndpointHandler<P, C, R>
```

In case of `authorization`, the `EndpointHandler` can define the `params` as [`AuthorizationParameters`](https://github.com/panva/node-openid-client/blob/51dc47d9ac619b71cd1c983b0be750a12bbae008/types/index.d.ts#L108-L143)

> Note: `authorization` does not implement `request` yet. We will have to see if there is demand for it.

From now on, instead of using the `...` spread operator when adding a new built-in provider, the user is expected to add `options` as a property at the end of the default config. This way, we can deep merge the user config with the default one. This is needed  to let the user do something like this:

```js
MyProvider({
  clientId: "",
  clientSecret: "",
  authorization: { params: {scope: ""} }
})
```
So even if the default config defines anything in `authorization`, only the user-defined parts will be overridden.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants