[fix] Prevent IPAM changes when VPN clients exist #1096#1219
[fix] Prevent IPAM changes when VPN clients exist #1096#1219
Conversation
Blocks modification of Subnet and IpAddress objects if they are currently linked to an active VPN connection with connected clients. Fixes #1096
WalkthroughLoads Sequence Diagram(s)sequenceDiagram
participant Admin as User/Admin
participant UI as Django Admin UI
participant Model as Subnet/IP Model
participant Signal as pre_save Signal
participant Handler as check_ipam_change_handler
participant DB as Database (VpnClient)
Admin->>UI: Submit edit to subnet/IP
UI->>Model: call save()
Model->>Model: invoke clean() (patched -> calls validation)
Model->>Signal: emit pre_save
Signal->>Handler: invoke(sender, instance)
Handler->>DB: query VpnClient for matching ip or subnet
DB-->>Handler: return matches (or none)
alt matches found
Handler->>Model: raise ValidationError
Model->>UI: surface error to admin
else no matches
Handler->>Model: allow save
Model->>DB: persist changes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🧬 Code graph analysis (1)openwisp_controller/config/handlers.py (2)
⏰ 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). (11)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/handlers.py`:
- Around line 160-184: In check_ipam_change_handler, guard the DB lookups and
skip raw saves: first return early if kwargs.get("raw") is True; then replace
the direct .get(pk=instance.pk) calls (in both the ip_address branch and the
subnet branch) with a safe fetch that handles DoesNotExist (either try/except
sender.DoesNotExist around the .get or use
sender.objects.only(...).filter(pk=instance.pk).first() and return if None);
keep the existing comparison logic and only proceed to the VpnClient queries if
the old instance was successfully retrieved.
🧹 Nitpick comments (6)
openwisp_controller/config/utils.py (1)
274-286:if original_clean:is always truthy — consider removing the guard.
model.cleanis always a callable (at minimum,django.db.models.Model.clean), so theif original_clean:branch is always entered. The guard is misleading. Just calloriginal_clean(instance)unconditionally.Also, the
validation_funchere (check_ipam_change_handler) receives(model, instance)which maps to its(sender, instance, **kwargs)signature — this works, but passing a handler designed for signal dispatch through a completely different call path is fragile. If anyone later adds logic that depends on**kwargs(e.g., checkingkwargs.get('raw')), the clean path will silently diverge.Proposed simplification
def apply_model_clean_patch(model, validation_func): """ Injects a validation function into a model's clean() method at runtime to maintain clean ui """ original_clean = model.clean def patched_clean(instance): - if original_clean: - original_clean(instance) + original_clean(instance) validation_func(model, instance) model.clean = patched_cleanopenwisp_controller/config/handlers.py (1)
163-173: Relying onhasattr(instance, "ip_address")to distinguish model types is brittle.If the Subnet model ever gains an
ip_addressattribute (e.g., through a mixin or property), this logic will silently take the wrong branch. Usingisinstancechecks or comparingsenderagainst the known model class would be more robust.Alternative approach
- if hasattr(instance, "ip_address"): + from swapper import load_model + IpAddress = load_model("openwisp_ipam", "IpAddress") + if isinstance(instance, IpAddress):Or compare
senderagainst the model loaded at module level.openwisp_controller/config/apps.py (1)
163-174: Double validation: bothpre_savesignal andclean()patch invoke the same handler.For admin saves,
full_clean()→ patchedclean()→ handler runs, and if it passes,save()→pre_savesignal → handler runs again, hitting the DB a second time to re-fetch the old value. This is the cost of covering both admin (clean) and programmatic (pre_save) code paths.Consider short-circuiting the pre_save path when the check has already passed during
clean()in the same request cycle (e.g., with an instance-level flag), or accept the extra query as acceptable overhead.openwisp_controller/config/tests/test_vpn.py (3)
729-736: Address unused variables flagged by linter, and add a positive test for subnet changes.The linter flags
deviceandtemplateas unused (RUF059). Prefix with_to signal intentional disuse.Also, there's no test for the complementary case: allowing subnet changes when no VPN clients exist (analogous to
test_allow_ip_change_without_vpn_clients). This would ensure the guard doesn't over-block.Fix unused variables
def test_prevent_subnet_change_with_vpn_clients(self): - device, vpn, template = self._create_wireguard_vpn_template() + _device, vpn, _template = self._create_wireguard_vpn_template() subnet = vpn.subnet with self.subTest("Test subnet change blocked when clients exist"): subnet.subnet = "10.0.2.0/24" with self.assertRaises(ValidationError) as cm: subnet.full_clean() self.assertIn("Cannot modify this subnet", str(cm.exception))Do you want me to generate the
test_allow_subnet_change_without_vpn_clientstest?
738-752: Prefix unusedtemplatevariable.Linter flags
templateas unused on line 739.Fix
def test_prevent_ip_change_with_vpn_clients(self): - device, vpn, template = self._create_wireguard_vpn_template() + device, vpn, _template = self._create_wireguard_vpn_template()
754-762: Consider also testing thepre_savesignal path directly.These tests only exercise the
full_clean()→ patchedclean()path. A test that callssave()directly (skippingfull_clean()) would verify that thepre_savesignal handler also blocks changes, confirming the defense-in-depth works for programmatic saves.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/utils.py
🧰 Additional context used
🧠 Learnings (3)
📚 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/config/tests/test_vpn.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.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/config/tests/test_vpn.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/apps.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/apps.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_vpn.py (3)
openwisp_controller/config/tests/utils.py (2)
_create_wireguard_vpn_template(168-183)_create_wireguard_vpn(148-166)openwisp_controller/config/base/config.py (2)
full_clean(558-562)save(578-605)openwisp_controller/config/base/vpn.py (2)
save(244-293)save(870-878)
🪛 Ruff (0.14.14)
openwisp_controller/config/tests/test_vpn.py
[warning] 730-730: Unpacked variable device is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 730-730: Unpacked variable template is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 739-739: Unpacked variable template is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
openwisp_controller/config/handlers.py
[warning] 160-160: Unused function argument: kwargs
(ARG001)
⏰ 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). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (1)
openwisp_controller/config/apps.py (1)
64-65: openwisp_ipam is a required dependency—no changes needed.
openwisp_ipamis explicitly listed inrequirements.txt, making it a required dependency of this package. Loading its models (SubnetandIpAddress) at app startup is the correct and expected behavior, consistent with how other required dependencies likedjango_x509andopenwisp_usersare handled in the same method.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
output.mp4output2.mp4 |
stktyagi
changed the title
2 participants
Blocks modification of Subnet and IpAddress objects if they are currently linked to an active VPN connection with connected clients.
Fixes #1096
Checklist
Reference to Existing Issue
Closes #1096.
Description of Changes
Prevents changing subnet and ip addresses if they are currently
linked to an active VPN connection with connected clients.