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

[bug] DRF mixins which restrict who can set objects to organization=None (shared) #235

Closed
nemesifier opened this issue Apr 3, 2021 · 9 comments · Fixed by #239
Closed
Assignees
Labels

Comments

@nemesifier
Copy link
Member

nemesifier commented Apr 3, 2021

Make sure shared objects can be created and edited only by super administrators.
Non super administrators should only be able to view shared objects.

Otherwise operators with limited access to the system can create objects which can affect the entire system also for other organizations.

EG: the user of an org may create a required shared template which adds their SSH keys to the devices of all the organizations in the system, effectively gaining access to devices that they are not authorized to see.

I'm not sure how we could implement this, maybe we could implement this directly in FilterSerializerByOrganization?

What do you think @purhan @ManishShah120?

BTW @purhan, this is a question for you, are the children of FilterSerializerByOrganization supposed to be used always in serializers? Because if so, which I think it's the case, it would make the code more readable to move these classes in serializers.py.

@nemesifier nemesifier added the bug label Apr 3, 2021
@purhan
Copy link
Contributor

purhan commented Apr 3, 2021

BTW @purhan, this is a question for you, are the children of FilterSerializerByOrganization supposed to be used always in serializers? Because if so, which I think it's the case, it would make the code more readable to move these classes in serializers.py.

Yes, They have to be used always. We should move them into serializers.py.

@nemesifier
Copy link
Member Author

Any suggestion for fixing this issue? What could be a good generic way to disallow non superusers from creating shared objects? Could we handle it at validation level or can we avoid showing the empty organization choice if the logged in user is not superuser?

@ManishShah120
Copy link
Member

ManishShah120 commented Apr 7, 2021

@purhan ,I think this can be handled by the class FilterSerializerByOrganization by adding a few more logics to it.
Am I correct Purhan?? Can I please get your suggestion regarding this. This is a major concern I am facing while working on REST API

@purhan
Copy link
Contributor

purhan commented Apr 7, 2021

Sorry this was lost in my notifications.

@purhan ,I think this can be handled by the class FilterSerializerByOrganization by adding a few more logics to it.
Am I correct Purhan?? Can I please get your suggestion regarding this. This is a major concern I am facing while working on REST API

@ManishShah120 Can you point me to the issue you are facing (reference to the line in the code)? I had to create this class in openwisp-ipam that you might want to look at https://github.com/openwisp/openwisp-ipam/blob/03b6a28f7a6cdb1d3d2e3436c7593f3181be94e7/openwisp_ipam/api/utils.py#L8-L35 maybe this can be turned into something more reusable. Also reminder to change the code in IPAM accordingly.

@ManishShah120
Copy link
Member

maybe this can be turned into something more reusable.

Yes, this will be a better Idea, I realised that that this logic will be required when working with other apps API as well. Let me give a try and I'll get back to you.

Can you point me to the issue you are facing (reference to the line in the code)?

openwisp/openwisp-controller#386 (review)

@ManishShah120
Copy link
Member

ManishShah120 commented Apr 10, 2021

Hi, @nemesisdesign I need little clarification with this point:-

Non super administrators should only be able to view shared objects.

Should this not be valid for the Admin page as well. If yes, then presently we can't view the shared objects, suppose admin has created two shared template, and then the operator logs in, then the operator will be able to see the shared templates in the devices config section, but the shared template is not be present in the template page/section, only those template is found their which are belonging to the operator's organization. Is this a bug?? Or this is how it is meant to be.

@ManishShah120
Copy link
Member

ManishShah120 commented Apr 11, 2021

Make sure shared objects can be created and edited only by super administrators.
I'm not sure how we could implement this, maybe we could implement this directly in FilterSerializerByOrganization?

Yes, this can be done, I tested it in FilterSerializerByOrganization. Waiting for your review here #386, after that I'll open a PR to resolve this.

@nemesifier
Copy link
Member Author

Hi, @nemesisdesign I need little clarification with this point:-

Non super administrators should only be able to view shared objects.

Should this not be valid for the Admin page as well. If yes, then presently we can't view the shared objects, suppose admin has created two shared template, and then the operator logs in, then the operator will be able to see the shared templates in the devices config section, but the shared template is not be present in the template page/section, only those template is found their which are belonging to the operator's organization. Is this a bug?? Or this is how it is meant to be.

You are right, in the admin it is currently not possible to view shared objects but we should find a way to allow this, it has been on the back of my mind for months and therefore I have opened an issue right now: #238, thanks for bringing this up!

However, in order to be consistent with the admin, let's skip this part and let's just focus on making sure the API does not allow creating shared objects to non superusers, only superusers must be able to create shared objects.

Only superusers will be able to see shared-objects in this phase.

Non superusers can only select these shared objects when editing/creating objects which have a relation which list these shared objects.

Is it clear?

@ManishShah120
Copy link
Member

Is it clear?

Yes, Thank you I'll soon resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants