lucia-auth / lucia

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

RFC: Remove adapter packages #1639

Open pilcrowOnPaper opened 1 month ago

pilcrowOnPaper commented 1 month ago

Database adapters have been a fundamental part of Lucia from the very start. It allows Lucia to work with any databases and Lucia provides adapter packages to make the integration easier. But is this correct approach?

I've... actually never considered that. It just made sense when starting, especially since that's how libraries like NextAuth did it. But now I have a definite answer. The adapter model is fine but we shouldn't provide any built-in adapters.

The ready-made adapters are a pain to work with and has the domino effect of making Lucia core bloated. From the start, it's been a struggle make each adapter flexible while keeping the API simple. Take for example the many SQL adapters. We have to dynamically create prepared statements based on custom attributes and escape the table and column names. And it affects the core as well. I don't think anybody likes getUserAttributes() and define module. These API had to be included to provide flexibility and type-inference. But is it worth it? We added all these layers of abstractions so that so devs don't have to manually write queries, but really that's probably the easiest thing for web devs to do.

I'm now convinced that it's a bad idea to create libraries around database drivers and ORMs if you want to provide any sort of flexibility.

So if Lucia doesn't write your queries for you, what can the next version of Lucia look like?

// auth.ts
import { Lucia } from "lucia";
import { db } from "./db.js";

import type { DatabaseAdapter, SessionAndUser } from "lucia";

const adapter: DatabaseAdapter<Session, User> = {
    getSessionAndUser: (sessionId: string): SessionAndUser<Session, User> => {
        const row = db.queryRow(
            "SELECT session.id, session.user_id, session.expires_at, session.login_at, user.id, user.username FROM session INNER JOIN user ON user.id = session.user_id WHERE session.id = ?",
            [sessionId]
        );
        if (row === null) {
            return { session: null, user: null };
        }
        const session: Session = {
            id: row[0],
            expiresAt: new Date(row[2] * 1000),
            loginAt: new Date(row[3] * 1000),
        };
        const user: User = {
            id: row[4],
            username: row[5],
        };
        return { session, user };
    },
    deleteSession: (sessionId: string): void => {
        db.execute("DELETE FROM session WHERE id = ?", [sessionId]);
    },
    updateSessionExpiration: (sessionId: string, expiresAt: Date): void => {
        db.execute("UPDATE session SET expires_at = ? WHERE id = ?", [
            expiresAt,
            sessionId,
        ]);
    },
};

export const lucia = new Lucia(adapter, {
    sessionCookie: {
        secure: false,
    },
});

export interface User {
    id: string;
    username: string;
}

export interface Session extends LuciaSession {
    userId: string;
    loginAt: Date;
}
const { session, user } = await lucia.validateSession(sessionId);
if (session === null) {
    throw new Error("Invalid session");
}
const username = user.username;

The biggest and obvious change is that you have to manually define your adapter. I've used a SQL driver as an example so it's a bit verbose (also see the very clean Prisma adapter), but the queries aren't that complicated at all. We could add sample code for popular drivers and ORMs in the docs, which would make the setup easier too.

From an API design perspective, I think this is beautiful. For starters, minimal generic usage and config options. But I really love the most is that LuciaSession is just an interface that your custom session object needs to satisfy. No more getSessionAttributes() or declare module funkiness. This also means that discriminated unions just works for both sessions and users

const adapter: DatabaseAdapter<Session, User> = {
    /* ... */
};

type Session = SessionA | SessionB;

export interface SessionA extends LuciaSession {
    a: any;
}

export interface SessionB extends LuciaSession {
    b: any;
}

For creating session, just write a single query. We'll provide the utilities for generating IDs and calculating expirations.

import { lucia, generateSessionIdWithWebCrypto } from "lucia";
import { db } from "./db.js";

export async function createSession(userId: string): string {
    const sessionId = generateSessionIdWithWebCrypto();
    const expiresAt = lucia.getNewSessionExpiration();
    db.execute(
        "INSERT INTO session (id, expires_at, user_id, login_at) VALUES (?, ?, ?)",
        [sessionId, expiresAt, userId, Math.floor(new Date().getTime() / 1000)]
    );
    return sessionId;
}

At this point, why keep the user handling? I've thought about this for quite a while, and the simple answer is that it's better to have an API that works with one thing (user + session) rather than two things (user + session or session). If you just need any more flexible than this - just build your own. It's likely that whatever API we provide won't be enough. I think it's fine for Lucia to have some opinions and this feels like the place to draw the line.

On a final note, this could allow us to bring back framework-specific APIs. I don't have the appetite to deal with the mess of JS frameworks, but maybe :D

This will be part of Lucia v4, which will probably be released late this year or early next year. It's very possible that v4 will be our last major update.

OllieJT commented 1 month ago

...and by version 7, Lucia will just be a README on how to setup auth. 😅 (jk)

This makes a lot of sense to me.

Lutefd commented 1 month ago

This makes a lot of sense, previously at work I had to rewrite the drizzle adapter, which was a little pain because of the typing, just to change the users queries to query a junction table along with it. This suggested approach would've made that job a lot easier, although it might steep up the learning curve for inexperienced developers that aren't that comfortable with SQL yet, but even that could be a good thing if the docs has a basic guide/example on how to build adapters, creating an opportunity for them to learn.

bhavishya-sahdev commented 1 month ago

My experience with the Drizzle adapter has been fine so far but I appreciate this change, having more control over how the queries work would be great.

Giggiux commented 1 month ago

Would providing some adapters for "simple"/"base" uses makes sense? Maybe not in the core Lucia, but under different packages?

If I need just a "quick" user authentication with Drizzle without any "customization," could we still have the adapter?

To avoid copying/pasting eventual examples?

But I agree with whoever came before me: having more control is very nice and allows devs to have more control, even if there is a little bit of a cost in terms of speed.

pilcrowOnPaper commented 1 month ago

@Lutefd Fortunately the adapter code is going to be significantly simpler for ORM users. Like for Prisma:

const result = await this.sessionModel.findUnique({
  where: {
    id: sessionId
  },
  include: {
    user: true
  }
);
if (result === null) {
  return { session: null, user: null };
}
const { user, ...session } = result;
pilcrowOnPaper commented 1 month ago

@Giggiux For Prisma and Drizzle maybe, though I think it'll be just easier to copy-paste the code from the docs. Unlike the SQL drivers, you won't need to edit the code for a basic setup.

mirorauhala commented 1 month ago

Consider creating an init command like npx lucia-auth@latest init --provider=discord and it would scaffold the correct files in place.

This command would plop in place the fully written out auth code, allowing the dev to focus more on the app while retaining the access to edit the adapter code if need be.

Similar to how shadcn/ui does UI components, could be done for auth.

pilcrowOnPaper commented 1 month ago

For Prisma, this will just work with full type inference. I'm guessing something similar is possible with drizzle

import type { Session, User } from "@prisma/client";
import type { DatabaseAdapter, SessionAndUser } from "lucia";

const adapter: DatabaseAdapter<Session, User> = {
    getSessionAndUser: async (
        sessionId: string
    ): Promise<SessionAndUser<Session, User>> => {
        const result = await client.session.findUnique({
            where: {
                id: sessionId,
            },
            include: {
                user: true,
            },
        });
        if (result === null) {
            return { session: null, user: null };
        }
        const { user, ...session } = result;
        return { session, user };
    },
    deleteSession: async (sessionId: string): Promise<void> => {
        await client.session.delete({
            where: {
                id: sessionId,
            },
        });
    },
    updateSessionExpiration: async (
        sessionId: string,
        expiresAt: Date
    ): Promise<void> => {
        await client.session.update({
            where: {
                id: sessionId,
            },
            data: {
                expiresAt,
            },
        });
    },
};
theorib commented 1 month ago

This makes a lot of sense, it's so much better to empower users to create their own database queries, wether through an ORM or not and have total control over that aspect of things.

Giving users a clear API on how to build an adapter is a much better approach and I'm very happy with the proposal!

One of side effect of using Lucia is having ownership and clarity over the whole auth process and I believe this is another step in that direction.

pilcrowOnPaper commented 1 month ago

@mirorauhala I've considered that on numerous occasions but I'm not sure how well that'd work. I'd love to see a PoC tho.

Lutefd commented 1 month ago

@pilcrowOnPaper I think the drizzle one will be pretty similar to what you've shared, the current way already works well like this, so I guess it should work in the new way with complete type inference, the snippet you proposed looks way cleaner as well.

export class DrizzleSQLiteAdapter implements Adapter {
    private db: BaseSQLiteDatabase<any, any, typeof schema>;
    private sessionTable: SQLiteSessionTable;
    private userTable: SQLiteUserTable;

    constructor(
        db: BaseSQLiteDatabase<any, any, any>,
        sessionTable: SQLiteSessionTable,
        userTable: SQLiteUserTable
    ) {
        this.db = db;
        this.sessionTable = sessionTable;
        this.userTable = userTable;
    }

    async deleteSession(sessionId: string): Promise<void> {
        await this.db
            .delete(this.sessionTable)
            .where(eq(this.sessionTable.id, sessionId));
    }

    async deleteUserSessions(userId: string): Promise<void> {
        await this.db
            .delete(this.sessionTable)
            .where(eq(this.sessionTable.userId, userId));
    }
    async getSessionAndUser(
        sessionId: string
    ): Promise<[session: DatabaseSession | null, user: DatabaseUser | null]> {
        const result = await this.db.query.sessionTable.findFirst({
            where: eq(this.sessionTable.id, sessionId),
            with: {
                user: {
                    with: {
                        usersToRoles: {
                            with: {
                                role: true,
                            },
                        },
                    },
                },
            },
        });

        if (!result?.user) return [null, null];
        const session = {
            id: result.id,
            userId: result.userId,
            expiresAt: result.expiresAt,
            is_oauth: result.is_oauth,
        };
        return [
            transformIntoDatabaseSession(session),
            transformIntoDatabaseUser(result.user),
        ];
    }

Building on what @mirorauhala suggested, you'd need to have it prompt the db that's being used to properly set it up, as well if there's an ORM being used and which if so, that would require to have base adapters to all the major DBs, as sometimes the typing changes between them (eg. SQLite), and supported ORMs. It should work but it seems like a ton of work and a bit of a headache to maintain without heavy community support.

Shadcn/ui works because it's already assuming you're using react and from the folder Layout it can detect if it's using Next to add the "use client" directives or not. With auth there's a lot more variables and it seems more complex to set it up, but it's a great idea nonetheless if it had more people interested in maintaining it.

tomwatkins1994 commented 1 month ago

I think this makes a tonne of sense. Lucia already gives you a lot of control compared to other Auth libraries so I think users of it will not be put off by writing the database implementation themselves and will probably appreciate it, if anything.

I think it also makes sense because if a library like Drizzle suddenly ships a breaking change then the Drizzle adapter library suddenly has to change too or pin a peer dependency that could stop users of Drizzle from updating, not great either way.

If decent examples were provided on how to set it up that can be copied and pasted into your code base then I'm sure that'd be a great starting point.

ERmilburn02 commented 1 month ago

Shadcn/ui works because it's already assuming you're using react and from the folder Layout it can detect if it's using Next to add the "use client" directives or not.

@Lutefd Not entirely correct, the shadcn/ui package does try to detect if you're using Next.js, but it's only used to set some default options for the components.json file, which it uses when writing components. It doesn't need to detect the framework, it's just for convenience to make it faster to setup.

Lutefd commented 1 month ago

Shadcn/ui works because it's already assuming you're using react and from the folder Layout it can detect if it's using Next to add the "use client" directives or not.

@Lutefd Not entirely correct, the shadcn/ui package does try to detect if you're using Next.js, but it's only used to set some default options for the components.json file, which it uses when writing components. It doesn't need to detect the framework, it's just for convenience to make it faster to setup.

@ERmilburn02 Yeah, I've just simplified because that isn't really the point, I never said it needs to, only that it can, usually if you're doing a init that need to be iterated on you'll need to write a config file, @shadcn/ui does prompt you if you're using RSC or not which would be one of the only variables on the way the components are written, other than the file paths that mostly change where they are written to and the style/color variants for the css config.

And even that is in most cases a insertion of the "use client" directive. It detects if it's Next.js because as of now it's the only major framework to use RSC extensively, but when dealing with auth there would be a lot of other variables than that, even in Next.js land the way the oauth callbacks are written would differ if it were written for the Pages router or the App router. Outside of that it would be different for Astro, Sveltekit, Nuxt, etc..

It would also need to have different base inserts, due to session creation, for some dbs due to their column typing, so it would need to prompt that, as it would need to create a base db adapter, if you're using an ORM on top of that it would need to have a different template for each of those frameworks and the list goes on, not even stoping to consider different runtime environments, like vercel's edge, hono, node, which all have their particularities (like edge couldn't really handle drizzle base connection to postgres out of the box last time I've checked), that seems like a nightmare to handle for each Provider as suggested, of course there could always be limits and starting with only a few, but even then it would be complicated.

It would be a large project that should be outside of the main Lucia project scope, even if you limit which frameworks and environments, don't get me wrong it's a great idea and maybe lucia's new design could really make it possible, but it would still require a lot of thought and work.

JS land is just a nightmare.

maietta commented 1 month ago

This looks like a great direction and I welcome this proposed change.

ThimoDEV commented 1 month ago

This makes a lot of sense so we have maximum flexibility. I would love to see in the docs the changes about how V3 did things and how you can do the same in V4 and maybe where you can start doing your own things. Then its perfect

hansaskov commented 1 month ago

I'm assuming this would also empower the user to implement their own Redis session cache, which was somewhat of a struggle to do in V3.

pilcrowOnPaper commented 1 month ago

@hansaskov yes! it'll be significantly easier to build adapters compared to v3

cotyhamilton commented 1 month ago

Having built an adapter for Lucia, I think this is a great move.

I think my project would better as some copy/paste code from a README or blog to encourage others to control how Lucia works, rather than provide a package that needs to be maintained and potentially not live up to everyone's needs/wants/demands.

saturnonearth commented 1 month ago

I think the trade off is definitely worth it - I like the changes!

pilcrowOnPaper commented 1 month ago

You can see the full implementation on the v4-remove-adapter-package branch (note this isn't the v4 branch)

https://github.com/lucia-auth/lucia/tree/v4-remove-adapter-package

daniellovera commented 1 month ago

Fully support this. Just this week I spent 4 hours debugging (yes, yes, a bit of a skill issue) before I realized that Lucia wasn't working because my postgres.js connection the adapter was using included transform: postgres.camel,. I created a new Lucia specific postgres.js connection without that transform and it instantly worked.

I think clear and explicit is better than obfuscated and implicit, even if it's more verbose at first, as long as that verbosity is well encapsulated (as it is in your example). It's easier for people new to Lucia to grasp what Lucia is doing, and once people are comfortable with that then they can hide the verbosity in a way that suits them.

I switched to Lucia from Supabase, after spending days trying to resolve this https://github.com/supabase/supabase-js/issues/1010, among some other issues. AFAIK, Supabase STILL hasn't resolved this beyond suppressing the warning, and in general Supabase is far more sequential database calls than necessary too - a performance killer if your backend isn't in the same datacenter as your Supabase instance. If I wanted a "do everything for me and keep me reliant on the service" package I'd have stayed with Supabase or similar.

I'd suggest that if you're trying to make money with Lucia as as service, you eventually have to go the route of trying to do everything for customers to make it as one click as possible, and that will only grow the abstractions and difficulty necessary to turn everything into a one-liner of a function call. If you're not going down that route, clearing away the high costs (to you) of abstractions in favor of returning control to devs yields solid performance and maintenance benefits for everyone at a small cost of requiring devs to manually write queries. And if the devs wanting to use Lucia cannot or will not manually write queries even with Claude, Copilot, or ChatGPT, then is Lucia the right tool for them in the first place?

Edit: Worded closing question more appropriately.

CanRau commented 1 month ago

Also agree with this change, especially as we have a little unusual db schema I think so the defaults wouldn't really work for us and a good point @daniellovera made (I think) is "you probably shouldn't use Lucia (or similar packages or even roll your own auth) if you can't write those queries, much safer to go with a full-blown service or start (really) understanding the necessary queries / Lucia.

pilcrowOnPaper commented 1 month ago

Here's a quick demo using Drizzle ORM

https://github.com/pilcrowOnPaper/lucia-remove-adapter-package-demo

There are no docs but you can play around with the implementation with npm i lucia@experimental-remove-adapter-package

pilcrowOnPaper commented 1 month ago

One thing I'm not 100% sure about is whether we should have lucia.createSession(). Right now, I think it's better to just provide generateSessionId() and getNewSessionExpiration() and let devs query the db themselves

theorib commented 1 month ago

One thing I'm not 100% sure about is whether we should have lucia.createSession(). Right now, I think it's better to just provide generateSessionId() and getNewSessionExpiration() and let devs query the db themselves

I think this makes a lot of sense, that way we can manage our own database schema/queries however we want and just pass on data from lucia.

Basically lucia becomes a series of utility classes/functions but with a clear set of suggested guidelines for how to build your own auth. It sounds quite good. It almost makes the documentation harder to write than the code itself which is not a bad thing 😀

ThimoDEV commented 1 month ago

Having the freedom in every aspect of how to implement every single Lucia aspect makes total sense. So yes query our database ourself, structure it the way we want and let lucia know how we structured it to make it all work is perfect

ivanjeremic commented 1 month ago

adapters is what made me choose lucia, I agree that they don't need to be in core but instead just make them separate packages.

JoltCode commented 1 month ago

This should also prevent the gotcha of having an attribute named id conflicting with the pre-existing db column!

jshear commented 1 month ago

One thing I'm not 100% sure about is whether we should have lucia.createSession(). Right now, I think it's better to just provide generateSessionId() and getNewSessionExpiration() and let devs query the db themselves

With adapters separated from this library and lucia.createSession() removed, it seems like the only place User is actually handled is in the return type of getSessionAndUser() in the adapter type definition. Would allowing a session with a null user (or removing it completely) be as simple as changing this one type definition, or is there another part of this library that utilizes the User model?

Looking at the current V4, it seems like letting the developer decide if there's a User in the Session would give the most flexibility with no detriment to the functionality of the library. For example, you could remove the User type completely. If a developer wants to fetch a User with the session, their session definition could simply be

interface Session {
    id: string;
    expiresAt: Date;
    user: User;
}

Since the developer now has to build there own queries for session data, is there any reason to enforce the return type of those queries (e.g. requiring a User) and limiting the use cases of the library?

At this point, why keep the user handling? I've thought about this for quite a while, and the simple answer is that it's better to have an API that works with one thing (user + session) rather than two things (user + session or session)

Is the API working with the user in a meaningful way after the changes in V4? It seems like these changes (putting the onus of the queries on the developer) dissociate the User model from this library completely aside from adapter.getSessionAndUser() being expected to return both. If that's the case, maybe this library's API is more suited for just session management (with the User management being left to the adapters and the implementation around Lucia)?

Edit: I'm relatively new to using this library, so I don't mean to overstep, but honest question -- with the direction this library is going, it doesn't seem like it has any authentication aspects to it. Of course the underlying libraries you've created have tons of authentication utilities that are insanely helpful (thank you for all your work!!!), but from my new and potentially naive perspective this is looking much more like a session library than an authentication library. Is that the end goal -- for Lucia to be a session management tool and Oslo and Arctic to be utilities for building the authentication methods? Please correct me if I'm completely misguided here!

theorib commented 1 month ago

I would much rather decide for myself what is the exact shape of that user object and how to query it. Whether Lucia ends up dealing with the user object at all is a good question though. One way of doing this could be to pass arguments to Lucia, one of them being the query function that returns the user object. If you go that route though, I would ask myself why are we also not also passing as arguments the functions that create and return the session objects.

pilcrowOnPaper commented 1 month ago

@jshear This is probably just personal preference, but I don't like nesting the user object in the session object. Like, it doesn't work well if you have a createSession() method that returns the created user.

pilcrowOnPaper commented 1 month ago

Is that the end goal -- for Lucia to be a session management tool and Oslo and Arctic to be utilities for building the authentication methods

Honestly the end goal for me would be for Lucia docs to be a collection of tutorials and examples, much more fleshed out than what we have now.

jshear commented 1 month ago

@jshear This is probably just personal preference, but I don't like nesting the user object in the session object. Like, it doesn't work well if you have a createSession() method that returns the created user.

Agreed on that, good point. But there are still a few ways to get the best of both worlds I believe. Currently, we have Session and User types, and we have an adapter method getSessionAndUser that must return either null for session and user or a full value for session and user.

A small change in terminology and minor typing tweaks could make this much more dynamic with no loss in clarity (in my opinion).

Instead of having the type SessionAndUser<_Session, _User> (which requires either null for both session and user, or full session and user values), we could have something like SessionAndData<_Session, _Data>. The return type from the adapter when fetching session data would then be { session: _Session | null; } & _Data.

To make the implementation match the capabilities of the current version, _Data could be { user: User | null; }, making the full type { session: Session | null; user: User | null; }. However, if we don't want a user -- or, if we even want to associate completely different relations with a session, we can do so by altering the _Data generic. All other interactions where you only want to return the session and no other data (like a user) can still be done with this approach.

Not sure exactly what that would mean for createSession(), but you mentioned you were thinking of getting rid of that in favor of the lower-level utilities that can be used to create a session.

If you favor keeping the concept of a User, is there any reason that User can't be null when Session isn't null? That small tweak would still provide the majority of what I'm looking for in my personal projects.

If this goes against any other design decisions that I'm unaware of just let me know. But I've been getting familiar with this library recently and a change like this is the only thing I would need to use Lucia for all of my session use-cases instead of co-mingling Lucia with other session management depending on whether or not a user is authenticated. Since users are no longer intertwined with any part of the library other than the adapter getter function, I was optimistic that this would be a good time to make a change like this. Appreciate your time and consideration!

tobiasdiez commented 1 month ago

I really like the general direction. Good job!

A few suggestions:

// in user code lucia.createSession( { id: 'custom id' user: { id: 'my user' } })

// **** // or with only the user id stored export interface Session extends LuciaSession { userId: string; } const adapter: DatabaseAdapter = { getSession: (sessionId: string): Session => { const row = db.queryRow( "SELECT session.id, session.user_id, session.expires_at, session.login_at FROM session WHERE session.id = ?", [sessionId] ); if (row === null) { return null; } const session: Session = { id: row[0], expiresAt: new Date(row[2] 1000), loginAt: new Date(row[3] 1000), userId: row[1] }; return session; }, createSession(session: Session) { db.execute( "INSERT INTO session (id, expires_at, user_id, login_at) VALUES (?, ?, ?)", [session.id, session.expiresAt, session.userId, Math.floor(new Date().getTime() / 1000)] ); return { ... session, loginAt = ... } } // other methods as above }

// in user code const session = lucia.createSession( { id: 'custom id' userId: 'my user' }) const user = db.fetchUser(session.userId)


// in lucia core: public async createSession( session?: DeepPartial ): Promise { const sessionId = session?.id ?? generateIdFromEntropySize(25); const sessionExpiresAt = session?.expiresAt ?? createDate(this.sessionExpiresIn); return await this.adapter.createSession({ ...session, id: sessionId, expiresAt: sessionExpiresAt, }) }

lts20050703 commented 2 weeks ago

@pilcrowOnPaper There hasn't been any update to the v4-remove-adapter-package branch for a month. What's the current status of this RFC? I would love to try to migrate from v3 to v4-remove-adapter-package but the lack of update makes me question if it's dead, still a work in progress, or already finalized.

machak commented 4 days ago

@lts20050703 "This will be part of Lucia v4, which will probably be released late this year or early next year. It's very possible that v4 will be our last major update."