search

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
921 stars 167 forks source link

Unclean exception when trying to set grouped roles to a user with a non-grouped role #233

Closed mbittmann closed 7 years ago

mbittmann commented 7 years ago

I think I may have found a bug in error handling.

I had a user with a role that was not associated with any groups. Roles.setUserRoles(Meteor.userId(), ['some-role']);

Then after that I tried to add that user to grouped roles: Roles.setUserRoles(Meteor.userId(), ['other1', 'other2'], group_name)

Both were set server side. I would have expected to get an error ...Can't mix grouped and non-grouped ... but instead got:

I20170224-10:33:55.582(-5)? Exception in onLogin callback: TypeError: Cannot read property 'indexOf' of undefined
I20170224-10:33:55.582(-5)?     at strContains (packages/alanning_roles.js:789:25)
I20170224-10:33:55.582(-5)?     at packages/alanning_roles.js:784:12
I20170224-10:33:55.582(-5)?     at Array.some (native)
I20170224-10:33:55.583(-5)?     at Function._.some._.any (packages/underscore.js:267:59)
I20170224-10:33:55.583(-5)?     at isMongoMixError (packages/alanning_roles.js:783:12)
I20170224-10:33:55.583(-5)?     at Object._.extend._updateUserRoles (packages/alanning_roles.js:762:39)
I20170224-10:33:55.583(-5)?     at Object._.extend.addUsersToRoles (packages/alanning_roles.js:221:11)
I20170224-10:33:55.583(-5)?     at app/app.js:2064:17
alanning commented 7 years ago

Hi Mark, thanks for reporting this. I have noticed this happen when MongoDB changes their implementation. Which DB version did you see this with?

Adrian

On Fri, Feb 24, 2017 at 10:49 AM, Mark Bittmann notifications@github.com wrote:

I think I may have found a bug in error handling.

I had a user with a role that was not associated with any groups. Roles.setUserRoles(Meteor.userId(), ['some-role']);

Then after that I tried to add that user to grouped roles: Roles.setUserRoles(Meteor.userId(), ['other1', 'other2'], group_name)

Both were set server side. I would have expected to get an error ...Can't mix grouped and non-grouped ... but instead got:

I20170224-10:33:55.582(-5)? Exception in onLogin callback: TypeError: Cannot read property 'indexOf' of undefined I20170224-10:33:55.582(-5)? at strContains (packages/alanning_roles.js:789:25) I20170224-10:33:55.582(-5)? at packages/alanningroles.js:784:12 I20170224-10:33:55.582(-5)? at Array.some (native) I20170224-10:33:55.583(-5)? at Function..some._.any (packages/underscore.js:267:59) I20170224-10:33:55.583(-5)? at isMongoMixError (packages/alanningroles.js:783:12) I20170224-10:33:55.583(-5)? at Object..extend._updateUserRoles (packages/alanningroles.js:762:39) I20170224-10:33:55.583(-5)? at Object..extend.addUsersToRoles (packages/alanning_roles.js:221:11) I20170224-10:33:55.583(-5)? at app/app.js:2064:17

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/alanning/meteor-roles/issues/233, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtvDGDLmSBWntWBsxc1c4NB2hbuu5rbks5rfvv9gaJpZM4MLWoU .

mbittmann commented 7 years ago

I'm using the embedded mongo 3.2.6 with Meteor 1.4.2.3

mitar commented 7 years ago

I think they again changed the exception format?

alanning commented 7 years ago

Yes, that's usually the cause when this breaks. Strangely enough the unit test for this case is passing with latest Meteor (METEOR@1.4.2.7).

Mark, could you try running the roles unit tests and see if the "Mixing groups and non-groups" test fails, please? Not quite sure how to get it to use your version of meteor. Perhaps you can clone the meteor-roles repo into your packages dir and run the tests that way.

meteor test-packages ./roles

On Fri, Feb 24, 2017 at 4:33 PM, Mitar notifications@github.com wrote:

I think they again changed the exception format?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/alanning/meteor-roles/issues/233#issuecomment-282410350, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtvDMFnH8jCmZvrsUDN-sprJrIrSzcxks5rf0y4gaJpZM4MLWoU .

mbittmann commented 7 years ago

Yep, it passes, and it appears to be running the same version of meteor. PASS 73 ms S: mixing group with non-group throws descriptive error

cd .meteor/local/build/programs/server/packages/
git clone https://github.com/alanning/meteor-roles.git
cd meteor-roles/
meteor test-packages ./roles

Not sure if it matters, but meteor is telling me Your development database is using mmapv1, the old, pre-MongoDB 3.0 database engine. You should consider upgrading to Wired Tiger, the new engine.

alanning commented 7 years ago

Hmm, I'd like to have the code support the older versions too. With the code in your packages directory you should be able to console log the actual error that is being thrown. Could you re-run your in-app test and send the results to us please?

Even better if you'd like to do a pull request for handling whatever error message you are seeing from MongoDB! :-)

mbittmann commented 7 years ago

Weird. If I meteor remove alanning:roles and instead clone it into a top level packages directory, then I get the appropriate error messages. In both cases the version is 1.2.15. I'm totally new to using custom packages, so it is very possible I'm doing something wrong. I have so far only used meteor add for third party packages.

mitar commented 7 years ago

I think the issue is that some recent fixes have not yet been released. Like this one: https://github.com/alanning/meteor-roles/commit/eb3376ae0c288a0d6c3b8acbfc2730dc722a3dd0

@alanning: maybe we should make a release for 1x version?

alanning commented 7 years ago

Sounds good, would you like to do the honors? :-) On Mon, Feb 27, 2017 at 1:58 PM Mitar notifications@github.com wrote:

I think the issue is that some recent fixes have not yet been released. Like this one: eb3376a https://github.com/alanning/meteor-roles/commit/eb3376ae0c288a0d6c3b8acbfc2730dc722a3dd0

@alanning https://github.com/alanning: maybe we should make a release for 1x version?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/alanning/meteor-roles/issues/233#issuecomment-282814834, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtvDPEbMkGtd-u8qfXqZa4Wc2v-_OFtks5rgxzAgaJpZM4MLWoU .

mbittmann commented 7 years ago

I can confirm that my .meteor/local/build/programs/server/packages/alanning_roles.js when using meteor add is:

if (ex.name === 'MongoError' && isMongoMixError(ex.err)) {

mitar commented 7 years ago

Done. Released 1.2.16.

mbittmann commented 7 years ago

You guys are awesome. I just updated the package and can confirm that I'm getting the correct message:

I20170228-17:05:41.680(-5)? Exception in onLogin callback: Error: Roles error: Can't mix grouped and non-grouped roles for same user
I20170228-17:05:41.680(-5)?     at Object._.extend._updateUserRoles (packages/alanning_roles.js:763:15)
I20170228-17:05:41.680(-5)?     at Object._.extend.setUserRoles (packages/alanning_roles.js:250:11)
I20170228-17:05:41.681(-5)?     at app/app.js:2058:19
I20170228-17:05:41.681(-5)?     at runAndHandleExceptions (packages/callback-hook.js:152:24)
I20170228-17:05:41.681(-5)?     at packages/callback-hook.js:159:12