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

[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 opened this issue Feb 11, 2024 · 21 comments · Fixed by #1444
Labels
documentation Improvements or additions to documentation

Comments

@callmeberzerker
Copy link
Contributor

callmeberzerker commented Feb 11, 2024

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. 🙏

@callmeberzerker callmeberzerker added the bug Something ain't right... label Feb 11, 2024
@pilcrowonpaper
Copy link
Member

What version of node-postgres are you using?

@callmeberzerker
Copy link
Contributor Author

callmeberzerker commented Feb 11, 2024

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
Copy link
Member

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

@callmeberzerker
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

Hm, that's weird since everything is tested

@callmeberzerker
Copy link
Contributor Author

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
Copy link
Member

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

@callmeberzerker
Copy link
Contributor Author

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

Do you mean in the drizzle configuration or?

@pilcrowonpaper
Copy link
Member

pilcrowonpaper commented Feb 13, 2024

Yes, in timestamp() in the schema

@callmeberzerker
Copy link
Contributor Author

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
Copy link
Contributor Author

callmeberzerker commented Feb 13, 2024

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

const result = await adapter.getSessionAndUser(databaseSession.id);

@callmeberzerker
Copy link
Contributor Author

callmeberzerker commented Feb 14, 2024

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
Copy link
Contributor Author

callmeberzerker commented Feb 15, 2024

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
Copy link
Member

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

@pilcrowonpaper pilcrowonpaper added documentation Improvements or additions to documentation and removed bug Something ain't right... labels Feb 15, 2024
@Quatton
Copy link

Quatton commented Feb 19, 2024

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

@Hebilicious
Copy link
Contributor

Hebilicious commented Feb 19, 2024

Literally just hit this one as well 😅 Thanks @callmeberzerker
Regarding #1424 (comment) I believe this is Drizzle intended behaviour (see drizzle-team/drizzle-orm#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
Copy link

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
Copy link

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

@pilcrowonpaper
Copy link
Member

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

@zbeyens
Copy link

zbeyens commented Jun 11, 2024

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];
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants