lucia-auth / lucia

Authentication, simple and clean
https://lucia-auth.com
MIT License
9.25k stars 471 forks source link

[Bug]: date.getTime() is not a function (originating in oslo) -> but the core problem is that nodejs-adapter from Lucia doesn't convert expiresAt as a date #1424

Closed callmeberzerker closed 7 months ago

callmeberzerker commented 7 months ago

Package

​@lucia-auth/adapter-postgresql

Describe the bug

So my first time deep delving into the codebase of Lucia so bear with me:

  1. I first verified that expires_at is timestamp in the database with timezone.
  2. When Lucia tries to validate the session it uses this.adapter.getSessionAndUser(sessionId); -> which will in turn go the database, fetch the session, convert the fields_with_underscores to fieldsWithUnderscores (ie. expires_at -> expiresAt).
  3. Next it will use oslo to check if the session isWithinExpirationDate -> but the expiresAt has been loaded as a string even though it is defined as a timestamp with zone.

I am using the following adapter for Postgres

const adapter = new NodePostgresAdapter(pool, {
    user: 'auth_user',
    session: 'session'
});

Here is how the database session looks like when loaded from the db by lucia.

{
  userId: 'scp1x8lzy270qh5',
  id: 'rqcui4nih4o6lix32xmxp1zzbukr6jp5qozm7m0q',
  expiresAt: '2024-03-06 19:29:17.726+00',
  attributes: {}
}

Here is the stack trace where it fails ultimately:

TypeError: date.getTime is not a function
    at isWithinExpirationDate (file:///E:/Projects/my-project/node_modules/.pnpm/oslo@1.0.1/node_modules/oslo/dist/index.js:34:30)

This is a severe issue I can't circumvent since upgrading (still in progress obviously) to v3. Any insight into this issue is more than welcomed. πŸ™

pilcrowOnPaper commented 7 months ago

What version of node-postgres are you using?

callmeberzerker commented 7 months ago

~Hmmm I don't have any such dependency installed (only pg) -> I don't know how it is working now...~

~I've tried running pnpm why node-postgres but it's definitely not present.~

EDIT: Sorry I saw that pg is actually node-postgres -> to answer your question.

devDependencies:
drizzle-dbml-generator 0.6.1
└─┬ drizzle-orm 0.29.3
  └─┬ pg 8.11.3 peer
    └─┬ pg-pool 3.6.1
      └── pg 8.11.3 peer
pilcrowOnPaper commented 7 months ago

Can you share your Drizzle/db schema? There's also a new Drizzle adapter https://lucia-auth.com/database/drizzle

callmeberzerker commented 7 months ago

I don't think the drizzle schema matter since the table is instantied with the sql scripts provided in the v3 upgrade documentation. Here is the relevant bit from the upgrade:

ALTER TABLE session ADD COLUMN expires_at TIMESTAMPTZ;

UPDATE session SET expires_at = to_timestamp(idle_expires / 1000);

ALTER TABLE session
DROP COLUMN active_expires,
DROP COLUMN idle_expires,
ALTER COLUMN expires_at SET NOT NULL;
COMMIT;

Here is the SQL to verify that the column is defined as expected.

SELECT column_name, data_type FROM information_schema.columns where table_schema = 'public' 
and table_name = 'session';
"column_name","data_type"
"id","character varying"
"user_id","character varying"
"expires_at","timestamp with time zone"

None the less here is the drizzle db schema definition on top of it (which at the moment Lucia should have no awareness off, but will try using the drizzle adapter so that I can start the app properly).

export const session = pgTable('session', {
    id: varchar('id', {
        length: 128
    }).primaryKey(),
    userId: varchar('user_id', {
        length: 15
    }).notNull()
      .references(() => authUser.id),
    expiresAt: timestamp('expires_at', { mode: 'date' }).notNull()
});
callmeberzerker commented 7 months ago

I can confirm that using DrizzlePostgreSQLAdapter solves the issue and the expiresAt is deserialized as a Date succeessfully.

  userId: 'scp1x8lzy270qh5',
  id: 'rqcui4nih4o6lix32xmxp1zzbukr6jp5qozm7m0q',
  expiresAt: 2024-03-06T19:29:17.726Z,
  attributes: {}
}

The NodePostgresAdapter still has the issue I describe above which I managed to trace down to the getSession and to the transformIntoDatabaseSession. At no point there is a conversion from the raw select query values (which are apparently strings) into a Date.

pilcrowOnPaper commented 7 months ago

Hm, that's weird since everything is tested

callmeberzerker commented 7 months ago

False positive test - has to be from what I've been observing. None the less I will check out the repository today or tomorrow and try to replicate the test (or explain why is it falsely positive).

pilcrowOnPaper commented 7 months ago

Hm yeah the tests are fine. Can you try setting withTimezone to true in timestamp?

callmeberzerker commented 7 months ago

Hm yeah the tests are fine. Can you try setting withTimezone to true in timestamp?

Do you mean in the drizzle configuration or?

pilcrowOnPaper commented 7 months ago

Yes, in timestamp() in the schema

callmeberzerker commented 7 months ago

As I said, the problem can't be with drizzle since that is only used for descriptive purposes since:

  1. The SQL table definition is the one as described in the Lucia v3 migration docs (you have a sample up top).
  2. NodePostgresAdapter doesn't use or care about drizzle schema.
callmeberzerker commented 7 months ago

I just checked here -> and I see no getSession() test in question. Am I seeing this correct @pilcrowOnPaper ? (trying to create/replicate it now)

EDIT: found it https://github.com/lucia-auth/lucia/blob/af2e48921e5378fe7fdf33fafb4d69940aaa823a/packages/adapter-test/src/index.ts#L36

callmeberzerker commented 7 months ago

Just a small update, I managed to checkout the Lucia repo -> and I can verify that the test works. Trying to find now what is the discrepancy between my project and the setup - or potentially some other function is being used in the production code that is not used by the test -> that forces the expiresAt to be marshalled as string.

It's Valentines Day so I guess I have nothing better to do. 🀣

callmeberzerker commented 7 months ago

After a lot of digging I found the source of my issue -> and I think the docs should warn of this.

If you use drizzle ORM in your project you must use the DrizzlePostgreSQLAdapter as well, since the drizzle PostgreSQL driver will override the default handling(parsers) for several database types.

See source code for drizzle-orm here:

https://github.com/drizzle-team/drizzle-orm/blob/0da1cba84da08bc0407821c9ab55b3e780ff5e3f/drizzle-orm/src/node-postgres/driver.ts#L41

Mystery solved.

@pilcrowOnPaper shout if you agree on the docs and if you would accept a PR

pilcrowOnPaper commented 7 months ago

Oh wow, that's sneaky. Yeah it should be probably mentioned in the docs - I guess at the top of the PostgreSQL database page.

Quatton commented 7 months ago

I've encountered this as well and thank you for your contribution to the docs!

Hebilicious commented 7 months ago

Literally just hit this one as well πŸ˜… Thanks @callmeberzerker Regarding https://github.com/lucia-auth/lucia/issues/1424#issuecomment-1947238448 I believe this is Drizzle intended behaviour (see https://github.com/drizzle-team/drizzle-orm/pull/1659)

@pilcrowOnPaper it might be worth fixing this at the adapter level or at the oslo level (accept string) as this could happen with other ORMs, not only Drizzle.

Something like this would work : date instanceof new Date() ? new Date(date) : date (I'm using this in a pnpm patch)

AsadSaleh commented 7 months ago

For me, this is the solution: change the timestamp options.mode from "string" to "date".

From this:

expiresAt: timestamp("expires_at", {
    withTimezone: true,
    mode: "string",
  }).notNull(),

To this:

expiresAt: timestamp("expires_at", {
    withTimezone: true,
    mode: "date",
  }).notNull(),

So the session table schema will look something like this:

export const sessionsTable = pgTable("sessions", {
  id: text("id").primaryKey().notNull(),
  userId: text("user_id")
    .notNull()
    .references(() => usersTable.id),
  expiresAt: timestamp("expires_at", {
    withTimezone: true,
    mode: "date",
  }).notNull(),
});
davidgomes commented 4 months ago

I was also only able to fix this by using DrizzlePostgreSQLAdapter instead of the Neon or the NodeJS PostgreSQL adapters.

pilcrowOnPaper commented 4 months ago

Yeah it's an issue with Drizzle since they override your driver's default date behavior

zbeyens commented 3 months ago

Getting the same error using Neon + NodePostgresAdapter with Prisma.

Fixed using:

class CustomNodePostgresAdapter extends NodePostgresAdapter {
  override async getSessionAndUser(
    sessionId: string
  ): ReturnType<
    (typeof NodePostgresAdapter)['prototype']['getSessionAndUser']
  > {
    const [session, user] = await super.getSessionAndUser(sessionId);

    if (session) {
      session.expiresAt = new Date(session.expiresAt);
    }

    return [session, user];
  }
}