-
-
Notifications
You must be signed in to change notification settings - Fork 286
fix(geo): skip WHOIS and estimated location on deactivated devices #1335
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
Changes from all commits
869be48
d3173be
08710c6
ec28726
0bbf434
8d798b4
99173a5
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||
| from datetime import timedelta | ||||||||||||
| from io import StringIO | ||||||||||||
| from unittest import mock | ||||||||||||
| from unittest.mock import patch | ||||||||||||
| from uuid import uuid4 | ||||||||||||
|
|
||||||||||||
| from django.conf import settings | ||||||||||||
|
|
@@ -21,7 +22,22 @@ | |||||||||||
| from selenium.webdriver.common.by import By | ||||||||||||
| from swapper import load_model | ||||||||||||
|
|
||||||||||||
| <<<<<<< HEAD | ||||||||||||
| from openwisp_controller.config.models import Device | ||||||||||||
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | ||||||||||||
| from openwisp_controller.config.whois.service import WhoIsService | ||||||||||||
|
Comment on lines
+26
to
+28
Contributor
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: #!/bin/bash
# Verify the actual service class name exported from whois.service
rg -nP '^\s*class\s+\w*[Ww]ho[Ii][Ss]\w*\s*[:\(]' --type=py
# And check what's importable
rg -nP '\bfrom\s+openwisp_controller\.config\.whois\.service\s+import\b' --type=pyRepository: openwisp/openwisp-controller Length of output: 279 🏁 Script executed: # Check the actual service file for class definitions
cat openwisp_controller/config/whois/service.py | head -50
# Also search for any class definition with WHOIS in the name
rg -n 'class.*[Ww]ho' openwisp_controller/config/whois/service.pyRepository: openwisp/openwisp-controller Length of output: 1569 🏁 Script executed: # Check the test file imports and Device assignment
sed -n '1,50p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller Length of output: 2099 🏁 Script executed: # Check TestWhoisDeactivated class implementation
sed -n '1194,1230p' openwisp_controller/config/whois/tests/tests.py
# Check lines 350-355 for stale comment
sed -n '350,360p' openwisp_controller/config/whois/tests/tests.py
# Check if WhoIsService is used anywhere in the test file
rg -n 'WhoIsService' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller Length of output: 1540 🏁 Script executed: # Check Device model to see required fields
rg -A 20 'class Device' openwisp_controller/config/models.py | head -50
# Search for organization field requirement in Device
rg -n 'organization.*required|organization.*null.*False' openwisp_controller/config/models.py | head -20
# Check lines 350-360 more carefully for the stale comment
sed -n '348,365p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller Length of output: 1678 🏁 Script executed: # Find AbstractDevice and its required fields
rg -A 50 'class AbstractDevice' openwisp_controller/config/models.py | head -80
# Check if organization is a required FK field
rg -n 'organization.*ForeignKey|organization.*models\.' openwisp_controller/config/models.py | head -10Repository: openwisp/openwisp-controller Length of output: 54 🏁 Script executed: # Search for AbstractDevice in abstract_models or base files
fd -t f AbstractDevice openwisp_controller/
# Find where AbstractDevice is defined
rg -l 'class AbstractDevice' --type=py
# Get device creation pattern from test utils
rg -B2 -A5 '_create_device' openwisp_controller/config/whois/tests/tests.py | head -40Repository: openwisp/openwisp-controller Length of output: 1576 🏁 Script executed: # Find the _create_device method definition in test utils
rg -B5 -A15 'def _create_device' openwisp_controller/config/whois/tests/tests.py | head -40
# Also check the CreateWHOISMixin that's imported
rg -B5 -A15 'def _create_device' openwisp_controller/config/whois/tests/utils.pyRepository: openwisp/openwisp-controller Length of output: 54 🏁 Script executed: # Check the test file imports to understand the inheritance chain
grep -n 'class.*TestCase\|CreateWHOISMixin\|TestAdminMixin' openwisp_controller/config/whois/tests/tests.py | head -20
# Check CreateWHOISMixin definition
rg -A20 'class CreateWHOISMixin' openwisp_controller/config/whois/tests/utils.py
# Also check TestAdminMixin
fd -t f 'TestAdminMixin' openwisp_controller/Repository: openwisp/openwisp-controller Length of output: 1177 🏁 Script executed: # Find CreateConfigMixin to see what _create_device does
rg -B5 -A15 'class CreateConfigMixin\|def _create_device' openwisp_controller/config/tests/utils.py | head -60
# Check AbstractDevice to see required fields
sed -n '1,100p' openwisp_controller/config/base/device.py | grep -A5 -B5 'organization\|mac_address'Repository: openwisp/openwisp-controller Length of output: 1040 🏁 Script executed: # Check OrgMixin to see organization field requirements
rg -B5 -A20 'class OrgMixin' openwisp_controller/config/base/device.py
# Check CreateConfigMixin to see _create_device implementation
rg -B2 -A20 'class CreateConfigMixin\|def _create_device' openwisp_controller/config/tests/utils.py | head -80Repository: openwisp/openwisp-controller Length of output: 54 🏁 Script executed: # Search for OrgMixin definition
rg -l 'class OrgMixin' --type=py
# Check the CreateConfigMixin file directly
head -150 openwisp_controller/config/tests/utils.py | tail -100
# Search for _create_device method
rg 'def _create_device' -A15 openwisp_controller/config/tests/utils.pyRepository: openwisp/openwisp-controller Length of output: 4846 🏁 Script executed: # Confirm the stale comment - check lines 350-365 again more carefully
sed -n '347,365p' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller Length of output: 1014 🏁 Script executed: # Check if TestWhoisDeactivated has access to _get_org or _get_org() method
rg -B5 'class TestWhoisDeactivated' openwisp_controller/config/whois/tests/tests.py
# Check if any test uses Device.objects.create without organization
rg 'Device\.objects\.create.*name=' openwisp_controller/config/whois/tests/tests.py | head -20
# Verify that Device requires organization by checking model constraints
rg 'organization.*null.*False|null.*False.*organization' openwisp_controller/config/base/device.py | head -10
# Check if mac_address is required
rg 'mac_address.*blank.*False|mac_address.*required|blank=False.*mac_address' openwisp_controller/config/base/device.pyRepository: openwisp/openwisp-controller Length of output: 351 🏁 Script executed: # Check the Device model field definitions directly
sed -n '1,200p' openwisp_controller/config/base/device.py | grep -A3 'organization\|mac_address' | head -40
# Check if organization is inherited from OrgMixin
rg -B5 -A15 'class OrgMixin' --type=py
# Try to find where OrgMixin is defined
fd -t f '*.py' | xargs grep -l 'class OrgMixin' | head -5Repository: openwisp/openwisp-controller Length of output: 1625 🏁 Script executed: # Find where OrgMixin is defined
rg -l 'class OrgMixin' --type=py
# Check if Device.objects.create() call in TestWhoisDeactivated will fail
# First, find if organization field is null=True
rg 'organization.*ForeignKey' openwisp_controller/config/base/ -A3 | head -20
# Count how many tests use _create_device vs Device.objects.create directly
rg 'Device\.objects\.create' openwisp_controller/config/whois/tests/tests.pyRepository: openwisp/openwisp-controller Length of output: 179 🏁 Script executed: # Find OrgMixin definition
rg -B2 -A25 'class OrgMixin' --type=py | head -60
# Check if organization field has null=False or blank=False
rg 'organization.*models\.ForeignKey' openwisp_controller/ -A5 | head -30Repository: openwisp/openwisp-controller Length of output: 54 Fix broken import: Line 27 attempts to import Additionally:
Suggested fix-from openwisp_controller.config.models import Device
from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped
-from openwisp_controller.config.whois.service import WhoIsService
from openwisp_utils.tests import SeleniumTestMixin, catch_signalUpdate Update line 352 comment to "7 queries" instead of "4 queries". 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| ======= | ||||||||||||
| <<<<<<< HEAD | ||||||||||||
| from openwisp_controller.config import settings as app_settings | ||||||||||||
| from openwisp_controller.config.models import Device | ||||||||||||
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | ||||||||||||
| from openwisp_controller.config.whois.service import WHOISService | ||||||||||||
| ======= | ||||||||||||
| from openwisp_controller.config.models import Device | ||||||||||||
| from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped | ||||||||||||
| from openwisp_controller.config.whois.service import WhoIsService | ||||||||||||
| >>>>>>> 8d798b46448620c8536e84519b69134dc556bd56 | ||||||||||||
| >>>>>>> 38cb7ad (test(geo): add regression tests for deactivated device WHOIS handling) | ||||||||||||
| from openwisp_utils.tests import SeleniumTestMixin, catch_signal | ||||||||||||
|
|
||||||||||||
| from ....tests.utils import TestAdminMixin | ||||||||||||
|
|
@@ -346,8 +362,15 @@ def test_last_ip_management_command_queries(self): | |||||||||||
| name="default.test.device4", mac_address="66:33:44:55:66:77" | ||||||||||||
| ) | ||||||||||||
| args = ["--noinput"] | ||||||||||||
| <<<<<<< HEAD | ||||||||||||
|
|
||||||||||||
| ======= | ||||||||||||
| # 4 queries (3 for each device's last_ip update) and 1 for fetching devices | ||||||||||||
| with self.assertNumQueries(4): | ||||||||||||
| <<<<<<< HEAD | ||||||||||||
| ======= | ||||||||||||
| >>>>>>> 8d798b46448620c8536e84519b69134dc556bd56 | ||||||||||||
| >>>>>>> 38cb7ad (test(geo): add regression tests for deactivated device WHOIS handling) | ||||||||||||
| with self.assertNumQueries(7): | ||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||
| call_command("clear_last_ip", *args, stdout=out, stderr=StringIO()) | ||||||||||||
|
|
||||||||||||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||||||||||||
|
|
@@ -1186,3 +1209,54 @@ def _assert_no_js_errors(): | |||||||||||
| _assert_no_js_errors() | ||||||||||||
| except UnexpectedAlertPresentException: | ||||||||||||
| self.fail("XSS vulnerability detected in WHOIS details admin view.") | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| <<<<<<< HEAD | ||||||||||||
| class TestWhoisDeactivated(TestCase): | ||||||||||||
| def setUp(self): | ||||||||||||
| self.device = Device.objects.create(name="test-device") | ||||||||||||
|
Comment on lines
+1216
to
+1217
Contributor
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.
🤖 Prompt for AI Agents |
||||||||||||
|
|
||||||||||||
| @patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||||||||||||
| def test_process_ip_skips_when_deactivated(self, mock_task): | ||||||||||||
| self.device._is_deactivated = True | ||||||||||||
|
|
||||||||||||
| service = WhoIsService(self.device) | ||||||||||||
| service.process_ip_data_and_location() | ||||||||||||
|
|
||||||||||||
| mock_task.assert_not_called() | ||||||||||||
|
|
||||||||||||
| @patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||||||||||||
| def test_update_whois_skips_when_deactivated(self, mock_task): | ||||||||||||
| self.device._is_deactivated = True | ||||||||||||
|
|
||||||||||||
| service = WhoIsService(self.device) | ||||||||||||
| service.update_whois_info() | ||||||||||||
|
|
||||||||||||
| mock_task.assert_not_called() | ||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||
| ======= | ||||||||||||
| class TestWHOISDeactivated(TransactionTestCase): | ||||||||||||
| def setUp(self): | ||||||||||||
| self.device = self._create_device() | ||||||||||||
| self.device.last_ip = "8.8.8.8" # public IP | ||||||||||||
| self.device.save() | ||||||||||||
|
|
||||||||||||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||||||||||||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||||||||||||
| def test_process_ip_skips_when_deactivated(self, mock_task): | ||||||||||||
| self.device._is_deactivated = True | ||||||||||||
|
|
||||||||||||
| service = WHOISService(self.device) | ||||||||||||
| service.process_ip_data_and_location() | ||||||||||||
|
|
||||||||||||
| self.assertEqual(mock_task.call_count, 0) | ||||||||||||
|
|
||||||||||||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||||||||||||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||||||||||||
| def test_process_ip_runs_when_active(self, mock_task): | ||||||||||||
| self.device._is_deactivated = False | ||||||||||||
|
|
||||||||||||
| service = WHOISService(self.device) | ||||||||||||
| service.process_ip_data_and_location() | ||||||||||||
|
|
||||||||||||
| self.assertEqual(mock_task.call_count, 1) | ||||||||||||
| >>>>>>> 38cb7ad (test(geo): add regression tests for deactivated device WHOIS handling) | ||||||||||||
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.
🧩 Analysis chain
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 1490
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 2594
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 4237
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 6499
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 11670
🏁 Script executed:
Repository: openwisp/openwisp-controller
Length of output: 818
Add test coverage for deactivated device scenarios.
Tests are missing to verify:
process_ip_data_and_locationandupdate_whois_infoskip processing whendevice.is_deactivated()isTruewhois_lookup_skippedsignal is emitted for deactivated devicesThe PR checklist indicates tests are not yet completed. These guards prevent critical side effects (task scheduling and signal emission) and require test coverage to prevent regressions.
🤖 Prompt for AI Agents