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

Revisit our auth middleware behaviour Queries, Actions and APIs #1133

Open
infomiho opened this issue Apr 13, 2023 · 1 comment
Open

Revisit our auth middleware behaviour Queries, Actions and APIs #1133

infomiho opened this issue Apr 13, 2023 · 1 comment
Assignees
Labels
shouldfix We should do/fix this at some point

Comments

@infomiho
Copy link
Contributor

We have multiple places where we have some sort of auth related logic. Some of the logic is maybe a bit unclear and we might want to extend the existing logic.

1️⃣ Current state

Backend

Users can use the auth middleware with three features (Actions, Queries, APIs) which can be configured in the Wasp file by passing in the auth option.

// main.wasp
query getTasks {
  auth: true // <---
  fn: import { getTasks } from "@server/queries.js",
  entities: [Task],
}

// queries.js
export const getTasks = async (_args, context) => {
  if (!context.user) {
    throw new HttpError(401)
  }
  // ...
}

The logic in the auth middleware is like this:

Feature Option Behaviour
Actions, Queries, APIs auth: true (default) Allow request. If there a JWT token on request, put user -> req..
Actions, Queries, APIs auth: false Allow request. Don't put user on req.

If there is not auth token on the request, request is still allowed.

Frontend

Users can put authRequired: true on their pages to make them inaccessible to public.

page ProfilePage {
  authRequired: true, // <---
  component: import { ProfilePage } from "@client/pages/ProfilePage.tsx"
}
Feature Default Meaning
Pages authRequired: true Page can't be opened by unauthenticated users.
Pages authRequired: false (default) Page can be opened by unauthenticated users.

2️⃣ Issues with the current state

Backend

Behaviour auth: true is not the a regular user would expect. Also, we didn't document it for Actions and Queries, but only for APIs.

Based on experiences with other frameworks and with our own implementation for Pages, people assume that putting auth: true means -> no access for unauthenticated users. But in our case, it just means extract user from the auth token. To be fair, that's correct if we look at the strict definition, we are doing "authentication" and identifying the source of the request. But people don't really think about that when they put auth: true and they want to protect a route.

Frontend

Users can use authRequired to control access to certain pages, which is all good. But they can't control per page the redirect URL. That might a thing we could improve on. But overall, the logic works as expected ✅

3️⃣ What we could do

Backend

We could do several things:

  1. Remove the auth option and just keep the default behaviour (extract user if auth token present)
  2. Introduce a new concept of authRequired on the backend
    • If authRequired: true then the route is returning 401 for unauthenticated users
    • This would simplify the implementation of authenticated routes
    • Users wouldn't need to writeif (!context.user) { throw new HttpError(401) } in most cases
  3. In the future, explore User roles and offer proper role based access (not in the scope for this issue)

Frontend

Explore giving more control to the users in terms of redirects for unauthenticated visits to protected pages (not in scope for this issue).

@infomiho infomiho added the shouldfix We should do/fix this at some point label Apr 13, 2023
@infomiho infomiho self-assigned this Apr 13, 2023
@shayneczyzewski
Copy link
Sponsor Contributor

We should also keep in mind how it should behave with WebSockets, once that feature is added 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shouldfix We should do/fix this at some point
Projects
None yet
Development

No branches or pull requests

3 participants