Skip to content

[Security] Lazy load guard authenticators and authentication providers #21450

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.

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

Merged
merged 2 commits into from
Feb 16, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jan 29, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Authentication stops on the first authenticator that fails or succeeds, let's instantiate them only if actually needed.

@ogizanagi
Copy link
Contributor

ogizanagi commented Jan 29, 2017

In case a logger is provided, the number of authenticators is provided in the context array (GuardAuthenticationListener.php#L69).
Using count on the generator won't provide the right value. I don't think this information is really useful though...

Do you think it'll be worth it to make RewindableGenerator implement \Countable? (It'll still require an extra check anyway for such cases).

@nicolas-grekas
Copy link
Member

I think it's a good idea to make RewindableGenerator implement Countable, since we always have the info, isn't it?

@ogizanagi
Copy link
Contributor

@chalasr , @nicolas-grekas : See #21455

@chalasr chalasr force-pushed the guard/iterator branch 2 times, most recently from 062580c to 6e4e1ba Compare January 30, 2017 13:20
@@ -15,6 +15,7 @@
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
Copy link
Member

Choose a reason for hiding this comment

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

alpha order, should be first I guess

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍 with minor comment

@chalasr
Copy link
Member Author

chalasr commented Jan 31, 2017

Uses order fixed (must be the last time, moved to a real IDE everywhere)

@chalasr
Copy link
Member Author

chalasr commented Jan 31, 2017

Same done for authentication providers in last commit cd6422a. This service is injected into all listeners. WDYT? I can do it in another PR if you prefer.

@chalasr chalasr changed the title [Security][Guard] Lazy load authenticators [Security] Lazy load guard authenticators and authentication providers Feb 1, 2017
nicolas-grekas added a commit that referenced this pull request Feb 2, 2017
…anagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Allow to count on lazy collection arguments

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21450 (comment)
| License       | MIT
| Doc PR        | todo (with symfony/symfony-docs#7336)

When using the new iterator feature of the DI component to lazy load collection, we always know the number of arguments in the collection (only the invalidBehavior set to `IGNORE_ON_INVALID_REFERENCE` may change this number). So we are able to generate and use a `RewindableGenerator` implementing `\Countable` by computing this value ahead.

So, in a service accepting `array|iterable`, like in the `GuardAuthenticationListener` (see #21450):

```php
class GuardAuthenticationListener implements ListenerInterface
{
    private $guardAuthenticators;

    /**
       * @param iterable|GuardAuthenticatorInterface[]  $guardAuthenticators   The authenticators, with keys that match what's passed to GuardAuthenticationProvider
       * @param LoggerInterface                         $logger                A LoggerInterface instance
    */
    public function __construct($guardAuthenticators, LoggerInterface $logger = null)
    {
          // ...
    }

    public function handle(GetResponseEvent $event)
    {
        if (null !== $this->logger) {
            $context = array()
            if (is_array($this->guardAuthenticators) || $this->guardAuthenticators instanceof \Countable) {
                $context['authenticators'] = count($this->guardAuthenticators);
            }
            $this->logger->debug('Checking for guard authentication credentials.', $context);
        }
        // ...
    }
}
```

we still keep the ability to call count without loosing the lazy load benefits.

Commits
-------

f23e460 [DI] Allow to count on lazy collection arguments
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull request Feb 2, 2017
…anagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Allow to count on lazy collection arguments

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#21450 (comment)
| License       | MIT
| Doc PR        | todo (with symfony/symfony-docs#7336)

When using the new iterator feature of the DI component to lazy load collection, we always know the number of arguments in the collection (only the invalidBehavior set to `IGNORE_ON_INVALID_REFERENCE` may change this number). So we are able to generate and use a `RewindableGenerator` implementing `\Countable` by computing this value ahead.

So, in a service accepting `array|iterable`, like in the `GuardAuthenticationListener` (see #21450):

```php
class GuardAuthenticationListener implements ListenerInterface
{
    private $guardAuthenticators;

    /**
       * @param iterable|GuardAuthenticatorInterface[]  $guardAuthenticators   The authenticators, with keys that match what's passed to GuardAuthenticationProvider
       * @param LoggerInterface                         $logger                A LoggerInterface instance
    */
    public function __construct($guardAuthenticators, LoggerInterface $logger = null)
    {
          // ...
    }

    public function handle(GetResponseEvent $event)
    {
        if (null !== $this->logger) {
            $context = array()
            if (is_array($this->guardAuthenticators) || $this->guardAuthenticators instanceof \Countable) {
                $context['authenticators'] = count($this->guardAuthenticators);
            }
            $this->logger->debug('Checking for guard authentication credentials.', $context);
        }
        // ...
    }
}
```

we still keep the ability to call count without loosing the lazy load benefits.

Commits
-------

f23e460 [DI] Allow to count on lazy collection arguments
@chalasr
Copy link
Member Author

chalasr commented Feb 14, 2017

@nicolas-grekas still ok for you?

@nicolas-grekas
Copy link
Member

Still OK

@fabpot
Copy link
Member

fabpot commented Feb 16, 2017

Thank you @chalasr.

@fabpot fabpot merged commit cd6422a into symfony:master Feb 16, 2017
fabpot added a commit that referenced this pull request Feb 16, 2017
…cation providers (chalasr)

This PR was squashed before being merged into the 3.3-dev branch (closes #21450).

Discussion
----------

[Security] Lazy load guard authenticators and authentication providers

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Authentication stops on the first authenticator that fails or succeeds, let's instantiate them only if actually needed.

Commits
-------

cd6422a [SecurityBundle] Lazy load authentication providers
b8a23de [Security][Guard] Lazy load authenticators
@chalasr chalasr deleted the guard/iterator branch February 16, 2017 14:38
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants