[make:registration] drop guard authentication support #1243

jrushlow · 2022-11-28T17:57:32Z

A wee bit late - but this pr drops the use of the old guard authenticators from 4.x

DX

login after registration:
Previously we asked which firewall to use -> which authenticator, etc... Now we just grab all of the authenticators available and let the user choose in one question.

internally we iterate through all of the security.firewalls to looking for a built in authenticator or a custom_authenticator. If multiple choices are found, we ask the user which one to use. e.g.

If only 1 authenticator was found, obviously don't ask which one to use...

we support

custom_authenticator: App\My\Authenticator

&&

custom_authenticator:
    - App\My\Authenticator

Both of which appear to be valid way to declare a custom authenticator.

Under the hood

an Authenticator value object && a AuthenticatorType enum are introduced to get us away from passing around array's of array's of array's... Goal is to bring better visibility at a glance when reviewing code && get us closer to implementing static analysis without headaches..
we generate the code to login a user by calling Security::login($user, 'name_of_authenticator', 'name_of_firewall') in the template. Most users do not need the 3rd firewall parameter, but I'm pulling the "keep maintainers lives easier by writing less code" card. If this bug's folks, later, we can add in additional logic && conditionals to determine if there are multiple firewalls.

refs: SymfonyCasts/verify-email-bundle#126

tests/fixtures/make-registration-form/expected/RegistrationControllerNoLogin.php

tests/fixtures/make-registration-form/expected/RegistrationControllerFormLogin.php

src/Maker/MakeRegistrationForm.php

src/Security/InteractiveSecurityHelper.php

src/Maker/MakeRegistrationForm.php

Co-authored-by: Ryan Weaver <weaverryan+github@gmail.com>

jrushlow · 2024-02-28T22:54:09Z

src/Security/InteractiveSecurityHelper.php

+                }
+
+                foreach ($configData as $customAuthenticatorClass) {
+                    //                    if (!class_exists($customAuthenticatorClass)) {


I'm not sure if we should check if the authenticator exists and that it implements AuthenticatorInterface::class. Doing so would be a bonus? but it would also require refactoring the test suite to implement a concrete CustomAuthenticatorFixture::class Thoughts?

I'm actually a bit lost in this method. We're looping over the keys under a firewall, right? And the first if statement with tryFrom will exit unless the key matches the specific set we have in AuthenticatorType, right? So then, $configData is represents the values beneath those keys. So if I have:

firewalls: main: form_login: path: /login

Won't $configData at this point be ['path' => '/login']? Or am I totally misreading the method?

weaverryan · 2024-03-01T14:51:26Z

src/Maker/MakeRegistrationForm.php

                    'redirect_route_name' => $this->redirectRouteName,
-                    'password_hasher_class_details' => $generator->createClassNameDetails(UserPasswordHasherInterface::class, '\\'),
+                    'password_hasher_class_details' => $hasherDetails = $generator->createClassNameDetails(UserPasswordHasherInterface::class, '\\'),
+                    'password_hasher_variable_name' => sprintf('$%s', lcfirst($hasherDetails->getShortName())),


Does this stuff still need to be dynamic?

Dunno, I thought it was a good idea 2 years ago :D ~~I'll check haha If it doesn't - ill refactor the template before hitting the big ol merge button~~ nope, i think i was going for "if they use another hasher interface..." but thats their problem to change the hasher signature here...

done

weaverryan · 2024-03-01T14:56:07Z

src/Security/InteractiveSecurityHelper.php

+                }
+
+                foreach ($configData as $customAuthenticatorClass) {
+                    //                    if (!class_exists($customAuthenticatorClass)) {


I'm actually a bit lost in this method. We're looping over the keys under a firewall, right? And the first if statement with tryFrom will exit unless the key matches the specific set we have in AuthenticatorType, right? So then, $configData is represents the values beneath those keys. So if I have:

firewalls: main: form_login: path: /login

Won't $configData at this point be ['path' => '/login']? Or am I totally misreading the method?

jrushlow · 2024-03-01T18:24:13Z

src/Security/InteractiveSecurityHelper.php

+    {
+        $authenticators = [];
+
+        foreach ($firewallConfig as $potentialAuthenticator => $configData) {


ahh... lost the review comment on push...

@weaverryan:
I'm actually a bit lost in this method. We're looping over the keys under a firewall, right? And the first if statement with tryFrom will exit unless the key matches the specific set we have in AuthenticatorType, right? So then, $configData is represents the values beneath those keys. So if I have:

firewalls: main: form_login: path: /login

Won't $configData at this point be ['path' => '/login']? Or am I totally misreading the method?

no I think you're thinking 1 level too deep..

i refactored this to split up iterating over firewalls, the keys of firewalls (main, anotherFirewall), and the custom_authenticators

We pass in security.firewalls -> iterate over each key in firewalls to give us:

main: path: '/some-path' form_login: - option .... custom_authenticators: - some authenticator anotherFirewall: dev: test:

tryFrom($potentialAuthenticator) checks if the "key" is form_login, custom_authenticator, or path. If it is path, tryFrom() returns null and we continue.

Now $potentialAuthenticator is either form_login or custom_authenticator and $configData is the array of either of their stuff..

If its form_login, we create the value object and add it to authenticators[] otherwiser we now know that $potentialAuthenticator === 'custom_authenticator' and configData is either (string) MyCustom or [MyCustom, NotSureWhyYouCanHave2ButYouCan]

Atleast this is what im trying todo, but i could be missing something myself 🤕

Yup, it looks clear now 👍

jrushlow · 2024-03-01T18:36:00Z

src/Security/InteractiveSecurityHelper.php

+     *
+     * @return Authenticator[]
+     */
+    public function getAuthenticatorsFromConfig(array $firewalls): array


getAuthenticatorsInFirewallsConfig?

weaverryan

If things are working, let's roll with this 👍

weaverryan · 2024-03-01T20:37:40Z

src/Security/InteractiveSecurityHelper.php

+    {
+        $authenticators = [];
+
+        foreach ($firewallConfig as $potentialAuthenticator => $configData) {


Yup, it looks clear now 👍

jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels Nov 28, 2022

jrushlow force-pushed the refactor/make-reg branch from 2c2468f to e66b5b9 Compare January 3, 2023 19:00

weaverryan reviewed Jan 10, 2023

View reviewed changes

thobaier mentioned this pull request Feb 15, 2023

make:registration-form is not recognizing configured built-in authenticators for autologin #1289

Closed

jrushlow force-pushed the refactor/make-reg branch from 5c1acfd to 05078c8 Compare February 27, 2024 19:23

jrushlow and others added 6 commits February 28, 2024 14:30

[make:registration] drop guard authentication support

977835c

words mean everything

7c25f0b

Co-authored-by: Ryan Weaver <weaverryan+github@gmail.com>

return auth response instead

13348d5

Co-authored-by: Ryan Weaver <weaverryan+github@gmail.com>

yup - we dont need that anymore

340ab01

manage our expectations

824451f

lets see how many pass with this

ef75722

jrushlow force-pushed the refactor/make-reg branch from 4476787 to ef75722 Compare February 28, 2024 19:30

use built in authenticators for login

97fcbb1

jrushlow commented Feb 28, 2024

View reviewed changes

jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 28, 2024

jrushlow marked this pull request as ready for review February 28, 2024 23:14

jrushlow changed the title ~~WIP - [make:registration] drop guard authentication support~~ [make:registration] drop guard authentication support Feb 28, 2024

weaverryan requested changes Mar 1, 2024

View reviewed changes

improve help readability

caaae50

jrushlow commented Mar 1, 2024

View reviewed changes

these vars on constant

0d96a67

weaverryan approved these changes Mar 1, 2024

View reviewed changes

jrushlow merged commit cbf2646 into symfony:main Mar 4, 2024
6 checks passed

jrushlow deleted the refactor/make-reg branch March 4, 2024 06:08

jrushlow mentioned this pull request Mar 4, 2024

[release] v1.56.0 #1471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[make:registration] drop guard authentication support #1243

[make:registration] drop guard authentication support #1243

jrushlow commented Nov 28, 2022 •

edited

jrushlow Feb 28, 2024

weaverryan Mar 1, 2024

weaverryan Mar 1, 2024

jrushlow Mar 1, 2024 •

edited

weaverryan Mar 1, 2024

jrushlow Mar 1, 2024

jrushlow Mar 1, 2024 •

edited

weaverryan Mar 1, 2024

jrushlow Mar 1, 2024

weaverryan left a comment

weaverryan Mar 1, 2024

[make:registration] drop guard authentication support #1243

[make:registration] drop guard authentication support #1243

Conversation

jrushlow commented Nov 28, 2022 • edited

DX

Under the hood

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrushlow Mar 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrushlow Mar 1, 2024 • edited

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

jrushlow commented Nov 28, 2022 •

edited

jrushlow Mar 1, 2024 •

edited

jrushlow Mar 1, 2024 •

edited