[SecurityBundle] Add a login form Type #20426

julienfalque · 2016-11-06T15:40:16Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Add a UserLoginType (actually the one that already exists in tests) to ease creating simple login forms.

linaori · 2016-11-06T19:41:44Z

src/Symfony/Bundle/SecurityBundle/Form/Type/UserLoginType.php

@@ -29,11 +28,11 @@
 */
 class UserLoginType extends AbstractType


I would rather have this called a UsernamePasswordType, but tbh, I think this is something the developer should handle him/herself, because in the end, you don't do anything with the form as the security component handles it.

I changed the name.
Despite the Security component handling the form request, I think this can be useful to ease rendering the form, as it will use form themes and automatically retrieve the authentication error.

linaori · 2016-11-07T07:45:53Z

Personally I would like to support the form mapping completely in the security component (optional), you could easily extend the form and add an extra field or flag.

julienfalque · 2016-11-07T11:37:55Z

I'm not sure I understand. Do you suggest to move the Type class to the Security component, e.g. Symfony\Component\Security\Http\Type namespace?

linaori · 2016-11-07T11:45:47Z

Possibly, or a FormBridge in the security component. A form type is to map data to forms, but why read it manually from the request afterwards? Why not simply allow a plug & play variant to handle that form and get the data from there?

The data used in the form could then easily be passed to the right method so you could load user by username with an object instead (think company + username as login).

ro0NL · 2016-11-08T13:58:17Z

src/Symfony/Bundle/SecurityBundle/Form/Type/UsernamePasswordType.php

                $event->getForm()->addError(new FormError($error->getMessage()));
            }

            $event->setData(array_replace((array) $event->getData(), array(
-                'username' => $request->getSession()->get(Security::LAST_USERNAME),
+                '_username' => $this->authenticationUtils->getLastUsername(),


This is the same as passing an option right?

->add('_username', 'Symfony\Component\Form\Extension\Core\Type\TextType', [ 'data' => $this->authenticationUtils->getLastUsername() ]);

That's how we did it.. and it works great :)

@ro0NL : That's not exactly the same. Using data will lock the data and any event listener on PRE_SET_DATA would not be able to change it anymore.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L323

If this form type is meant to be extended (using getParent()) to provide more features in the final application, it must be considered.

ro0NL · 2016-11-08T18:21:00Z

src/Symfony/Bundle/SecurityBundle/Form/Type/UsernamePasswordType.php

-
-            if ($error) {
+        $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) {
+            if (null !== $error = $this->authenticationUtils->getLastAuthenticationError()) {
                $event->getForm()->addError(new FormError($error->getMessage()));


Would be nice if these are translated in the current domain :) And not sure.. but i think in that case you should use $error->getMessageKey().

ro0NL · 2016-11-08T18:28:59Z

src/Symfony/Bundle/SecurityBundle/Form/Type/UsernamePasswordType.php

@@ -42,31 +41,23 @@ public function __construct(RequestStack $requestStack)
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
-            ->add('username', 'Symfony\Component\Form\Extension\Core\Type\TextType')
-            ->add('password', 'Symfony\Component\Form\Extension\Core\Type\PasswordType')
+            ->add('_username', 'Symfony\Component\Form\Extension\Core\Type\TextType')


Should specific field options be configurable?
Ie. add($options['username_field_name'], $options['username_field_type'], $options['username_field_options'])

To clarify, our login was email/password based and field names are configured via security (username/password without leading _).

~~👎 for having custom type and options, this overcomplicates things.~~

However I wonder if we can take advantage of the new FirewallConfig class to get the right field names dynamically instead of hardcoding _username, _password and _csrf_token values in the type, that would be cool!

Agree.. but if we cant do EmailType instead of TextType and assign a label or something im not sure how convenient this actually is. Ie. i think i prefer having this definition in the application layer anyway (like tests did before).

Hmm having an EmailType as you suggest is a good use case to me.

However I wonder if we can take advantage of the new FirewallConfig class to get the right field names dynamically instead of hardcoding _username, _password and _csrf_token values in the type, that would be cool!

That's probably too much for the need IMHO.
This will probably require defining parameters in the container from the Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory, inject the FirewallMap inside the UserLoginType along with the RequestStack, retrieve the firewall name and guess the right parameters to use from it. 😕

Also this would tied the new form type to the SecurityBundle.
So @iltar 's suggestion could not be applied.

As i literally built this today, i think a cookbook example (How to built a login form type with security) would have been most useful. As this is mostly is about explaining howto integrate it with the security component and what the caveats are.

Actually I think you're right @ro0NL. I don't see much value having this form type in the core.

I kind of like the feature giving any form login controller the ability to do
$loginForm = $this->createLoginForm() and having the abstract to do
$this->createForm(UsernamePasswordType::class), some options could also be injected at this step.

It heavily relies on the parameter configuration in security, you probably end up with createNamedForm('', UsernamePasswordType::class), again, depending on your security configuration.

So i think this is only viable, if a) it's fully driven by security (it just works) and b) it's customizable.

Did i mention remember me services already? Imo. this should add a checkbox if needed...

tkleinhakisa · 2016-11-26T13:06:08Z

Hello,

I was wandering about this issue lately. My idea for handling this case was not to provide a form type in the framwork (a default one can still be provided) but to provide a new listener for the firewall to handle a form (something like UsernamePasswordFormAuthenticationListener but working with a form rather than the request)

The user could provide it's form type class in the config (or use a default form type like in this pr)
The listener would submit the form and if it's valid extract username and password and send them to the authenticationManager. If it's not valid, render the login form with the errors (i think this would require login_path and login_check to be the same)

What do you think ?

ro0NL · 2016-11-26T15:40:34Z

I think it's the better approach indeed 👍

i think this would require login_path and login_check to be the same

Not sure it must be a requirement, as the form error comes from the session storage anyway.

julienfalque · 2016-11-26T20:16:01Z

If it's not valid, [the listener would] render the login form with the errors

I don't think this is a good idea: you would have to configure the template used to render the error page. What if you need some custom logic then? What if the template requires additional data normally provided by a controller?

A form type can be useful for rendering the login and error pages because we can map security errors to it and use form themes. Using it to validate the request has no real added value IMO.

julienfalque · 2016-11-26T22:20:06Z

src/Symfony/Bundle/SecurityBundle/Form/Type/UsernamePasswordType.php

+        ));
+    }
+
+    private function getFirewallOption($firewallName, $optionName, $default)


This is the part where I need something like #20591.

tkleinhakisa · 2016-11-27T00:04:29Z

The main idea was to have the listener let your login action do it's rendering when there are form errors, and that's why i suggested the login_path and login_check should be the same.

On GET request matching the login_path (= login_check) the listener creates the form and make it accessible from a controller (maybe through authenticationUtils ?)
On POST request matching the login_path the listener creates the form, submit it and if it's not valid let the login action do the rendering (the listener would not render the form itself)

A form type can be useful for rendering the login and error pages because we can map security errors to it and use form themes. Using it to validate the request has no real added value IMO.

Maybe i missed something but I found it difficult to add a simple email validation to my username field and notblank to the password field. It seems a common use case.

Anyway the listener could be configured to default to the form type of this PR so it could be really easy to setup and configure for default setup.

@HeahDude @javiereguiluz I don't want to pollute this PR with non related things, so maybe it should be worth re-opening #20645 ?

julienfalque · 2018-01-03T20:55:23Z

For people still interested in a login form type, I published a bundle for that: https://github.com/julienfalque/login-form-bundle.

carsonbot added Status: Needs Review Feature labels Nov 6, 2016

linaori reviewed Nov 6, 2016

View reviewed changes

javiereguiluz added the Form label Nov 6, 2016

julienfalque added 2 commits November 6, 2016 23:04

Make test login form available as a regular form type

c9eae9d

Retrieve error and last username from dedicated service

b380f66

julienfalque force-pushed the user-login-form branch from a8b7a36 to b380f66 Compare November 6, 2016 22:07

ro0NL reviewed Nov 8, 2016

View reviewed changes

julienfalque changed the title ~~Add a login form Type~~ [SecurityBundle] Add a login form Type Nov 17, 2016

julienfalque mentioned this pull request Nov 22, 2016

[SecurityBundle] Add a FirewallConfigRegistry service as main entry point #20591

Closed

HeahDude mentioned this pull request Nov 26, 2016

[Security] Integration with form for login #20645

Closed

julienfalque added 2 commits November 26, 2016 21:31

Add options for field names

24092b5

Add target_firewall option

e4bf5b5

julienfalque commented Nov 26, 2016

View reviewed changes

Add shortcut method to create a login form

dcca8a4

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

julienfalque closed this Mar 1, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

julienfalque deleted the user-login-form branch January 3, 2018 20:55

Koc mentioned this pull request Feb 11, 2018

[FrameworkBundle] [SecurityBundle][WIP] Added LoginType #26135

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecurityBundle] Add a login form Type #20426

[SecurityBundle] Add a login form Type #20426

julienfalque commented Nov 6, 2016

linaori Nov 6, 2016 •

edited

Loading

julienfalque Nov 6, 2016

linaori commented Nov 7, 2016

julienfalque commented Nov 7, 2016

linaori commented Nov 7, 2016

ro0NL Nov 8, 2016

ogizanagi Nov 8, 2016 •

edited

Loading

ro0NL Nov 8, 2016

ro0NL Nov 8, 2016

HeahDude Nov 8, 2016 •

edited

Loading

ro0NL Nov 8, 2016

HeahDude Nov 8, 2016

ogizanagi Nov 8, 2016 •

edited

Loading

ogizanagi Nov 8, 2016

ro0NL Nov 8, 2016 •

edited

Loading

ogizanagi Nov 8, 2016 •

edited

Loading

HeahDude Nov 8, 2016

ro0NL Nov 8, 2016

tkleinhakisa commented Nov 26, 2016

ro0NL commented Nov 26, 2016

julienfalque commented Nov 26, 2016

julienfalque Nov 26, 2016

tkleinhakisa commented Nov 27, 2016

julienfalque commented Jan 3, 2018

		@@ -29,11 +28,11 @@
		*/
		class UserLoginType extends AbstractType

[SecurityBundle] Add a login form Type #20426

[SecurityBundle] Add a login form Type #20426

Conversation

julienfalque commented Nov 6, 2016

linaori Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Nov 7, 2016

julienfalque commented Nov 7, 2016

linaori commented Nov 7, 2016

Choose a reason for hiding this comment

ogizanagi Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

ogizanagi Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkleinhakisa commented Nov 26, 2016

ro0NL commented Nov 26, 2016

julienfalque commented Nov 26, 2016

Choose a reason for hiding this comment

tkleinhakisa commented Nov 27, 2016

julienfalque commented Jan 3, 2018

linaori Nov 6, 2016 •

edited

Loading

ogizanagi Nov 8, 2016 •

edited

Loading

HeahDude Nov 8, 2016 •

edited

Loading

ogizanagi Nov 8, 2016 •

edited

Loading

ro0NL Nov 8, 2016 •

edited

Loading

ogizanagi Nov 8, 2016 •

edited

Loading