Skip to content

Conversation

@balazsorban44
Copy link
Member

@balazsorban44 balazsorban44 commented Dec 13, 2023

Ref: https://twitter.com/Jarpiino/status/1734764485829923040

If the profile() callback of a provider (that is used to create a new user) does not return an id or it's missing before reaching a database Adapter, we should generate one via crypto.randomUUID() in the core library. We already did this at the Adapter level, but it's better to do it in the core so it's consistently there.

Do the same for the credentials provider's authorize() method too, marking id optional when it's not really needed.

This should also fix a rather annoying issue that Adapters need to generate an ID in createUser. Instead we will always pass one for certain, and the adapter can choose to either ignore or use it.

NOTE: Using the modified adapters might break next-auth v4, since it would always assume there is an ID coming from next-auth. We can make the same change for the v4 package, but it's likely a breaking change for the adapters. 🤔 I'll likely have to move the adapter changes to a different PR, where we can add a BREAKING CHANGE note to bump these adapters with a major. Will think about it. #9381

@vercel
Copy link

vercel bot commented Dec 13, 2023

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

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2023 4:59am
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
auth-docs-nextra ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2023 4:59am
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2023 4:59am

@github-actions github-actions bot added providers adapters Changes related to the core code concerning database adapters core Refers to `@auth/core` labels Dec 13, 2023
@github-actions github-actions bot added dynamodb @auth/dynamodb-adapter neo4j @auth/neo4j-adapter pouchdb @auth/pouchdb-adapter upstash-redis @auth/upstash-redis-adapter drizzle @auth/drizzle-adapter kysely d1 azure-tables labels Dec 13, 2023
@github-actions github-actions bot removed dynamodb @auth/dynamodb-adapter neo4j @auth/neo4j-adapter pouchdb @auth/pouchdb-adapter upstash-redis @auth/upstash-redis-adapter typeorm @auth/typeorm-adapter kysely labels Dec 13, 2023
@github-actions github-actions bot added neo4j @auth/neo4j-adapter typeorm @auth/typeorm-adapter labels Dec 13, 2023
@balazsorban44 balazsorban44 marked this pull request as ready for review December 13, 2023 05:01
@balazsorban44 balazsorban44 merged commit 5c1df64 into main Dec 13, 2023
@balazsorban44 balazsorban44 deleted the fix/user-id branch December 13, 2023 05:02
balazsorban44 added a commit that referenced this pull request Dec 13, 2023
* fix: add default user id generation

* refactor: simplify `createUser` in adapters

* fix typeorm

* fix next-auth

* fix tests

* revert adapter changes

* revert

* revert

* simplify adapter util

* fix build errors
@balazsorban44 balazsorban44 changed the title fix: add default user id generation feat: add default user id generation Dec 13, 2023
@pippinmole
Copy link

pippinmole commented Jan 24, 2024

Hi,

Seriously confused why you have changed id to optional on the user object. How are you meant to uniquely identify users now?

Some of the logic in your initial post content doesn't make any sense:

If the profile() callback of a provider (that is used to create a new user) does not return an id or it's missing before reaching a database Adapter, we should generate one via crypto.randomUUID() in the core library

This would imply that the user always has an id, because the core library generates one if it's missing.

Thanks

@balazsorban44
Copy link
Member Author

balazsorban44 commented Jan 25, 2024

Yep, that's the case. Users will always have an id, the whole point of this PR. Feel free to open a bug report if you are experiencing a bug! 🙏

@pippinmole
Copy link

Hi,

Would you be able to explain why the user id field is nullable here then? https://github.com/nextauthjs/next-auth/pull/9380/files#diff-4c2458380e50c967c70d7afe819dc7412180f9f817043f27441de3498e4d0479R469

Thanks

@balazsorban44
Copy link
Member Author

balazsorban44 commented Jan 25, 2024

The User interface is for the developer/user-land code. Even if they don't set an id, we'll generate one.

The one used in database adapters is AdapterUser

@pippinmole
Copy link

By 'user-land' code, do you mean the user object returned from calling await auth():

  const session = await auth();
  const user = session?.user; // This

Surely if you always generate an ID no matter what, it should never be null on the 'user-land' code.

Screenshot 2024-01-25 at 10 19 19

Further, why is the database user ID never null, but the session user id is nullable?

@davidwinter
Copy link
Contributor

Is there a reason why the user object isn't available anymore in the session callback?

image

Unless I'm misreading the type information, it should be available? But I may have overlooked something... I'm using the Dynamo database adapter and it had been working up until 5.0.0-beta.5. It looks like this issue was introduced with this change 🤔

@balazsorban44
Copy link
Member Author

balazsorban44 commented Jan 25, 2024

user.id still can be nullable, since we don't expose it by default:

user: {
name: session.user?.name,
email: session.user?.email,
image: session.user?.image,
},

So this is the correct type Session["user"] at the moment.

You can follow #9702 where we might change this and always return AdapterUser (with id: string) instead.

Please open issues for bug reports or other concerns, this is a closed PR and not going to be be monitored for further activity.

#9380 (comment) this is already fixed just not released yet. It's just a type bug, the object is there as before.

@sparanoid
Copy link

After upgrading to 1.0.12 this change breaks PrismaAdapter:

Type 'import("/Users/me/Git/project/node_modules/.pnpm/@[email protected][email protected]/node_modules/@auth/core/adapters").Adapter' is not assignable to type 'import("/Users/me/Git/project/node_modules/.pnpm/[email protected][email protected][email protected][email protected][email protected]/node_modules/next-auth/adapters").Adapter'.
  Types of property 'createUser' are incompatible.
    Type '((user: AdapterUser) => Awaitable<AdapterUser>) | undefined' is not assignable to type '((user: Omit<AdapterUser, "id">) => Awaitable<AdapterUser>) | undefined'.
      Type '(user: AdapterUser) => Awaitable<AdapterUser>' is not assignable to type '(user: Omit<AdapterUser, "id">) => Awaitable<AdapterUser>'.
        Types of parameters 'user' and 'user' are incompatible.
          Property 'id' is missing in type 'Omit<AdapterUser, "id">' but required in type 'AdapterUser'.ts(2322)

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` neo4j @auth/neo4j-adapter next-auth providers typeorm @auth/typeorm-adapter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants