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

Creating authorized route #119

Closed
jordantomax opened this issue Feb 10, 2018 · 9 comments
Closed

Creating authorized route #119

jordantomax opened this issue Feb 10, 2018 · 9 comments

Comments

@jordantomax
Copy link

jordantomax commented Feb 10, 2018

I'm trying to create a protected route that will redirect to /login when user is not authorized. I think this is more appropriate for Stack Overflow, here's the post. But wanted to post here as well.

Is there a standard way of handling this situation?

@taion
Copy link
Member

taion commented Feb 11, 2018

To do that on a route, in getData or render, check for authorization (e.g. via match.context), then throw a RedirectException.

We actually handle these cases a little differently, though – we throw HttpError(401), then render the login page in renderError on the router itself.

If you don't want to attempt to fetch data, then you can also use a custom resolver that checks for auth status and throws the relevant exception before calling the base resolver.

@taion taion closed this as completed Feb 11, 2018
@jordantomax
Copy link
Author

@taion Thanks for the reply. That is more of less what I was doing, and it seems very close to working. One thing I'm caught up on is that I still do want to have a route for /login in addition to rendering it inside the renderError. That seems to cause some problems with redirection, do you guys have that situation as well?

@jordantomax
Copy link
Author

Okay, I created a solution in which I have a <PrivateRoute/> that encapsulates all private routes and a sibling <LoginRoute/> that handles login page. They both handle checking if user is authenticated and throw new RedirectException() to the appropriate route. It does feel a little heavy handed but it seems to work well for now.

Is it okay in your opinion to return null from a <Route/> render method when there are no props. I'm assuming that this implies that things are still in a loading state. In my case it causes a flash if the user goes for instance to /login but is logged in because if I don't return null in that case, it will briefly show the <Login/> component. i.e:

class LoginRoute extends Route {
  render({ Component, props }) {
    if (!props) return null
    if (Component && props && props.viewer) {
      throw new RedirectException('/')
    }
    return (<Component {...props} />)
  }
}

Thanks again for your help!

@taion
Copy link
Member

taion commented Feb 11, 2018

You shouldn’t return null – see 4Catalyzer/found#125. During loading, return either undefined or a loading indicator.

@taion
Copy link
Member

taion commented Feb 11, 2018

In your example of having both /login and an error handler, if you want the pages to be identical, the easiest solution would be to have render on the login route immediately throw the error.

@jordantomax
Copy link
Author

Thanks @taion! Would returning null still be a problem for routes that do have components?

In any case, I'll change it to undefined. Since there is a component, that seems slightly more semantic anyway.

Makes sense re: login. I think I'm going to forego adding the login as an error, and keep it only as a route for now since I would have to add the routing logic anyway if I made it render only on errors (unless there are some benefits that I'm not seeing). Is it strange to be throwing 403 errors when it's a public page?

@taion
Copy link
Member

taion commented Feb 12, 2018

Component doesn't do much if you have render, unless you're doing code splitting (or using Relay Classic). It doesn't directly affect the render results. So no matter what, if you return null, you're basically saying "render everything below this route as if this route didn't have a Component or a render function". It's not ideal and I'd like to clean it up, but I don't have a good idea for how to do so right now.

In terms of errors, I don't know... maybe? If it's purely client-side, it's irrelevant since the error codes are just internal. If you're using those codes to drive response codes on the server, then I guess it is a little weird. You can always just render whatever you would from renderError, though – just re-use the <LoginPage> component or whatever.

@jordantomax
Copy link
Author

Got it, thanks for the info. Yeah it seems harmless, though a little annoying that those errors show up uncaught inside the console.

@taion
Copy link
Member

taion commented Feb 16, 2018

Yeah – you're supposed to handle them for yourself in renderError. They're really meant to be internal signals to stop rendering the current set of routes and instead hit your own error handler.

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

No branches or pull requests

2 participants