keeping aliens at bay with maker + new security 5.2 features #736

jrushlow · 2020-11-13T14:45:06Z

Hello Security51! Allows make:auth to take advantage of the new security features that were introduced. Starting in Symfony 5.2 when you run make:auth MakerBundle will automatically check if you have set:

security:
    enable_authenticator_manager: true

If so, MakerBundle will generate the required classes to authenticate users leveraging the new Authenticators.

A positive side effect of this PR, all templates can check $use_typed_properties to determine if the host is capable of utilizing typed properties that were introduced in PHP 7.4.

e.g. -

private <?= $use_typed_properties ? 'UrlGeneratorInterface ' : null ?>$urlGenerator;

Internally, we've added the PhpCompatUtil::canUseTypedProperties() method that is called anytime a Maker needs to generate a twig template. We then inject $use_typed_properties into all templates so the developer can add typed properties to a generated template without having to worry about a bunch of behind the scenes logic.

wouterj

Looking good! Just a very minor comment for now.

You probably know already, but the EmptyAuthenticator.tpl.php should also get a 5.1 version.

src/Maker/MakeAuthenticator.php

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

maxhelias · 2020-11-14T10:42:34Z

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

+
+    protected function getLoginUrl(Request $request): string
+    {
+        return $this->urlGenerator->generate(self::LOGIN_ROUTE);


I love the use of const but for the router it breaks the discoverability in the IDE (PhpStorm + Plugin Symfony).
Shouldn't we make exceptions for some use cases?

jrushlow · 2020-11-15T18:08:46Z

@jrushlow TODO:

make:user INPUT (User, yes, email, yes)
make:auth ===

~~Gracefully handle security features not permitted w/ security 51~~ not maker related

jrushlow · 2020-11-16T01:20:38Z

src/Maker/MakeAuthenticator.php

                'user_fully_qualified_class_name' => trim($userClassNameDetails->getFullName(), '\\'),
+                'user_repository_fully_qualified_class_name' => $userRepoFullyQualifiedName,


The idea is to use the UserRepository::class created by make:user directly instead of leaning on an EntityManagerInterface instance to get the user as we currently do with the "old" login form authenticator. This of course introduces its own set of challenges if a UserRepository (or whatever the name of the repository is) does not exist. Thoughts on moving in this direction? (The actual implementation of this is incomplete as I have not gracefully handled failover yet)

Granted, I have not taken into account the new features in 5.2 that @wouterj mentioned w/ the lazy user loading at the time of this comment.

Yea, I would prefer to inject the repository class if there is a custom repository class. But, if we do that, we will need to:

A) Read the Doctrine metadata to see if there is a custom repository (and get its class name if there is one).
B) If there is not one, fallback to using EntityManagerInterface.

Needing to do (B) is kind of annoying... but it also seems aggressive to throw an error if the user doesn't have a custom UserRepository. So, probably we should do it / try it.

I'm not very sure about the policy of MakerBundle, but wouldn't it be possible to only support the experimental authenticator maker in Symfony 5.2? That would make this logic a lot easier, as this logic is done by the Security system. All that's needed is to pass the username to the password and configure the user provider (as is done in the "legacy" part of the maker).

@wouterj ya you're correct, thats what im planning on doing for 5.2. But where it gets tricky is in 5.1 - i started working on implementing this way back when and time came hard to come by. So now we're in a situation where <= 5.0 we have the original make:auth, === 5.1 we'll have this implementation as it written now, and in >= 5.2 we'll use those new features. Hopefully >5.3 there wont be any significant changes.. But in the meantime I think this is where we're at with this.

when we merge this the goal is to have make:auth work in all 3 scenerios

I'm with ya - I am chewing on the idea myself.. I would almost guarantee if we did skip support in 5.1, someone would open an issue in the coming months about it. That aside -

Pro's of skipping 5.1 support:

easier to maintain in the future

quicker to get out the door now

Cons:

Loosing out on the ability to coin Security51 (Pop culture reference to Area 51 here in the states).

Potentially cutting off / alienating a segment of "less experienced" users who are stuck on 5.1 for whatever reason in the future.

walking back the functionality that I've written to support 5.1 then saying we didn't want to support this functionality because it makes life easier.

That last 2 cons are the biggest things holding me back from coming out and saying yup, lets skip it. But that is not to say I'm not completely against the idea considering it would make life easier going forward.

Curious as to what others think as well.

Supporting only 5.2 would be very cool for me

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

src/Maker/MakeAuthenticator.php

wouterj · 2020-11-23T11:28:15Z

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

+    public function supports(Request $request): ?bool
+    {
+        return self::LOGIN_ROUTE === $request->attributes->get('_route')
+            && $request->isMethod('POST');
+    }


💡 No review comment, but DX idea: we can make this the default implementation in AbstractFormLoginAuthenticator. If someone needs a different support call, they can always override but this implementation would cover like 80% of all login forms, wouldn't it?

I like it, to go one step further on that - what if we had security.firewalls.Xmain.login.path = some_route? this would allow us to do away with the constant and make it easier to change the login route for the majority of use cases. Thoughts?

abstract support added in symfony/symfony#39213

removed from template

wouterj · 2020-11-23T11:28:55Z

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

+            new UserBadge($userIdentifier),
+            new PasswordCredentials($password),
+            [
+                new PasswordUpgradeBadge($password),


💡 Also no review comment: didn't we decide to automatically upgrade passwords if UserBadge has a password upgrading user provider and PasswordCredentials is given, @weaverryan?

I just pinged you on this on Slack. I think it IS still required, though maybe it shouldn't be in a perfect world.

no longer needed symfony/symfony#39213 thanks for that @wouterj

removed badge in the template

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

src/Security/SecurityConfigUpdater.php

@weaverryan

… + default support() impl in AbstractFormLoginAuthenticator (wouterj) This PR was squashed before being merged into the 5.2 branch. Discussion ---------- [Security] [DX] Automatically add PasswordUpgradeBadge + default support() impl in Ab A3D4 stractFormLoginAuthenticator | Q | A | ------------- | --- | Branch? | 5.2 (hopefully? sorry to keep pushing the barrier here) | Bug fix? | no | New feature? | yes (sort of) | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - These are 2 suggestions we found while implementing `make:auth` for the new system (symfony/maker-bundle#736): Impact on a custom login form authenticator ([as generated by the new maker](https://github.com/symfony/maker-bundle/pull/736/files#diff-528164b6c24778d5e81fa3819b0552f0e68a9fea33c7d3446a012f3da7d0af60)): * **Automatically add `PasswordUpgradeBadge`** if there is a user password with valid password credentials. ```diff // ... return new Passport( new UserBadge($userIdentifier), new PasswordCredentials($password), [ - new PasswordUpgradeBadge($password), new CsrfTokenBadge('authenticate', $csrf), ] ) ``` Note that this does not automatically migrate all passwords: it still relies on `PasswordUpgraderInterface` to be implemented on the user loader/provider. * **Add default implementation of `AbstractFormLoginAuthenticator::support()`** ```diff - public function supports(Request $request): ?bool - { - return self::LOGIN_ROUTE === $request->attributes->get('_route') - && $request->isMethod('POST'); - } ``` cc @weaverryan @jrushlow Commits ------- 27450c0 [Security] [DX] Automatically add PasswordUpgradeBadge + default support() impl in AbstractFormLoginAuthenticator

jrushlow · 2020-12-04T19:20:37Z

coding standards are failing due to unrelated sprintf() usage. fix is available in #757

weaverryan

Very nice work - down to some last details!

src/Maker/MakeAuthenticator.php

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

weaverryan · 2020-12-07T14:30:41Z

src/Security/SecurityConfigUpdater.php

            $newData['security']['firewalls'][$firewallName] = ['anonymous' => true];
+        } elseif (!isset($newData['security']['firewalls'][$firewallName]) && $useSecurity51) {
+            $newData['security']['firewalls'][$firewallName] = ['lazy' => true];
        }


Logically it might read better as:

if (!isset($newData['security']['firewalls'][$firewallName])) { if ($useSecurity51) { } else { } }

Also, it's minor, but I think we were already missing that lazy should be included in ALL firewalls, at least back to Symfony 4.4. And... I guess we should check the version of Symfony to see if lazy exists? Or, we (in a separate PR) could drop support for 3.4 - bump it to 4.4, as 3.4 is no longer supported.

lazy: true is introduced in 5.1, it used to be anonymous: lazy. If I read correctly, the 4.4 code uses anonymous: true, so is there a specific reason to enable lazy firewalls in 5.2?

src/Security/SecurityConfigUpdater.php

weaverryan

We're very close now! Nice work on this complex feature!

src/Maker/MakeAuthenticator.php

src/Security/SecurityConfigUpdater.php

weaverryan · 2020-12-17T19:56:40Z

Thanks for the hard work Jesse!

wouterj · 2020-12-17T20:19:22Z

Great!!! Thanks @jrushlow and @weaverryan!

jrushlow added the Status: Needs Work Additional work is needed label Nov 13, 2020

wouterj reviewed Nov 14, 2020

View reviewed changes

src/Maker/MakeAuthenticator.php Outdated Show resolved Hide resolved

src/Resources/skeleton/authenticator/Security51LoginFormAuthenticator.tpl.php Outdated Show resolved Hide resolved

maxhelias reviewed Nov 14, 2020

View reviewed changes

jrushlow commented Nov 16, 2020

View reviewed changes

weaverryan changed the base branch from master to main November 16, 2020 20:00

jrushlow commented Nov 18, 2020

View reviewed changes

src/Resources/skeleton/authenticator/Security51LoginFormAuthenticator.tpl.php Outdated Show resolved Hide resolved

jrushlow commented Nov 18, 2020

View reviewed changes

src/Resources/skeleton/authenticator/Security51LoginFormAuthenticator.tpl.php Outdated Show resolved Hide resolved

jrushlow force-pushed the feature/new-security branch 2 times, most recently from 983980e to d9c29d8 Compare November 22, 2020 16:38

jrushlow changed the title ~~WIP - keeping aliens at bay with maker + new security 5.1 features~~ keeping aliens at bay with maker + new security 5.1 features Nov 22, 2020

jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Nov 22, 2020

wouterj reviewed Nov 23, 2020

View reviewed changes

wouterj mentioned this pull request Nov 28, 2020

[Security] [DX] Automatically add PasswordUpgradeBadge + default support() impl in AbstractFormLoginAuthenticator symfony/symfony#39213

Merged

jrushlow force-pushed the feature/new-security branch 2 times, most recently from c7c5027 to 596b9ff Compare December 3, 2020 19:30

jrushlow mentioned this pull request Dec 3, 2020

fix indentation when adding new key after multiline array #753

Closed

jrushlow added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Dec 3, 2020

jrushlow added Related Tests Pass Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Dec 4, 2020

weaverryan reviewed Dec 7, 2020

View reviewed changes

jrushlow force-pushed the feature/new-security branch from 7613cf2 to ea3e16d Compare December 10, 2020 15:46

jrushlow changed the title ~~keeping aliens at bay with maker + new security 5.1 features~~ keeping aliens at bay with maker + new security 5.2 features Dec 10, 2020

jrushlow mentioned this pull request Dec 14, 2020

[WIP] Feat: Type properties with PHP >= 7.4 #689

Closed

weaverryan reviewed Dec 15, 2020

View reviewed changes

jrushlow force-pushed the feature/new-security branch from d9b7a29 to f69fc99 Compare December 17, 2020 16:02

keeping aliens at bay with maker + new security 5.2 features

820ee25

weaverryan force-pushed the feature/new-security branch from 7a5af49 to 820ee25 Compare December 17, 2020 19:56

weaverryan merged commit fba06d8 into symfony:main Dec 17, 2020

jrushlow deleted the feature/new-security branch March 3, 2021 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keeping aliens at bay with maker + new security 5.2 features #736

keeping aliens at bay with maker + new security 5.2 features #736

		'user_fully_qualified_class_name' => trim($userClassNameDetails->getFullName(), '\\'),
		'user_repository_fully_qualified_class_name' => $userRepoFullyQualifiedName,

keeping aliens at bay with maker + new security 5.2 features #736

keeping aliens at bay with maker + new security 5.2 features #736

Conversation

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

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