-
-
Notifications
You must be signed in to change notification settings - Fork 64
[tests] Non-superuser can view shared objects #187
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -71,20 +71,21 @@ function initHostsInfiniteScroll( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "</a>" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<a href=\"' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| address_add_url + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "?_to_field=id&_popup=1&ip_address=" + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr.address + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "&subnet=" + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_subnet + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '"onclick="return showAddAnotherPopup(this);" ' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'id="addr_' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '">' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr.address + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "</a>" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var anchorAttributes = 'class="disabled"'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (hasSubnetChangePermission === "true") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| anchorAttributes = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'href=\"' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| address_add_url + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "?_to_field=id&_popup=1&ip_address=" + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr.address + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "&subnet=" + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_subnet + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '"onclick="return showAddAnotherPopup(this);" ' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'id="addr_' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '"'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "<a " + anchorAttributes + " >" + addr.address + "</a>"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+74
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix boolean comparison so authorized users keep links.
🛠️ Proposed fix- if (hasSubnetChangePermission === "true") {
+ if (hasSubnetChangePermission) {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function pageContainer(page) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var div = $('<div class="page"></div>'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| import json | ||
|
|
||
| import django | ||
| from django.contrib.auth import get_user_model | ||
| from django.contrib.auth.models import Group | ||
| from django.core.files.uploadedfile import SimpleUploadedFile | ||
| from django.test import TestCase | ||
| from django.urls import reverse | ||
| from openwisp_users.tests.utils import TestMultitenantAdminMixin | ||
| from swapper import load_model | ||
|
|
||
| from . import CreateModelsMixin, PostDataMixin | ||
|
|
@@ -14,7 +16,7 @@ | |
| IpAddress = load_model("openwisp_ipam", "IpAddress") | ||
|
|
||
|
|
||
| class TestAdmin(CreateModelsMixin, PostDataMixin, TestCase): | ||
| class TestAdmin(TestMultitenantAdminMixin, CreateModelsMixin, PostDataMixin, TestCase): | ||
| app_label = "openwisp_ipam" | ||
|
|
||
| def setUp(self): | ||
|
|
@@ -438,3 +440,76 @@ def assert_response(response): | |
| reverse("admin:ipam_export_subnet", args=[subnet.id]), follow=True | ||
| ) | ||
| assert_response(response) | ||
|
|
||
| def test_superuser_create_shared_subnet(self): | ||
| admin = self._get_admin() | ||
| self.client.force_login(admin) | ||
| response = self.client.post( | ||
| reverse(f"admin:{self.app_label}_subnet_add"), | ||
| data={ | ||
| "name": "test-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| "organization": "", | ||
| }, | ||
| follow=True, | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(Subnet.objects.count(), 1) | ||
|
|
||
| def test_org_admin_view_shared_subnet(self): | ||
| subnet = self._create_subnet(organization=None, subnet="10.8.0.0/24") | ||
| self._test_org_admin_view_shareable_object( | ||
| reverse(f"admin:{self.app_label}_subnet_change", args=(subnet.id,)), | ||
| ) | ||
|
|
||
| def test_org_admin_create_shared_subnet(self): | ||
| self._test_org_admin_create_shareable_object( | ||
| reverse(f"admin:{self.app_label}_subnet_add"), | ||
| payload={ | ||
| "name": "test-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| "organization": "", | ||
| }, | ||
| model=Subnet, | ||
| ) | ||
|
|
||
| def test_superuser_create_shared_ip(self): | ||
| admin = self._get_admin() | ||
| self.client.force_login(admin) | ||
| shared_subnet = self._create_subnet(subnet="10.0.0.0/24", organization=None) | ||
| response = self.client.post( | ||
| reverse(f"admin:{self.app_label}_ipaddress_add"), | ||
| data={ | ||
| "subnet": str(shared_subnet.id), | ||
| }, | ||
| follow=True, | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(Subnet.objects.count(), 1) | ||
|
|
||
|
Comment on lines
+476
to
+489
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test doesn’t prove IP creation.
✅ Example fix (assert creation) response = self.client.post(
reverse(f"admin:{self.app_label}_ipaddress_add"),
data={
"subnet": str(shared_subnet.id),
+ "ip_address": "10.0.0.1",
},
follow=True,
)
self.assertEqual(response.status_code, 200)
- self.assertEqual(Subnet.objects.count(), 1)
+ self.assertEqual(IpAddress.objects.count(), 1)🤖 Prompt for AI Agents |
||
| def test_org_admin_view_shared_ip(self): | ||
| shared_subnet = self._create_subnet(subnet="10.0.0.0/24", organization=None) | ||
| ip = shared_subnet.request_ip() | ||
| self._test_org_admin_view_shareable_object( | ||
| reverse(f"admin:{self.app_label}_ipaddress_change", args=(ip.id,)), | ||
| expected_element=( | ||
| '<div class="form-row field-ip_address">\n\n\n<div>\n\n' | ||
| '<div class="flex-container">\n\n' | ||
| "<label>Ip address:</label>\n\n" | ||
| '<div class="readonly">10.0.0.1</div>\n\n\n' | ||
| "</div>\n\n</div>\n\n\n</div>" | ||
| ), | ||
| ) | ||
|
|
||
| def test_org_admin_create_shared_ip(self): | ||
| shared_subnet = self._create_subnet(subnet="10.0.0.0/24", organization=None) | ||
| self._test_org_admin_create_shareable_object( | ||
| reverse(f"admin:{self.app_label}_ipaddress_add"), | ||
| payload={ | ||
| "subnet": str(shared_subnet.id), | ||
| }, | ||
| model=IpAddress, | ||
| error_message=( | ||
| '<ul class="errorlist"{}><li>This field is required.</li></ul>' | ||
| ).format(' id="id_ip_address_error"' if django.VERSION >= (5, 2) else ""), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,8 @@ | |
|
|
||
| from django.contrib.auth import get_user_model | ||
| from django.core.files.uploadedfile import SimpleUploadedFile | ||
| from django.test import TestCase | ||
| from django.urls import reverse | ||
| from openwisp_users.tests.utils import TestMultitenantAdminMixin | ||
| from openwisp_users.tests.test_api import APITestCase | ||
| from swapper import load_model | ||
|
|
||
| from . import CreateModelsMixin, PostDataMixin | ||
|
|
@@ -14,7 +13,7 @@ | |
| IpAddress = load_model("openwisp_ipam", "IpAddress") | ||
|
|
||
|
|
||
| class TestApi(TestMultitenantAdminMixin, CreateModelsMixin, PostDataMixin, TestCase): | ||
| class TestApi(CreateModelsMixin, PostDataMixin, APITestCase): | ||
| def setUp(self): | ||
| super().setUp() | ||
| self._login() | ||
|
|
@@ -351,3 +350,83 @@ def test_subnet_single_hosts_first_address(self): | |
| host_address_32 = response.data["results"][0]["address"] | ||
| self.assertEqual(host_address_128, "2001:db00::") | ||
| self.assertEqual(host_address_32, "192.168.0.0") | ||
|
|
||
| def test_superuser_access_shared_subnet(self): | ||
| self._logout() | ||
| self._test_superuser_access_shared_object( | ||
| token=None, | ||
| listview_name="ipam:subnet_list_create", | ||
| detailview_name="ipam:subnet", | ||
| create_payload={ | ||
| "name": "test-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| "description": "Test Subnet", | ||
| "organization": None, | ||
| }, | ||
| update_payload={ | ||
| "name": "updated-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| }, | ||
| expected_count=1, | ||
| ) | ||
|
|
||
| def test_org_manager_access_shared_subnet(self): | ||
| self._logout() | ||
| shared_subnet = self._create_subnet(organization=None, subnet="10.0.0.0/24") | ||
| self._test_org_user_access_shared_object( | ||
| listview_path=reverse("ipam:subnet_list_create"), | ||
| detailview_path=reverse("ipam:subnet", args=[shared_subnet.pk]), | ||
| create_payload={ | ||
| "name": "test-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| "description": "Test Subnet", | ||
| "organization": None, | ||
| }, | ||
| update_payload={ | ||
| "name": "updated-subnet", | ||
| "subnet": "10.0.0.0/24", | ||
| }, | ||
| expected_count=1, | ||
| ) | ||
|
|
||
| def test_superuser_access_shared_ip(self): | ||
| self._logout() | ||
| subnet = self._create_subnet(subnet="10.0.0.0/24", organization=None) | ||
| self._test_superuser_access_shared_object( | ||
| token=None, | ||
| listview_path=reverse("ipam:list_create_ip_address", args=[subnet.id]), | ||
| detailview_name="ipam:ip_address", | ||
| create_payload={ | ||
| "ip_address": "10.0.0.1", | ||
| "subnet": str(subnet.id), | ||
| "description": "Test IP", | ||
| }, | ||
| update_payload={ | ||
| "description": "updated-ip", | ||
| "ip_address": "10.0.0.1", | ||
| "subnet": str(subnet.id), | ||
| }, | ||
| expected_count=1, | ||
| ) | ||
|
|
||
| def test_org_manager_access_shared_ip(self): | ||
| self._logout() | ||
| shared_subnet = self._create_subnet(subnet="10.0.0.0/24", organization=None) | ||
| shared_ip = shared_subnet.request_ip() | ||
| self._test_org_user_access_shared_object( | ||
| listview_path=reverse( | ||
| "ipam:list_create_ip_address", args=[shared_subnet.id] | ||
| ), | ||
| detailview_path=reverse("ipam:ip_address", args=[shared_ip.id]), | ||
| create_payload={ | ||
| "ip_address": "10.0.0.2", | ||
| "subnet": str(shared_subnet.id), | ||
| "description": "Test IP", | ||
| }, | ||
| update_payload={ | ||
| "description": "updated-ip", | ||
| "ip_address": "10.0.0.1", | ||
| "subnet": str(shared_subnet.id), | ||
| }, | ||
| expected_count=1, | ||
| ) | ||
|
Comment on lines
+354
to
+432
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: rg -n "def _test_superuser_access_shared_object" -A 10Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: rg -n "def _test_org_user_access_shared_object" -A 10Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: rg -n "def _test_superuser_access_shared_object|def _test_org_user_access_shared_object" -B 2 -A 10Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: # Find the test class definition and parent classes
rg -n "class.*Test.*:" openwisp_ipam/tests/test_api.py -A 2 | head -20Repository: openwisp/openwisp-ipam Length of output: 181 🏁 Script executed: # Find where the mixins are defined
fd -e "py" -x grep -l "class CreateModelsMixin\|class PostDataMixin" {} \;Repository: openwisp/openwisp-ipam Length of output: 99 🏁 Script executed: # Look for _test_superuser_access_shared_object in the entire codebase
find . -name "*.py" -type f -exec grep -l "_test_superuser_access_shared_object" {} \;Repository: openwisp/openwisp-ipam Length of output: 99 🏁 Script executed: # Find the helper function definitions in test_api.py
rg -n "_test_superuser_access_shared_object|_test_org_user_access_shared_object" openwisp_ipam/tests/test_api.py | head -30Repository: openwisp/openwisp-ipam Length of output: 283 🏁 Script executed: # Get line count to understand file size
wc -l openwisp_ipam/tests/test_api.pyRepository: openwisp/openwisp-ipam Length of output: 101 🏁 Script executed: # Get the helper function definitions from __init__.py
cat -n openwisp_ipam/tests/__init__.py | grep -A 15 "_test_superuser_access_shared_object"Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: # Also check for _test_org_user_access_shared_object
cat -n openwisp_ipam/tests/__init__.py | grep -A 15 "_test_org_user_access_shared_object"Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: # Check size of __init__.py
wc -l openwisp_ipam/tests/__init__.pyRepository: openwisp/openwisp-ipam Length of output: 100 🏁 Script executed: # Look for openwisp_users package
find . -type d -name "openwisp_users" | head -5Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: # Search more broadly for the helper functions
grep -r "_test_superuser_access_shared_object" . --include="*.py" 2>/dev/nullRepository: openwisp/openwisp-ipam Length of output: 235 🏁 Script executed: # Read the __init__.py to understand imports and class hierarchy
cat -n openwisp_ipam/tests/__init__.pyRepository: openwisp/openwisp-ipam Length of output: 1944 🏁 Script executed: # Check if there's any openwisp_users available in the repo or requirements
find . -name "*requirements*" -o -name "setup.py" -o -name "pyproject.toml" | xargs grep -l "openwisp_users" 2>/dev/nullRepository: openwisp/openwisp-ipam Length of output: 50 🏁 Script executed: # Search in site-packages or installed packages (if available)
python3 -c "import openwisp_users.tests.utils; import inspect; print(inspect.getfile(openwisp_users.tests.utils.TestOrganizationMixin))" 2>/dev/null || echo "Not installed"Repository: openwisp/openwisp-ipam Length of output: 79 🌐 Web query:
💡 Result: I can't find that helper in the public OpenWISP repos. Can you paste the test file or a link to the repository/commit where TestOrganizationMixin._test_superuser_access_shared_object is defined? 🏁 Script executed: # Check setup.py or pyproject.toml for openwisp_users URL/version
find . -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" | xargs cat 2>/dev/null | grep -A 5 -B 5 "openwisp_users" || echo "Not found"Repository: openwisp/openwisp-ipam Length of output: 75 🏁 Script executed: # Check git log for clues about these test methods
git log -p --all -S "_test_superuser_access_shared_object" -- openwisp_ipam/tests/test_api.py 2>/dev/null | head -100 || echo "Git not available"Repository: openwisp/openwisp-ipam Length of output: 48 🏁 Script executed: # Extract the exact helper calls to see all argument names used
sed -n '356,373p' openwisp_ipam/tests/test_api.pyRepository: openwisp/openwisp-ipam Length of output: 665 🏁 Script executed: # Extract the second call
sed -n '374,393p' openwisp_ipam/tests/test_api.pyRepository: openwisp/openwisp-ipam Length of output: 808 🏁 Script executed: # Extract the third call
sed -n '394,414p' openwisp_ipam/tests/test_api.pyRepository: openwisp/openwisp-ipam Length of output: 890 🏁 Script executed: # Extract the fourth call
sed -n '415,432p' openwisp_ipam/tests/test_api.pyRepository: openwisp/openwisp-ipam Length of output: 757 Fix inconsistent parameter names in The first call (line 356) uses 🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not merge with a feature-branch dependency.
Installing
openwisp-usersfrom a feature branch (issues/238-view-shared-objects) is not suitable for merging tomaster:The commit message
[temp] Upgraded openwisp-usersand the PR description's reference to blockeropenwisp/openwisp-users#444confirm this is a temporary workaround.Before merging, either:
# TODO:comment explaining the temporary dependency and linking to the blocker issue so it's not forgotten.Additionally, the
cryptography~=43.0.3pin is unexplained—please add a comment documenting why this specific version constraint is required.🤖 Prompt for AI Agents