-
-
Notifications
You must be signed in to change notification settings - Fork 287
fix(geo): skip WHOIS and estimated location on deactivated devices #1340
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
77e8413
b9666e5
a7b9842
efea089
8d528e7
eadb209
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| fatal: no rebase in progress | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ def clear_last_ip_command(stdout, stderr, interactive=True): | |
| ) | ||
| # include the FK field 'organization' in .only() so the related | ||
| # `organization__config_settings` traversal is not deferred | ||
| .only("last_ip", "organization", "key") | ||
| .only("last_ip", "organization", "key", "_is_deactivated") | ||
|
Comment on lines
40
to
+42
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. 🧹 Nitpick | 🔵 Trivial Good catch preventing the N+1 from the new Including The existing comment above only motivates the ♻️ Proposed comment tweak- # include the FK field 'organization' in .only() so the related
- # `organization__config_settings` traversal is not deferred
+ # Include the FK 'organization' so the `organization__config_settings`
+ # traversal is not deferred, and '_is_deactivated' so the per-device
+ # save() -> process_ip_data_and_location() -> is_deactivated() path
+ # does not trigger an N+1 on the deferred field.
.only("last_ip", "organization", "key", "_is_deactivated")🤖 Prompt for AI Agents |
||
| ) | ||
| # Filter out devices that have WHOIS information for their last IP | ||
| devices = devices.exclude(last_ip=None).exclude( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,6 +210,8 @@ def process_ip_data_and_location(self, force_lookup=False): | |
| Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`. | ||
| Tasks are triggered on commit to ensure redundant data is not created. | ||
| """ | ||
| if self.device.is_deactivated(): | ||
| return | ||
|
Comment on lines
+213
to
+214
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
# Check whether the estimated_location service also has a deactivation guard,
# since issue `#1325` requires both WHOIS and Estimated Location to be skipped.
fd -t f 'service\.py$' openwisp_controller/geo/estimated_location --exec cat {} \;
echo "---"
# Look for any is_deactivated / _is_deactivated usage inside estimated_location
rg -nP -C3 '(is_deactivated|_is_deactivated)' openwisp_controller/geo/estimated_locationRepository: openwisp/openwisp-controller Length of output: 5427 Estimated Location service is missing the deactivation guard—fix is incomplete. The WHOIS service guard is correct: However, issue Add an 🤖 Prompt for AI Agents |
||
| new_ip = self.device.last_ip | ||
| initial_ip = self.device._initial_last_ip | ||
| if force_lookup or self._need_whois_lookup(new_ip): | ||
|
|
@@ -229,6 +231,8 @@ def update_whois_info(self): | |
| when the data is older than | ||
| ``OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS``. | ||
| """ | ||
| if self.device.is_deactivated(): | ||
| return | ||
| ip_address = self.device.last_ip | ||
| if not self.is_valid_public_ip_address(ip_address): | ||
| return | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -346,7 +346,7 @@ def test_last_ip_management_command_queries(self): | |||||
| name="default.test.device4", mac_address="66:33:44:55:66:77" | ||||||
| ) | ||||||
| args = ["--noinput"] | ||||||
| # 4 queries (3 for each device's last_ip update) and 1 for fetching devices | ||||||
|
|
||||||
| with self.assertNumQueries(4): | ||||||
| call_command("clear_last_ip", *args, stdout=out, stderr=StringIO()) | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
|
|
@@ -1186,3 +1186,70 @@ def _assert_no_js_errors(): | |||||
| _assert_no_js_errors() | ||||||
| except UnexpectedAlertPresentException: | ||||||
| self.fail("XSS vulnerability detected in WHOIS details admin view.") | ||||||
|
|
||||||
|
|
||||||
| class TestWHOISDeactivated( | ||||||
| CreateWHOISMixin, WHOISTransactionMixin, TransactionTestCase | ||||||
| ): | ||||||
| def setUp(self): | ||||||
| super().setUp() | ||||||
| self.device = self._create_device() | ||||||
| org_settings = self.device.organization.config_settings | ||||||
| org_settings.whois_enabled = True | ||||||
| org_settings.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): | ||||||
| # Public IP that would normally trigger a lookup, so the only reason | ||||||
| # the task is not enqueued is the deactivation guard under test. | ||||||
| self.device.last_ip = "8.8.8.8" | ||||||
| self.device.save() | ||||||
| mock_task.reset_mock() | ||||||
|
|
||||||
| self.device.deactivate() | ||||||
|
|
||||||
| service = WHOISService(self.device) | ||||||
| service.process_ip_data_and_location() | ||||||
|
|
||||||
| self.assertEqual(mock_task.call_count, 0) | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| @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.last_ip = "8.8.8.8" | ||||||
| self.device.save() | ||||||
|
|
||||||
| mock_task.reset_mock() | ||||||
|
|
||||||
| service = WHOISService(self.device) | ||||||
| service.process_ip_data_and_location() | ||||||
|
|
||||||
| # transaction.on_commit executes immediately in TransactionTestCase, | ||||||
| # so the task is triggered synchronously here | ||||||
| mock_task.assert_called_once_with( | ||||||
| device_pk=self.device.pk, | ||||||
| initial_ip_address=self.device._initial_last_ip, | ||||||
| ) | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||||||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||||||
| def test_update_whois_skips_when_deactivated(self, mock_task): | ||||||
| ip = "8.8.8.8" | ||||||
| self.device.last_ip = ip | ||||||
| self.device.save() | ||||||
|
|
||||||
| threshold = app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1 | ||||||
| expired_time = timezone.now() - timedelta(days=threshold) | ||||||
|
|
||||||
| whois_obj = self._create_whois_info(ip_address=ip) | ||||||
| WHOISInfo.objects.filter(pk=whois_obj.pk).update(modified=expired_time) | ||||||
|
|
||||||
| self.device.deactivate() | ||||||
|
|
||||||
| mock_task.reset_mock() | ||||||
|
|
||||||
| service = WHOISService(self.device) | ||||||
| service.update_whois_info() | ||||||
|
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. CRITICAL: Variable name typo -
Suggested change
|
||||||
|
|
||||||
| mock_task.assert_not_called() | ||||||
|
Comment on lines
+1235
to
+1255
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.
Even after fixing the
To actually pin the 🧪 Sketch of a stronger regression pair `@mock.patch.object`(app_settings, "WHOIS_CONFIGURED", True)
`@mock.patch`("openwisp_controller.config.whois.service.fetch_whois_details.delay")
def test_update_whois_skips_when_deactivated(self, mock_task):
- self.device.last_ip = "8.8.8.8"
- self.device.save()
- mock_task.reset_mock()
-
- self.device.deactivate()
-
- ervice = WHOISService(self.device)
- service.update_whois_info()
-
- mock_task.assert_not_called()
+ ip = "8.8.8.8"
+ self.device.last_ip = ip
+ self.device.save()
+ # Make an expired WHOISInfo so update_whois_info() would normally
+ # schedule a refresh; the deactivation guard is the only reason it won't.
+ threshold = app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1
+ expired = timezone.now() - timedelta(days=threshold)
+ whois_obj = self._create_whois_info(ip_address=ip)
+ WHOISInfo.objects.filter(pk=whois_obj.pk).update(modified=expired)
+ self.device.deactivate()
+ mock_task.reset_mock()
+
+ service = WHOISService(self.device)
+ service.update_whois_info()
+
+ mock_task.assert_not_called()🤖 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.
Critical: Invalid file content - Git error message instead of code.
This file contains only a Git error message (
fatal: no rebase in progress) rather than actual code changes. The PR objectives indicate that guard conditions should be added toprocess_ip_data_and_locationandupdate_whois_infofunctions to skip WHOIS lookups for deactivated devices, but no such code is present in the files provided for review.Please verify the PR contents and ensure the actual code changes are included.
🤖 Prompt for AI Agents