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.

Sign up for GitHub

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

Jump to bottom

[fix] Validate organization membership when importing subnets #77 #97

Merged
merged 5 commits into from
Oct 13, 2021
Merged

[fix] Validate organization membership when importing subnets #77 #97

merged 5 commits into from
Oct 13, 2021

Conversation

purhan
Copy link
Contributor

@purhan purhan commented Apr 28, 2021

Closes #77

@purhan purhan added this to In progress in OpenWISP Priorities for next releases via automation Apr 28, 2021
@purhan purhan moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Apr 28, 2021
@purhan purhan self-assigned this Apr 28, 2021
@coveralls
Copy link

coveralls commented Apr 28, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.003%) to 99.854% when pulling 7d51798 on fix-admin-subnet-import into ab63ccf on master.

pandafy
pandafy reviewed May 3, 2021
View reviewed changes
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

The CSV file format in the README needs to be updated as well.

openwisp_ipam/admin.py Outdated Show resolved Hide resolved
openwisp_ipam/tests/test_multitenant.py Outdated Show resolved Hide resolved
openwisp_ipam/admin.py Outdated
@@ -145,6 +161,11 @@ def import_view(self, request):
return redirect('/admin/{0}/subnet'.format(self.app_label))
return render(request, form_template, context)

def get_csv_organization(self, request):
data = Subnet._get_csv_reader(self, deepcopy(request.FILES['csvfile']))
org = Organization.objects.get(name=list(data)[2][0].strip())
Copy link
Member

Choose a reason for hiding this comment

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

The name of the organization is not unique. We can use organization slug or secret instead.
But this is not the only such occurrence, I will open a separate issue for this.

Copy link
Member

Choose a reason for hiding this comment

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

Gagan is right, we need to use the organization slug here.
Looking at the code we found similar issues: #99 and #101.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I leave it here, and take care of this separately in #99?

Copy link
Member

Choose a reason for hiding this comment

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

@purhan sorry I forgot to reply! Definitely change this one here, since we're adding it, there's no reason to keep adding it in the wrong way 😊

Please could you ping me when ready so I don't forget? Thanks!

@purhan purhan force-pushed the fix-admin-subnet-import branch 2 times, most recently from a83d1d0 to 0f76ca9 Compare May 11, 2021 11:58
nemesifier
nemesifier requested changes May 17, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

The solutions works! Please see below for requests for improvements and then we should be done with this 👍

openwisp_ipam/admin.py Outdated Show resolved Hide resolved
openwisp_ipam/admin.py Outdated Show resolved Hide resolved
openwisp_ipam/admin.py Outdated Show resolved Hide resolved
openwisp_ipam/admin.py Outdated
@@ -145,6 +161,11 @@ def import_view(self, request):
return redirect('/admin/{0}/subnet'.format(self.app_label))
return render(request, form_template, context)

def get_csv_organization(self, request):
data = Subnet._get_csv_reader(self, deepcopy(request.FILES['csvfile']))
org = Organization.objects.get(name=list(data)[2][0].strip())
Copy link
Member

Choose a reason for hiding this comment

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

Gagan is right, we need to use the organization slug here.
Looking at the code we found similar issues: #99 and #101.

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress May 17, 2021
@purhan purhan moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases May 19, 2021
nemesifier
nemesifier requested changes Aug 1, 2021
View reviewed changes
openwisp_ipam/admin.py Outdated
@@ -145,6 +161,11 @@ def import_view(self, request):
return redirect('/admin/{0}/subnet'.format(self.app_label))
return render(request, form_template, context)

def get_csv_organization(self, request):
data = Subnet._get_csv_reader(self, deepcopy(request.FILES['csvfile']))
org = Organization.objects.get(name=list(data)[2][0].strip())
Copy link
Member

Choose a reason for hiding this comment

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

@purhan sorry I forgot to reply! Definitely change this one here, since we're adding it, there's no reason to keep adding it in the wrong way 😊

Please could you ping me when ready so I don't forget? Thanks!

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Aug 1, 2021
nemesifier
nemesifier requested changes Sep 3, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

There's a conflict in requirements.txt, could you please fix it?

nemesifier
nemesifier requested changes Sep 26, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@purhan the build failure should be fixed in the latest master, can you merge again? For some reason the button to merge does not appear to me.

@nemesifier nemesifier changed the title [fix] Validate subnet import in admin #77 [fix] Validate organization membership when importing subnets #77 Oct 7, 2021
nemesifier
nemesifier reviewed Oct 7, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I renamed the PR name to make it clearer.

nemesifier
nemesifier requested changes Oct 7, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Are we also ensuring that when exporting a subnet, the user exporting has access to that organization?

There's a few pending changes requested below.

openwisp_ipam/api/utils.py Outdated
organization = self.get_csv_organization()
if str(organization.pk) in self.get_user_organizations():
organization = self.get_csv_organization(request)
if str(organization.pk) in self.get_user_organizations(request):
return
Copy link
Member

Choose a reason for hiding this comment

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

I think the code in this file could be simplified and moved directly to the view which inherits this class, since it seems to me is only used in one place.

The code below which checks permission to create an org should not be relevant anymore and can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

It is used in two places:
openwisp_ipam/api/views.py
openwisp_ipam/admin.py

Copy link
Member

Choose a reason for hiding this comment

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

ah ok didn't see it was used in admin.py, let's leave it here, but please double check if the code below which checks permission to create an org still makes sense.

nemesifier
nemesifier approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

OpenWISP Priorities for next releases automation moved this from In progress to Reviewer approved Oct 13, 2021
@nemesifier nemesifier merged commit c94ec0c into master Oct 13, 2021
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Oct 13, 2021
@pandafy pandafy mentioned this pull request Jan 19, 2022
Closed
@nemesifier nemesifier deleted the fix-admin-subnet-import branch April 28, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mips171 mips171 mips171 left review comments

@pandafy pandafy pandafy left review comments

@codesankalp codesankalp codesankalp left review comments

@nemesifier nemesifier nemesifier approved these changes

Assignees

@purhan purhan

Labels
None yet
Projects
OpenWISP Priorities for next releases
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] Users can import subnets for organizations they're not managing
6 participants
@purhan @coveralls @nemesifier @mips171 @pandafy @codesankalp