IBX-8323: Reworked RepositoryAuthenticationProvider and moved its logic to a dedicated subscriber

[konradoboza]

🎫 Issue	IBX-8323

Description:

RememberMeRepositoryAuthenticationProvider is another part of our codebase relying on deprecated security layer. It basically covers some additional checks performed on repository users like "forgot password redirect" being the subject of the underlying bug.

In order to replace the deprecated approach I did the following:

• removed mentioned provider alongside its unit test counterpart,
• I removed registering it in SecurityPass as the original class being replaced will be removed in Symfony 6 anyway,
• I moved ibexa.security.authentication.constant_auth_time setting from mentioned compiler pass and provided the same default value,
• I registered RepositoryUserAuthenticationSubscriber that will take over checks performed in the provider,
• I provided adjusted test coverage taken from the original provider's test counterpart.

The choice of CheckPassportEvent is intentional - I wanted to have possibility to stop and resume the authenticator process and according to my research this was the only suitable event for such purpose. Excerpt from Symfony doc:

CheckPassportEvent
Dispatched after the authenticator created the security passport. Listeners of this event do the actual authentication checks (like checking the passport, validating the CSRF token, etc.)

Note

Getting rid of injecting user into our PermissionResolver is intentional - the entire system got it covered within recently introduced https://github.com/ibexa/core/blob/main/src/lib/MVC/Symfony/Security/Authentication/EventSubscriber/OnAuthenticationTokenCreatedRepositoryUserSubscriber.php#L30.

The main part I am concerned about and have little knowledge of is:

// $currentUser can either be an instance of UserInterface or just the username (e.g. during form login).
        /** @var \Ibexa\Core\MVC\Symfony\Security\UserInterface|string $currentUser */
        $currentUser = $token->getUser();
        if ($currentUser instanceof UserInterface) {
            if ($currentUser->getAPIUser()->passwordHash !== $apiUser->passwordHash) {
                throw new BadCredentialsException('The credentials were changed in another session.');
            }

            $apiUser = $currentUser->getAPIUser();
        } else {
            $credentialsValid = $this->userService->checkUserCredentials($apiUser, $token->getCredentials());

            if (!$credentialsValid) {
                throw new BadCredentialsException('Invalid credentials', 0);
            }
        }

I don't think we have any documented use-cases for that part but still believe it should be approached differently if needed. Will discuss that internally but also summon @adamwojs to check if he remembers what it was all about.

Update:
We believe this is already covered by Symfony and was needed long time ago to make sure that user password change performed in another browser tab is effectively caught. Most likely we are fine due to usage of Symfony\Component\Security\Core\User\EquatableInterface which tends to be used to test if two objects are equal in security and re-authentication context.

For QA:

Documentation:

Dropping RememberMeRepositoryAuthenticationProvider should be mentioned.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[konradoboza]

I also dropped lib/MVC/Symfony/Security/Authentication/RememberMeRepositoryAuthenticationProvider since it wasn't loaded in SecurityPass anyway.