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

Implementation of class-based permission system #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implementation of class-based permission system #19

wants to merge 3 commits into from

Conversation

dankrause
Copy link
Contributor

Implements #16 and fixes #18

@@ -900,32 +893,15 @@ def delete(self, model, property_name=None):
# Utility methods/properties
#

def _get_permission(self, method):
accepted_permission = None
permissions = self.permissions[method]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be:

permissions = self.permissions.get(method, [])

@budowski
Copy link
Owner

Awesome implementation dude! :-)
I've wrote down some notes on the commit...

def _get_permission(self, method):
accepted_permission = None
permissions = self.permissions[method]
for permission_object in permissions:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add support for a singular permission instead of just a list? e.g.

if not isinstance(permissions, list):
    permissions = [permissions]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Adding this.

@budowski
Copy link
Owner

Also - we need to verify these changes don't mess up the users.py module (that it'll still operate normally).

@dankrause
Copy link
Contributor Author

I have tested with a trivial application that uses the users module, and it didn't break anything. That's not saying much though. I think we really need to address that test suite next.

@dankrause
Copy link
Contributor Author

This should address the temp code that was left behind, as well as allow single permission objects instead of requiring lists.

@dankrause
Copy link
Contributor Author

Looks like I totally neglected to implement this in users.py - I'll get that soon.

@budowski
Copy link
Owner

Another small comment - if doing a multi-PUT - there is no validation that the user actually owns those updated models (this check should somehow be done by the Permission instance)

@dankrause
Copy link
Contributor Author

Converting the user handler to use the permission system is going to take some time. I'm going to get some of it done tonight, but it may be a couple more days before I find the time to finish it.

@dankrause
Copy link
Contributor Author

This is going to be more work than I thought. The users handler embeds other routes within it, such as "me", "login", "verify", and "reset". To avoid lots of special cases, I'm going to break them out into separate routes and stuff them in a multi-route. After that is done, I may be able to use our regular rest handler for users, and get rid of A lot of duplicate code.

@budowski
Copy link
Owner

Hmmm, yeah - sorry for the work load :-/
Tell me if you want a hand in this

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

Successfully merging this pull request may close these issues.

Doing a POST with an already existing id will overwrite the old instance
2 participants