Skip to content

Refactor NAS Management: Enforce strict validation and update NAS types list#674

Open
retho-p wants to merge 2 commits intolirantal:masterfrom
retho-p:clean-nas-page
Open

Refactor NAS Management: Enforce strict validation and update NAS types list#674
retho-p wants to merge 2 commits intolirantal:masterfrom
retho-p:clean-nas-page

Conversation

@retho-p
Copy link
Copy Markdown
Collaborator

@retho-p retho-p commented May 1, 2026

Description

This Pull Request improves the NAS management forms by updating the valid NAS types list and ensuring strict validation.

Changes

  • Updated the $valid_nastypes array in validation.php to use the official FreeRADIUS 3.x list.
  • Changed the "NAS Type" input to a native HTML <select> dropdown in mng-rad-nas-new.php and mng-rad-nas-edit.php.
  • Added contextual tooltips (tooltipText) to all fields in the NAS creation and editing forms.
  • Enabled help modals for mngradnasnew and mngradnasedit by adding help texts in lang/en.php.

- Updated '\' in 'validation.php' to include the comprehensive official list of FreeRADIUS NAS types.

- Converted the NAS Type input in 'mng-rad-nas-new.php' and 'mng-rad-nas-edit.php' to a strict HTML '<select>' dropdown.

- Reinstated strict server-side validation using 'in_array()' to prevent unauthorized values, falling back to 'other' by default.

- Added helpful tooltips to NAS form inputs and updated help pages in lang/en.php.
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Refactor NAS management with strict validation and improved UX

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Updated NAS types list to official FreeRADIUS 3.x comprehensive types
• Converted NAS Type input from text with datalist to strict HTML select dropdown
• Changed default fallback value from first array element to "other"
• Added contextual tooltips to all NAS form fields for better UX
• Enhanced help documentation for NAS management pages
Diagram
flowchart LR
  A["NAS Types List"] -->|"Updated to official FreeRADIUS 3.x"| B["validation.php"]
  C["Form Inputs"] -->|"Convert to select dropdown"| D["mng-rad-nas-new.php<br/>mng-rad-nas-edit.php"]
  E["Default Fallback"] -->|"Changed to 'other'"| D
  F["Form Fields"] -->|"Add tooltips"| D
  G["Help Pages"] -->|"Enhanced documentation"| H["lang/en.php"]
Loading

Grey Divider

File Changes

1. app/common/includes/validation.php ✨ Enhancement +4/-2

Expand NAS types to official FreeRADIUS list

• Expanded $valid_nastypes array from 13 to 20 NAS vendor types
• Added new types: cvx, ascend, pr3000, pr4000, digitro, versanet, bay, cisco_l2tp, mikrotik,
 mikrotik_snmp, redback, dot1x
• Reordered list to match official FreeRADIUS 3.0.21 specification
• Moved "other" to end of array for consistency

app/common/includes/validation.php


2. app/operators/mng-rad-nas-edit.php ✨ Enhancement +17/-10

Convert to select dropdown and add field tooltips

• Changed NAS Type field from text input with datalist to HTML select dropdown
• Updated default fallback from $valid_nastypes[0] to hardcoded "other"
• Added tooltipText parameter to all form input descriptors (8 fields total)
• Replaced "datalist" with "options" and "value" with "selected_value" for select field

app/operators/mng-rad-nas-edit.php


3. app/operators/mng-rad-nas-new.php ✨ Enhancement +17/-10

Convert to select dropdown and add field tooltips

• Changed NAS Type field from text input with datalist to HTML select dropdown
• Updated default fallback from $valid_nastypes[0] to hardcoded "other"
• Added tooltipText parameter to all form input descriptors (8 fields total)
• Replaced "datalist" with "options" and "value" with "selected_value" for select field

app/operators/mng-rad-nas-new.php


View more (1)
4. app/operators/lang/en.php 📝 Documentation +7/-3

Add NAS management help documentation

• Added comprehensive help text for mngradnas explaining NAS role and security importance
• Added help text for mngradnasnew describing new NAS device addition
• Added help text for mngradnasedit describing NAS device editing
• Kept existing help text for mngradnasdel and mngradnaslist

app/operators/lang/en.php


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 1, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Legacy NAS type overwritten🐞 Bug ≡ Correctness
Description
In mng-rad-nas-edit.php, the NAS type field is now rendered as a `` backed only by
$valid_nastypes; if the existing DB value isn’t in that list, no option will be selected and the
browser will submit the first option on save, unintentionally changing the NAS type. This causes
silent data corruption and may change FreeRADIUS client behavior even when the operator edits an
unrelated field (e.g., secret).
Code

app/operators/mng-rad-nas-edit.php[R186-192]

       $input_descriptors0[] = array(
                                       "name" => "type",
                                       "caption" => t('all','NasType'),
-                                        "type" => "text",
-                                        "datalist" => $valid_nastypes,
-                                        "value" => ((isset($type)) ? $type : $valid_nastypes[0])
+                                        "type" => "select",
+                                        "options" => $valid_nastypes,
+                                        "selected_value" => ((isset($type)) ? $type : "other"),
+                                        "tooltipText" => "Select the NAS vendor type from the predefined list."
Evidence
The DB schema allows arbitrary nas.type strings (no enum/constraint), so legacy/custom values can
exist. The edit page binds selected_value to the DB value, but print_select() only applies
selected when there’s an exact match; when there’s no match, HTML select falls back to the first
option. Because the updated $valid_nastypes list begins with "livingston" (and "other" is last),
an unsupported existing type will be displayed/saved as the first option and then persisted via the
UPDATE query.

app/operators/mng-rad-nas-edit.php[67-101]
app/operators/mng-rad-nas-edit.php[158-193]
app/common/includes/layout.php[957-1035]
app/common/includes/validation.php[134-140]
contrib/db/fr3-mariadb-freeradius.sql[166-180]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
After changing NAS Type to a strict `<select>`, editing an existing NAS whose stored `type` is not present in `$valid_nastypes` will render with no matching selected option, and the browser will submit the first option (currently `livingston`). Saving the form can therefore silently overwrite the NAS `type` even if the operator didn’t intend to change it.
### Issue Context
- `nas.type` is a plain `varchar(30)` and may contain legacy/custom values.
- The UI now only offers `$valid_nastypes` as selectable options.
- `print_select()` selects only when `selected_value === option_value`; otherwise no option gets `selected`.
### Fix Focus Areas
- app/operators/mng-rad-nas-edit.php[158-193]
- app/common/includes/validation.php[134-140]
- app/common/includes/layout.php[957-1035]
### Recommended fix approach
Implement one of the following (preferred order):
1. **Preserve legacy/custom value during edit rendering**: if `$type` loaded from DB is non-empty and not in `$valid_nastypes`, prepend it to the options (e.g., `array_unshift($valid_nastypes, $type)`), optionally labeling it as `"$type (legacy)"` if you switch to an associative options map.
2. **Fail safe to `other` and warn**: if `$type` is not in `$valid_nastypes`, force the selected value to `other` *and* show a clear warning message that the stored type is unsupported and will be changed on save.
3. **Server-side safety**: if the submitted type equals the first option but the previous DB type was unsupported, require an explicit operator confirmation before updating `type`.
Ensure the edit form never changes `type` implicitly without the operator knowingly selecting a new value.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread app/operators/mng-rad-nas-edit.php Outdated
@retho-p retho-p self-assigned this May 4, 2026
…rwrite

When a NAS stored a type not in $valid_nastypes, the select rendered
with no matching option and the form would silently save the first
option (livingston). Now legacy types are prepended to the select
with a "(legacy)" label and accepted by server-side validation.
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.

1 participant