frontarm / navi

🧭 Declarative, asynchronous routing for React.
https://frontarm.com/navi/
MIT License
2.07k stars 71 forks source link

Trying to access a sub route under an already mounted route does not trigger wildcard pattern #120

Closed maliiiith closed 5 years ago

maliiiith commented 5 years ago

I'm trying authenticated routes with permissions. I'm following a similar method explained in here: https://frontarm.com/navi/en/guides/authenticated-routes/#setting-authentication-state

On wildcard stage, I use a method to authorize routes by comparing required permissions to access the route against permissions that the logged in user have to have more granular control over every route.

export default mount({
  '*': map((request, context) => {
    const { authService, } = context;
    if(!authService.currentUser) {
      const originalUrl = encodeURIComponent(request.originalUrl);
      return redirect(
        `${routeRegistry.login.path}?redirectTo=${originalUrl}`,
        {exact: false,}
      )
    }

    const route = RouteAuthorizer(request, authService.currentUser);
    if (route) {
      return mount(route);
    }
  }),
  [routeRegistry.root.path]: redirect(routeRegistry.login.path),
  [routeRegistry.login.path]: mount(routes[routeRegistry.login.path].route),
});

I have all the routes listed and I use a separate method to authorize routes.

// routes authorizer
export default (req, userObj) => {
  const { path, } = req;
  const { route, permissions, } = Routes[path];
  if (permissions && permissions.length > 0) {
    if (!(userObj && isAuthorized(userObj.abilities, permissions))) {
      return null;
    }
  }
  return {
    [path]: route,
  };
};
// Routes registry
export default {
[routeRegistry.courses.path]: {
    permissions: routeRegistry.courses.permissions,
    route: route({
        title: 'Your courses',
        view: <Courses/>,
    }),
  },
  [routeRegistry.courses.courseDetail.path]: {
    permissions: routeRegistry.courses.courseDetail.permission,
    route: route({
      title: 'Specific course',
      view: <Course/>,
    }),
  },
  ...
}

The issue I'm facing is when I'm trying to navigate to a page while it's parent route is already being mounted.

Ex: I'm trying to access courses/1 but courses/ is already mounted. So instead of going through the wildcard it jumps straight into courses/ route.

Is there a way for me to force each path as unique?

Or do you have a different pattern you use to achieve the same functionality?

jamesknelson commented 5 years ago

I'm having trouble understanding the above code, so just to confirm that I understand, it sounds like the issue is that you have something like:

mount({
  '/courses': route(...),
  '/courses/:id': route(...)
})

And the issue is that /courses and /courses/:id have different permissions?

If this is the case, usually what I'd do is something like this:

mount({
  '/courses': mount({
    '/': route(...)
    '/:id': route(...)
  })
})

Not sure if this answers your question though. If not, it'll be easier to help if you can simplify the example down to the minimal possible size.

maliiiith commented 5 years ago

Yea sorry about that. Let me try to explain again.

So what I'm trying to achieve is to authorize routes by their permissions. So the logged in user have a set of permissions and a route have a set of permissions which are required for user to access that route.

The authorization process happens inside the wildcard route.

I'm keeping all the routes in a flat object without any nestings. This is because I want to have more granular control over each and every route.

This is how the routes registry looks like:

{
  'courses/': {
    permissions: ...,
    route: {
      '/': route({
        title: 'Your courses',
        view: <Courses/>,
      }),

    },
  },
  'courses/:id': {
    permissions: ...,
    route: route({
      title: 'Specific course',
      view: <Course/>,
    }),
  },
  'courses/create': {
    permissions: ...,
    route: route({
      title: 'Create new course',
      view: <CourseCreate/>,
    }),
  },
}

Lets say user is trying to access courses/ route, since it hasn't being mounted yet.

  1. The request will invoke wildcard route. Then the authorization process happens and if user has necessary permissions and route will get mounted.
  2. Then the user tries to access courses/create route. Since the courses/ route is already mounted, the request will go to courses/ route instead of wildcard and I'm not able to do the authorization process.

What I want to do is to force Navi to route to wildcard route if the path has not being mounted yet disregarding the parent route.

Is this clear enough?

jamesknelson commented 5 years ago

So the way that Navi routes work, if you add a /courses route, then any URL starting with /courses will be routed through it -- it's not actually possible to route these to the wildcard instead.

The reason for this is to do with code splitting. For example, frontarm.com has a route that looks like this:

mount({
  '/courses': lazy(() => import('./courses'))
})

The ./courses file won't actually be loaded until it is first requested, and because of that, it's impossible to know whether it is a single route, or a mount() that holds a bunch of nested routes.

The thing is, a mount() doesn't actually know anything about what its paths are mapped to. Matchers like mount() and route() return plain old functions -- they're a lot like React components. So even if you mount a plain old route at /courses, mount() doesn't know this, and it'll still pass every request that begins with /courses/ through to that route -- assuming that you could have mounted a mount() or redirect() there.

The simplest way to fix this is to just name your index route differently to your individual course routes, e.g.

mount({
  '/courses': route(...),

  // notice that this isn't a plural
  '/course/:id': route(...),
})

You could even place a redirect within /courses, using a nested mount:

mount({
  '/courses': mount({
    '/': route(...),
    '/:id': redirect(({ params }) => '/course/'+encodeURICompoent(params.id))
  })
})
maliiiith commented 5 years ago

I see. Thank you for the clear explanation!

vongohren commented 5 years ago

The problem here was that when accessing subroutes it tried to look for the subroute in the routes group.

Explained in this unfiltered text from our internal discussion
It lies inside this line: `const selectedRoute = Routes[path];` You do this `const { path, } = req;` This means that you are trying to get this json object with this value `/courses/42589650-ae1c-11e9-99e2-d58b15df4bbf` ``` const route = routes[`/courses/42589650-ae1c-11e9-99e2-d58b15df4bbf`] route === undefined #true ``` So in `index.js` it then tries to redirect to that same path and that fails, because nothing is mounted. So what happens with this approach you are basically trying to make another router on top of a router system. That is not optimal. Either you can do some regex and load the right route block, which is courses. You can regex the root route and fetch out that bolk of routes. Then you can keep the redirect and the route loads properly. So as long as you only the bolk of navi routes from the first bolk we can be good. But we need to remember to not build a complex stuff ontop of a well suited router.

But @jamesknelson, I might be wondering if we are going about this in the wrong approach. Because we are using permissions to restrict the routs you are trying to restrict. But I feel that we are building a strange layer on top of a router. Any thoughts here?

jamesknelson commented 5 years ago

This is hard to say without a lot more familiarity with the code.

The way I see it, is that Navi is a library for implementing routing, just like React is a library for building UIs. I try and keep the feature set as minimal as possible - in fact, some features will probably be removed before 1.0. As a result of this, you may sometimes need to build abstractions on top of the router to get the behavior that you want, which is absolutely okay.

On the other hand, if you don't need to build an abstraction on top of the router, then it's probably best to avoid it. From what I've seen, I feel like there must be a way to make it work reasonably well without abstractions, but only your team has the familiarity required to make that decision.

vongohren commented 5 years ago

@jamesknelson thanks for those wise words!