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

Add has permission class decorator #150

Closed

Conversation

Maillol
Copy link

@Maillol Maillol commented Apr 19, 2018

Decorator that set permission for each aiohttp.web.View method.

@class_has_permission(prefix='booking')
class BookingView(web.View):
     def post(self):
         """
         Require `booking.create` permission
         """

     def get(self):
         """
         Require `booking.read` permission
         """

     def put(self):
         """
         Require `booking.update` permission
         """

     def patch(self):
         """
         Require `booking.update` permission
         """

     def delete(self):
         """
         Require `booking.delete` permission
         """

Note: This pull request is based on #148

This decorator adds permission for each
method of `aiohttp.web.View` class
@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #150 into master will increase coverage by 0.47%.
The diff coverage is 95.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   94.97%   95.44%   +0.47%     
==========================================
  Files          10       11       +1     
  Lines         517      702     +185     
  Branches       18       25       +7     
==========================================
+ Hits          491      670     +179     
- Misses         20       23       +3     
- Partials        6        9       +3
Impacted Files Coverage Δ
aiohttp_security/__init__.py 100% <100%> (ø) ⬆️
aiohttp_security/api.py 90.62% <81.25%> (+0.38%) ⬆️
tests/test_class_has_permission.py 96.11% <96.11%> (ø)
tests/test_dict_autz.py 96.47% <98.52%> (+0.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 797d892...2371a95. Read the comment docs.

@@ -147,6 +156,41 @@ def wrapper(fn):
return wrapper


def class_has_permission(permission_prefix, context=None):
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, may be name this function view_has_permission to indicate that decorator works for web.View?

Copy link
Author

Choose a reason for hiding this comment

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

Yes we can change the name, but if the has_permission decorator is deprecated (#169) I think that we cannot use this API to have set global permission.

Copy link
Member

Choose a reason for hiding this comment

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

I was thought that not just has_permission was deprecated, but the decorator-way to check permissions is deprecated. Wouldn't this one face the same fate?

'delete': 'delete'}

for method_name, permission in methods.items():
method = getattr(cls, method_name, None)
Copy link
Member

Choose a reason for hiding this comment

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

should we also check type of cls object and throw TypeError if clsis not web.Veiw?

@jettify
Copy link
Member

jettify commented Jun 13, 2018

I have 2 questions, in general I like PR and it has also lot of tests. @kxepal @asvetlov what do you think?

@asvetlov
Copy link
Member

If we'll decide to deprecate decorators the PR should be revisited.
Let's postpone merging for now.

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2018

Thank you for PR.
Unfortunately, I've chosen another solution: no more decorators but check_permission explicit call.

@asvetlov asvetlov closed this Sep 6, 2018
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.

None yet

4 participants