Tarea 4041 - Mejora la autentificación en dos pasos#2
Tarea 4041 - Mejora la autentificación en dos pasos#2daniel89fg wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements improvements to two-factor authentication, updating dependency versions and refactoring both the view and controller logic.
- Upgrades the endroid/qr-code dependency in composer.json.
- Refactors the two-factor authentication view to improve UI flow and variable usage in UserTwoFactor.html.twig.
- Revises user secret key management and controller actions in User.php, TwoFactorManager.php, and EditUser.php to better handle authentication and deauthentication flows.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| composer.json | Upgrades endroid/qr-code to ^4.8 and adjusts dependency ordering. |
| Core/View/Tab/UserTwoFactor.html.twig | Refactors variable usage and UI flow for two-factor authentication view. |
| Core/Model/User.php | Removes auto-generation of two_factor_secret_key for improved design. |
| Core/Lib/TwoFactorManager.php | Adds try/catch error handling with detailed logging for QR generation and code verification. |
| Core/Controller/EditUser.php | Introduces new authentication and deauthentication actions; removes redundant code. |
Comments suppressed due to low confidence (1)
Core/Model/User.php:198
- The automatic generation of two_factor_secret_key has been removed. Ensure that the secret key is set elsewhere before calling getTwoFactorUrl to avoid unintended empty values.
}
| </button> | ||
| </form> | ||
| {% else %} | ||
| {% set twoFactorUrl = fsc.model.getTwoFactorUrl() %} |
There was a problem hiding this comment.
Consider updating the twoFactorUrl assignment to use the 'user' variable (i.e. user.getTwoFactorUrl()) for consistency with the rest of the template.
| {% set twoFactorUrl = fsc.model.getTwoFactorUrl() %} | |
| {% set twoFactorUrl = user.getTwoFactorUrl() %} |
| Tools::log()->error('record-not-found'); | ||
| return false; | ||
| } elseif (false === $user->two_factor_enabled) { | ||
| Tools::log()->error('two-factor-authentication-already-enabled', ['%nick%' => $user->nick]); |
There was a problem hiding this comment.
In the deauthentication2factorAction, the error message is misleading when two_factor_enabled is false. Consider changing it to reflect that two-factor authentication is not enabled, e.g. 'two-factor-authentication-not-enabled'.
| Tools::log()->error('two-factor-authentication-already-enabled', ['%nick%' => $user->nick]); | |
| Tools::log()->error('two-factor-authentication-not-enabled', ['%nick%' => $user->nick]); |
Tarea #4041
Cuando el core esté en la versión de php 8.1 en adelante, podremos cambiar la librería de QR por la que realmente usa la que usamos actualmente.