Skip to content

[change] Added generic enforcement of deactivated devices #1338#1365

Draft
pandafy wants to merge 1 commit into
masterfrom
issues/1338-generic-deactivated-device
Draft

[change] Added generic enforcement of deactivated devices #1338#1365
pandafy wants to merge 1 commit into
masterfrom
issues/1338-generic-deactivated-device

Conversation

@pandafy
Copy link
Copy Markdown
Member

@pandafy pandafy commented May 18, 2026

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #1338

Description of Changes

Added generic enforcement of deactivated device

@pandafy pandafy self-assigned this May 18, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR systematically enforces deactivation checks across the OpenWISP controller codebase. It introduces a new is_fully_deactivated() helper on the AbstractDevice model to centrally determine device operability, then applies this check (and related is_deactivated() checks) across configuration management, API endpoints, WHOIS processing, device connections, background tasks, and event handlers. The changes prevent operations on deactivated devices either by skipping them in bulk/idempotent operations or by rejecting them in user-initiated operations with appropriate error responses.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • #1338: This PR directly implements the systematic enforcement of deactivated device handling described in the issue, adding the centralized is_fully_deactivated() helper method and progressively adopting deactivation checks across network and management operations.

Possibly related PRs

  • openwisp/openwisp-controller#1197: Both PRs modify WHOIS and estimated-location workflow; this PR adds deactivation short-circuiting while the related PR prevents estimated-location recreation on unchanged WHOIS data.

Suggested labels

enhancement


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 inconclusive)

Check name Status Explanation Resolution
Bug Fixes ❌ Error PR fixes deactivated device enforcement bug but lacks regression tests (tests checklist unchecked) and has critical bugs: missing parentheses on is_deactivated(), inconsistent helper usage. Add regression tests. Fix missing () on is_deactivated (line 72). Use consistent is_fully_deactivated() per review. Mark strings for i18n.
Description check ❓ Inconclusive The description includes reference to issue #1338 and a brief summary, but multiple checklist items remain unchecked and lacks detailed explanation of implementation approach and testing. Complete the description by checking manual testing and documentation items, and provide more detailed explanation of the implementation strategy and testing approach used.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format [type] Descriptive title with [change] prefix and references issue #1338, clearly summarizing the main change.
Linked Issues check ✅ Passed The PR implements all three expected outcomes: introduces is_fully_deactivated() helper, adds deactivation checks across APIs and tasks, and systematically reviews/updates device operations.
Out of Scope Changes check ✅ Passed All changes are directly related to the issue #1338 objective of enforcing deactivated device handling across the codebase; no out-of-scope modifications detected.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issues/1338-generic-deactivated-device

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openwisp_controller/config/base/device_group.py`:
- Line 113: The loop currently iterates over
device_group.device_set.exclude(_is_deactivated=True).iterator() which misses
other non-operable states; update the query on device_group.device_set to use
the project's canonical operability filter/manager (e.g., the operable()
queryset or the is_operable/is_active boolean field or provided manager method)
instead of only excluding _is_deactivated, so the iterator only yields truly
operable devices (replace the exclude(...) call with the canonical operability
filter and keep the iterator()).

In `@openwisp_controller/config/controller/views.py`:
- Around line 400-402: The guard currently checks device.is_deactivated() which
allows config-level deactivations to be bypassed; change the condition to call
the canonical strict helper device.is_fully_deactivated() instead so the
ControllerResponse("error: device deactivated", status=403) is returned for
fully-deactivated devices; locate this in the view where device =
self.model.objects.select_related("config").get(key=key) and replace the
is_deactivated() call with is_fully_deactivated() (no other behavior changes
needed).

In `@openwisp_controller/connection/base/models.py`:
- Around line 349-350: The hardcoded error message in the RuntimeError raised
after the self.device.is_fully_deactivated() check must be marked for
translation; import Django's i18n helper (e.g. from django.utils.translation
import gettext_lazy as _) in openwisp_controller/connection/base/models.py and
replace RuntimeError("Device is deactivated") with RuntimeError(_("Device is
deactivated")) so the failure_reason stored can be translated when surfaced.
- Around line 470-471: Replace the call to self.device.is_deactivated() with the
stricter self.device.is_fully_deactivated() to match other deactivation checks
(e.g., the ones around is_fully_deactivated() used elsewhere in this class), and
add a defensive guard to ensure self.device is not None before calling the
method so the validation block handles cases where device isn't set yet; update
the ValidationError to remain the same field ("device") and message if the
fully-deactivated check returns True.

In `@openwisp_controller/geo/estimated_location/handlers.py`:
- Around line 71-75: The condition in the handler uses the method object
device.is_deactivated instead of calling it, causing the if to always be truthy;
update the condition to invoke the method (device.is_deactivated()) so it
behaves like the other callsites, and keep the rest of the check
(EstimatedLocationService.check_estimated_location_enabled(device.organization_id))
unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7694feac-56b3-419e-bd3e-36f00e30d24b

📥 Commits

Reviewing files that changed from the base of the PR and between c3ea70f and 635306e.

📒 Files selected for processing (12)
  • openwisp_controller/config/base/config.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/connection/tasks.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/geo/estimated_location/handlers.py
  • openwisp_controller/geo/estimated_location/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}

📄 CodeRabbit inference engine (Custom checks)

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously

Files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}

📄 CodeRabbit inference engine (Custom checks)

Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries

Files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}

📄 CodeRabbit inference engine (Custom checks)

Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable

Files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
**/*.{py,html}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
🧠 Learnings (4)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.

Applied to files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

  • openwisp_controller/connection/tasks.py
  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/config/base/device_group.py
  • openwisp_controller/config/controller/views.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/config/base/device.py
  • openwisp_controller/config/base/config.py
  • openwisp_controller/connection/tests/test_api.py
  • openwisp_controller/connection/apps.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/geo/estimated_location/handlers.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.

Applied to files:

  • openwisp_controller/config/whois/service.py
  • openwisp_controller/config/whois/tasks.py
  • openwisp_controller/geo/estimated_location/tasks.py
  • openwisp_controller/geo/estimated_location/handlers.py
🔇 Additional comments (10)
openwisp_controller/connection/tests/test_api.py (1)

290-290: LGTM!

openwisp_controller/config/base/device.py (1)

188-191: LGTM!

Also applies to: 307-307, 484-484

openwisp_controller/config/base/config.py (1)

999-1000: LGTM!

openwisp_controller/config/whois/service.py (1)

213-214: LGTM!

openwisp_controller/config/whois/tasks.py (1)

75-77: LGTM!

Also applies to: 120-121

openwisp_controller/geo/estimated_location/tasks.py (1)

124-128: LGTM!

openwisp_controller/geo/estimated_location/handlers.py (1)

46-46: LGTM!

openwisp_controller/connection/base/models.py (1)

144-145: LGTM!

Also applies to: 271-274, 384-386

openwisp_controller/connection/tasks.py (1)

49-51: LGTM!

openwisp_controller/connection/apps.py (1)

92-93: LGTM!

templates = Template.objects.filter(pk__in=template_ids)
old_templates = Template.objects.filter(pk__in=old_template_ids)
for device in device_group.device_set.iterator():
for device in device_group.device_set.exclude(_is_deactivated=True).iterator():
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 | 🟠 Major | ⚡ Quick win

Use the canonical operability criteria for device filtering.

At Line 113, filtering only on _is_deactivated=True can still include devices whose config is already non-operable (for example, deactivating/deactivated), so group template updates may run on devices that should be skipped.

Suggested change
-        for device in device_group.device_set.exclude(_is_deactivated=True).iterator():
+        for device in (
+            device_group.device_set.exclude(_is_deactivated=True)
+            .exclude(config__status__in=["deactivating", "deactivated"])
+            .iterator()
+        ):
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/config/base/device_group.py` at line 113, The loop
currently iterates over
device_group.device_set.exclude(_is_deactivated=True).iterator() which misses
other non-operable states; update the query on device_group.device_set to use
the project's canonical operability filter/manager (e.g., the operable()
queryset or the is_operable/is_active boolean field or provided manager method)
instead of only excluding _is_deactivated, so the iterator only yields truly
operable devices (replace the exclude(...) call with the canonical operability
filter and keep the iterator()).

Comment on lines +400 to +402
device = self.model.objects.select_related("config").get(key=key)
if device.is_deactivated():
return ControllerResponse("error: device deactivated", status=403)
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 | 🟠 Major | ⚡ Quick win

Use the canonical strict deactivation helper here

At Line 401, this guard should call the shared strict helper (is_fully_deactivated()) rather than is_deactivated() to keep behavior aligned with the stack’s canonical enforcement contract and avoid allowing re-registration on config-level deactivation paths.

Suggested fix
-            if device.is_deactivated():
+            if device.is_fully_deactivated():
                 return ControllerResponse("error: device deactivated", status=403)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/config/controller/views.py` around lines 400 - 402, The
guard currently checks device.is_deactivated() which allows config-level
deactivations to be bypassed; change the condition to call the canonical strict
helper device.is_fully_deactivated() instead so the ControllerResponse("error:
device deactivated", status=403) is returned for fully-deactivated devices;
locate this in the view where device =
self.model.objects.select_related("config").get(key=key) and replace the
is_deactivated() call with is_fully_deactivated() (no other behavior changes
needed).

Comment on lines +349 to +350
if self.device.is_fully_deactivated():
raise RuntimeError("Device is deactivated")
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 | 🟡 Minor | ⚡ Quick win

Make the deactivation failure message translatable.

At Line 350, the new string is stored in failure_reason and can surface in notifications; mark it for i18n.

Proposed fix
-                raise RuntimeError("Device is deactivated")
+                raise RuntimeError(gettext("Device is deactivated."))

As per coding guidelines: "For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/connection/base/models.py` around lines 349 - 350, The
hardcoded error message in the RuntimeError raised after the
self.device.is_fully_deactivated() check must be marked for translation; import
Django's i18n helper (e.g. from django.utils.translation import gettext_lazy as
_) in openwisp_controller/connection/base/models.py and replace
RuntimeError("Device is deactivated") with RuntimeError(_("Device is
deactivated")) so the failure_reason stored can be translated when surfaced.

Comment on lines +470 to +471
if self.device.is_deactivated():
raise ValidationError({"device": _("Device is deactivated.")})
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 | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify risky FK dereference in clean() and full_clean invocation path.
rg -n -C3 'def clean\(self\):|self\.device\.is_deactivated\(|self\.device\.is_fully_deactivated\(' openwisp_controller/connection/base/models.py
rg -n -C3 'if adding:|self\.full_clean\(' openwisp_controller/connection/base/models.py

Repository: openwisp/openwisp-controller

Length of output: 1963


🏁 Script executed:

# Examine the device FK definition in AbstractCommand
rg -B5 -A10 "class AbstractCommand" openwisp_controller/connection/base/models.py | head -40

Repository: openwisp/openwisp-controller

Length of output: 565


🏁 Script executed:

# Check device field definition and nullability
rg -n "device\s*=" openwisp_controller/connection/base/models.py | grep -E "(ForeignKey|OneToOneField)"

Repository: openwisp/openwisp-controller

Length of output: 144


🏁 Script executed:

# Examine the is_deactivated() vs is_fully_deactivated() methods
rg -B2 -A8 "def is_deactivated\(\)|def is_fully_deactivated\(" openwisp_controller/connection/base/models.py

Repository: openwisp/openwisp-controller

Length of output: 54


🏁 Script executed:

# Check how RuntimeError at line 350 is caught and handled
rg -B10 -A10 "Device is deactivated" openwisp_controller/connection/base/models.py

Repository: openwisp/openwisp-controller

Length of output: 1667


🏁 Script executed:

# Find Device model definition
fd -type f "models.py" | xargs rg -l "class Device" | head -5

Repository: openwisp/openwisp-controller

Length of output: 464


🏁 Script executed:

# Search for is_deactivated and is_fully_deactivated methods across entire codebase
rg "def is_deactivated\(|def is_fully_deactivated\(" --type py

Repository: openwisp/openwisp-controller

Length of output: 294


🏁 Script executed:

# Get implementations of both deactivation methods
rg -B2 -A8 "def is_deactivated\(\)|def is_fully_deactivated\(" openwisp_controller/config/base/device.py

Repository: openwisp/openwisp-controller

Length of output: 429


🏁 Script executed:

# Check device FK field definition with full context including null parameter
rg -B3 -A8 'device = models\.ForeignKey' openwisp_controller/connection/base/models.py | head -40

Repository: openwisp/openwisp-controller

Length of output: 978


🏁 Script executed:

# Check if AbstractCommand requires device to be set before clean/save
rg -B15 "def save\(" openwisp_controller/connection/base/models.py | grep -A15 "class AbstractCommand" | tail -20

Repository: openwisp/openwisp-controller

Length of output: 54


🏁 Script executed:

# Examine error handling in connect() method to see if RuntimeError reaches users
rg -B5 -A15 "def connect\(self\):" openwisp_controller/connection/base/models.py | grep -A15 "class AbstractDeviceConnection" | head -30

Repository: openwisp/openwisp-controller

Length of output: 54


🏁 Script executed:

# Check if failure_reason string is user-facing (logged/displayed)
rg -n "failure_reason" openwisp_controller/connection/base/models.py | head -20

Repository: openwisp/openwisp-controller

Length of output: 540


Use is_fully_deactivated() for consistency with other deactivation checks in this class.

Line 470 uses is_deactivated(), but the same class uses is_fully_deactivated() at lines 349 and 384. For consistency with the operability contract elsewhere, use the stricter is_fully_deactivated() here.

Additionally, while the device field is required (no null=True), adding a defensive check prevents potential issues if accessed before full validation completes.

Proposed fix
 def clean(self):
-    if self.device.is_deactivated():
+    if self.device_id and self.device.is_fully_deactivated():
         raise ValidationError({"device": _("Device is deactivated.")})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.device.is_deactivated():
raise ValidationError({"device": _("Device is deactivated.")})
if self.device_id and self.device.is_fully_deactivated():
raise ValidationError({"device": _("Device is deactivated.")})
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/connection/base/models.py` around lines 470 - 471,
Replace the call to self.device.is_deactivated() with the stricter
self.device.is_fully_deactivated() to match other deactivation checks (e.g., the
ones around is_fully_deactivated() used elsewhere in this class), and add a
defensive guard to ensure self.device is not None before calling the method so
the validation block handles cases where device isn't set yet; update the
ValidationError to remain the same field ("device") and message if the
fully-deactivated check returns True.

Comment on lines +71 to +75
if (
device.is_deactivated
or not EstimatedLocationService.check_estimated_location_enabled(
device.organization_id
)
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 | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Confirm Device exposes is_deactivated as a method.
rg -nP --type=py 'def\s+is_deactivated\s*\(' -g '**/device.py'

# 2) Show the non-invoked callsite in this handler.
rg -nP --type=py 'device\.is_deactivated\b(?!\s*\()' -g '**/geo/estimated_location/handlers.py' -C 2

# 3) Compare with invoked callsites in nearby modules.
rg -nP --type=py '\.is_deactivated\s*\(' -g '**/geo/estimated_location/**/*.py' -g '**/config/whois/**/*.py' -C 1

Repository: openwisp/openwisp-controller

Length of output: 1623


Fix missing method invocation on device.is_deactivated.

At line 72, device.is_deactivated references the method object instead of invoking it, causing the condition to always evaluate as truthy and the handler to always return early without scheduling the task. The same file correctly invokes this method with parentheses at line 46, and all other callsites in the codebase do the same.

Proposed fix
     if (
-        device.is_deactivated
+        device.is_deactivated()
         or not EstimatedLocationService.check_estimated_location_enabled(
             device.organization_id
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (
device.is_deactivated
or not EstimatedLocationService.check_estimated_location_enabled(
device.organization_id
)
if (
device.is_deactivated()
or not EstimatedLocationService.check_estimated_location_enabled(
device.organization_id
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/geo/estimated_location/handlers.py` around lines 71 - 75,
The condition in the handler uses the method object device.is_deactivated
instead of calling it, causing the if to always be truthy; update the condition
to invoke the method (device.is_deactivated()) so it behaves like the other
callsites, and keep the rest of the check
(EstimatedLocationService.check_estimated_location_enabled(device.organization_id))
unchanged.

@openwisp-companion
Copy link
Copy Markdown

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[change] Generic enforcement of deactivated devices

1 participant