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

User subscription not yet ready in onBeforeAction #871

Closed
aldeed opened this issue Sep 30, 2014 · 14 comments
Closed

User subscription not yet ready in onBeforeAction #871

aldeed opened this issue Sep 30, 2014 · 14 comments

Comments

@aldeed
Copy link

aldeed commented Sep 30, 2014

See Meteor-Community-Packages/meteor-roles#61

Basically I'm seeing a regression with iron:router 0.9.4 and Meteor 0.9.3 where Meteor.user() is undefined in onBeforeAction, so checks like Roles.userIsInRole always fail.

EDIT: Specifically I think maybe it is no longer automatically waiting on null (all client) subscriptions?

@tmeasday
Copy link
Contributor

tmeasday commented Oct 6, 2014

Hi @aldeed - I don't think anything has changed in the way that IR waits on subscriptions in the 0.9.4 release. Perhaps it is just a coincidence? (Which version were you one before?)

IR does not wait on null subscriptions. There's no way to do so.

@tmeasday tmeasday added this to the Requires Triage milestone Oct 6, 2014
@aldeed
Copy link
Author

aldeed commented Oct 6, 2014

Maybe something changed in the order of things on the Meteor side, so that null subscriptions might not be ready when the routing begins. Aren't other people seeing this? It basically means you can't check security in onBeforeAction anymore. Checking whether logged in works fine because the userId is set, but the actual user doc isn't on the client yet.

@aldeed
Copy link
Author

aldeed commented Oct 6, 2014

I'm not sure what iron:router I upgraded from that was working. It upgraded from Meteor 0.9.2.2 at the same time. I can't downgrade now, so I guess I'd have to create a test app to try to figure out which exact update broke things.

@merrington
Copy link

I think I'm seeing the issue. I don't have a test app at the moment, I'll try and describe my scenario though.

I have a onBeforeAction hook defined on most of my routes:

Iron.Router.hooks.loggedInFilter = function() {
    Meteor.log.info('User, loggingIn', {'user':Meteor.user(), 'loggingIn': Meteor.loggingIn()});
    if (!Meteor.user() && !Meteor.loggingIn()) {
        Meteor.log.info('Going to login');
        Router.go('login');
    } else {
        this.next();
    }
...
Router.onBeforeAction('loggedInFilter', {except: ['login', 'verify']});

The route for the root URL is:

Router.route('/', {
    name: 'queues',
    waitOn: function() {
        return [
            Meteor.subscribe('userData'),
            Meteor.subscribe('jobs')
        ];
    },

When I go to the root URL without having logged in (open in an incognito window) I just get stuck at my loading template - the hook doesn't even seem to run as I don't get any of the logging output. If I comment out the subscription to userData then it works as expected and I get sent to the login route.

This is with iron:router 1.0.0.-pre3 and Meteor 0.9.3.1

@tmeasday
Copy link
Contributor

tmeasday commented Oct 6, 2014

@aldeed - you can either use Meteor.loggingIn() to wait on the user data; or if you are publishing custom stuff, either name the subscription, or failing that you can perhaps just inspect the user to tell if the sub has loaded? Something like if (! user.roles) keepWaiting();

@tmeasday
Copy link
Contributor

tmeasday commented Oct 6, 2014

@merrington this sounds like a different problem. Can you open another ticket with a reproduction like so: https://github.com/meteor/meteor/wiki/Contributing-to-Meteor#reporting-a-bug-in-meteor

Cheers.

@aldeed
Copy link
Author

aldeed commented Oct 6, 2014

It's actually @alanning's roles pkg that's publishing the roles field with a null subscription. He could name the subscription and export the handle for use in waitOn, but I still don't understand why it's suddenly necessary when it was working fine before.

@tmeasday
Copy link
Contributor

tmeasday commented Oct 7, 2014

@aldeed I'm not sure either but I don't think it's the bump in IR version that's caused it. More likely it's the upgrade to 0.9.3 but you'd have to investigate further to say for sure.

@xumx
Copy link

xumx commented Oct 9, 2014

I'm seeing the same problem

Router.onBeforeAction(function(pause) {
    if (!Meteor.user()) {
        Router.go('/login');
    }
}, {
    only: ['home', 'protected']
});

I will try using Meteor.loggingIn() for now. Just wasn't expecting this behaviour.

@aldeed
Copy link
Author

aldeed commented Oct 9, 2014

@xumx, for your case, just changing Meteor.user() to Meteor.userId() should work. Because the user ID is always known separately due to being logged in, whereas the full user record isn't on the client until pub/sub is ready.

@aldeed
Copy link
Author

aldeed commented Oct 9, 2014

I made a simple app that reproduces the issue, but when I run it with various combinations of older Meteor and older iron:router, it always behaves the same. Meteor.user() is undefined the first time onBeforeAction runs. So apparently this is not actually a regression. I must have had something in my app that was delaying things enough for the sub to be ready before, and then whatever was doing that must have been removed. The app is too complex to know for sure.

Either way, @tmeasday, my only remaining question is: How do you intend for people to potentially redirect based on a property in the user doc? I can use Meteor.loggingIn() or if (Meteor.userId() && !Meteor.user()) return to ignore the first run, but that seems more like a workaround than a legitimate way to code it. Should I submit a meteor issue asking them to name their Meteor.users publication so that people can wait on it if necessary?

@alanning
Copy link
Contributor

alanning commented Oct 9, 2014

+1 for naming the subscription in Roles as @aldeed suggested.

+1 for doing the same in accounts-base: https://github.com/meteor/meteor/blob/devel/packages/accounts-base/accounts_server.js#L1200

@merrington, I'm also curious to see your repro. Once you open the new ticket please reference here.

@tmeasday
Copy link
Contributor

tmeasday commented Oct 9, 2014

@aldeed - I've always used Meteor.loggingIn() as a proxy to the ready handle of the unnamed user sub. I guess you can turn it into something that looks like a waitOn handle easily enough.

FYI: I'd avoid using Meteor.user() in hook as it'll force the hook to re-run whenever the user changes. This might be annoying in some cases.

@aldeed
Copy link
Author

aldeed commented Oct 12, 2014

I filed a meteor issue, so I'll close this one.

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

5 participants