nextauthjs / next-auth

Authentication for the Web.
https://authjs.dev
ISC License
23.24k stars 3.18k forks source link

The TypeScript signature for `DefaultAdapter.updateUser` seems wrong #6429

Closed nbouvrette closed 1 year ago

nbouvrette commented 1 year ago

Environment

This applies to the latest version of next-auth.

Reproduction URL

https://github.com/nextauthjs/next-auth/blob/6132c3fa751b647dd8261ac9b30e24fb140a2f1a/packages/next-auth/src/adapters.ts#L87

Describe the issue

When creating an adapter, because updateUser expects a user: Partial<AdapterUser> this means that the user.id ca be undefined.

This means if we follow the type definition that this method can be called without a user id, and the expectation is that it returns a valid user object... which makes no sense.

I think that the correct signature should be:

updateUser: (user: Pick<AdapterUser, 'id'> & Partial<Omit<AdapterUser, 'id'>>) => Awaitable<AdapterUser>

How to reproduce

And try to create an adapter that implements the updateUser method - you will see that the user id can be undefined as per the type definition here: https://github.com/nextauthjs/next-auth/blob/6132c3fa751b647dd8261ac9b30e24fb140a2f1a/packages/next-auth/src/adapters.ts#L87

Expected behavior

I would expect the user id to always be provided in updateUser, or if there is a reason it won't be provided, then the method should also allow returning null instead of a user object.

I'm not familiar enough with next-auth to know if this could happen, but the current signature does feel odd.

stale[bot] commented 1 year ago

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!

nbouvrette commented 1 year ago

Bump

stale[bot] commented 1 year ago

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!

nbouvrette commented 1 year ago

Looks like this issue has already been fixed by looking at the current types.

MonstraG commented 11 months ago

I think this still must allow returning null - what if I provide incorrect id?

nbouvrette commented 11 months ago

Not sure what you mean @MonstraG - this is about the argument's type - it should always pass a valid id, otherwise how can there be a session?

MonstraG commented 11 months ago

well getUser returns AdapterUser | null, and updateUser calls getUser, so it have to return null?

https://github.com/nextauthjs/next-auth/blob/c111b436d26fc7cfd3c3f86d82ceb0ce7dce72cd/packages/adapter-upstash-redis/src/index.ts#L238-L242

nbouvrette commented 11 months ago

I think the Adapter's getUser can return null is the user doesn't exist, but I don't think it make sense in the updateUser context. I actually have this code in my Adapter, which should honestly never be triggered but that does fix the type issue you are describing:

    if (!user) {
      throw new Error('user was not found while updating a next-auth user')
    }