redwood icon indicating copy to clipboard operation
redwood copied to clipboard

[RFC]: Add a authenticated props for Private Set

Open Olyno opened this issue 2 years ago • 9 comments

Summary

Add an authenticated props to the Private Set that would redirect the user if he is authenticated.

Motivation

Currently, we only have the unauthenticated props for Private Set, which works well in most cases. However, let's take the example of the "Signup" and "Signin" routes. These routes are only supposed to be accessible if the user is not logged in. Apart from using the "isAuthenticated" of the "isAuth" hook, it is not possible to protect the routes in question.

Detailed proposal

It would be a simple copy and paste of the unauthenticated props logic, but with the condition reversed.

Are you interested in working on this?

  • [X] I'm interested in working on this

Olyno avatar Jul 07 '22 16:07 Olyno

Thanks for the RFC @Olyno!

If I remember correctly there is a more general RFC somewhere about protecting routes based on anything, not just auth status. I want to revisit that before I know what the best next step here is. It might have been an issue or PR here on GitHub, or maybe a community forum post. If you want to help me find it, that'd be great. Otherwise I'll look for it myself later today.

Tobbe avatar Jul 07 '22 17:07 Tobbe

I'm not sure to see which RFC you're looking for, sorry 😅

Olyno avatar Jul 07 '22 18:07 Olyno

I found it! Was on the forums

https://community.redwoodjs.com/t/feature-proposal-useguard-hook-for-checking-preconditions-on-page-render/2732/13

I'll read it and get back to you when I have thought about it all some more 🙂 @Olyno would be great if you could have a look as well and see if you think it would help for your use case

Tobbe avatar Jul 07 '22 20:07 Tobbe

Ok, so reading that forum post again it's about this exact same use case! That's great 🙂 So the value of such a feature is getting clearer, but I'd still really like a few more use-cases before adding it to the framework I think.

For now, would something like this work for you? Possibly used as a wrapper component in a <Set> around the Sign up/Sign in routes

const { currentUser } = useAuth()
if (currentUser) return <Redirect to={routes.userDashboard()} />

Tobbe avatar Jul 07 '22 23:07 Tobbe

@Tobbe What if I have a landing splash page / public and then a/dashboard that needs auth. If I am already logged and authenticated and go to /, I'd like them to be easily redirected to /dashboard?

But / needs to be public, too (I think). That might be different.

dthyresson avatar Jul 08 '22 00:07 dthyresson

Ok, so reading that forum post again it's about this exact same use case! That's great slightly_smiling_face So the value of such a feature is getting clearer, but I'd still really like a few more use-cases before adding it to the framework I think.

For now, would something like this work for you? Possibly used as a wrapper component in a <Set> around the Sign up/Sign in routes

const { currentUser } = useAuth()
if (currentUser) return <Redirect to={routes.userDashboard()} />

This is the most efficient way to do the redirection indeed. The problem is that it quickly becomes redundant if several pages use the same logic (e.g the /signin and /signup pages). That's why I suggested a new prop, or at the limit to rework the Private Set workflow (even if I am less favorable to this rework idea, because it would bring a breaking change).

Olyno avatar Jul 08 '22 02:07 Olyno

To incorporate the redirect after a user has logged in and refrain them from ever being able to go back to those signup and login pages, this is what I do:

Routes.js

<Private unauthenticated="login" roles="admin">
  <Set wrap={LoggedInLayout}>
    <Route path="/admin/users" page={UsersPage} name="UsersPage" />
  </Set>
</Private>

<Private unauthenticated="login" roles="user">
  <Set wrap={LoggedInLayout}>
    <Route path="/dashboard" page={DashboardPage} name="dashboard" />
  </Set>
</Private>

<Set wrap={[LoggedOffLayout, AuthRedirect]}>
  <Route path="/login" page={LoginPage} name="login" />
  <Route path="/signup" page={SignupPage} name="signup" />
  <Route path="/forgot-password" page={ForgotPasswordPage} name="forgotPassword" />
  <Route path="/reset-password" page={ResetPasswordPage} name="resetPassword" />
</Set>

AuthRedirect.js

import { useAuth } from '@redwoodjs/auth'
import { Redirect, routes } from '@redwoodjs/router'

const AuthRedirect = ({ children }) => {
  const { loading, isAuthenticated } = useAuth()

  if (loading) {
    return null
  }

  if (isAuthenticated) {
    return <Redirect to={routes.dashboard()} />
  }

  return children
}

export default AuthRedirect

ladderschool avatar Jul 25 '22 08:07 ladderschool

Thank you @ladderschool that's exactly how I meant you can do it! If you want you can combine <Private> and <Set>

Routes.js

<Set private unauthenticated="login" wrap={LoggedInLayout}>
  <Route path="/dashboard" page={DashboardPage} name="dashboard" />
</Set>

<Set wrap={[LoggedOffLayout, AuthRedirect]}>
  <Route path="/" page={HomePage} name="home" />
  <Route path="/login" page={LoginPage} name="login" />
  <Route path="/signup" page={SignupPage} name="signup" />
  <Route path="/forgot-password" page={ForgotPasswordPage} name="forgotPassword" />
  <Route path="/reset-password" page={ResetPasswordPage} name="resetPassword" />
</Set>

AuthRedirect.js

import { useAuth } from '@redwoodjs/auth'
import { Redirect, routes } from '@redwoodjs/router'

const AuthRedirect = ({ children }) => {
  const { loading, isAuthenticated } = useAuth()

  if (loading) {
    return null
  }

  if (isAuthenticated) {
    return <Redirect to={routes.dashboard()} />
  }

  return children
}

export default AuthRedirect

@dthyresson I think what you mentioned is also possible with what @ladderschool showed. I added it to my tweaked version of his solution above

@Olyno You said

The problem is that it quickly becomes redundant if several pages use the same logic (e.g the /signin and /signup pages).

Do you think this solution solves that problem? Or is it still too redundant?

Tobbe avatar Jul 28 '22 21:07 Tobbe

Do you think this solution solves that problem? Or is it still too redundant?

I wasn't thinking of adding multiple layouts, so I thought I would have to check each time. By adding multiple layouts, it solves the redundancy problem :)

Olyno avatar Jul 29 '22 00:07 Olyno

Hi there! Didn't know if it is proper to make another issue for this question/proposal, just stumbled upon this minor wishy thing: <Private unauthenticated="login" rolesMismatch="page404" roles="user"> I think the point is obvious, separate redirect for logged in yet unauthorized folks, is there a way we can implement something alike?

NetLancer avatar Oct 24 '22 03:10 NetLancer

Oh sorry, Routes.js example: <Private unauthenticated="login" rolesMismatch='page404' roles="user">

NetLancer avatar Oct 24 '22 03:10 NetLancer

@NetLancer I think, again, the real solution is what's described in that forum post I linked above. So thanks for providing yet another usecase for it 🙂

And, also again, the workaround for now is to have a <Set> with a redirect. It would check the user's roles (by destructuring the hasRole function from the useAuth hook), and redirect as appropriate.

Tobbe avatar Oct 27 '22 00:10 Tobbe

Pardon me @Tobbe, the above workaround i'm trying to get it working: So, unless the current user hasRole('admin')
<Set private unauthenticated={() => (hasRole('someuser') ? "home" : "login")}> or <Set private unauthenticated={hasRole('someuser') ? "home" : "login"}> -with the above destructured hasRole in Routes.js, both don't do what i'm aiming at, could you please exemplify the solution, would be much grateful :)

NetLancer avatar Nov 23 '22 19:11 NetLancer

If the user is unauthenticated they will never have any roles. So checking hasRole in the unauthenticated component is never going return anything useful.

Instead I'd do something like (untested)

<Set private unauthenticated="login" wrap={RolesRedirect}>
  <Route ...
</Set>
// RolesRedirect.jsx

import { useAuth } from '@redwoodjs/auth'
import { Redirect, routes } from '@redwoodjs/router'

const RolesRedirect = ({ children }) => {
  const { loading, hasRole } = useAuth()

  if (loading) {
    return null
  }

  if (hasRole('someuser')) {
    return <Redirect to={routes.home()} />
  } else if (hasRole('admin')) {
    return <Redirect to={routes.admin()} />
  }

  return children
}

export default RolesRedirect

You'll have to adjust the roles you check for and where you redirect. With the implementation I showed here a user with none of the roles will get whatever route(s) is nested inside the Set

Tobbe avatar Nov 24 '22 06:11 Tobbe

Thank u so much! <Set wrap={RolesRedirect} private unauthenticated="login"> worked for me, perhaps the order (wrap, unauthenticated) matters too

NetLancer avatar Nov 24 '22 08:11 NetLancer