Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openwisp_controller/config/whois/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 thread
coderabbitai[bot] marked this conversation as resolved.
)
# Filter out devices that have WHOIS information for their last IP
devices = devices.exclude(last_ip=None).exclude(
Expand Down
4 changes: 4 additions & 0 deletions openwisp_controller/config/whois/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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_location

Repository: openwisp/openwisp-controller

Length of output: 5427


Estimated Location service is missing the deactivation guard—fix is incomplete.

The WHOIS service guard is correct: is_deactivated() is called before _need_whois_lookup and transaction.on_commit, properly suppressing the Celery task and signal for deactivated devices.

However, issue #1325 explicitly requires the same protection for both WHOIS and Estimated Location. The EstimatedLocationService lacks an equivalent guard in trigger_estimated_location_task(). Deactivated devices will still enqueue estimated location tasks, leaving the fix incomplete.

Add an is_deactivated() guard at the entry point of EstimatedLocationService.trigger_estimated_location_task() (or the method that calls it) to match the WHOIS service implementation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/config/whois/service.py` around lines 213 - 214, Add the
same deactivation guard used in WHOIS: at the start of
EstimatedLocationService.trigger_estimated_location_task (or the method that
invokes it), call self.device.is_deactivated() and immediately return if True so
no Celery task or signals are enqueued; mirror the WHOIS pattern that prevents
_need_whois_lookup and transaction.on_commit from running for deactivated
devices.

new_ip = self.device.last_ip
initial_ip = self.device._initial_last_ip
if force_lookup or self._need_whois_lookup(new_ip):
Expand All @@ -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
Expand Down
59 changes: 59 additions & 0 deletions openwisp_controller/config/whois/tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,65 @@ def test_get_whois_info_when_not_configured(self):
result = get_whois_info(pk=device.pk)
self.assertIsNone(result)

@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.
device = self._create_device()
org_settings = device.organization.config_settings
org_settings.whois_enabled = True
org_settings.save()
device._initial_last_ip = None
device.last_ip = "8.8.8.8"
device.save()
mock_task.reset_mock()
device.deactivate()
service = WHOISService(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):
device = self._create_device()
org_settings = device.organization.config_settings
org_settings.whois_enabled = True
org_settings.save()
device._initial_last_ip = None
device.last_ip = "8.8.8.8"
device.save()
mock_task.reset_mock()
service = WHOISService(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=device.pk,
initial_ip_address=device._initial_last_ip,
)

@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):
device = self._create_device()
org_settings = device.organization.config_settings
org_settings.whois_enabled = True
org_settings.save()
ip = "8.8.8.8"
device._initial_last_ip = None
device.last_ip = ip
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)
device.deactivate()
mock_task.reset_mock()
service = WHOISService(device)
service.update_whois_info()
mock_task.assert_not_called()


@tag("selenium_tests")
class TestWHOISSelenium(CreateWHOISMixin, SeleniumTestMixin, StaticLiveServerTestCase):
Expand Down
5 changes: 4 additions & 1 deletion openwisp_controller/connection/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,11 @@ def test_launch_command_exception(self, *args):
class TestTransactionTasks(
TestRegistrationMixin, CreateConnectionsMixin, TransactionTestCase
):
@mock.patch("openwisp_controller.connection.tasks.launch_command.delay")
@mock.patch.object(tasks.update_config, "delay")
def test_update_config_hostname_changed_on_reregister(self, mocked_update_config):
def test_update_config_hostname_changed_on_reregister(
self, mocked_update_config, mocked_launch_command
):
device = self._create_device_config()
self._create_device_connection(device=device)
# Trigger re-registration with new hostname
Expand Down
2 changes: 2 additions & 0 deletions openwisp_controller/geo/estimated_location/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ def is_estimated_location_enabled(self):
return self.check_estimated_location_enabled(self.device.organization_id)

def trigger_estimated_location_task(self, ip_address):
if self.device.is_deactivated():
return
try:
current_app.send_task(
"whois_estimated_location_task",
Expand Down
Loading