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

feat: protected routes defined from nuxt config #47

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

DanielRivers
Copy link
Collaborator

@DanielRivers DanielRivers commented Jan 27, 2024

This adds the ability to protect routes based on permissions of the logged in user

this is all handled by the auth-logged-in middleware

example configuration

    routeRules: [
      {
        "/dashboard": {
		  appMiddleware: ['auth-logged-in'],  // 3.11 or greater
          kinde: {
            permissions: ['example_permission'],
            redirectUrl: "/"
          }
        },
       "/admin/**": {
		  appMiddleware: ['auth-logged-in'], // 3.11 or greater
          kinde: {
            permissions: ['admin_user'],
            redirectUrl: "/"
          }
        }
      }
    ]

This will protect the route protected to only users with the permission of example_permission and all routes under admin need to have admin_user permission

An array of permissions can be defined, along with an optional redirectUrl. If not redirectUrl is supplied the user will be presented with a 401 error as per the current behaviour.

For versions 3.10 and lower you have to define the middleware on the you want to be protected

definePageMeta({
  middleware: ['auth-logged-in'],
})

This is a built in solution for #45

@danielroe
Copy link
Collaborator

What about using custom routeRules for this?

- added couple of example rules to the playground config
- reworked where the rules were defined
@DanielRivers
Copy link
Collaborator Author

@danielroe I can't see what I have done to break the virtual file src/runtime/server/utils/client.ts

None of my seem to marry up to cause the error.

src/kit.ts Outdated Show resolved Hide resolved
@danielroe
Copy link
Collaborator

One question is whether we instead move forward with nuxt/nuxt#25841 rather than implementing this specifically in this module. That would allow kinde middleware to be applied in a more generic way. Do you see any limitations in that PR?

@DanielRivers
Copy link
Collaborator Author

DanielRivers commented Mar 14, 2024

One question is whether we instead move forward with nuxt/nuxt#25841 rather than implementing this specifically in this module. That would allow kinde middleware to be applied in a more generic way. Do you see any limitations in that PR?

@danielroe That looks good, I was hoping to find a way to trigger the middleware without having to add middleware: ['auth-logged-in'] to every page.

one would assume that the rest of the route payload would be accessible so can continue to use the following style of config and extend as needed?

    "/protected": {
      kinde: {
        permissions: ['example_permission'],
        redirectUrl: '/',
      }
    },

I can pull the PR and have a look in more detail, but if I understand right not a huge amount would change just really how its triggered?

@DanielRivers
Copy link
Collaborator Author

@danielroe What changes are needed to get this over the line?

Copy link
Collaborator

@danielroe danielroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterated on this a little bit - hope it still looks good to you.

My main concern right now is that this only works on server-side. Is that intended? If so we should add some handling in the middleware for client/server.

@DanielRivers
Copy link
Collaborator Author

@danielroe Mostly your changes looked good, I had to change slightly the implementation of getRouteRules as it crashed when accessing any protected route.

I have tested on both direct access and using NuxtLink.

Updates the `getRouteRules` import and implementaton, adds `NitroRouteRules['kinde']` type back as type inference was not working
@DanielRivers
Copy link
Collaborator Author

@danielroe What can we do to get this PR through? We are seeing an increased usage of the module and route protection is a common question.

@danielroe
Copy link
Collaborator

danielroe commented Aug 16, 2024

@DanielRivers I don't believe this PR currently would work on client-side navigation. For example, this code:

const usersPermissions = await nuxt.ssrContext!.event.context.kinde.getPermissions()

will not work in the browser.

@DanielRivers
Copy link
Collaborator Author

@danielroe I have implemented client support, added an access endpoint which checks if the logged in user has access to the route, returns the returnUrl and access boolean.

src/module.ts Outdated Show resolved Hide resolved
@DanielRivers
Copy link
Collaborator Author

@danielroe little nudge on a review 🙏

@DanielRivers
Copy link
Collaborator Author

@danielroe Can this get a review please?

Comment on lines 36 to 45
if (import.meta.client) {
const fetchResult = await $fetch<AccessResponse>('/api/access', { method: 'POST', body: JSON.stringify({
path: to.path,
}) })
if (!fetchResult.access && fetchResult.redirectUrl) {
window.location.href = fetchResult.redirectUrl
}
return
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should be done entirely on the client side rather than requiring an extra request every time someone tries to navigate to a given route.

are the permissions exposed/accessible to the client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're currently not as they are only on the token, I can certainly fetch and store in memory store on client when attempting to use and not there yet.

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

Successfully merging this pull request may close these issues.

2 participants