Repository navigation
issue#130: per-device-type post-login prompt/response handling - #199
Conversation
17f946f to
b41ec24
Compare
b41ec24 to
813bf03
Compare
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Blocker: a prompt containing / never matches, and the response is sent anyway. ProcessLoginPrompts() builds the pattern as '/' . $entry['prompt'] . '/i' and suppresses the error. A prompt such as [y/n] or Select member (1/2) breaks that pattern, so preg_match() returns false on every try. After 5 attempts the loop sends the response anyway. The same fallthrough happens for any prompt that never appears, so a configured response like y or 2 can land on the CLI prompt and be run as a command.
- Use a delimiter that can't clash with the prompt, or escape it.
- Validate the patterns in
save_devicetypes(). - Skip
DoCommand()when the prompt wasn't seen.
Suggestion: this overlaps the existing anykey device-type field, which already answers "press any key" prompts. The field description should say which one runs first, or whether both can be set together.
SonarCloud duplicated-lines failure (4.0%): the only new duplicated block is the 7-line loginprompts textarea definition in includes/arrays.php. It has the same shape as the other field arrays in that file, and the PR adds so little code that those 7 lines alone come to 4%. There are two ways to clear it:
- Build that field from a shared definition. The device-type form already repeats the same textarea keys, so a small
plugin_routerconfigs_textarea_field($name, $label, $description)helper could cover them. - Exclude the declarative form arrays from copy-paste detection with
sonar.cpd.exclusions=includes/arrays.php, since repeated array shapes there aren't real duplicated logic.
…le on change Monthly without a day of month used the day of the current run, so a Jan 31 run asked for "February 31", which strtotime() turned into March 3, and the device drifted from then on. A Monthly schedule saved, or bulk-set, without a day now stores the day it was set up on (Last, 32, after the 28th so short months still run), and the calculator falls back to the 1st rather than the run day. Days a month does not have (February 30, April 31) are skipped, as RFC 5545 section 3.3.10 requires, instead of rolling into the next month. The replica is now used on every Cacti version. Core's api_scheduler_calculate_next_start() rolls nonexistent dates over, so delegating to it on 1.3+ would give the same device different backup dates on 1.2 and 1.3. Bulk edit validated nothing: sched_type, account and devicetype were cast to int, so an unknown sched_type resolved to 0 and the device ran on every scheduled poll. plugin_routerconfigs_bulk_edit_validate() now accepts only the offered Schedule Types, Rerun Every 1-31, a known Connection Type, and existing accounts and device types; anything else raises an error and nothing is changed. save_devices() checks Schedule Type and Rerun Every the same way. Changing the schedule now recomputes nextbackup on the device form and in bulk edit, so a device moved from Daily to Monthly no longer runs once more on the old schedule. Manual stores 0, which the download query excludes. A device that has never been scheduled keeps NULL so its first backup still runs next. Bulk edit reads the selected rows once and groups devices with identical results into one UPDATE ... WHERE id IN (...). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sue-117-schedule-days-datedir Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Now that nonexistent dates are skipped, a Monthly schedule on February 29 alone has no date in 2026 or 2027. The calculator tried only this year and next, so it returned 0 and the device became due on every scheduled run. It now searches 8 years, the longest gap between leap years (2096 to 2104), and still returns false for a day that never exists, such as February 30. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sue-117-schedule-days-datedir Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…p missing days Bulk edit offered every Schedule Type, including Monthly on Day, but never sets Months, Weeks of Month or Days of Week. A device bulk-set to Monthly on Day had nothing to resolve, so nextbackup became 0 and it was backed up on every scheduled run. Monthly on Day is now left out of the bulk list, a bulk Monthly gets every month when none is stored (and a fixed day of month, as on the device form), and each device's resulting row must pass plugin_routerconfigs_schedule_validate(). If any device fails, nothing is changed and the failing device ids are reported. plugin_routerconfigs_schedule_validate() also rejects a monthly selection that never occurs (only February 30 and 31, say), which would otherwise resolve to 0. Days a month lacks are skipped rather than rolled over, so a 31st selected for every month no longer adds May 1, July 1, October 1 and December 1 runs; tests cover that through the multi-select save path. Weekly with Days of Week selected runs on those days every week and ignored Rerun Every, while the field said "every X weeks". Rerun Every is now hidden once Days of Week are selected, and both descriptions say how they combine. When validation fails the redirect still returns to a fresh edit form. The plugin does not use Cacti's form_input_validate() field retention anywhere, so keeping the entered values would mean adopting it for the whole form; that is left for a separate change. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…mpts safely ProcessLoginPrompts() built each pattern as '/' . $prompt . '/i' and suppressed the error, so a prompt containing '/' (such as '[y/n]' or 'Select member (1/2)') never compiled and never matched. After 5 reads the loop sent the response anyway, and the same happened for any prompt that did not appear, so a configured 'y' or '2' could land on the CLI prompt and run as a command. plugin_routerconfigs_regex() now compiles an operator-supplied pattern: it escapes an unescaped '/', keeps an already escaped '\/', and returns null for a pattern that does not compile, without emitting a warning. A response is only sent once its prompt has been seen; otherwise a WARNING is logged and nothing is sent. Download() then carries on, and EnsureEnabled() fails the backup if the device is still waiting at an unanswered prompt. save_devicetypes() rejects prompts that are not valid regular expressions. The field description now says that a response is only sent after its prompt is seen, that a prompt cannot contain '|', and that Post-Login Prompts are answered before the Any Key prompt, which is handled later while the device is being enabled. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ssue-130-custom-login-prompts Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Rechecked against the previous review.
- Prompt containing
/: fixed.plugin_routerconfigs_regex()handles/and\/,save_devicetypes()rejects patterns that don't compile, andProcessLoginPrompts()no longer sends a response when its prompt wasn't seen. - Overlap with
anykey: fixed, the field description now says which runs first. - SonarCloud duplication: the gate passes now.
Suggestion: classes/PHPShellConnection.php:338: chained prompts. $response starts empty for each entry and DoCommand() at line 359 consumes the device's reply. If the second prompt is printed in reply to the first response (for example "Press any key" then "Select member"), it has already been read and discarded. The second entry then logs "not seen" and is skipped, where the old code sent it regardless. Check the previous DoCommand() result against the next prompt before reading again.
Suggestion: a prompt typed as [y/n] is a valid character class, so it passes the save-time check and then matches any output containing y, n or /. The response goes out on the first read. Mention in the field description that brackets must be escaped (\[y/n\]).
No blockers. Only SonarCloud runs on this stacked PR (it passes); the PHP integration jobs run on #197.
Each prompt started from an empty buffer and the reply to the previous response was thrown away, so a chained prompt (Press any key, then Select member) had already been read and was never seen: it was logged as missing and skipped. The reply to each response, and any output read while waiting for a prompt that never came, is now checked against the next prompt before reading more. The field description also says that brackets and other special characters must be escaped, since an unescaped [y/n] is a character class that matches almost any output. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Pushed for the review and the follow-up suggestions:
Tests: To revisit:
|
Compiles more distinct patterns than the 256-entry cache holds, so the reset branch runs; clears the 100% changed-line coverage gate. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Post-login menu handling can discard prompts, is unreachable for Telnet devices stopped at a menu, and sends LF rather than the required carriage return on Linux, preventing login completion.
3 open issues
What changed in this PR
Adds per-device-type post-login prompt handling to the Cacti routerconfigs plugin, supporting devices such as HPE Aruba stacks that require extra input before configuration backups.
Changes:
- Adds a Post-Login Prompts form field, database column, and regular-expression validation.
- Processes ordered prompt/response pairs before shell backup commands.
- Adds parser and scripted-shell tests, updates translations and release notes, and bumps the version to 1.11.
| File | Description |
|---|---|
tests/Unit/RouterconfigsLoginPromptsTest.php |
Tests parsing, regex validation, and scripted prompt handling. |
router-devtypes.php |
Validates and saves configured login prompts. |
locales/po/cacti.pot |
Adds field-help and validation-message translations. |
INFO |
Bumps the plugin version to 1.11. |
includes/functions.php |
Adds prompt parsing and regex validation helpers. |
includes/database.php |
Adds the loginprompts device-type column. |
includes/arrays.php |
Defines the Post-Login Prompts form field and help text. |
classes/PHPShellConnection.php |
Adds prompt processing before shell backup commands. |
CHANGELOG.md |
Documents the feature for issue #130. |
Review effort: Balanced
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Chained prompts are handled now: the reply to one response, and any output read while waiting for a prompt that never came, is checked against the next prompt before reading again. The two new tests cover both paths, and the cache-reset test clears the patch-coverage gate.
The earlier suggestion about [y/n] being a character class still stands as a field-description note. No blockers, and CI is green on 8.2-8.6.
…ith CR PHPTelnet::Connect() disconnected when the password reply was a menu rather than a CLI prompt, so Download() never got to answer the configured prompts. Connect() now answers them there, passing the reply already read, and each connection answers them at most once. Responses went through DoCommand(), which ends the line with PHP_EOL (LF on Linux) and trims the reply to its last line, dropping a prompt printed with it. SendResponse() writes the answer followed by CR and returns the reply untrimmed. A socket-pair test drives the real transport. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
77c3526
Resolve conflicts after #198 (issue#117 + issue#133) landed on develop: - scheduler.php, RouterconfigsSchedulerTest.php: take develop's version; it is a superset of this branch's issue#117-era copy (adds the DST-aware strtotime Daily/Weekly math, plugin_routerconfigs_schedule_fields_valid() and its test). issue#130 does not touch scheduling. - router-devices.php: keep develop's save_devices() flow that validates the multi-select schedule inputs (schedule_fields_valid) before saving; the issue#130 post-login-prompt handling merged cleanly alongside it. - database.php: keep develop's day_of_month varchar(128) (a full Days of Month selection is an 86-char CSV); the issue#130 device-type columns merged cleanly. - INFO: keep this branch's version bump to 1.11. - CHANGELOG: union - issue#130 entry plus develop's issue#117/#133 entries (including the DST line). - cacti.pot: regenerated from the merged source (issue#130 + issue#117 strings); per-language .po files taken from develop unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a65997d
|
Description
Third PR in the stacked chain (#133 → #117 → #130 → #109), built on top of the #117 branch. It adds a per-Device-Type Post-Login Prompts field so devices that interpose extra prompts between login and the CLI can be backed up.
loginpromptscolumn onplugin_routerconfigs_devicetypes. Each line is aprompt|responsepair.PHPShellConnection::Download()calls a newProcessLoginPrompts(): for each configured pair it waits briefly for the prompt to appear (matched as a case-insensitive regular expression, consistent with how the existingpromptconfirmis matched) and then sends the response. A response of<CR>(or an empty response) sends just a carriage return, so "Press any key to continue" style prompts and<CR>/numeric stacked-switch selections can both be answered.plugin_routerconfigs_parse_login_prompts()); the transport method that drives the live shell lives in the allowlisted connection class.Example configuration for the HPE Aruba stack in the issue:
Related Issue
#130
Motivation and Context
Some devices (e.g. an HPE Aruba switch that is a stack member) present prompts after SSH login — "Press any key to continue" and "Enter switch number to connect to or
<CR>:" — that the existing Any-Key / Confirmation fields only handle after the copy-to-TFTP command, so the backup fails. This lets an operator declare those prompts and their responses per Device Type, answered at the right point in the session.How Has This Been Tested?
Ran the plugin's Pest suite from a Cacti checkout with the plugin as a real
plugins/routerconfigsdirectory (the stacked diff covers #133 + #117 + #130):pest --configuration phpunit.xml→ 179 passed (1123 assertions). Addedtests/Unit/RouterconfigsLoginPromptsTest.phpcovering empty/whitespace input, ordered trimmed parsing,<CR>/empty normalization, skipping of blank / separator-less / empty-prompt lines, CRLF/CR line endings, and first-separator-only splitting.php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100→ 100.00% (211/211 lines).php tests/bin/check-i18n-pot.php origin/develop→ OK (locales/po/cacti.potregenerated; per-language.po/.moleft to Weblate).php -lclean on every changed file.php tests/bin/validate-manifest.php→ matches.The
ProcessLoginPrompts()transport method needs a live SSH/Telnet shell and is not exercised by unit tests (its class is in the patch-coverage allowlist); the pure parser it depends on is fully unit-tested.Types of changes
Checklist: