8000 [Security] Improve DX when invalid custom authenticators by alamirault · Pull Request #49938 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Improve DX when invalid custom authenticators #49938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 19, 2023
Merged

[Security] Improve DX when invalid custom authenticators #49938

merged 1 commit into from
May 19, 2023

Conversation

alamirault
Copy link
Contributor
@alamirault alamirault commented Apr 5, 2023 •
edited
Loading
Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

Before this PR error was

security:
    firewalls:
        main:
            lazy: true
            provider: users_in_memory
            custom_authenticators:
                - App\Security\FooProvider # Mistake is here, must be an AuthenticatorInterface

image

@carsonbot carsonbot added this to the 6.3 milestone Apr 5, 2023
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features / 5.4 or 6.2 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

alamirault
alamirault commented Apr 5, 2023
View reviewed changes
src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php Outdated
@@ -60,6 +60,12 @@ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthent
*/
public function __construct(iterable $authenticators, TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher, string $firewallName, LoggerInterface $logger = null, bool $eraseCredentials = true, bool $hideUserNotFoundExceptions = true, array $requiredBadges = [])
Copy link
Contributor Author
@alamirault alamirault Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We mus 10000 t probably also update validation of config in SecurityBundle, but I didn't find an example which check interface from FQCN . Should we you ReflectionClass ?

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/CustomAuthenticatorFactory.php

Lines 49 to 56 in 3040d43

->validate()
->ifTrue(fn ($v) => isset($v['custom_authenticators']) && empty($v['custom_authenticators']))
->then(function ($v) {
unset($v['custom_authenticators']);
return $v;
})
->end()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that what you configure here is a service id and not a FQCN, we cannot validate the interface in the configuration validation (as it does not have access to the service definition)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can do it in the extension. This is help new comers more than in src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php
(but we can keep both checks)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this should be checked in a compiler pass, not in the extension (the DI extension runs with a dedicated container builder and so does not have access to the services defined elsewhere)

chalasr
chalasr requested changes May 15, 2023
View reviewed changes
@alamirault alamirault requested a review from chalasr May 18, 2023 16:59
@nicolas-grekas nicolas-grekas force-pushed the feature/improve-dx-when-invalid-custom-authenticators branch from 194b1e3 to 93cc075 Compare May 19, 2023 08:16
@nicolas-grekas
Copy link
Member

Thank you @alamirault.

@nicolas-grekas nicolas-grekas merged commit cf5f103 into symfony:6.3 May 19, 2023
@alamirault alamirault deleted the feature/improve-dx-when-invalid-custom-authenticators branch May 19, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx lyrixx left review comments

@stof stof stof left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
6 participants
@alamirault @carsonbot @nicolas-grekas @lyrixx @stof @chalasr
0