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 168 forks source link

V2.0 #159

Closed mitar closed 8 years ago

mitar commented 8 years ago

Ready for review and merging. Feedback welcome.

I changed the style to use ; at the end of the lines because otherwise you can have strange bugs. This is the recommended practice.

TODO:

mitar commented 8 years ago

All old tests are passing. All old functionality is there. Groups are renamed to partitions. Partitions are now always used. There are some backwards incompatible changes to make code more secure, especially important for such package (I throw more exceptions when there are invalid inputs to functions, no auto-creation of roles when assigning them).

Edit methods can be called only on the server side or from methods, but not directly (allow/deny approach is getting deprecated anyway).

alanning commented 8 years ago

Thanks @mitar !! I'm working my way through your changes.

Initial thoughts are all positive. Awesome work!

Function-wise, it would help me if you could update the usage examples section in the Readme with changes / using the hierarchy.

I have to think further on if there should be a difference between a default partition and a "global" partition (to use the old terminology).

Stylistically semi-colons shouldn't be used in this repo but that's not a big deal.

mitar commented 8 years ago

Function-wise, it would help me if you could update the usage examples section in the Readme with changes / using the hierarchy.

Yes, README is on the todo.

mitar commented 8 years ago

BTW, this pull requests is build upon #145 and #146, so it would be great if you could merge them in first.

mitar commented 8 years ago

Stylistically semi-colons shouldn't be used in this repo but that's not a big deal.

Yes, I am proposing to change this. Because for me this is not just a question of style (for other things you choose whatever you see fit), but ending ; can really change how does parser parse it so for me is beyond just style. It just makes things more robust and predictable. Especially in a security-sensitive libraries I would not go for things without ; where then two statements can become one and some condition might never be checked.

mitar commented 8 years ago

Function-wise, it would help me if you could update the usage examples section in the Readme with changes / using the hierarchy.

Done. README and examples and API documentation updated.

mitar commented 8 years ago

This is done. It has all the functionalities we discussed, examples, updated documentation, and full test suite.

I would recommend that you give me commit access to the repository and I can merge this and help you maintain the package, as I am more familiar with the code now. I can also publish a new package if you give me permissions on the Meteor package. If you give me access also to the roles.meteor.com, I can also release new version there. I would also go through all open pull requests and tickets and see which one are still relevant and address them, or close others.

Any feedback welcome.

alanning commented 8 years ago

@mitar, thank you! What a great Christmas present! :-)

Glad you are interested in joining as a maintainer and I'm happy to have your help. I'm back at my parent's home for the Christmas holiday now but timing-wise I'd like to review this and get this into production early Jan.

One feature that I will need shortly for my own app is granting users temporary access to certain functionality without forcing them to log in first. So that will be a great test case to see how the new design handles extension.

I'm looking forward to going through your work!

mitar commented 8 years ago

One feature that I will need shortly for my own app is granting users temporary access to certain functionality without forcing them to log in first. So that will be a great test case to see how the new design handles extension.

Hm, I do not really see an easy way to do so. Maybe rather use something like:

alanning commented 8 years ago

Thanks for doing the README. Big help for me.

I merged your PR into this repo's v2.0 branch so that we can work on it more there.

I created an issue so we can track discussion of a few things. Not sure if that's the best forum for it but we can try it out.

mitar commented 8 years ago

Not sure if that's the best forum for it but we can try it out.

I prefer discussion in the tickets. So for me is good.

mitar commented 8 years ago

Closing this in favor of #165.