Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Error with lock_factory in login_throttling #51347

Closed
BaptisteContreras opened this issue Aug 10, 2023 · 2 comments
Closed

[Security] Error with lock_factory in login_throttling #51347

BaptisteContreras opened this issue Aug 10, 2023 · 2 comments

Comments

@BaptisteContreras
Copy link
Contributor

BaptisteContreras commented Aug 10, 2023

Symfony version(s) affected

'>= 6.2'

Description

I just upgraded my project from 5.4 to 6.3 and this error popped up :

Rate limiter "_login_local_main" requires the Lock component to be configured.

After some tests, I found out that this line (in my security.yaml) is the cause :

lock_factory: lock.default.factory

When I looked in

Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\LoginThrottlingFactory

from the Security Bundle, I noticed that this if is the problem :

### LoginThrottlingFactory line 101
if (!$container->hasDefinition('lock.factory.abstract')) {
     throw new LogicException(sprintf('Rate limiter "%s" requires the Lock component to be configured.', $name));
}

If I dump $container->getDefinitions() before this if I don't see any lock in the definitions array, which seems to be the issue here.

This check was added in this PR : #45569

I don't see the usefulness of this check here.
The 'lock.factory.abstract' is created in the Framework Bundle and the Security Bundle can't see it at this stage.

From my understanding we can either discard this change and let an error pop later saying that this service does not exist
or move this check in a CompilerPass. (I personally would go for the first solution)

How to reproduce

Using PHP 8.2.7

symfony new --webapp demo
composer require symfony/rate-limiter
composer require symfony/lock
### src/Controller/DemoController.php
<?php

namespace App\Controller;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

class DemoController
{
    #[Route('/demo')]
    public function index(): Response
    {
        return new Response();
    }
}
### security.yaml
security:
    password_hashers:
        Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface: 'auto'
    providers:
        users_in_memory: { memory: null }
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            lazy: true
            provider: users_in_memory
            login_throttling:
                max_attempts: 5
                interval: '15 minutes'
                lock_factory: lock.default.factory

    access_control:
        - { path: ^/demo, roles: ROLE_ADMIN }
### lock.yaml
framework:
    lock: '%env(LOCK_DSN)%'

Possible Solution

No response

Additional Context

No response

@xabbuh
Copy link
Member

xabbuh commented Aug 11, 2023

PR welcome I guess :)

@BaptisteContreras
Copy link
Contributor Author

Great ! I'll do that :)

@fabpot fabpot closed this as completed Aug 12, 2023
fabpot added a commit that referenced this issue Aug 12, 2023
… (BaptisteContreras)

This PR was merged into the 6.3 branch.

Discussion
----------

[Security] Fix error with lock_factory in login_throttling

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #51347
| License       | MIT
| Doc PR        |

Remove incorrect check for Lock configuration in **Security Bundle** which leads to an exception when using lock_factory of the **login_throttling** (as explained in #51347)

Commits
-------

a22e891 [Security] Fix error with lock_factory in login_throttling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants