kadirahq / flow-router

Carefully Designed Client Side Router for Meteor
MIT License
1.09k stars 195 forks source link

Triggers autorun with Meteor.userId() #704

Open gaillota opened 7 years ago

gaillota commented 7 years ago

Hey guys, I have an issue that's been bothering me for a while now. Most of my routes need the user to be logged in.

FlowRouter.triggersFunctions = {
    isLoggedIn(context, redirect) {
        if (!Meteor.userId()) {
            setDispatcherPath(context.path);
            redirect(FlowRouter.path('public.auth.login'));
        }
    }
};
const NON_AUTH_ROUTES = [] // Routes where no auth is required
FlowRouter.triggers.enter(FlowRouter.triggersFunctions.isLoggedIn, {
    except: NON_AUTH_ROUTES
});

IF the user get logged out by the server (because of Meteor.logoutOtherClients() or because the loginToken is not valid anymore, the trigger is not re-run and the template obviously loses all data sent from the client. It's a really poor User eXperience and I'm trying to figure out a way to re-run the triggersEnter function which would redirect the user to the login page if he gets logged out.

Any ideas on how to do that ? Thank you for your help.

EDIT: Whole code is available on this repository: https://github.com/gaillota/ceerebro/blob/master/imports/startup/client/router/security.js

jebarjonet commented 7 years ago

You could write this somewhere in your file :

Tracker.autorun(() => {
    if (!Meteor.userId()) { // if the user is not logged in (since userId() is reactive)
        if (Session.get('loggedIn')) { // if the user was logged in
            FlowRouter.go(FlowRouter.path('public.auth.login'))
        }
    }
})

You still need to create a variable in Session like loggedIn and put it to true when the user logs in, like :

Accounts.onLogin(() => {
    // whatever you want
    Session.set('loggedIn', true)
})

so that you can track that the user was already logged in before redirecting it (if you don't track that and just use if (!Meteor.userId()) as your redirection condition, the user will not be able to move anywhere else). Don't forget to put it to false when the user logs out.

This should do the trick

gaillota commented 7 years ago

Yes, I thought about putting a Tracker, in the triggersEnter function first, but I got the afterFlush error. I'd prefer to rest on Meteor and FlowRouter built-in variables and functions, but yes I think using Session variables would work. I might come to that if I don't find any other solution (which I think doesn't exist)

Thanks for your help m8

jebarjonet commented 7 years ago

The Tracker should not be put in the triggersEnter but next to the FlowRouter config like :

// User must be logged in
FlowRouter.triggers.enter(FlowRouter.triggersFunctions.isLoggedIn, {
    except: NON_AUTH_ROUTES
});

Tracker.autorun(() => {
    if (!Meteor.userId()) { // if the user is not logged in (since userId() is reactive)
        if (Session.get('loggedIn')) { // if the user was logged in
            FlowRouter.go(FlowRouter.path('public.auth.login'))
        }
    }
})

The thing is Meteor.userId() and Session.get('loggedIn') have nothing to do with routing actually, you probably need to create this autorun outside of any FlowRouter context, then if both condition pass then FlowRouter takes action and go somewhere. The FlowRouter redirection being a consequence and not a cause, this may not be defined in FlowRouter triggers.

gaillota commented 7 years ago

Yeah, I figured it out for the Tracker inside the trigger function. I partially agree with you on the fact that those variables are not related with the routing logic, however the Meteor.userId() is what the triggerEnter is based on to determine whether to re-route the user or not. But in a general point of view, I agree with you.

I tried to implement this solution but it didn't work because when the server log out the user, the client runs the onLogout function where I put the Session.set('logged', false), so when the Tracker function re-run, the redirection is not reached.

I guess I should set the logged var to false somewhere else but I can't figure out where...

jebarjonet commented 7 years ago

Actually you could something like :

Tracker.autorun(() => {
    if (!Meteor.userId()) { // if the user is not logged in (since userId() is reactive)
        if (Session.get('loggedIn')) { // if the user was logged in
            FlowRouter.go(FlowRouter.path('public.auth.login'))
            Session.set('loggedIn', false)
        }
    }
})

You could also remove the Session.set('logged', false) and use the Accounts.onLogout callback, that should be triggered when logoutOtherClients is called from another client

Accounts.onLogout(() => {
    FlowRouter.go(FlowRouter.path('public.auth.login'))
})

You could add a condition before the redirection that checks if the current route is a protected route (only accessible for logged in users) or not (based on a list of route names or on a property of the current route)

gaillota commented 7 years ago

Resetting the Session variable in the Tracker.autorun() is a good idea. And yes, the Accounts.onLogout() is called whenever the client gets logged out by the server. That's why resetting the Session variable in this callback was not working.

However, redirecting the user to the login page everytime he logs out is not a behaviour that I particularly want, even if it's only when he comes from protected routes. I don't know...

Anyway, I'll try the first option as soon as I can.