[Security] Add Guard authenticator <supports> method #16835

Amo · 2015-12-04T13:27:47Z

This method will be called before starting an authentication against a guard authenticator.
The authentication will be tried only if the supports method returned true
This improves understanding of code, increase consistency and removes responsability for getCredentials method to decide if the current request should be supported or not.

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

Amo · 2015-12-04T13:29:32Z

Note: as being not familiar with the recent deprecation mechanisms, i'm not sure i did it well in src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php, line 109

Amo · 2015-12-04T14:07:51Z

About tests, i get, i'm not sure how is this wrong or ok.

$ ./phpunit --filter GuardAuthenticationListenerTest
PHPUnit 4.8.19 by Sebastian Bergmann and contributors.

Testing Symfony Test Suite
.......

Time: 4.1 seconds, Memory: 173.00Mb

OK (7 tests, 22 assertions)

Remaining deprecation notices (6)

Class Mock_GuardAuthenticatorInterface_a5f0cf1d should provide a method <supports(Request $request)>. see GuardAuthenticatorSupportInterface: 6x
    2x in GuardAuthenticationListenerTest::testReturnNullToSkipAuth from Symfony\Component\Security\Guard\Tests\Firewall
    1x in GuardAuthenticationListenerTest::testHandleCatchesAuthenticationException from Symfony\Component\Security\Guard\Tests\Firewall
    1x in GuardAuthenticationListenerTest::testHandleSuccessStopsAfterResponseIsSet from Symfony\Component\Security\Guard\Tests\Firewall
    1x in GuardAuthenticationListenerTest::testHandleSuccess from Symfony\Component\Security\Guard\Tests\Firewall
    1x in GuardAuthenticationListenerTest::testHandleSuccessWithRememberMe from Symfony\Component\Security\Guard\Tests\Firewall

KO symfony

linaori · 2015-12-04T15:46:56Z

src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

+            if (method_exists($guardAuthenticator, 'supports')) {
+                if (true !== $guardAuthenticator->supports($request)) {
+                    return;
+                }


You add an interface but you check if the method exists here. Why not simply check if the interface is in use?

That was because i did not handled it this way originally. In fact now it would make more sense to check the interface implementation.

Amo · 2015-12-04T17:34:35Z

by the way @weaverryan , that's what i was talking with you this morning.

stof · 2015-12-05T10:04:59Z

I'm not sure we need this. It would probably have to duplicate some of the work done in getCredentials in many time.

@weaverryan what do you think ?

stof · 2015-12-05T10:05:48Z

@Amo tests are failing because you are using the deprecated way in a non-legacy test.

weaverryan · 2015-12-07T12:30:49Z

It's "clearer" in some ways because supports() can return a clean true/false value and then getCredentials() can cause an AuthException if it returns null, which would make it more consistent with getUser and checkCredentials.

I discussed this at the conference - I'm mixed on it - the only negative I see is exactly what Stof mentions, but that is a practical concern. For an API token auth, I'll either have to duplicate code that gets/checks for the API token value in supports() and getUser() or just always return true from supports, which defeats the purpose.

@Amo Can you convince us that this will be a better user experience?

Thx!

Amo · 2015-12-07T13:26:50Z

The idea is to separate the responsabilities of

Identifying if the authentication context is elligible/supported
Extracting the credentials from the context

and doing so, improve the readability, support of variations and testability of an authentication method.

Supports would be responsible to detect a specific context, guaranteeing that when getCredentials is called, this context has been met.

As @stof said, it could end-up in duplicating the same checks in supports and getCredentials. But, imho, that would occurs if the authentication system supports many context and is poorly designed.

Let's take an example :

Access to /api requires a X-API-Token header.
An authenticator is created :

    // ...

    /**
     * Ensure the given request provides an API token in the headers
     * 
     * @param  Request $request
     * 
     * @return null|mixed
     */
    public function supports(Request $request)
    {
        if (false === $request->headers->has('X-API-Token')) {
            return false;
        }

        return true;
    }

    /**
     * Extract the credentials from the request.
     * 
     * @param  Request $request
     * 
     * @return string
     */
    public function getCredentials(Request $request)
    {
        return $request->headers->get('X-API-Token'));
    }

If additional credentials retrieval methods are required (qs, payload ...) it is likely that the getCredentials method will get many if and else, which would make the code harder to read, maintain, test ...

This separation supports vs getCredentials will spur the developer to create dedicated guard authenticator for each specificities. A common abstract class or a trait could be used to share the common stages of the authentication. But concrete final classes will explicitly indicates with use-cases they manage :

Security
└── Authenticator
    ├── ApiBaseAuthenticator.php
    ├── ApiHeaderAuthenticator.php
    ├── ApiPayloadAuthenticator.php
    └── ApiQueryAuthenticator.php

Where ApiBaseAuthenticator.php handles the common logic for

getUser
checkCredentials
createAuthenticatedToken
onAuthenticationFailure
onAuthenticationSuccess
supportsRememberMe

And each dedicated authenticator provides the dedicated logic to retrieve credentials from header, query string or payload

Amo · 2015-12-07T13:29:07Z

@Amo tests are failing because you are using the deprecated way in a non-legacy test.

How can i avoid it ? googling about Symfony 3 and deprecation conventions didn't give any results.

weaverryan · 2015-12-08T13:57:12Z

@Amo I think your arguments are quite good. So, let's see if we can complete this PR :).

Forgetting about BC, how would you want the interface to look in 4.0? You added a new GuardAuthenticatorSupportsInterface, but I imagine that you'd really want supports() to be in the GuardAuthenticatorInterface, correct? There will be some work that will need to be done to create a new interface (that has the new method) and deprecate the old interface.

weaverryan · 2015-12-08T13:59:45Z

@Amo about the tests, the GuardAuthenticationListenerTest is mocking the GuardAuthenticatorInterface, which then triggers your deprecation warning. This mock should be changed to GuardAuthenticatorSupportsInterface so that the deprecated functionality isn't triggered (or changed to whatever final interface we decide on that has the new supports() method). There should also be a test that verifies that if you use the original GuardAuthenticatorInterface, then the old behavior is used. Put an @legacy above tests that specifically test how legacy functionality works.

Amo · 2015-12-08T15:15:08Z

ok thanks, i'm on it

Amo · 2015-12-08T21:13:37Z

Tests are still running.
But, (take a deep breath), i moved the interface into the Authenticator sub namespace, Symfony\Component\Security\Guard\Authenticator\GuardAuthenticatorInterface which makes sense to me.
This interface adds the supports method and keep the same the signature as Symfony\Component\Security\Guard\GuardAuthenticatorInterface. It extends Symfony\Component\Security\Guard\GuardAuthenticatorInterfaceas GuardAuthenticatorLegacyInterface to avoid BC break. the Symfony\Component\Security\Guard\GuardAuthenticatorInterface has now a @deprecated annotation and is still used as type hint in Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener alongside the new Symfony\Component\Security\Guard\Authenticator\GuardAuthenticatorInterface interface.

Amo · 2015-12-08T21:14:21Z

Should i fixup and git push --force all the changes on the Amo:symfony branch ?

linaori · 2015-12-09T07:07:01Z

I notice the handle()/executeGuardAuthenticator() are becoming big_(ger with those changes)_. I think the flow and changes are good, but this will become harder and harder to maintain. Some suggestions I have for this method which could be done (after this PR is merged):

Using a null logger instead of a PSR logger
It will improve readability and reduce complexity and would remove 8 if statements in just those 2 methods

Move code out of the try catch which is unrelated
This try catch is pretty much just a sort of "failsafe" at the moment. However, things like supports() should not throw an AuthenticationException. A list of method calls:

$this->logger->debug('...');
$guardAuthenticator->supports($request)
$guardAuthenticator->getCredentials($request)
$this->logger->debug('...');
$this->authenticationManager->authenticate($token)
$this->logger->info('...');
$this->guardHandler->authenticateWithToken($token, $request);

Out of all available methods in the authenticator, only getUser() and checkCredentials() throw an exception and they are not being called here. In fact, the only method that throws an exception is $this->authenticationManager->authenticate($token). It's actually making it possible for people to throw all kinds of AuthenticationExceptions which can cause unwanted behavior or break when someone decides this try/catch can be narrowed down in say 3.4.

Make GuardAuthenticationListener final
We've created a very nice extension point which people will abuse in the future. My suggestion is to make this class final as it makes it easier for people to fix things here. Behavioral changes should still be avoided as much as possible though.

After this the code will still be quite massive but it will at least be easier to read and understand what it does.

fabpot · 2016-01-25T12:04:19Z

@weaverryan What's the status of this one?

fabpot · 2016-02-18T16:24:40Z

@weaverryan I'd like to move this one forward. Can you have a look at it? Thanks.

stof · 2016-02-18T16:34:04Z

src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

+                }
+
+                // otherwise something went wrong and the authentication must fail
+                throw new \InvalidArgumentException('Guard authenticator must provide user credentials');


A RuntimeException (or even better its child UnexpectedValueException) would be better. This is not an invalid argument here, but an invalid return value

And with the message:

Did you forget to return some credentials from Foo\Bar\ClassName::getCredentials()? Credentials can be anything: an array, string or object.

Amo · 2016-02-18T17:04:23Z

Updated.

weaverryan · 2016-02-18T18:16:30Z

src/Symfony/Component/Security/Guard/Authenticator/GuardAuthenticatorInterface.php

+     *
+     * @return mixed|null
+     */
+    public function getCredentials(Request $request);


@Amo What do you think about actually removing getCredentials() now? And instead changing getUser() to be getUser(Request $request, UserProviderInterface $userProvider);

Now that getCredentials() only has the responsibility of returning the credentials, it seems simpler just to ask "give me a user" instead of breaking things into these 2 pieces.

Btw, if you agree, we will need to rename getUser() to something like fetchUser() so we can add it to the new interface.

It could be simpler, but it would add much more complexity in the fetchUser method.
This "merge" of the two responsabilities will make the fetchUser method a bit more complex to unit test.
Aditionnaly, the UserProviderInterface is known by the GuardAuthenticationProvider and here we are in the GuardAuthenticationListener so it needs many changes.

It's just a question of "DX", which is subjective:

Before:

public function getCredentials(Request $request) { return $request->headers->get('Authorization'); } public function getUser($credentials, UserProviderInterface $userProvider) { $authorizationHeader = $credentials; // ... }

After

public function fetchUser(Request $request, UserProviderInterface $userProvider) { $authorizationHeader = $request->headers->get('Authorization'); // ... }

@javiereguiluz @iltar Do you guys have a preference?

@Amo This new method would be called from GuardAuthenticationProvider - we would set the entire request in the PreAuthenticationGuardToken so that we could pass it to the new method.

I'm not 100% sure about the "fetch" part, it deviates from the currently popular find and get (and makes me feel like talking to my dog). DX wise I'd go for the second solution, however the first option gives you the ability to make a chain credential provider in symfony. That might be overkill and people can still implement a similar pattern in their fetchUser. If getCredentials only purpose is to be passed to getUser, they should be merged imo.

Then how about we name it findUser()? I agree with you on the chaining - there may be a use for this, but I was also thinking: implement it yourself.

Personally I'd lean more towards getUser(), this because it's rather confusing with the Repositories otherwise. By default those start with find and it might confuse developers when those names start getting mixed up. Ideally I'd throw an exception in a get* method if unable to get whatever was expected.

How would that work out if the could would always throw an exception if the user could not be found?

@iltar The problem with getUser() is that we already have a method named that - and this would cause a signature change. So we need a new method name so we can keep the old one for BC. So, it's not idea. Perhaps getUserFromRequest()?

It's starting to sound more like a builder pattern;

$guard->buildUser(new UserFromRequestBuilder($request), $userProvider); // potential to decouple from the request $guard->buildUser(new UserFromConsoleBuilder($input, $output), $userProvider);

Anyway, getUserFromRequest() sounds decent enough, what will happen to getUser()?

We can re-use the same method name. With get_func_args(), it's possible to detect which version the user is using, like we've done in the past elsewhere in the framework. I'd go with the current getUser() method name.

weaverryan · 2016-02-18T18:19:25Z

Thanks for the quick update @Amo - and sorry for the extra comments - I had to pause in the middle of reviewing :). Btw, when I talked about Guard at the last conference, someone else suggested this exact change afterwards.

Amo · 2016-02-23T10:15:27Z

I've been updating the code according to the discussion about getUserFromRequest.
Unit tests need to be updated.

chalasr

👍

nicolas-grekas · 2017-10-04T12:38:21Z

UPGRADE-3.4.md

@@ -294,7 +294,10 @@ Security

 * Deprecated the HTTP digest authentication: `NonceExpiredException`,
   `DigestAuthenticationListener` and `DigestAuthenticationEntryPoint` will be
-   removed in 4.0. Use another authentication system like `http_basic` instead.
+   removed in 4.0. Use another authentication system like `http_basic` instead.   


extra space at end of line (same on the line below)

nicolas-grekas · 2017-10-04T12:55:47Z

src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

+            // abort the execution of the authenticator if it doesn't support the request.
+            if ($guardAuthenticator instanceof GuardAuthenticatorInterface) {
+                // deprecated since version 3.4, to be removed in 4.0
+                @trigger_error(sprintf('The %s interface is deprecated since version 3.4 and will be removed in 4.0. Implement %s instead.', GuardAuthenticatorInterface::class, AuthenticatorInterface::class), E_USER_DEPRECATED);


this runtime deprecation should be removed: it has already been trigger by DebugClassLoader

nicolas-grekas · 2017-10-04T12:55:59Z

src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

+                }
+
+                // otherwise something went wrong and the authentication must fail
+                throw new \UnexpectedValueException(sprintf(


should be on one line

nicolas-grekas · 2017-10-04T12:56:44Z

src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php

     *
     * @return null|Response
     */
-    public function handleAuthenticationSuccess(TokenInterface $token, Request $request, GuardAuthenticatorInterface $guardAuthenticator, $providerKey)
+    public function handleAuthenticationSuccess(TokenInterface $token, Request $request, AuthenticatorInterface $guardAuthenticator, $providerKey)


that's a BC break unfortunately (same for all public interfaces)

nicolas-grekas · 2017-10-04T12:57:52Z

src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php

 */
-interface GuardAuthenticatorInterface extends AuthenticationEntryPointInterface
+interface GuardAuthenticatorInterface extends AuthenticatorInterface


I think this should be done the other way around, to still allow passing legacy type-hints.
this would mean removing the runtime notice above, but DebugClassLoader will take care or triggering it anyway

AuthenticatorInterface extends GuardAuthenticatorInterface

nicolas-grekas · 2017-10-04T12:58:12Z

src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php

@@ -142,6 +142,6 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti

    public function supports(TokenInterface $token)
    {
-        return $token instanceof GuardTokenInterface;
+        return $token instanceof TokenInterface;


why this change?

chalasr · 2017-10-04T14:04:13Z

src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php

@@ -56,7 +56,7 @@ public function __construct($guardAuthenticators, UserProviderInterface $userPro
    /**
     * Finds the correct authenticator for the token and calls it.
     *
-     * @param GuardTokenInterface $token
+     * @param TokenInterface|GuardTokenInterface $token


I reverted this change as it should have been TokenInterface from day 1. I'm going to fix it on 2.7

chalasr · 2017-10-04T15:10:41Z

@nicolas-grekas comments addressed, thanks

nicolas-grekas

(but there are more FQCN strings that could be replaced by ...::class)

chalasr · 2017-10-05T08:16:02Z

Remaining FQCNs fixed

xabbuh · 2017-10-05T11:02:30Z

src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

     */
-    private function triggerRememberMe(GuardAuthenticatorInterface $guardAuthenticator, Request $request, TokenInterface $token, Response $response = null)
+    private function triggerRememberMe($guardAuthenticator, Request $request, TokenInterface $token, Response $response = null)


IMO we should be consistent in removing the type hint (like here) or not (like in GuardAuthenticatorHandler below). Keeping the type hint might be a good idea to catch all places where we need to adjust them when removing code in 4.0.

👍 I re-added the typehint in this class so that it breaks if we forget to change them when removing the deprecated interface.

fabpot · 2017-10-05T12:19:56Z

Thank you @Amo.

…Amo, chalasr) This PR was merged into the 3.4 branch. Discussion ---------- [Security] Add Guard authenticator <supports> method This method will be called before starting an authentication against a guard authenticator. The authentication will be tried only if the supports method returned `true` This improves understanding of code, increase consistency and removes responsability for `getCredentials` method to decide if the current request should be supported or not. | Q | A | | --- | --- | | Bug fix? | no | | New feature? | yes | | BC breaks? | yes | | Deprecations? | yes | | Tests pass? | yes | | Fixed tickets | none | | License | MIT | | Doc PR | | Commits ------- 78eecba Fix BC layer a7a6f8a [Security] Add Guard authenticator <supports> method

Amo · 2017-10-05T16:19:04Z

Thank you ! Now i'm a proud contributor of Symfony

chalasr · 2017-10-05T16:28:02Z

@Amo Thanks for this and congrats for your first PR on github! You did it right, in the right repo :)

This PR was merged into the 4.0-dev branch. Discussion ---------- [Security] Remove GuardAuthenticatorInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#8485 Removes BC layers for #16835. Commits ------- 3408152 [Security][Guard] Remove GuardAuthenticatorInterface

kbond · 2017-10-24T19:04:01Z

I am getting what I think is a false positive deprecation notice in 3.4:

User Deprecated: The "Symfony\Component\Security\Guard\AbstractGuardAuthenticator::supports()" 
Method is deprecated since version 3.4, to be removed in 4.0. You should not extend it from 
"...\MyAuthenticator".

I have added MyAuthenticator::supports() but am still getting this notice. Did I not upgrade correctly or is this a bug?

kbond · 2017-10-24T19:08:43Z

src/Symfony/Component/Security/Guard/AbstractGuardAuthenticator.php

 {
    /**
+     * {@inheritdoc}
+     *
+     * @deprecated since version 3.4, to be removed in 4.0


If I remove this line, the false positive deprecation notice is removed. Should I open a PR?

Please do. This method must be implemented by concrete subclasses and it is unlikely to be called directly.

…ardAuthenticator::supports() (kbond) This PR was merged into the 3.4 branch. Discussion ---------- [Security] remove invalid deprecation notice on AbstractGuardAuthenticator::supports() | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16835 (comment) | License | MIT | Doc PR | n/a This deprecation flag causes a false positive. Commits ------- 5fb44e7 [Guard] remove invalid deprecation notice

carsonbot added the Status: Needs Review label Dec 4, 2015

linaori reviewed Dec 4, 2015
View reviewed changes

Amo changed the title ~~Add Guard authenticator <supports> method~~ [Security] Add Guard authenticator <supports> method Dec 4, 2015

stof reviewed Feb 18, 2016
View reviewed changes

weaverryan reviewed Feb 18, 2016
View reviewed changes

javiereguiluz added Security Feature labels Mar 3, 2016

chalasr changed the base branch from master to 3.4 October 4, 2017 12:14

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 4, 2017

chalasr approved these changes Oct 4, 2017

View reviewed changes

nicolas-grekas reviewed Oct 4, 2017

View reviewed changes

chalasr reviewed Oct 4, 2017

View reviewed changes

nicolas-grekas approved these changes Oct 4, 2017

View reviewed changes

chalasr added the Ready label Oct 5, 2017

xabbuh reviewed Oct 5, 2017

View reviewed changes

xabbuh approved these changes Oct 5, 2017

View reviewed changes

Fix BC layer

78eecba

fabpot approved these changes Oct 5, 2017

View reviewed changes

fabpot merged commit 78eecba into symfony:3.4 Oct 5, 2017

This was referenced Oct 5, 2017

[Security][Guard] Deprecated GuardAuthenticatorInterface symfony/symfony-docs#8485

Closed

[Security] Remove GuardAuthenticatorInterface #24446

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

chalasr mentioned this pull request Oct 19, 2017

[Security] Fix missing BC layer for AbstractGuardAuthenticator::getCredentials() #24624

Merged

kbond reviewed Oct 24, 2017

View reviewed changes

kbond mentioned this pull request Oct 24, 2017

[Security] remove invalid deprecation notice on AbstractGuardAuthenticator::supports() #24684

Merged

[Security] Add Guard authenticator <supports> method #16835

[Security] Add Guard authenticator <supports> method #16835

Conversation

Amo commented Dec 4, 2015

Amo commented Dec 4, 2015

Amo commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amo commented Dec 4, 2015

stof commented Dec 5, 2015

stof commented Dec 5, 2015

weaverryan commented Dec 7, 2015

Amo commented Dec 7, 2015

Amo commented Dec 7, 2015

weaverryan commented Dec 8, 2015

weaverryan commented Dec 8, 2015

Amo commented Dec 8, 2015

Amo commented Dec 8, 2015

Amo commented Dec 8, 2015

linaori commented Dec 9, 2015

fabpot commented Jan 25, 2016

fabpot commented Feb 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amo commented Feb 18, 2016

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 Feb 18, 2016

Amo commented Feb 23, 2016

chalasr left a comment

Choose a reason for hiding this comment

nicolas-grekas Oct 4, 2017 • 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

chalasr commented Oct 4, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

chalasr commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 5, 2017

Amo commented Oct 5, 2017

chalasr commented Oct 5, 2017

kbond commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Oct 4, 2017 •

edited

Loading