Skip to content

Closes #18987: Preserve restricted related values on edit forms#22390

Draft
pheus wants to merge 1 commit into
mainfrom
18987-loss-of-object-properties-when-editing-with-restricted-view
Draft

Closes #18987: Preserve restricted related values on edit forms#22390
pheus wants to merge 1 commit into
mainfrom
18987-loss-of-object-properties-when-editing-with-restricted-view

Conversation

@pheus

@pheus pheus commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Closes: #18987

Add read-only preservation of related object values hidden by object permissions. Single-value fields are disabled entirely; multi-value fields remain editable for visible values while restricted current values appear as disabled options and are merged back on save.

Fixes data loss when users edit objects with related values they cannot view, such as tags or tenants constrained by permissions.

Add read-only preservation of related object values hidden by object
permissions. Single-value fields are disabled entirely; multi-value
fields remain editable for visible values while restricted current
values appear as disabled options and are merged back on save.

Fixes data loss when users edit objects with related values they cannot
view, such as tags or tenants constrained by permissions.
@pheus

pheus commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pheus's task in 4m 59s —— View job


Code Review

  • Read changed files and diff
  • Analyze core implementation
  • Check security considerations
  • Post review findings

The approach is sound — reading current values from the DB instance (not submitted POST data), adding restricted members back only for already-assigned PKs, and enforcing preservation server-side rather than relying on the disabled HTML attribute. The security model is correct. A few issues worth discussing below.


1. select_option.html is a global change for all APISelect-backed fields

netbox/utilities/templates/widgets/select_option.html is the option_template_name for APISelect (and APISelectMultiple). The new rendering logic:

{% if widget.label.disabled %} disabled="disabled"{% endif %}>{{ widget.label.label|default:widget.label }}

affects every option in every dynamic select field across the application. Any code — including plugins — that uses a non-string object as a label_from_instance result (e.g., a model instance or proxy that happens to have a truthy .disabled attribute) would have its options silently rendered as disabled with this template. There's no explicit guard to ensure only RestrictedChoiceLabel objects trigger the disabled path.

Additionally, {{ widget.label.label|default:widget.label }} uses default which returns the fallback for any falsy value. If a choice label happens to be an empty string "", the rendering would still be correct (empty string is the original label), but a label of 0 or False would be rendered empty. These edge cases are unlikely in practice, but since this is a global template, the risk surface is wider than the feature boundary.


2. N+1 queries in _restricted_value_is_hidden

netbox/netbox/forms/model_forms.py — the hidden_objects computation calls .filter(pk=obj.pk).exists() twice per element of current_objects:

hidden_objects = [
    obj for obj in current_objects
    if self._restricted_value_is_hidden(field, restricted_queryset, original_queryset, obj, user, action)
]
was_choice = original_queryset.filter(pk=obj.pk).exists()   # 1 query
is_visible = restricted_queryset.filter(pk=obj.pk).exists()  # 1 query

For a scalar FK field this is two queries maximum and is fine. For a multi-value field with N currently assigned members (e.g., an object with many tags), this is 2N queries per field per form render. A single batched version would be:

current_pks = [obj.pk for obj in current_objects]
original_pks  = set(original_queryset.filter(pk__in=current_pks).values_list('pk', flat=True))
visible_pks   = set(restricted_queryset.filter(pk__in=current_pks).values_list('pk', flat=True))
hidden_objects = [obj for obj in current_objects if obj.pk in original_pks and obj.pk not in visible_pks]

That collapses the whole loop to 2 queries regardless of set size.

Fix this →


3. Inaccurate comment in utils.py

netbox/utilities/forms/utils.py lines 234–237:

# restrict() returns the same queryset object only when no restriction applies (e.g. superusers). A
# non-superuser with unconstrained permissions gets an equivalent but new queryset; the per-value checks
# in prepare_restricted_queryset_fields() ensure nothing is locked when no current value is hidden.

Looking at RestrictedQuerySet.restrict(), unconstrained users (those with the permission but no attribute-level constraints) also hit return self at the end of the method — same identity, same as superusers. The second sentence is inaccurate: they do not get a new queryset, so they are correctly excluded from restricted_fields by the identity check, not by the per-value checks. The comment could mislead a future maintainer into thinking per-value checks are the safety net here.


4. restricted_related_selectors override semantics — silent loss on subclassing

The docstring on restricted_related_selectors says "Subclasses override (not merge) this attribute." This works today because no form that inherits from ScopedForm (which contributes scope) also defines its own restricted_related_selectors. But it's a latent hazard: if a future developer subclasses PrefixForm or WirelessLANForm and adds their own selector, they'll silently lose the inherited scope entry with no error or warning. This might be worth calling out in the docstring or protecting with a __init_subclass__ merge, though that's a larger change.


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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loss of Object Properties When Editing with Restricted View Permissions

1 participant