Conversation
4b15d22 to
4c5f567
Compare
|
lots of files changed because of a method signature change. important files to review : User.php CommonX.php Security/ReAuthxxx ReAuth controller. |
ad4c71e to
e255a00
Compare
743f71b to
0e80332
Compare
| { | ||
| global $CFG_GLPI; | ||
|
|
||
| $this->setSuccessRedirectURL(\Html::getRefererUrl() ?? $CFG_GLPI['url_base']); |
There was a problem hiding this comment.
I think the target URL should be the current URL instead of the referer. Indeed, if you display a form with the /Software/DoSomething (current URL) action from the Computer/X (referer URL) page, then after the reauth you need to submit the form data on the /Software/DoSomething to make the form submission processed.
8413e75 to
14502be
Compare
|
There is a conflict to solve |
14502be to
197c4e8
Compare
|
|
||
| Html::back(); | ||
| } else { | ||
| $user->check((int) $_GET['id'], READ, $input); |
There was a problem hiding this comment.
The CommonDBTM::displayFullPageForItem() already perform a $item->can($id, READ). This may be replaced by a call to the check() method.
Anyway, I think we should find a way to not have to alter all legacy controllers that call CommonDBTM::displayFullPageForItem().
| methods: ['GET'] | ||
| )] | ||
| #[SecurityStrategy(Firewall::STRATEGY_AUTHENTICATED)] | ||
| public function prompt(Request $request, string $error = ''): Response |
There was a problem hiding this comment.
Please ensure the presence of the string $error parameter cannot be used to force a specifi message throught the URL parameters. It could be used by an attacker to display a custom message during a prompt authentication, and I guess it could be used to encourage users to take an unexpected action, such as contacting a fictitious support team.
A solution could be to mutualize the template rendering in a private function getPromptResponse(?string $error = null).
| return [ | ||
| 'redirect' => $this->reAuthManager->getRedirectURL(), | ||
| 'cancel_url' => $this->reAuthManager->getCancelURL(), | ||
| 'action' => $this->router->generate('reauth_verify'), |
There was a problem hiding this comment.
Please verify that the generated URL is correct when GLPI is served under an Alias (e.g. http://example.org/glpi). We did not used the URL generator for the moment because we did not checked it is fully working the GLPI context.
| // catched in RedirectPostExceptionListener | ||
| throw new ReauthRedirectException( | ||
| $this->reAuthManager->getRedirectURL(), | ||
| $this->reAuthManager->getRedirectData(), | ||
| $this->reAuthManager->getRedirectMethod(), | ||
| ); |
There was a problem hiding this comment.
The response could be built here directly, since this is the only place where this exception is thrown.
| if ($this->http_method === 'POST' && isset($this->data['id'])) { | ||
| return $this->url . '?id=' . $this->data['id']; | ||
| } |
There was a problem hiding this comment.
IMHO, this could lead to unexpected issues. I guess this parameter was present in the initial request URL. Maybe $_GET parameters should be preserved in the URL.
| enum ReAuthStrategyEnum: string | ||
| { | ||
| public function __construct( | ||
| string $date, | ||
| string $author, | ||
| ) { | ||
| parent::__construct( | ||
| label: __("Current version"), | ||
| description: __("Updated by"), | ||
| date: $date, | ||
| author: $author, | ||
| ); | ||
| case TOTP = 'totp'; | ||
| case PASSWORD = 'password'; | ||
|
|
||
| public function createStrategy(): ReAuthStrategyInterface | ||
| { | ||
| return match ($this) { | ||
| self::TOTP => new TOTPReAuthStrategy(), | ||
| self::PASSWORD => new PasswordReAuthStrategy(), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
You could instanciate the ReAuthManager directly from the dependency injection logic, to not have to maintain this enum.
We have sucessfully done it for the Glpi\Application\ViewTemplateRendererclass. The constructor is initialized by the DI system, and thegetInstance()` method can be used where it is not possible to une the DI system.
There was a problem hiding this comment.
There are no changes in this class except some code reorganization. This add 10% more lines changes to this PR, increasing the review complexity. Please try to not do too much refactoring. We have thousand lines of code in GLPI that are indeed pretty hard to understand, but since our code is far from being entirely covered by tests and since we still do not use strict types everywhere, this kind of changes can result in unexpected side effects and should be avoid when it is not necessary.
# Conflicts: # src/CommonDBTM.php
no code change, just move the else. - may be reverted # Conflicts: # src/Session.php
# Conflicts: # src/CommonDBTM.php # src/CommonGLPI.php # src/Glpi/Controller/Security/ReAuthController.php # src/Glpi/Security/ReAuth/ReAuthManager.php
- autosubmit totp on last char typed - remove useless array handling
GLPI_REAUTH to false for tests
- ReauthRedirecException handle $_GET requests - move rights checks form \CommonDBTM::displayFullPageForItem() to front/user.form.php - refacto ReAuthManager.php - just redirect to previous url (not to form on form submit) - fix \Glpi\Security\ReAuth\ReAuthManager::isReAuthenticated() constant usage - reauth can be done on all rights (not only UPADTE). - simplify \CommonDBTM::check()
197c4e8 to
c6976eb
Compare
just a fallback, should never happen
still a wip. no review needed.
https://outline.teclib.com/doc/sudo-mode-reauth-dev-doc-XfUBa9nO8A