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

Dicussion - login form with default Symfony security components #214

Open
kevinpapst opened this issue Jan 8, 2018 · 2 comments
Open

Dicussion - login form with default Symfony security components #214

kevinpapst opened this issue Jan 8, 2018 · 2 comments
Assignees
Milestone

Comments

@kevinpapst
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Contributor Author

kevinpapst commented Jan 12, 2018

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:

  • using security_login as route for the form
  • or using a new config setting in the avanzu_theme_context if you want a dynamic route eg. for the demo mode
  • adding the csrf token with the hidden input type

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

@shakaran shakaran added this to the Future milestone Jan 17, 2018
@shakaran shakaran self-assigned this Jan 17, 2018
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

2 participants