kadirahq / flow-router

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

Meteor.user() isn't ready in triggersEnter function when user clicks on bookmarked link #625

Closed proehlen closed 7 years ago

proehlen commented 8 years ago

I want to turn off access to a bunch of admin routes if the user is not logged in or doesn't have sufficient priveledges. I've got it working in most instances using a FlowRouter group and triggersEnter. However, I'm having a problem in the below code.

// Admin routes group
const adminRoutes = FlowRouter.group({
  prefix: '/admin',
  name: 'admin',
  triggersEnter: [(context, redirect) => {
    if (!Meteor.user() || Meteor.user().username !== 'admin') {
      redirect('/not-authorized');
    }
  }],
});

It works perfectly in all situations except when:

The problem seems to be that Meteor.user() isn't available yet when the trigger runs. I've experimented with different Tracker autorun options but they don't seem to work.

I've created a repro here: https://github.com/proehlen/admin-pages-route-group-redirect.

Relevant code is in /imports/startup/client/routing.js starting at line 28.

rezaxdi commented 8 years ago

That's why Meteor.loggingIn() exists. By checking this you can decide whether its time to check username or not, so the page can be in loading status while Meteor.loggingIn() is true.

It can be done like this, first some helpers :

  authInProcess: function() {
    return Meteor.loggingIn();
  },
  canShow: function() {
    return (Meteor.user()==='admin');
  }

Then in templates (e.g. blaze) :

  {{#if authInProcess}}
    <p>Loading ...</p>
  {{else}}
    {{#if canShow}}
      <p>Welcome admin :)</p>
    {{else}}
      <p>You are not authorized to view this page.</p>
    {{/if}}
  {{/if}}

You can also do the same thing by waiting for Meteor.loggingIn() and then initialize flow router which is not a good idea.

proehlen commented 8 years ago

Ok thanks, but that leaves me having to repeat that helper code and template code in as many places as it's needed. I want to close out 5 or 10 or 20 admin routes with a few lines of code in the trigger as it's the most direct (and IMO, appropriate) place to do it.

rezaxdi commented 8 years ago

I don't get it, why you need to repeat those codes ? for template helpers, they can be defined in global :

Template.registerHelper

And for template itself, you just need to put those lines in a parent template and then include everywhere you want. So what is your problem exactly ?

proehlen commented 8 years ago

Look the code works in the op original issue perfectly except under the circumstance I outlined - getting there from a link or by typing the url in the browser.

I know how to use global helpers. I don't wan't to repeat a call a helper in every admin template. I don't want to have to remember to tell the next junior dev who adds an admin route to not forget to add the call. I don't want to be worried about remembering to check whether he did or not.

The above method works whether there's 1 admin route or 5 today and whether there will be 5 or 50 next year without writing a single extra line of code. I'd like it to work under all circumstances. I know how to come up with alternatives, I'd appreciate it if you could confine your suggestions to the problem as outlined.

nilsi commented 7 years ago

+1

yoonwaiyan commented 7 years ago

+1.

I have the same problem too, except that my template is using React instead of Blaze which is not reactive with Meteor. It seems to me that Meteor.loggingIn() always returns true in triggersEnter. Is there a way I can get Meteor.user() in FlowRouter?

hectoroso commented 7 years ago

+1

KaitaniLabs commented 7 years ago

There is a pretty simple solution to this which I'm using:

// Logged In Routes Group
loggedInRoutes = exposedRoutes.group({
  prefix: '',
  name: 'loggedIn',
  triggersEnter: [function(context, redirect) {
    if(!Meteor.loggingIn() && !Meteor.userId()){
      let route = FlowRouter.current();
      if(route.route.name !== "Login"){
        Session.set("redirectAfterLogin",route.path);
        redirect('/login');
      }
    }
  }]
});

// Admin Routes Group (extends Logged In Routes)
adminRoutes = loggedInRoutes.group({
  prefix: '/admin',
  name: 'admin'
});

// Example Admin Route
adminRoutes.route('/dashboard', {
  name: "AdminDashboard",
  action: function () {
    BlazeLayout.render('AdminLayout', {topbar: 'AdminTopbar',content: 'AdminDashboard'});
  }
});

// AdminLayout is a layout template, but it is still a template so use the onCreated hook
Template.AdminLayout.onCreated(function() {
  const instance = this;
  instance.autorun(function () {
    var subscription = instance.subscribe('userData');
    if (subscription.ready()) {
      /**
       * Do your check here for access
       * E.g. if(user doesn't have access){FlowRouter.go("Some route for non-admins");}
       */
    }
  });
});

// AdminLayout Template
<template name="AdminLayout">
  {{#if Template.subscriptionsReady}}   
    {{>Template.dynamic template=topbar}}
    {{>Template.dynamic template=content}}
  {{else}}
{{> Loading}}
  {{/if}}
</template>

// Publish userData
Meteor.publish("userData",function(){
  if(this.userId){
    return Meteor.users.find({_id : this.userId});  
  }else{
    this.ready();
  }
});

This removes subscriptions from the Router making it do what it's supposed to do and nothing more. Since all of your admin templates will use the same layout (or at least they probably should) the above code will work for any new admin routes you add to an app so long as the layout is 'AdminLayout'.

Because Admin Routes group extends LoggedIn Routes group the triggersEnter get used automatically. So there is no need to check for Meteor.loggingIn() or Meteor.userId() in the Admin Routes group.

Hope that helps :)

proehlen commented 7 years ago

I've moved on since the issue was raised to another platform for the project that prompted it so closing this issue. Thanks @Allerion for posting your work around. I'm sure it will come in handy for others facing this problem.