Changes in Routing for Authentication

[StephanieKeck]

For the last few days I've been working on getting authentication routing logic working. I've uploaded a working example of what I came up with under the authentication-routing branch. I thought it'd be a good idea to have a discussion about these design decisions before submitting a pull request.

I've restructured the routing hierarchy so that (currently) there are two main 'gatekeeper' components which restrict and control access to different parts of the app based on whether the user is logged in. The top level component, App.js, is responsible for handling all top-level app state. If a logged in user enters '/', it will redirect to either 'home/' or this.state.redirectUrl if defined. It passes a setRedirectUrl hook to its children so that lower level gatekeepers can set it if they bounce a visitor to 'login/'. If a logged out visitor goes to '/', App.js will redirect them to 'login/'. App.js listens for authentication as soon as it mounts (via base.onAuth()), so it will redirect the user to their desired destination as soon as they log in without the login form needing to explicitly set any redirect behavior (this will not work until our app is connected to our database).

All routes requiring users to be logged in are located under the LoginRequired plain route. LoginRequiredWrapper.js handles the access-control logic. If the user is not logged in, it calls this.props.setRedirectUrl and bounces the user to 'login/'.

Any route not '/' or under LoginRequired does not currently redirect.

The routing hierarchy look like this:

appRoute
--Register
--Login
--LoginRequired
----Home
----Events
----Activity
----etc.

I have included some buttons to simulate log in/log out behavior.

I got the components-as-gatekeepers idea from here: https://medium.com/the-many/adding-login-and-authentication-sections-to-your-react-or-react-native-app-7767fd251bd1#.r6pcdfsxi.

I also started on the firebase logic in this branch, but did not include the api credentials to the quick example database I set up. If you need it to get this example working I can post the file to the slack.

[NilsG-S]

I'm still doing research and formulating my response to your proposal, so you might not hear back from me on this topic for a day or so.

[NilsG-S]

I had hoped to finish the implementation of my own authentication-routing scheme before commenting, but I figured that I've delayed long enough. I'll probably be pushing the rest of my code to the repository within the next 1-2 days. This includes the higher order component that I discuss later.

My main concern with your proposal is related to LoginRequiredWrapper.js. It seems as though you're proposing that we put all of the routes that require the user to be logged in under the LoginRequired route. In particular, I'm concerned about the implications this method would have on deep routes that require authentication. For example, the spec for this project stipulates that the events page be viewable by everyone, regardless of authentication state. The events page will probably have some method by which users can sign up for events, which would require them to be logged in. A natural routing scheme would be something like this: root => events => sign up. Putting all pages that require login under the LoginRequired route would result in a routing scheme like this: root => events => login required => sign up. From an organizational perspective, the issue is that LoginRequired is a direct child of root, just like Events. From a utility perspective, the issue is that the EventSignUp route couldn't be rendered as a child of Events. A potential solution would be to have a new instance of the LoginRequired route for each deep route that requires authentication. However, this would end up being just as complicated as using the onEnter hook.

My proposal is that we use the higher order components (HOC) design pattern to wrap all of the components that require authentication. This would allow us to keep our original routing scheme, while redirecting users to the login page when necessary.

Other than that, I had a few suggestions I'm implementing in my authentication-routing scheme that I think would be helpful.

• Use redux. This would allow application state to be shared by all components in the app. So when authentication status changes, all the relevant components could be notified without passing the value down as a prop.
• Use the firebase module. re-base doesn't seem to give us that much more than the official module, which has much better documentation and support.
• Turn login and register into normal components rather than routes, rendering them as fixed floating windows. This would allow us to avoid having to store redirectURL.
• Don't have a central onAuthStateChanged listener, because it can result in race conditions when creating a user. Instead, dispatch the setAuthState action from the callback when performing an auth operation.

[StephanieKeck]

I actually agree with the objections you've raised. I've spent the last few days experimenting with different ways to improve my proposal and have found my approach quite fragile. The HoC approach would be more flexible and likely more stable. Though I haven't shown it here yet, I actually wrote and tested an HoC for use in making permissions-aware components. The code of the HoC itself looks like this:

import React from 'react'

function hasPermissions(WrappedComponent) {
    return class HasPermissionsWrapper extends WrappedComponent {
        constructor(props) {
            super(props)
            this.state = {authorized: (this.props && this.props.user) ? super.isAuthorized(this.props.user) : false}
        }

        componentWillReceiveProps(nextProps) {
            this.setState({authorized: super.isAuthorized(nextProps.user)})
        }

        render() {
            if (this.state.authorized)
                return super.render()
            else {
                const Denied = this.props.denied
                if (Denied) return <Denied {...this.props}/>
                else return null
            }
        }
    }
}

export default hasPermissions

A component that uses it looks like this:

import React from 'react'
import hasPermissions from './hasPermissions'

class Test extends React.Component {
    constructor(props) {
        super(props);
        this.isAuthorized = this.isAuthorized.bind(this)
    }

    isAuthorized(user) {
        return user.permissions.includes('canCreateEvents')
    }

    render() {
        return <p>You have the permissions: {this.props.user.permissions.join(', ')}</p>
    }
}

export default hasPermissions(Test)

This HoC expects as props a user object and an optional denied component. The wrapped component implements an isAuthorized method which the HoC passes the user object to. The component is then free to decide if the user is authorized based on whatever criteria it likes (the user's permissions, roles, username, or any other prop passed to the component). The HoC then renders the wrapped component if the user is authorized, otherwise the denied component if one is passed in, or otherwise nothing at all.

This HoC could be easily modified to behave as you described, by checking for user login instead of calling super.isAuthorized() and rendering the login form instead of this.props.denied. I assume you're thinking along these lines (a component which wraps another and determines whether it is allowed to render)?

I still think we should keep the top-level app.js, which will handle the app's top-level state and initialization. I'd like to enforce the use of container components in our app that purely handle state that's handed down via props to components that purely handle UI (these would ideally be stateless functional components). This is considered a react-architecture best practice: https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.tm4u6bdfa. A top-level container component is key to this approach.

I don't know much about the differences between re-base and the firebase module, so that's something I will have to research. I also looked into redux but admit I don't quite grasp it yet. I'll spend some time studying it more thoroughly. Would you mind elaborating on what benefits we'd gain by using redux over passing down state through props?

[NilsG-S]

Yep, it seems that we're thinking along pretty similar lines with the HOC, although our implementations are different. I also agree with you on the use of the container-presenter model. I've already started using it in the code that I've been uploading to the repository.

I may not have made myself clear on the subject of the top level component (TLC). I think it's a good idea to have one for initializing certain aspects of the application state, I just don't think App.js should be used to keep track of it. That's where Redux comes in. The Redux store exists outside of any one component, serving as a single source of truth for application state. All state updates go through the store, and all components that are connected to it are updated when it updates. It's this functionality that leads me to suggest not using App.js to manage application state. The Redux store would do everything you're suggesting we use App.js to do, while allowing for several key advantages:

• We wouldn't have to pass application state down through all of our components. This is particularly important given that we're using the container-presenter model. Without Redux, we'd end up having to pass application state through a lot of presentational (stateless) components. With Redux, we simply use the connect HOC to allow any component, no matter how deep in the application hierarchy, access to the Redux store.
• Synergy with callbacks/promises. When I was first creating my authentication system, I used a central onAuthStateChanged listener to detect when the user logged in and alter application state. But I realized that this caused race conditions in at least one circumstance, creating users. Firebase stores users and data separately, so one has to create the data associated with the user's UID after the createUserWithEmailAndPassword promise is returned. But createUserWithEmailAndPassword automatically authenticates the user, which means that onAuthStateChanged could potentially run before data creation. This is an issue because the type of user (student or coordinator) is stored in the data that may not have been created yet. So when detecting the type of user in order to set application state, there is a chance that the data won't have been created yet. But using Redux, one simply dispatches an action to update the store after all of the promises have been fulfilled.