Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 164 forks source link

getRolesForUser - "Cannot read property '_id' of undefined" on client after edit #308

Closed cormip closed 4 years ago

cormip commented 4 years ago

I have a client-side "users" table that reactively displays the Meteor User accounts (name, email) and their roles. My template calls Roles.getRolesForUser() to retrieve each user's roles. Everything works fine on initial load. However, when a user's roles are updated, i.e. Roles.setUserRoles(user._id, roles), via a server method, I get a console error on the client when the table reactively updates itself:

Exception in template helper: TypeError: Cannot read property '_id' of undefined
    at http://localhost:3000/packages/alanning_roles.js?hash=b56e6b2c0ee0a6e1aa6095edd24e7d0cf4e47bc1:875:132
    at Array.map (<anonymous>)
    at Object.getRolesForUser (http://localhost:3000/packages/alanning_roles.js?hash=b56e6b2c0ee0a6e1aa6095edd24e7d0cf4e47bc1:875:121)
    at fn (/imports/ui/pages/authenticated/admin/users.js:117:31)
    at Object.getField (http://localhost:3000/packages/aslagle_reactive-table.js?hash=d5959cc90cbe650f00f5b58fa8d77953153cecee:1223:16)
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3051:16
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:1715:16
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3103:66
    at Function.Template._withTemplateInstanceFunc (http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3769:14)
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3102:27

I am also null-publishing the users' roles:

Meteor.publish(null, function () {
    if (this.userId) {
        return Meteor.roleAssignment.find({ 'user._id': this.userId });
    } else {
        this.ready()
    }
})

Any thoughts on how to address this? It seems like it may be a race condition where the user's roles are removed (and then added) while Roles.getRolesForUser()is running?

alanning commented 4 years ago

If you have a reproduction you could link to that would help.

Otherwise, could you post the "readable" javascript around where the exception is thrown, please?

cormip commented 4 years ago

The line that's crashing in roles_common.js is line 780:

return [...new Set(roles.map(r => r.inheritedRoles || [r.role]).reduce((rev, current) => rev.concat(current), []).map(r => r._id))]

roles = [] when that line of code is run, resulting in the crash.

However, the code automatically runs a second time with the roles array populated with the role assignment, which allows the code to work fine and the page to render correctly.

I think the app is reacting to the old roles being deleted by setUserRoles, resulting in roles = [] when querying Meteor.roleAssignment, then again reactively updating when the new roles are set by setUserRoles? Just to be clear, I'm only calling setUserRoles once with the new role assignments.

SimonSimCity commented 4 years ago

I've myself tested it here, and it only occurs when having a role-assignment without role - which is pretty weird.

Can you please confirm this - and also (in case you have it) look at a place this could come from?

A sandbox application, where I could try this out would be nice.

Shelagh-Lewins commented 4 years ago

I've experienced the same problem, but only when my app is open in two different tabs or browsers. And if it's two tabs in the same browser, then the order of the tabs makes a difference. No, really! I tested it like 15 times!

I am using observeChanges to dynamically track user roles based on whether their email address is verified:

    Meteor.users.find().observeChanges({
        'changed': function (_id) {
            const user = Meteor.users.findOne({ _id });
            if (!user) {
                return;
            }

            try {
                if (user.emails[0].verified) {
                    Roles.addUsersToRoles(_id, ['verified']);
                } else {
                    Roles.removeUsersFromRoles(_id, ['verified']);
                }
            } catch (err) {
                console.log(`error checking roles for user ${_id}`);
            }
        },
    });
});

The 'verified' role has been created in advance and works fine. If I run my app in a single tab, everything works. I verify an email address in one browser tab, while another is open on the app, getRolesForUser throws the error. I also saw the error appear in Firefox when I verified an email address in Chrome.

I am currently working around this by wrapping getRolesForUser in a try...catch block.

If I only have one browser / tab open on the app, there is no problem. I assume that there is some timing issue whereby addUserToRoles does not operate atomically but instead performs two database operations and a different threaded client can stumble in the gap.

SimonSimCity commented 4 years ago

@Shelagh-Lewins or @cormip would you have time to write a sample sandbox for this?

@cormip you describe it as "running in a server method". What do you mean by this? Is it in a Meteor method? If yes, are you sure this code is not run by the client first - see https://guide.meteor.com/methods.html#call-lifecycle?

@Shelagh-Lewins For your code it looks quite straight forward to be only available on the server.

If you yourself want to dive in, here's the only place I add a role to the system: https://github.com/Meteor-Community-Packages/meteor-roles/blob/master/roles/roles_common.js#L478

The failing code (https://github.com/Meteor-Community-Packages/meteor-roles/blob/master/roles/roles_common.js#L780 [...new Set(roles.map(r => r.inheritedRoles || [r.role]).reduce((rev, current) => rev.concat(current), []).map(r => r._id))] only crashes if the array roles has an element which is not an object having a property role.

I suppose it's a race condition, which are usually hard to reproduce. Creating a sandbox project would be a great time saver to us because we don't have to spend time creating an environment where this might fail.

Just create a new Meteor project, make sure you have all the necessary components to reproduce this bug and upload it somewhere we can access it - e.g. to github.

SimonSimCity commented 4 years ago

This issue should've been fixed in the just published version v3.2.1. Please request a reopen if the bug still persists.