feat: remove module path plus add YAML export option#34
Merged
Conversation
NetBox decided not to implement MODULE_PATH_TOKEN, so all related
code is dead weight. Remove:
- utils.py (entire file, only contained supports_module_path())
- InterfaceNameRuleListView.get_extra_context() in views.py
- {%if supports_module_path%} banner in interfacenamerule_list.html
- docs/configuration.md section and references
- engine.py docstring mention
- test_misc.py: SupportModulePathTest and UtilsModulePathFalseTest
- ModelCSVExportTest: asserts csv_headers + to_csv() exist and are correct - BulkImportCSVTest: regression for KeyError 'Ch' on CSV import - YAMLExportViewTest: asserts YAML export view returns correct response
- InterfaceNameRule.csv_headers: list of field names matching import form - InterfaceNameRule.to_csv(): returns a tuple of values for CSV export, using .model for ModuleType/DeviceType and .name for Platform FKs - tables.py: verbose_name 'Ch. Start' → 'Channel Start' (the dot caused NetBox's import normalizer to split on '.' and produce 'Ch', triggering KeyError: 'Ch' on round-trip import)
- InterfaceNameRuleYAMLExportView: GET exports all rules, POST exports selected rules identified by pk_<n> form fields - URL: /rules/export/yaml/ (name: interfacenamerule_export_yaml) - 'Export YAML' button added to list view via extra_controls block - Empty/falsy optional fields omitted from YAML output for readability; required fields (name_template) always included
…quiredMixin ConditionalLoginRequiredMixin only enforces auth when LOGIN_REQUIRED=True; in the test environment it may be False so we accept 200 as well as redirects
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoved {module_path} feature-detection and related docs/templates, added CSV/YAML export support for InterfaceNameRule, refactored several views to NetBox generic/base views with permission-aware behavior, created an export button template, and expanded tests for exports, bulk import, and permission scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (User)
participant View as InterfaceNameRuleListView
participant Exporter as to_yaml()/csv_headers
participant DB as Database (InterfaceNameRule)
participant Browser as Browser/Response
Note over User, Browser: YAML export flow (All Data)
User->>View: GET /interfacenamerule/?export=All
View->>DB: queryset: InterfaceNameRule.objects.all()
DB-->>View: list of rules
View->>Exporter: build rows via csv_headers + rule.to_csv()
Exporter-->>View: aggregated YAML doc
View->>Browser: 200 + YAML content-type + attachment
Browser-->>User: file download
sequenceDiagram
participant User as Client (User)
participant View as RuleTestView
participant Perm as PermissionSystem
participant DB as Database (InterfaceNameRule)
participant UI as Template/Redirect
Note over User, UI: RuleTestView behavior differs by permission
User->>Perm: check add/view permissions
alt has view_interfacenamerule
User->>View: GET ?rule_id=1
View->>DB: load rule (existing)
DB-->>View: rule data
View->>UI: render with loaded_rule
else only add permission
User->>View: GET ?rule_id=1
View->>Perm: detect no view permission
View-->>UI: render blank form + permission warning
end
User->>View: POST action=save_rule
alt has view_interfacenamerule
View->>DB: check duplicates -> may redirect to edit
else only add permission
View->>UI: redirect to add form (skip duplicate detection)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-03-30-remove-module-path-plus-fixes-design.md`:
- Line 137: Check whether the new test test_import_yaml_roundtrip (in
BulkImportViewTest) duplicates existing coverage for YAML import by searching
the test suite for tests referencing BulkImportView, YAML import endpoints, or
helpers that exercise import_yaml/import_from_yaml logic; if equivalent behavior
is already covered, remove or skip test_import_yaml_roundtrip, otherwise adjust
it to cover any missing edge cases (e.g., error handling, round-trip fidelity)
and reference BulkImportView and any import_yaml/import_from_yaml helpers to
make the intent explicit.
- Line 116: Update the response header that currently sets Content-Type to
"application/x-yaml" to use the standard MIME type (e.g., "application/yaml" or
"text/yaml") wherever the HttpResponse is constructed and the header string
"Content-Type: application/x-yaml" appears; ensure the Content-Disposition
header still uses the same filename format ("interface_name_rules.yaml") and
update any tests/docs that assert the old MIME string to the new standard value.
- Around line 171-172: Reorder the plan so tests are written before
implementation: update the checklist so Step 2 does not remove module_path
immediately; instead add/adjust steps to first add failing tests (including
tests that will be deleted or updated for module_path), then implement the code
changes that remove module_path, and finally run/adjust tests and clean up
deletions; reference the existing items "Step 2", "module_path", and the later
"steps 3-5" when updating the document to reflect “write tests first, then
implement, then delete/clean tests.”
In `@netbox_interface_name_rules/tests/test_views.py`:
- Around line 728-737: The test named test_yaml_export_requires_login is
misleading because ViewTestBase.setUp() logs in a user, so either rename the
test to something like test_yaml_export_responds_successfully (update the test
name and docstring) or adjust the test to actually exercise unauthenticated
access by calling self.client.logout() at the start of
test_yaml_export_requires_login before requesting self.YAML_URL; update
assertions/docstring accordingly to reflect the intended behavior.
In `@netbox_interface_name_rules/views.py`:
- Around line 126-134: The loop building CSV entries can be simplified: replace
the two-branch check that assigns entry[header] in both non-empty and required
cases with a single conditional. In the block iterating
InterfaceNameRule.csv_headers and rule.to_csv(), change the if/elif into one
condition like if ((value != "" and value is not None) or header in
{"name_template"}): entry[header] = value so the same assignment isn't
duplicated while preserving the rule that required headers (name_template) are
always included.
🪄 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: afbb7502-3c33-4fef-b90b-866cc559ef9d
📒 Files selected for processing (11)
docs/configuration.mddocs/superpowers/specs/2026-03-30-remove-module-path-plus-fixes-design.mdnetbox_interface_name_rules/engine.pynetbox_interface_name_rules/models.pynetbox_interface_name_rules/tables.pynetbox_interface_name_rules/templates/netbox_interface_name_rules/interfacenamerule_list.htmlnetbox_interface_name_rules/tests/test_misc.pynetbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/urls.pynetbox_interface_name_rules/utils.pynetbox_interface_name_rules/views.py
💤 Files with no reviewable changes (1)
- netbox_interface_name_rules/utils.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
marcinpsk
changed the title
- views.py: simplify _rules_to_yaml_data loop (single condition instead of if/elif); change Content-Type to standard 'application/yaml' - tests/test_views.py: rename test_yaml_export_requires_login to test_yaml_export_unauthenticated_responds with accurate docstring - .gitignore: add docs/superpowers/ (design specs are local-only) - delete docs/superpowers/specs/ design spec from git history
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_interface_name_rules/views.py`:
- Around line 143-150: The post() branch that reads pk_<n> fields is never
triggered because interfacenamerule_list.html currently uses a GET link; either
implement a POST form in the template that wraps the rule table and checkboxes
(ensuring each checkbox input is named using the pk_<n> pattern and the form
posts to the view) so InterfaceNameRule objects selected by those pk_<n> fields
reach post() and _build_response(queryset), or remove the unreachable pk
handling in post() and document the endpoint as API-only; update
interfacenamerule_list.html or the post() method accordingly to keep behavior
consistent.
🪄 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: de3d413a-d581-449d-a446-1644b609dcd7
📒 Files selected for processing (3)
.gitignorenetbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
- post() now reads request.POST.getlist('pk') (NetBox convention)
- template overrides bulk_buttons block with Export Selected YAML button
using formaction to piggyback on the existing bulk-select form
- update test to post {pk: [pk]} matching the new convention
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
netbox_interface_name_rules/tests/test_views.py (1)
728-736:⚠️ Potential issue | 🟡 MinorThis test is not actually unauthenticated.
ViewTestBase.setUp()logs in the superuser, so this test currently exercises authenticated access. Addself.client.logout()before the request (or rename the test to authenticated behavior).✅ Minimal fix
def test_yaml_export_unauthenticated_responds(self): @@ - response = self.client.get(self.YAML_URL) + self.client.logout() + response = self.client.get(self.YAML_URL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_interface_name_rules/tests/test_views.py` around lines 728 - 736, The test method test_yaml_export_unauthenticated_responds is not actually unauthenticated because ViewTestBase.setUp() logs in the superuser; update the test to call self.client.logout() before making the GET request to self.YAML_URL so it truly exercises anonymous access (or alternatively rename the test to reflect authenticated behavior if you intend to keep the login). Ensure the logout is placed immediately before response = self.client.get(self.YAML_URL) so the assertion on response.status_code still verifies unauthenticated access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_interface_name_rules/tests/test_views.py`:
- Line 726: Replace the hardcoded YAML_URL constant in tests/test_views.py with
a call to django.urls.reverse to make the test robust to route changes: import
reverse and set YAML_URL = reverse("<your_url_name>") using the actual URL
pattern name for the rules export-YAML view (locate the URL name in the plugin's
urls config; common patterns are like
"plugins:<plugin_namespace>:rules-export-yaml" or
"interface-name-rules:rules-export-yaml"), updating any tests that reference
YAML_URL accordingly.
In `@netbox_interface_name_rules/views.py`:
- Line 118: The class InterfaceNameRuleYAMLExportView currently subclasses
django.views.View; change it to inherit from a NetBox base view (e.g.,
netbox.views.generic.ObjectView) to comply with repository guidelines, update
the import to pull ObjectView from netbox.views.generic (or the correct NetBox
base view used elsewhere in the repo), and ensure any overridden methods
(dispatch/get/post) remain compatible with the NetBox base class.
- Around line 139-155: Both get() and post() currently allow any authenticated
user to export rules; add an explicit permission check for
"netbox_interface_name_rules.view_interfacenamerule" at the start of each view
method (get and post) and raise django.core.exceptions.PermissionDenied if the
user lacks it. In practice, call
request.user.has_perm("netbox_interface_name_rules.view_interfacenamerule") (or
use self.request.user if appropriate), and if False raise PermissionDenied
before computing the queryset or calling
_build_response(InterfaceNameRule.objects...); ensure both the pk-filtering
branch in post() and the fallback to InterfaceNameRule.objects.all() are
guarded.
---
Duplicate comments:
In `@netbox_interface_name_rules/tests/test_views.py`:
- Around line 728-736: The test method test_yaml_export_unauthenticated_responds
is not actually unauthenticated because ViewTestBase.setUp() logs in the
superuser; update the test to call self.client.logout() before making the GET
request to self.YAML_URL so it truly exercises anonymous access (or
alternatively rename the test to reflect authenticated behavior if you intend to
keep the login). Ensure the logout is placed immediately before response =
self.client.get(self.YAML_URL) so the assertion on response.status_code still
verifies unauthenticated access.
🪄 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: 193d0791-443e-4f38-a52c-d061eb6efcd2
📒 Files selected for processing (3)
netbox_interface_name_rules/templates/netbox_interface_name_rules/interfacenamerule_list.htmlnetbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
…uthenticated test - Replace hardcoded YAML_URL with reverse() in setUpTestData - Add view_interfacenamerule permission check to get() and post() - Add self.client.logout() to test_yaml_export_unauthenticated_responds so it actually exercises unauthenticated access
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
netbox_interface_name_rules/views.py (1)
118-118:⚠️ Potential issue | 🟠 MajorUse a NetBox base view for this export endpoint.
Line 118 still subclasses raw
django.views.View, which violates the repo rule forviews.pyfiles. Please switch this new export view to the appropriate NetBox base class instead of extendingViewdirectly.As per coding guidelines
**/{views,forms,serializers,tables}.py: “All views, forms, serializers, and tables must inherit from NetBox base classes (NetBoxModel, NetBoxModelViewSet, NetBoxModelForm, etc.), not raw Django/DRF equivalents”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_interface_name_rules/views.py` at line 118, The class InterfaceNameRuleYAMLExportView currently subclasses django.views.View; change it to inherit from NetBox's base view class (replace View with the appropriate NetBox base, e.g. netbox.views.base.NetBoxView or the NetBox export view base used across the project) so it conforms to repo rules for views.py; update the import to pull the NetBox base view and ensure existing mixins (ConditionalLoginRequiredMixin) remain in the inheritance list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_interface_name_rules/tests/test_views.py`:
- Around line 700-715: The test test_csv_round_trip_creates_rule is passing even
when the import fails because it accepts a 200 re-rendered form as success;
update the test (in tests/test_views.py) to ensure the imported CSV row is
unique and assert a successful import: generate a unique identifier (e.g.,
append a UUID or timestamp to the rule name produced by _csv_from_rule or build
a fresh CSV row), post that CSV to interfacenamerule_bulk_import, assert
response.status_code == 302, then assert InterfaceNameRule.objects.count() has
increased compared to before_count to guarantee a new rule was created.
- Around line 731-741: The test test_yaml_export_unauthenticated_responds should
assert the correct anonymous non-redirect outcome by including 403 and removing
200: update the assertion in tests/test_views.py to expect status codes [403,
301, 302] (or ensure 403 is accepted) because
InterfaceNameRuleYAMLExportView.get() raises PermissionDenied for anonymous
requests; locate the test method test_yaml_export_unauthenticated_responds and
change the allowed status list accordingly so anonymous-export regressions that
return 200 will fail.
- Around line 763-773: The test test_yaml_export_all_contains_rules currently
only asserts len(data) >= 1 which can miss dropped records; modify the assertion
to compare the exported list length to the actual DB row count by replacing that
check with something like assertEqual(len(data),
InterfaceNameRule.objects.count()) (import or reference the InterfaceNameRule
model used in ViewTestBase.setUpTestData()) so the test verifies the export
contains all rules.
---
Duplicate comments:
In `@netbox_interface_name_rules/views.py`:
- Line 118: The class InterfaceNameRuleYAMLExportView currently subclasses
django.views.View; change it to inherit from NetBox's base view class (replace
View with the appropriate NetBox base, e.g. netbox.views.base.NetBoxView or the
NetBox export view base used across the project) so it conforms to repo rules
for views.py; update the import to pull the NetBox base view and ensure existing
mixins (ConditionalLoginRequiredMixin) remain in the inheritance list.
🪄 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: e0031615-d6c0-4451-87d8-11b83cab4095
📒 Files selected for processing (2)
netbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
- test_csv_round_trip_creates_rule: use a unique ModuleType so the import doesn't silently fail on the UniqueConstraint, assert 302 and count increase unconditionally instead of accepting 200 as success - test_yaml_export_unauthenticated_responds: accept [301, 302, 403] instead of [200, 301, 302] since the view raises PermissionDenied for anonymous users - test_yaml_export_all_contains_rules: assert exact count matches DB instead of >= 1
Replace ConditionalLoginRequiredMixin + View with proper NetBox bases: - InterfaceNameRuleDuplicateView → generic.ObjectView uses self.get_object() instead of get_object_or_404 - InterfaceNameRuleYAMLExportView → BaseMultiObjectView removes manual has_perm() checks; permission now enforced at dispatch uses self.queryset (permission-restricted) for get() and post() - RuleTestView → BaseMultiObjectView removes _check_permission() method; permission enforced at dispatch - RuleApplyListView → BaseMultiObjectView uses self.queryset for the rule listing - RuleApplicableView → generic.ObjectView uses self.get_object() instead of get_object_or_404 - RuleApplyDetailView → generic.ObjectView removes _check_permission(); uses additional_permissions for dcim.change_interface enforcement at dispatch level Also fix test_views.BulkImportCSVTest: add csv_delimiter=auto to POST data (required by BulkImportForm since NetBox added csv_delimiter field)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_interface_name_rules/tests/test_views.py`:
- Around line 800-813: The test test_yaml_export_selected_rules_via_post only
asserts the returned list length; update it to assert the actual returned rule
matches the selected one by checking a stable field from self.rule (e.g. pk or
name). After loading data with yaml.safe_load(response.content) assert that data
is a list with length 1 and that data[0]["pk"] (or data[0]["name"] if that field
exists) equals self.rule.pk (or self.rule.name) to ensure the POST pk filter
returned the correct rule; use the YAML_URL, response and self.rule identifiers
already in the test.
In `@netbox_interface_name_rules/views.py`:
- Around line 147-163: The export view relies on the model's implicit ordering,
which can change and cause unstable YAML diffs; update both get and post paths
that pass self.queryset into _build_response to use an explicit ordering by
primary key (e.g., call queryset.order_by("pk") or self.queryset.order_by("pk"))
— in post, apply order_by("pk") to the filtered queryset (and to the fallback
queryset) after building pk_list, and in get return
self._build_response(self.queryset.order_by("pk")) so exports are deterministic.
- Around line 172-175: The current get_required_permission() permits users with
only add_interfacenamerule but the view paths still fetch existing rules; modify
the view to check the view permission before reading existing rules: before
using ?rule_id= to prefill and at the start of _handle_save_rule() guard any
InterfaceNameRule.objects.get()/filter lookups with a permission check like if
not request.user.has_perm("netbox_interface_name_rules.view_interfacenamerule"):
skip the prefill/lookup and treat as create-only (or raise/deny), ensuring no
rule data is read for add-only users; keep get_required_permission() unchanged
but add these permission checks around the prefilling and any InterfaceNameRule
queries.
🪄 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: 8a55dcb4-1244-43f5-aac3-705c331da0e3
📒 Files selected for processing (2)
netbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
- Add .order_by("pk") to YAML export querysets for stable diffs
- Guard rule prefill and duplicate-check lookups with view permission
- Strengthen POST export test to assert returned rule identity
…dd permission tests - Add parentheses around or-condition in _rules_to_yaml_data for clarity - Warn add-only users when rule_id prefill is skipped due to missing view permission - Add comment explaining why duplicate detection is skipped without view permission - Add tests for add-only user: blank form with warning on prefill, and save_rule skipping duplicate check
Django caches permissions on the User instance; after user_permissions.add() the in-memory object still lacks the new permission, causing dispatch to return 403.
SonarQube flagged _handle_save_rule at complexity 20 (limit 15). Extract the queryset filtering into _find_existing_rule (~6) bringing _handle_save_rule down to ~11.
…ission string" This reverts commit 304ce97.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_interface_name_rules/views.py`:
- Around line 125-127: Extract the repeated permission literal
"netbox_interface_name_rules.view_interfacenamerule" (and related add/change
literals) into module-level constants placed near the top of the file (after
imports), e.g. PERM_VIEW_RULE, PERM_ADD_RULE, PERM_CHANGE_RULE, and then replace
all inline usages with those constants (for example update
get_required_permission() to return PERM_VIEW_RULE and update any other view
methods/decorators that reference the same strings). Ensure you update every
occurrence in this file so all four instances are replaced and import scope is
correct for class/methods like get_required_permission.
🪄 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: fefb6d43-723a-46a1-b0e5-0da9568b55f3
📒 Files selected for processing (2)
netbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
Replace the standalone InterfaceNameRuleYAMLExportView with NetBox's native export mechanism: - Add to_yaml() to InterfaceNameRule model so NetBox offers YAML in the Export dropdown - Override export_yaml() on the list view for deterministic pk-ordered output with empty-field stripping - Remove standalone /rules/export/yaml/ URL and custom template buttons - Rewrite tests to use the list view's ?export= parameter
Django's built-in user_permissions.add() does not work with NetBox's ObjectPermissionRequiredMixin. Create an ObjectPermission with actions=["add"] instead.
The table CSV export uses verbose column names that don't match the import form fields, making round-trip import impossible. Replace the BulkExport action with a YAML-only variant that omits the "Current View" option, keeping only "All Data (YAML)".
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_interface_name_rules/views.py (1)
156-163:⚠️ Potential issue | 🔴 CriticalApply RestrictedQuerySet and .restrict() to scope rule lookups to user permissions.
has_perm("...view_interfacenamerule")checks only the global permission, not ObjectPermission constraints. Direct.objectscalls bypass filtering.NetBox provides
RestrictedQuerySetwith a.restrict(user, action)method to filter querysets by assigned ObjectPermissions. Either:
- Configure
InterfaceNameRulemodel to useRestrictedQuerySet.as_manager()as its default manager and apply.restrict(request.user, 'view')in each queryset, or- Use
self.get_object(**kwargs)(already used at lines 135, 373, 396, 424) instead of direct.get()lookups—this applies permission filtering automatically via the NetBox base class.Update:
- Line 156–163: Replace
.get(pk=int(rule_id))with permission-scoped queryset- Line 209–223:
_find_existing_rule()should start with.restrict(user, 'view')if called in user context, or accept user as a parameter- Line 235–247:
_handle_save_rule()calls need the user passed to_find_existing_rule()for scopingA user with constrained view access (e.g., device_type-scoped rules) can currently preload, search, or detect duplicates across all rules in the database.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_interface_name_rules/views.py` around lines 156 - 163, The current direct lookup using InterfaceNameRule.objects.get(pk=int(rule_id)) bypasses NetBox object-permission filtering; replace this with a permission-scoped lookup by either using InterfaceNameRule.objects.restrict(request.user, 'view').select_related(...).get(pk=int(rule_id)) or (preferably) call the existing self.get_object(...) pattern used elsewhere to fetch the rule so permissions are enforced; also update _find_existing_rule() to apply .restrict(user, 'view') (or accept a user parameter) and change callers (including _handle_save_rule()) to pass request.user so all searches, preload and duplicate checks are scoped to the requesting user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@netbox_interface_name_rules/templates/netbox_interface_name_rules/buttons/export_yaml_only.html`:
- Around line 16-19: The export link currently injects et.name raw into the
query string causing breaks for names with &, = or #; update the template in
export_yaml_only.html so the export query value is URL-encoded (use the template
urlencode filter on et.name) when building the href so the ?export= parameter is
safe (keep the existing url_params logic and title display as-is, only encode
et.name for the query).
In `@netbox_interface_name_rules/tests/test_views.py`:
- Around line 831-853: The tests test_yaml_export_all_contains_rules and
test_yaml_export_structure must assert deterministic export ordering and absence
of blank optional keys; change them to compare the loaded YAML list against the
queryset ordered by PK (e.g., InterfaceNameRule.objects.order_by("pk")) to lock
down ordering, and add assertions that each entry exactly contains the expected
non-optional keys (e.g., "name_template", "pk", etc.) and does not include
empty/blank optional keys (assert optional keys are either absent or not equal
to ""/None). Use LIST_URL to fetch the export as before and reference
InterfaceNameRule and the exported entries when making these assertions.
In `@netbox_interface_name_rules/views.py`:
- Around line 15-18: The import line referencing netbox.object_actions and the
symbols AddObject, BulkDelete, BulkEdit, BulkExport, BulkImport, BulkRename in
netbox_interface_name_rules/views.py is invalid and prevents the plugin from
loading; fix it by either replacing that import with the correct NetBox module
that actually exposes those action classes, or remove the import and implement
local equivalents if the view relies on their behavior, or simply delete the
unused symbols if they are not referenced. Search this file for usages of
AddObject, BulkDelete, BulkEdit, BulkExport, BulkImport, and BulkRename (and any
subclasses or references) and update those references to use the correct
imported names or your local implementations (e.g., create small wrapper
classes/functions in this plugin if needed) so the file no longer imports from
netbox.object_actions.
---
Outside diff comments:
In `@netbox_interface_name_rules/views.py`:
- Around line 156-163: The current direct lookup using
InterfaceNameRule.objects.get(pk=int(rule_id)) bypasses NetBox object-permission
filtering; replace this with a permission-scoped lookup by either using
InterfaceNameRule.objects.restrict(request.user,
'view').select_related(...).get(pk=int(rule_id)) or (preferably) call the
existing self.get_object(...) pattern used elsewhere to fetch the rule so
permissions are enforced; also update _find_existing_rule() to apply
.restrict(user, 'view') (or accept a user parameter) and change callers
(including _handle_save_rule()) to pass request.user so all searches, preload
and duplicate checks are scoped to the requesting user.
🪄 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: b6156f7e-4475-4439-a290-48727ce8b71d
📒 Files selected for processing (5)
netbox_interface_name_rules/models.pynetbox_interface_name_rules/templates/netbox_interface_name_rules/buttons/export_yaml_only.htmlnetbox_interface_name_rules/templates/netbox_interface_name_rules/interfacenamerule_list.htmlnetbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.py
💤 Files with no reviewable changes (1)
- netbox_interface_name_rules/templates/netbox_interface_name_rules/interfacenamerule_list.html
- Wrap netbox.object_actions import in try/except so the plugin loads on NetBox 4.3.7 where that module does not exist; _LIST_VIEW_ACTIONS falls back to None so ObjectListView uses its default actions - Use .restrict(user, 'view') in RuleTestView for permission-scoped lookups instead of bare objects.get() / objects.all() - URL-encode et.name in export_yaml_only.html to avoid query-string breakage on names containing &, = or # - Tighten YAML export tests: assert PK-ordered export and that no optional key carries a blank/None value
In NetBox 4.3.7 object_types M2M targets ObjectType not ContentType. Detect the actual related model at runtime so the correct type is passed to .add() regardless of NetBox version.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Permissions
Documentation
UI Improvements