avanzu / AdminThemeBundle

Admin Theme based on the AdminLTE Template for easy integration into symfony
MIT License
281 stars 149 forks source link

Dicussion - login form with default Symfony security components #214

Open kevinpapst opened 6 years ago

kevinpapst commented 6 years ago

Hi there,

I switched to the bundled login form today (was previously using a plain bootstrap login form) and stumbled upon some problems which I want to share for discussion.

First of all, I am using the default login form security implementation of symfony 3.4. A snippet from my security.yml:

    firewalls:
        secured_area:
            pattern: ^/
            anonymous: ~
            form_login:
                check_path: security_login
                login_path: security_login
                csrf_token_generator: security.csrf.token_manager
            logout:
                path: security_logout
                target: homepage

Here is my Security Controller action (second route only for demo purpose here):

    /**
     * @Route("/login", name="security_login")
     * @Route("/login", name="login_form")
     */
    public function loginAction()
    {
        $helper = $this->get('security.authentication_utils');

        return $this->render('security/login.html.twig', [
            'last_username' => $helper->getLastUsername(),
            'error' => $helper->getLastAuthenticationError(),
        ]);
    }

Now, when using my own layout and use {% extends 'AvanzuAdminThemeBundle:layout:login-layout.html.twig' %} I run into the following issues.

Route setup: I couldn'r setup a route for the default call {{ path('login'|route_alias) }} I tried a couple of things, but only two make sense to me.

security_login:
    path: /{_locale}/login
    options:
        avanzu_admin_route: login

But that seems to conflicts with the symfony implementation, a login is not happening.

login_form:
    path: /{_locale}/login
    options:
        avanzu_admin_route: login

That works, but I have to have the /login path in three places (config and twice in the Controller annotation).

Missing CSRF token support If I setup the form with a route config that works, it still doesn't login due to the missing CSRF form token: <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}"/>

Questions My solution for both problems was to add my own form by using the block {% block avanzu_login_form %} (stripped down for demo)

{% block avanzu_login_form %}
    <form action="{{ path('security_login') }}" method="post">
        <div class="form-group has-feedback">
            <input type="text" name="_username" class="form-control" placeholder="{{ 'security.label.username'|trans }}" value="{{ last_username }}">
            <span class="glyphicon glyphicon-envelope form-control-feedback"></span>
        </div>
        <div class="form-group has-feedback">
            <input name="_password" type="password" class="form-control" placeholder="{{ 'security.label.password'|trans }}">
            <span class="glyphicon glyphicon-lock form-control-feedback"></span>
        </div>
        <div class="row">
            <div class="col-xs-8">
            </div>
            <div class="col-xs-4">
                <button type="submit" class="btn btn-primary btn-block btn-flat">{{ 'security.action.sign_in'|trans }}</button>
            </div>
        </div>
        <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}"/>
    </form>
{% endblock %}

But wouldn't it be nice if the login would work out-of-the-box with the default security mechanism? Therefor we would need a new option to setup the login route without an alias, but as theme_context option for example.

And we could add another option for the CSRF token field or just always add the field to the form.

What do you think about that @shakaran ?

shakaran commented 6 years ago

@kevinpapst the route_alias thing was an unfinished implement thing by the previous author. The other things mentioned, I have to check it the code first and in deep the issue. So leaving as pending for now

kevinpapst commented 6 years ago

I would suggest (as 2.0 is ahead and BC breaks are still possible) to go for highest-compatibility with the symfony best practices. That is enabling full support for the default settings after using a "composer create-project" or following the official cookbook or even simpler compare it with the Symfony demo application (or use flex to create a new symfony 4 project). If someone opts-out of the default configuration thats fine and we should support that with clever config options as well, but that person should know what he is doing and that there might be additional work to make it compatible. But for Symfony newbies like me it feels way better to have a working out-of-the-box solution without the need to debug and customize so much.

That could easily be achieved by:

@shakaran Let me know if I can help in any way! And thanmks for your work here :-)