Allow requiring authenticated user in middleware on a matcher

[sirupsen]

Hey! I think it's far too easy to miss a getUser() to require authentication in server actions, so we moved all auth to be required in the middleware on URL prefix matches. We probably won't use this library until it's possible to enforce the middleware to auth all paths under some prefix (not just run the logic, as the matcher does), just too risky.

[PaulAsjes]

Would you mind elaborating a bit more on what you're after? You can match via a prefix in the middleware, e.g.

export const config = {
  matcher: ['/dashboard/:path*'],
}

but your comment suggests that that's not what you're after. Can you talk a bit more about what you'd like to see in this package?

[sirupsen]

@PaulAsjes as far as I understand from the docs/skimming the code, this matcher doesn't force authentication on the path, i.e. returning an HTTP 401 if the user isn't authenticated.

It just runs the authentication middlware. The user is responsible for calling getUser. This is too insecure for my liking, as it's far too easy to forget calling getUser e.g. in a server side mutation.

But maybe I don't have it right? 🤔

[PaulAsjes]

Thanks I think I get it now. At the moment we throw an error if you use getUser but don't have the path specified in your middleware. You'd like the opposite to be true as well that if you have the path in your middleware but don't call getUser we'd also error or return a 401.

I'll talk this over with the team on what we can do for this scenario.

[sirupsen]

@PaulAsjes to add concrete facts to the concern, we caught in a review a somewhat legit missing getUser() for a server mutation in our application. That's why I created our own middleware (that I'd like to replace with yours) to ensure that we authenticate by default, to not rely on manual calls—like how most auth frameworks I'm used to in Rails, Django, etc. would do 👍🏻

[PaulAsjes]

Just so I can get my head around this, what's the use case for wanting a page that is protected by auth but you don't want to fetch the current user?

There's some discourse in the community on whether your auth should be controlled via middleware or on a per-route basis, hence why I want to make sure I understand the use case before we implement anything.

[sirupsen]

Right, I agree that you should always fetch the user and perform the necessary checks/joins in the method to ensure the user indeed has permission to access the necessary data. In a multi-tenancy scenario, you should always call getUser() to ensure the action is scoped to that organization. Ideally, however, you check in the middleware that getUser() has access to perform actions on the organization's ID if it's e.g. in the URL or a header.

However, in a single-tenancy scenario, you might have sendMoney(amount: number, email: string). You don't need the current user here, as everyone has permission to sendMoney()... so you never thought to call getUser(). You may then revoke's that person's access in WorkOS, and your middleware would correctly expire their access token. However, you've leaked the signature for that POST call that calls sendMoney(). They would still be able to invoke that Next.js action because you don't call getUser(). The middleware would offer a layer of protection here

As for the discourse, I think it's far, far too risky to rely on developers always calling getUser() if there's not some protection in place... It's problematic that tenant scopes aren't enforced here, however, that's very difficult for you to do generically. But, I strongly believe that you should be secure-by-default for the single-tenant scenario

[pawel-alphadoc]

Hi, I agree with @sirupsen. It is very easy to forget to add getUser (I don't always need user data) on pages that require authentication. It would be very useful to use middleware on certain routes to make sure the user is authenticated. Are you planning to add this feature? @PaulAsjes

[PaulAsjes]

Thanks both, we have a good idea of the problem now and are thinking over what the best solution would be.

[PaulAsjes]

@sirupsen @pawel-alphadoc we've had a think and have come up with a hypothetical solution, but want to run it by you first before we fully implement it.

We're proposing a new option you can pass into authkitMiddleware that will make every page be protected regardless of whether you call getUser or not. You can provide an allow list of paths that you still want logged out users to be able to see (e.g. your homepage and log in page):

import { authkitMiddleware } from "@workos-inc/authkit-nextjs";

export default authkitMiddleware({
  strictMode: {
    allowedLoggedOutPaths: ["/"],
  },
});

export const config = { matcher: ["/", "/account/:path*", "/admin"] };

In the above example, all the pages in the matcher are protected by default, with the exception of the explicitly provided "/" path. Caveat is that this essentially puts the onus of auth on the middleware, rather than on a page level. There is still some discourse on whether this is a good idea or not, but this approach will give the user the option to pick which way they want to go.

Naming is TBD, but hopefully this gets the point across.

[sirupsen]

That would do it for me if I understand it correctly, as long as allowedLoggedOutPaths also supports the same glob/match syntax, and not just strings 👍🏻

It might be nice if the naming is symmetric, e.g. loginRequired and loginOptional

[benoitgrelard]

It might be nice if the naming is symmetric, e.g. loginRequired and loginOptional

You mean between getUser's ensureSignedIn and this new thing which is the opposite?

[sirupsen]

That'd be great too, I don't know all the vocabulary here; but yeah all for signin/login symmetry. Here I was specifically referring to the matcher and allowedLoggedOutPaths

[PaulAsjes]

Naming is hard :) we'll work on that part.

allowedLoggedOutPaths will use the same logic as Next.js' middleware matcher.

[sirupsen]

Perfect

[PaulAsjes]

Thanks for all your feedback, this has been released! Update to v0.5.0 and refer to the readme on how to use the new functionality.