Conversation
There was a problem hiding this comment.
Pull request overview
Refactors install-time output and related behaviors to reduce noisy/non-interactive logging, loosen packaging requirements, and fix a grants upgrade edge case.
Changes:
- Suppress “Login failed” exception details from being surfaced as frontend login errors; normalize the SQL auth failure message.
- Reduce backend/user message generation for certain commands (e.g., screenshot) and gate install progress markers behind terminal presence.
- Adjust install/upgrade SQL behavior (enable grants with
enabled=0, adddomains_bundle.id_bundleindex) and removesystemd-timesyncdfrom Ubuntu 20.04 package dependencies.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| script/rvd_front | Filters out “Login failed” exceptions from user-visible error list during login. |
| lib/Ravada/Request.pm | Stops sending user messages for screenshot requests. |
| lib/Ravada/Auth/SQL.pm | Adds trailing newline to “Login failed” die message to avoid location suffix. |
| lib/Ravada.pm | Gates install progress output behind $ENV{TERM}, removes default admin password from log message, adds bundle index, and enables grants where enabled=0. |
| debian/control-ubuntu-20.04 | Removes systemd-timesyncd from package dependencies. |
Comments suppressed due to low confidence (1)
lib/Ravada.pm:353
- The
_init_user_admininitializer creates a default admin account with usernameadminand hardcoded passwordadmin, which is a serious security risk. Anyone who can reach the login interface after installation can immediately authenticate as an administrator using these public credentials. Instead, avoid embedding default admin passwords in code by generating a strong random password (or forcing the installer to set one) and storing only a hashed value in the database.
);
warn "INFO: created default admin user 'admin'\n"
if $0 !~ /\.t$/;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sub _enable_all_grants($self) { | ||
| my $sth = $CONNECTOR->dbh->prepare( | ||
| "SELECT id,name,default_user,default_admin FROM grant_types " | ||
| ." WHERE enabled is null" | ||
| ." WHERE enabled is null OR enabled=0" | ||
| ." ORDER BY name" |
There was a problem hiding this comment.
_enable_all_grants now treats enabled=0 the same as enabled IS NULL. There are existing upgrade tests around grants (e.g. t/user/upgrade_grants.t covers the NULL case), but there’s no test covering the new enabled=0 path. Please add a regression test that sets a grant’s enabled to 0 and verifies _install_grants() (or _enable_all_grants()) flips it back to 1 and re-inserts missing grants_user rows as expected.
Show messages only in terminal, removed timesyncd requirement, fixed SQL and warning messages