Make login form #266

nikophil · 2018-09-11T17:31:05Z

Improvements for the command make:auth in order to create a form login.
Now, the make:auth command asks if the user wants an empty authenticator or a form login.
If "form login" is chosen, the user is asked for the security controller name, and for the user class, if needed.
If the controller already exists, the command adds the login / logout methods in the existing controller.

weaverryan

Phew! Big work! I'm very excited about this! Though, I'll admit, it's even more complex than I was thinking - probably the most complex maker yet. Some comments to get things rolling!

weaverryan · 2018-09-14T02:49:34Z

src/Maker/MakeAuthenticator.php


-        $manipulator = new YamlSourceManipulator($this->fileManager->getFileContents($path));
+        $manipulator = new YamlSourceManipulator($this->fileManager->getFileContents('config/packages/security.yaml'));


What if this doesn't exist? Should be a huge edge-case - the dependencies should already be checked by now. Maybe we just throw an exception?

weaverryan · 2018-09-14T02:52:20Z

src/Maker/MakeAuthenticator.php

-        $command->addOption('entry-point', null, InputOption::VALUE_OPTIONAL);
+        if (self::AUTH_TYPE_FORM_LOGIN === $input->getArgument('authenticator-type')
+            && !isset($securityData['security']['providers']) || !$securityData['security']['providers']) {
+            throw new RuntimeCommandException('You need to have at least one provider defined in security.yaml');


To generate a form login authentication, you must configure at least one entry under providers in security.yaml.

I'm just making it a bit more clear. There is actually a case (stateless API) where you do NOT need to have a user provider (having it doesn't hurt, but it literally would never be called). So, I want to be clear that WE think you need one for this type of authenticator (because a form login would never be "stateless").

And, could we move this down to right before we call InteractiveSecurityHelper::guessUserClass()? Up here, it feels a bit unrelated to anything. Also, let's make InteractiveSecurityHelper::guessUserClass() accept only the providers config, instead of all of the config. It will all just read a bit better: we (1) do the above check to make sure it exists, and throw an exception if it doesn't/is empty. THEN we pass just this for-sure-not-empty providers array into InteractiveSecurityHelper::guessUserClass.

This is pretty similar than the next comment you've made : once the user has chosen "form login", we already know that we should raise an exception, but, if i put it down before InteractiveSecurityHelper::guessUserClass(), we're asking 4 questions before raising the exception... i think this could be annoying for the user.
What's your feeling about that ? - but i'm ok that in the code it seems unrelated to anything... at the beginning this test was right before guessUserClass() call...

ok for other comments

weaverryan · 2018-09-14T02:59:30Z

src/Maker/MakeAuthenticator.php

+                        'Security\\'
+                    )->getFullName()
+                );
+            }


Ooof. This is ugly for a check for a common thing :). Ok. Let's keep it. Usually, we just let the generator throw an error later. So, I'm not sure I want to make a habit of doing this. However, because this authenticator asks several other questions after this, it would be super annoying to answer them all, and THEN get a fatal error. So, keep it :).

Maybe i could just not use the question's validator ? but we're losing the behavior when the console asks again the answer if the user has provided an existing class...
(even if i know this edge case will be very uncommon)

weaverryan · 2018-09-14T03:01:27Z

src/Maker/MakeAuthenticator.php

        );
+
+        if (self::AUTH_TYPE_FORM_LOGIN === $input->getArgument('authenticator-type')) {
+            $command->addArgument('controller-class', InputArgument::OPTIONAL);


Optional? I don't think it matters, because we're always passing it. But you use required above - let's be consistent. This looks required :).

weaverryan · 2018-09-14T03:04:51Z

src/Maker/MakeAuthenticator.php

+                    'user_fully_qualified_class_name' => $input->getArgument('user-class'),
+                    'user_class_name' => substr(strrchr($input->getArgument('user-class'), '\\'), 1),
+                ]
+                : []


I found the ternary you were asking about :p. I think an if statement around this entire block is much more clear. Actually, even a switch-case

switch ($input->getArgument('authenticator-type')) { case self::AUTH_TYPE_FORM_LOGIN: $generator->generateClass( // ... ); break; case self::self::AUTH_TYPE_EMPTY_AUTHENTICATOR: $generator->generateClass( // ... ); break; default: throw new \Exception('Invalid authenticator type'); }

ok that's fair ;)

weaverryan · 2018-09-14T03:44:01Z

src/Validator.php

@@ -184,4 +185,26 @@ public static function entityExists(string $className = null, array $entities =

        return $className;
    }
+
+    public static function classDoesNotExist($className)


returns : string

weaverryan · 2018-09-14T03:45:27Z

src/Validator.php

+        return $className;
+    }
+
+    public static function classIsUserInterface($userClassName)


returns : string

weaverryan · 2018-09-14T03:47:45Z

tests/Maker/FunctionalTest.php

+        )
+            ->addExtraDependencies('doctrine')
+            ->addExtraDependencies('security')
+            ->addExtraDependencies('twig')


This extra dependency thing should be rare. doctrine make sense to me - that's not a requirement of the Maker, but we're testing that case. But, shouldn't security be a dependency? I suppose twig is correct to stay here too, because it's only a conditional dependency - i.e. if the choose the "login form" version. Our test system doesn't support those "optional" dependencies. But, we should do a manual check in the maker to make sure they have this if the choose the login-form option. I think I do this in MakeUser - I do a manual check for Doctrine, if they choose the entity option.

weaverryan · 2018-09-14T03:48:30Z

tests/Maker/FunctionalTest.php

+                ->assert(
+                    function (string $output) {
+                        $this->assertContains('Success', $output);
+                    }


I don't think this is necessary. I think we either already check for this always, or, we are at least checking that the process exited successfully, which is basically the same as this.

weaverryan · 2018-09-14T03:51:54Z

tests/Maker/FunctionalTest.php

+            );
+
+        if (Kernel::VERSION_ID < 40100) {
+            $makerTestAuthenticatorLoginFormUserEntity->addExtraDependencies('symfony/form');


I think we could instead add this into the Maker. Then, the user would also see this dependency error. And, I think the tests would also automatically add it, as it WOULD be a dependency when the user is running 4.0.

weaverryan

Not a full review - I was just looking at your replies to my comments and the code associated with them.

weaverryan · 2018-09-14T12:48:22Z

src/Maker/MakeAuthenticator.php

+            $dependencies = new DependencyBuilder();
+            $dependencies->addClassDependency(
+                TwigBundle::class,
+                'tiwg'


oups, sorry for that

src/Maker/MakeAuthenticator.php

src/Generator.php

weaverryan · 2018-09-14T12:55:46Z

src/Maker/MakeAuthenticator.php

+                'authenticator/EmptySecurityController.tpl.php',
+                [
+                    'parent_class_name' => \method_exists(AbstractController::class, 'getParameter') ? 'AbstractController' : 'Controller',
+                ]


About Generator::generateController(), that's a good idea. We would need to allow a custom template name to be passed as an argument. But at least the new Generator::generateController() method could handle this logic, and always pass that parent_class_name. Good idea.

src/Resources/skeleton/authenticator/login_form.tpl.php

weaverryan · 2018-09-14T13:00:31Z

src/Resources/skeleton/authenticator/login_form.tpl.php

+        </div>
+    #}
+
+    <button class="btn btn-lg btn-primary btn-block" type="submit">


Remove btn-block - that only looks right if you have that full login markup/css file I linked to above

weaverryan · 2018-09-14T13:02:38Z

composer.json

        "symfony/framework-bundle": "^3.4|^4.0",
-        "symfony/http-kernel": "^3.4|^4.0"
+        "symfony/http-kernel": "^3.4|^4.0",
+        "symfony/security": "^3.4|^4.1"


symfony/security and symfony/form shouldn't be dependencies. What did you need them for?

weaverryan · 2018-09-14T13:03:06Z

src/Generator.php

        }

        $this->pendingOperations = [];
    }

+    private function generateFileContents(string $targetPath, array $templateData)


weaverryan · 2018-09-20T01:09:31Z

src/Generator.php

@@ -84,6 +85,18 @@ public function dumpFile(string $targetPath, string $contents)
        ];
    }

+    public function getFileContents(string $targetPath): string


Ah, for this, I meant that we should only have a getFileContentsForPendingOperation() - and it would contain the code here + the code that is currently in getFileContentsForPendingOperation(). No need for one called getFileContents() - just one public function called getFileContentsForPendingOperation()

ype, i don't know why i've done this ;)

weaverryan · 2018-09-20T01:10:32Z

src/Maker/MakeAuthenticator.php

            ->setHelp(file_get_contents(__DIR__.'/../Resources/help/MakeAuth.txt'));
    }

    public function interact(InputInterface $input, ConsoleStyle $io, Command $command)
    {
        if (!$this->fileManager->fileExists($path = 'config/packages/security.yaml')) {
-            return;
+            throw new RuntimeCommandException('File "security.yaml" does not exist!');


The file "config/packages/security.yaml" does not exist. This command requires that file to exist so that it can be updated.

weaverryan · 2018-09-20T01:10:55Z

src/Maker/MakeAuthenticator.php

+                TwigBundle::class,
+                'twig'
+            );
+            $missingPackagesMessage = 'Twig must be installed to display login form';


... to display the login form.

weaverryan · 2018-09-20T01:12:26Z

src/Maker/MakeAuthenticator.php

+            $missingPackagesMessage = $dependencies->getMissingPackagesMessage(self::getCommandName(), $missingPackagesMessage);
+            if ($missingPackagesMessage) {
+                throw new RuntimeCommandException($missingPackagesMessage);
+            }


I wonder if we could at least put all of this dependency-builder logic into some protected function in the base Maker so that it's easier to do these manual checks.

weaverryan · 2018-09-20T01:15:03Z

src/Maker/MakeAuthenticator.php

+                    Form::class,
+                    'symfony/form'
+                );
+                $missingPackagesMessage = 'Twig and symfony/form must be installed to display login form';


... to display the login form.

weaverryan · 2018-09-20T01:16:30Z

src/Maker/MakeAuthenticator.php

+                $input->getArgument('authenticator-class'),
+                'authenticator/EmptyAuthenticator.tpl.php',
+                []
+            );


Put a return here and then the rest of the code doesn't need to live in an else.

weaverryan · 2018-09-20T01:17:43Z

src/Maker/MakeAuthenticator.php

            );
            $text[] = "Your <info>security.yaml</info> could not be updated automatically. You'll need to add the following config manually:\n\n".$yamlExample;
        }
        $io->text($text);
    }

-    public function configureDependencies(DependencyBuilder $dependencies)
+    private function generateAuthenticatorClass(InputInterface $input)


It's a nit-pick, but I think we should pass the specific arguments (I think it's 5 in total) that we need here, instead of the $input. It just makes this method a bit simpler.

weaverryan · 2018-09-20T01:18:40Z

src/Maker/MakeAuthenticator.php

+        }
+    }
+
+    private function generateFormLoginFiles(InputInterface $input)


Same here - just pass in the args you need, instead of $input :)

src/Maker/MakeController.php

weaverryan

I just started playing with the command - super fun :). But, I'm out of time at this exact moment :/. One thing that I did notice is the "Next:" message. I think this might need to be a bit more customized for a few cases. The current one probably works for the "empty" authenticator. But if you are making a login form... isn't everything done for you? Except for the onAuthenticationSuccess() redirect? Is there any variance between the other situations? I mean, is this always the only step that a user needs to do, or is there sometimes other stuff? Probably if your user is not an entity... you should at least review getUser().

weaverryan · 2018-09-20T01:31:30Z

src/Maker/MakeAuthenticator.php

+        // authenticator type
+        $authenticatorTypeValues = [
+            'Empty authenticator' => self::AUTH_TYPE_EMPTY_AUTHENTICATOR,
+            'Form login' => self::AUTH_TYPE_FORM_LOGIN,


Login form authenticator

weaverryan · 2018-09-20T01:40:37Z

src/Resources/skeleton/authenticator/LoginFormAuthenticator.tpl.php

+            return new RedirectResponse($targetPath);
+        }
+
+        throw new \Exception('TODO: provide a valid redirection inside '.__FILE__);


We could put some example code here in a comment above:

// return new RedirectResponse($this->router->generate('some_route'));

weaverryan

I think this is the last round of changes :). I played with the command quite a bit - it's a delight. Even if I don't pay attention and run the most complex case (make:user with a model class + make:auth for a login form), each time I refresh, an exception tells me where to go. It's awesome!

weaverryan · 2018-09-21T01:01:03Z

src/Maker/MakeAuthenticator.php

+        }
+
+        if (self::AUTH_TYPE_FORM_LOGIN === $authenticatorType) {
+            $nextTexts[] = sprintf('- You must provide a valid redirection in the method <info>%s::onAuthenticationSuccess()</info>.', $authenticatorClass);


Finish the redirect "TODO" in the %s::onAuthenticationSuccess() method.

weaverryan · 2018-09-21T01:01:42Z

src/Resources/skeleton/authenticator/LoginFormAuthenticator.tpl.php

+            return new RedirectResponse($targetPath);
+        }
+
+        // e.g. : return new RedirectResponse($this->router->generate('some_route'));


// For example: return

weaverryan · 2018-09-21T01:01:58Z

src/Resources/skeleton/authenticator/LoginFormAuthenticator.tpl.php

+        }
+
+        // e.g. : return new RedirectResponse($this->router->generate('some_route'));
+        throw new \Exception('TODO: provide a valid redirection inside '.__FILE__);


... a valid redirect inside ...

weaverryan · 2018-09-21T01:02:29Z

src/Maker/MakeAuthenticator.php

+
+        if (self::AUTH_TYPE_FORM_LOGIN === $authenticatorType) {
+            $nextTexts[] = sprintf('- You must provide a valid redirection in the method <info>%s::onAuthenticationSuccess()</info>.', $authenticatorClass);
+            $nextTexts[] = '- Review & adapt the login template : <info>/templates/security/login.html.twig</info>.';


Review & adapt the login template: templates/security/login.html.twig.

And let's move this down below the next two. That way, if there are multiple things you need to do in the authenticator, we should all of them, and THEN the template text

weaverryan · 2018-09-21T01:03:03Z

src/Maker/MakeAuthenticator.php

+            $nextTexts[] = '- Review & adapt the login template : <info>/templates/security/login.html.twig</info>.';
+
+            if (!$this->doctrineHelper->isClassAMappedEntity($userClass)) {
+                $nextTexts[] = sprintf('- Review <info>%s::getUser()</info>, if it match your needs.', $authenticatorClass);


Review %s::getUser() to make sure it matches your needs.

weaverryan · 2018-09-21T01:05:09Z

src/Maker/MakeAuthenticator.php

+            }
+
+            if (!$this->userNeedsEncoder($securityData, $userClass)) {
+                $nextTexts[] = sprintf('- Check user\'s password in <info>%s::checkCredentials()</info>.', $authenticatorClass);


Check the user's password in %s::checkCredentials().

weaverryan · 2018-09-21T01:06:48Z

tests/fixtures/MakeAuthenticatorLoginFormCustomUsernameField/src/Security/UserProvider.php

+    {
+        return User::class === $class;
+    }
+}


Nit pick - but could we "slim down" some of these fixture classes, so that they only have what we need?

weaverryan · 2018-09-21T01:07:35Z

src/Resources/skeleton/authenticator/LoginFormAuthenticator.tpl.php

+    public function checkCredentials($credentials, UserInterface $user)
+    {
+        <?= $user_needs_encoder ? "return \$this->passwordEncoder->isPasswordValid(\$user, \$credentials['password']);\n"
+        : "// Check the user’s password or other credentials and return true or false


I think the ' in user's is the wrong type of apostrophe?

wow you got god eyes 👍

weaverryan · 2018-09-21T01:10:39Z

src/Maker/MakeAuthenticator.php

        }
-        $io->text($text);
+
+        return $nextTexts;


What about one more on the end?

When you're ready, see your login form by going to login.

weaverryan · 2018-09-21T01:21:13Z

src/Resources/skeleton/authenticator/login_form.tpl.php

+
+    <h1 class="h3 mb-3 font-weight-normal">Please sign in</h1>
+    <label for="inputEmail" class="sr-only">Email address</label>
+    <input type="email" value="{{ last_username }}" name="<?= $username_field; ?>" id="inputEmail" class="form-control" placeholder="Email address" required autofocus>


Ah.... this should only be type="email" if their "username" field contains the term email I think. I chose username for my User class, and so this field should not have been type="email".

Also, though annoying, we should probably make the id="" and label's for="" a bit dynamic - based on the actual identity field name.

AND... of course - the label's text - Email address - should be dynamic - probably we can turn their propery name into title-case words - e.g. emailAddress -> Email Address or email to just Email

ok you're absolutely right

weaverryan · 2018-09-21T17:57:43Z

Wow! Thank you Nicolas! This is really, really cool - and was a lot of work :). As we discussed, there is one new test failing - we'll address it in another PR before we tag.

This PR was squashed before being merged into the 1.0-dev branch (closes #266). Discussion ---------- Make login form Improvements for the command `make:auth` in order to create a form login. Now, the `make:auth` command asks if the user wants an empty authenticator or a form login. If "form login" is chosen, the user is asked for the security controller name, and for the user class, if needed. If the controller already exists, the command adds the login / logout methods in the existing controller. Commits ------- 5257b67 form login : PR review : batch 5 cd86b3b form login : PR review : batch 4 78abbf4 form login : fix after review - batch 3 bc0089d form login : use dynamic username field 388df31 form login : guess if encoder is needed c44f878 fix after review - batch 1 9ec9766 Test logging process with users as entity fd76519 unit tests 7fd5a35 Add tests into fixtures files 02ff624 Functional tests f96c51e Add logout method to already existing SecurityController 0bfd028 Handle when securityController provided already exist 011b1d6 Merge MakeLoginForm into MakeAuthenticator efb19e4 create MakeFormLogin command

nikophil changed the title ~~Make login form (WIP)~~ Make login form Sep 12, 2018

weaverryan mentioned this pull request Sep 14, 2018

[Controller] Adding the --no-annotations option to make:controller #263

Closed

weaverryan requested changes Sep 14, 2018

View reviewed changes

weaverryan reviewed Sep 14, 2018

View reviewed changes

weaverryan requested changes Sep 20, 2018

View reviewed changes

weaverryan reviewed Sep 20, 2018

View reviewed changes

nikophil added 13 commits September 20, 2018 20:37

create MakeFormLogin command

efb19e4

Merge MakeLoginForm into MakeAuthenticator

011b1d6

Handle when securityController provided already exist

0bfd028

Add logout method to already existing SecurityController

f96c51e

Functional tests

02ff624

Add tests into fixtures files

7fd5a35

unit tests

fd76519

Test logging process with users as entity

9ec9766

fix after review - batch 1

c44f878

form login : guess if encoder is needed

388df31

form login : use dynamic username field

bc0089d

form login : fix after review - batch 3

78abbf4

form login : PR review : batch 4

cd86b3b

weaverryan requested changes Sep 21, 2018

View reviewed changes

form login : PR review : batch 5

5257b67

weaverryan merged commit 5257b67 into symfony:master Sep 21, 2018


		$manipulator = new YamlSourceManipulator($this->fileManager->getFileContents($path));
		$manipulator = new YamlSourceManipulator($this->fileManager->getFileContents('config/packages/security.yaml'));

Make login form #266

Make login form #266

Conversation

nikophil commented Sep 11, 2018 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikophil Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Sep 21, 2018

nikophil commented Sep 11, 2018 •

edited

Loading

nikophil Sep 14, 2018 •

edited

Loading