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

[feature] Add class to implement view permissions in DRF #249

Closed
nemesifier opened this issue May 12, 2021 · 3 comments · Fixed by #251
Closed

[feature] Add class to implement view permissions in DRF #249

nemesifier opened this issue May 12, 2021 · 3 comments · Fixed by #251
Assignees

Comments

@nemesifier
Copy link
Member

nemesifier commented May 12, 2021

DRF DjangoModelPermission does not takes into account the view permissions, and hence any user is allowed to view the objects even though they may not have view permission or change permission on that specific object.

We recently dealt with this in openwisp-network-topology, we could use a similar solution, adapted to be more generic:
https://github.com/openwisp/openwisp-network-topology/blob/3c3404748a9ecab65f3f4a389f94d2c75fbde732/openwisp_network_topology/api/views.py#L31-L51.

I think we could extend DjangoModelPermissions and add our logic so that:

  • if user is superuser, any request is allowed
  • if user is not superuser and has view or change permission, GET requests are allowed
  • if user is not superuser and does not have view or change permission, GET requests are not allowed

Here's a few possible tests explained below.

Preconditions:

  • given the operator group does not have view permission to a specific model
  • given the administrator group has change permission to the same model (view permission is omitted, ensure is missing)
  • given two API endpoints for this model: list and detail

Tests:

  • ensure an user of the operator group (not flagged as supersuser) cannot view both endpoints
  • ensure an user of the administrator group (not flagged as supersuser) can view both endpoints
  • add the view permission to the operator group, ensure the user now can view both endpoints but cannot change anything (put or delete)
@nemesifier
Copy link
Member Author

Update: there's a an issue in django-rest-framework for this:
encode/django-rest-framework#6324
I think we should invest in helping out fixing it there.

@ManishShah120
Copy link
Member

Legends:

  1. is_admin_org1_administrator: Is admin of organization1 and belongs to administrator group
  2. not_is_admin_org1_administrator: Is not admin of any organization and belongs to administrator group
  3. is_admin_org1_operator: Is admin of organization1 and belongs to operator group
  4. not_is_admin_org1_operator: Is not admin of any organization and belongs to operator group

Hi, @nemesisdesign thise are the permission of different user:-

is_admin_org1_administrator not_is_admin_org1_administrator ❎
'config.add_device', 'config.add_device',
'config.change_device', 'config.change_device',
'config.delete_device', 'config.delete_device',
'config.add_config', 'config.add_config',
'config.change_config', 'config.change_config',
'config.delete_config', 'config.delete_config',
'config.add_vpn', 'config.add_vpn',
'config.delete_vpn', 'config.delete_vpn',
'config.change_vpn', 'config.change_vpn',
'config.view_vpn', 'config.view_vpn',
'config.add_template', 'config.add_template',
'config.delete_template', 'config.delete_template',
'config.change_template', 'config.change_template',
--------------------------- -------------------------------
is_admin_org1_operator not_is_admin_org1_operator
--------------------------- -------------------------------
'config.add_device', 'config.add_device',
'config.change_device', 'config.change_device',
'config.delete_device', 'config.delete_device',
'config.add_config', 'config.add_config',
'config.change_config', 'config.change_config',
'config.delete_config', 'config.delete_config',
'config.view_vpn', 'config.view_vpn',
'config.delete_template', 'config.change_template',
'config.change_template', 'config.delete_template',
'config.add_template' 'config.add_template',

The user's marked as ❎ is not allowed to create objects.

Here the issue which @atb00ker pointed out 👉 openwisp/openwisp-controller#386 (comment)
is may be due to this.

and how will we check on permissions to solve this this issue, Is not related to being a orgniazatin manager or not?

@ManishShah120
Copy link
Member

@ManishShah120 @atb00ker I am not sure I understand what we're discussing, can you be more explicit and provide some user stories as examples?
I am not following you.

As a general rule, for now, I'd simplify everything:

only organization managers (having is_admin=True) or superusers can use the API
non superusers have limited permissions, according to the permissions defined in django

So if user is not manager of the org, it can't do anything via the API right now.

If user is superuser, can do anything.

If user is not superuser and is manager of an org, it can do what their django permissions allow them to do.

Copy from Gitter response

This is what I wanted to explain 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants