feat(core): add allowedAccountTokens option

[skirsten]

☕️ Reasoning

Just came about the problem described in https://github.com/nextauthjs/next-auth/discussions/4516 when trying to use the GitLab provider. I could simply add the missing fields to my Prisma schema or remove it in a custom prisma adapter but I would rather not bloat it with unnecessary fields.

This PR introduces a new optional option to configure which fields from the provider will be stored on the Account:

By default NextAuth.js stores all information about an OAuth account gathered by the provider on the Account Model. These tokens can include fields that are not defined on the database schema. With this option the tokens can be filtered before being stored on the Account.

For example:

allowedAccountTokens: [
  "refresh_token",
  "access_token",
  "expires_at",
  "token_type",
  "scope",
  "id_token",
  "session_state",
],

ensures only the fields that are supported by the default Prisma schema are stored.

I am not fixed on a proper name yet. Maybe allowedAccountFields makes more sense to not confuse this with anything related to the JWT tokens.

🧢 Checklist

• [x] Documentation
• [ ] Tests
• [ ] Ready to be merged

🎫 Affected issues

Fixes #3818 (only the original issue, not the second part of the discussion) Fixes #4515 Fixes #3823 Fixes #3067

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
next-auth	✅ Ready (Inspect)	Visit Preview	Jul 10, 2022 at 8:13PM (UTC)

[ndom91]

Thanks for opening a PR about this!

I think explicitly having the user acknowledge the fields their provider returns and making them make adjustments in the config for them will drastically reduce these types of issues.

The user should know then, that if thye want to let through and store an additional field, that they will have to add it to their DB schema as well.

I dont really have a strong opinion on the API design aspect of this though - maybe @balazsorban44 does.

[balazsorban44]

I don't like the idea of adding yet another option.

We cannot control what comes back from an OAuth provider, so we just forward anything to the adapter. It's up to the adapter to handle the data and mediate with the database.

IMO, the correct way would be to simply better document overriding adapter methods to accommodate for this use case.

For any adapter, you could override the linkAccount method: https://github.com/nextauthjs/next-auth/blob/af3c2dd33dc9c1b7d2a99999bfa04a30b0ae16b4/packages/adapter-prisma/src/index.ts#L18

In a one-liner like this:

// You already have this somewhere
import { PrismaAdapter } from "@next-auth/prisma-adapter"
import { PrismaClient } from "@prisma/client"
const prisma = new PrismaClient()
const adapter = PrismaAdapter(prisma)

// Only change necessary
adapter.linkAccount = ({_unwanted, ...data}) => adapter.linkAccount(data)

One of the reasons of simplifying the adapter to a function that returns an object since v3 (where it was usually a class), was to make overriding methods super easy. It just hasn't been documented well yet.

[skirsten]

I am well aware of the code you posted but it is not any more safe to use than no filtering at all. If a provider simply adds a new field this will break production.

At least something like this would be required:

authAdapter.linkAccount = ({
  providerAccountId,
  userId,
  provider,
  type,
  refresh_token,
  access_token,
  expires_at,
  token_type,
  scope,
  id_token,
  session_state,
}) =>
  authAdapter.linkAccount({
    providerAccountId,
    userId,
    provider,
    type,
    refresh_token,
    access_token,
    expires_at,
    token_type,
    scope,
    id_token,
    session_state,
  });

IMHO its next-auth's responsibility to provide a "just works like in the docs" and "does not break randomly in the future" solution to this. And not manually patching the adapter (as can be seen by the issues that just follow the Getting Started guide with a different provider and run into this problem).

Moving this option to the adapters that have a strict schema like Prisma and the other ORMs would also be a option.

[ThangHuuVu]

While this addresses a pain point for the library usage with special OAuth providers, I too don't feel like adding a new configuration is a good approach. I'm thinking of an alternative: We make the adapter strip everything but the default fields as described in Model by default, use Zod or something to validate & throw an error if the object contains extra fields. Only when the user explicitly defines the extra fields via TS Generic or Module Augmentation, then the validation passes. This approach provides a robust default which I think would benefit more users 💭

[skirsten]

While I like your idea @ThangHuuVu there is still one pain point for me:

throw an error if the object contains extra fields

if the provider decides to add a new field to the response this will break all existing deployed instances of next-auth. For example if GitHub suddenly adds a new field to their response, this will throw type validation errors and nobody will be able to login with GitHub until its patched.

[palminha]

While I like your idea @ThangHuuVu there is still one pain point for me:

throw an error if the object contains extra fields

if the provider decides to add a new field to the response this will break all existing deployed instances of next-auth. For example if GitHub suddenly adds a new field to their response, this will throw type validation errors and nobody will be able to login with GitHub until its patched.

This have a HUGE impact... everytime a provider adds a new options, this will break all the applications in productions. A lot of related issues with different providers, that people are not aware of what's happening:

• https://github.com/nextauthjs/next-auth/issues/5454
• https://github.com/nextauthjs/next-auth/issues/4907
• https://github.com/nextauthjs/next-auth/issues/5397

[palminha]

@balazsorban44 @ThangHuuVu for sure this one is breaking a lot of production apps... at least for Azure AD or GitHub based (see related issues above)

[palminha]

more issues reporting similar OAuthAccountNotLinked problems with optional parameters: https://github.com/nextauthjs/next-auth/issues/1446 https://github.com/nextauthjs/next-auth/pull/5371

[palminha]

My 5 cents on the topic...

possible behavior could be simply next-auth ignores the optional parameter, launches a warning but does not crash.

[soulchild]

adapter.linkAccount = ({_unwanted, ...data}) => adapter.linkAccount(data)

If I'm not mistaken, this leads to a recursion and eventually to a Maximum call stack size exceeded error. You need to store a reference to the original function before overriding it.

Here's my approach to ignore Keycloak's not-before-policy and refresh_expires_in attributes:

const adapter = PrismaAdapter(prisma);
const _linkAccount = adapter.linkAccount;
adapter.linkAccount = (account) => {
  // eslint-disable-next-line @typescript-eslint/no-unused-vars
  const { 'not-before-policy': _, refresh_expires_in, ...data } = account;
  return _linkAccount(data);
};

not-before-policy is especially tricky because of the - character which is not allowed in field names. So it's impossible to just add it to the Prisma schema. @map doesn't help either, because that just controls the name of the column in the database, not the attribute name.

In general, I agree that it'd probably be better to explicitly specify all attributes to avoid breaking your application if your provider decides to deliver additional (unknown) properties in the future.

[lfDev28]

Is there any consensus on a fix for this? Trying to get Azure AD auth working...