Improve logging of the user when he logged in programmatically

[ste93cry]

Fixes #706. I had no time to test it on a real project, so please do it if you can before merging 🙏

[Jean85]

Overall a good PR, with a great set of complete and well described tests. Just a few nitpick.

[Jean85]

I'm not sure about this requirement. And to be frank, even the previous one could be invasive in some edge cases...

Maybe it's out of scope for this PR, but I think we should move these two requirements to require-dev, and make the code that uses them optional.

[ste93cry]

I agree, definitely. However, making the code optional is a completely different topic that I don't want to tackle here. We need the security-http package mostly because of the event classes that are part of it IIRC.

[ste93cry]

@cleptric can I ask for your review on this?

[cleptric]

Yep, can take a look tomorrow/next week.

[cleptric]

Do we need to listen to an additional event?

The user ID is only set on the scope during a request that contains credentials, all subsequent requests to a route guarded with access_control will only set the user's IP on the scope.

[ste93cry]

I found out that the RequestListener listener was overwriting the entire user context set on the scope. Since the LoginListener probably ran earlier than that, it may have been the cause of seeing only the user IP address for all subsequent requests. I still have some doubts about the fact that we need to listen on the kernel.request event to gather the user data from the token storage though because I'm not sure that the LoginSuccessEvent event is dispatched on every request...

[cleptric]

Both LoginListener::handleLoginSuccessEvent and LoginListener::handleAuthenticationSuccessEvent seem not to be called on each request. In my controller, $this->security->getUser() does return the logged-in user. The event sent to Sentry only contains the IP.

[ste93cry]

Do you mind sharing the version of the framework you're using and the security.yaml file you're using in your test project? In mine, I configured it like this and it seems to work fine:

security:
    password_hashers:
        Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface: 'plaintext'

    providers:
        users_in_memory:
            memory:
                users:
                    john: { password: 'doe', roles: ['ROLE_USER'] }

    firewalls:
        main:
            provider: users_in_memory
            http_basic:
                realm: Secured Area

    access_control:
        - { path: ^/, roles: ROLE_USER }

Note that with this config, even though I'm using HTTP basic authentication, a session is created so that the browser does not ask me the credentials at every page refresh

[cleptric]

security:
    # https://symfony.com/doc/current/security.html#registering-the-user-hashing-passwords
    password_hashers:
        Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface: 'auto'
    # https://symfony.com/doc/current/security.html#loading-the-user-the-user-provider
    providers:
        # used to reload user from session & other features (e.g. switch_user)
        app_user_provider:
            entity:
                class: App\Entity\User
                property: email
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            lazy: true
            provider: app_user_provider

            form_login:
                # "app_login" is the name of the route created previously
                login_path: login
                check_path: login

            logout:
                path: logout
                # where to redirect after logout
                target: /

            # activate different ways to authenticate
            # https://symfony.com/doc/current/security.html#the-firewall

            # https://symfony.com/doc/current/security/impersonating_user.html
            # switch_user: true

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        - { path: ^/sentry, roles: ROLE_USER }

when@test:
    security:
        password_hashers:
            # By default, password hashers are resource intensive and take time. This is
            # important to generate secure password hashes. In tests however, secure hashes
            # are not important, waste resources and increase test times. The following
            # reduces the work factor to the lowest possible values.
            Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface:
                algorithm: auto
                cost: 4 # Lowest possible value for bcrypt
                time_cost: 3 # Lowest possible value for argon
                memory_cost: 10 # Lowest possible value for argon

[ste93cry]

I can't replicate the problem with your setup either. There are still some minor differences between my setup and yours, e.g. the user provider and password hasher, but it shouldn't matter at all. I'm using Symfony 6.3, maybe you are using a different version and some behaviour changed? Otherwise, can you please share a repo reproducing the issue?

[cleptric]

https://github.com/cleptric/symfony - Most logic is in the SentryController.

[ste93cry]

I have no clue on why it was working in my test project, but indeed the framework does not dispatch the two login-related events for each request. I readded back a listener for the request.kernel event and it now works as expected.

[cleptric]

Looking good!

@Jean85 can you take another look, please?

stale