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

Avoid migration on stateless firewalls #27452

Merged
merged 1 commit into from Jun 10, 2018

Conversation

Projects
None yet
8 participants
@weaverryan
Member

weaverryan commented May 31, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets Related to #27395
License MIT
Doc PR symfony/symfony-docs#9860

This is a proof-of-concept. Once we agree / are happy, I need to add this to all of the other authentication mechanisms that recently got the session migration code & add tests.

Basically, this avoids migrating the session if the firewall is stateless. There were 2 options to do this:

A) Make the SessionAuthenticationStrategy aware of all stateless firewalls. This is the current approach
or
B) Make each individual authentication listener aware whether or not its firewall is stateless.

@stof

This comment has been minimized.

Member

stof commented May 31, 2018

third option: inject a no-op session strategy in listeners belonging to stateless firewalls

@stof

This comment has been minimized.

Member

stof commented May 31, 2018

your first approach requires every listener to set the firewall name as a request attribute before calling the session strategy (which is very easy to miss as it is not part of the explicit API of the session strategy). So IMO, that's not a good approach

@aschempp

This comment has been minimized.

Contributor

aschempp commented May 31, 2018

I like the noop-idea, It does not even need to be a new no-op class. A second session migration strategy for stateless firewalls would solve that, which gets a NONE strategy set in the constructor.

The respective listeners must then get the "stateless migration strategy" injected (instead of https://github.com/weaverryan/symfony/blob/4d5bc448ee2c192c9f4ef964e3006a63a707029e/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml#L178)

@weaverryan

This comment has been minimized.

Member

weaverryan commented May 31, 2018

No op is a great idea - I’ll update

@chalasr chalasr added the Security label May 31, 2018

@chalasr chalasr added this to the 2.8 milestone May 31, 2018

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 1, 2018

I've just updated to use a noop approach. I think this is solid. Unless anyone sees a major issue, I'll extend this to all of the other listeners & add some tests.

For SimplePreAuthenticationListener, I'm using setter injection because there are already so many optional constructor args. I'm not sure if, in the future, this should or should not be a required dependency. We could say "hey, you need to pass this in if you are storing the token in the session" OR we could eventually force (via some deprecations in 4.2) the user to pass this in, telling them to pass in a noop if they don't want session migration.

@aschempp

This comment has been minimized.

Contributor

aschempp commented Jun 1, 2018

Why do we need a noop strategy at all if you can simply not inject a strategy for the same behavior?

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 2, 2018

Good question - it’s just a technical work around. The listener service is created in the factory, but the factory doesn’t know whether or not it will live in a stateless firewall. So, it can’t be smart enough (unless I’m missing an idea - very possible) to know if it should or should not inject the session strategy. To work around that, we simply always inject a session strategy, but then in SecurityExtension, we change that session strategy to be a noop for stateless firewalls. It’s a tricky dance.

@@ -89,6 +89,8 @@ public function load(array $configs, ContainerBuilder $container)
$this->createAuthorization($config, $container);
$this->createRoleHierarchy($config, $container);
$container->setParameter('security.authentication.stateless_firewalls', $this->statelessFirewalls);

This comment has been minimized.

@xabbuh

xabbuh Jun 2, 2018

Member

not needed anymore now :)

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 4, 2018

This is ready for review. I was very careful, but as I don't believe there are tests on the "factory" classes that build the auth listeners, I appreciate close review.

I added a test for GuardAuthenticationHandler. We could also add tests for all the auth listeners - I'm happy to do that, but it would be quite a bit of test refactoring or duplication (as you need to build a test that successfully authenticates, which is often quite a lot of code). But if we feel better with those tests, let me know!

@chalasr chalasr changed the title from [WIP] Avoid migration on stateless firewalls to Avoid migration on stateless firewalls Jun 4, 2018

@nicolas-grekas

there is a failure on deps=low, a composer.json needs a tweak somewhere
cheers :)

/**
* Call this method if your authentication token is stored to a session.
*
* @param SessionAuthenticationStrategyInterface $sessionStrategy

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

can be removed

/**
* Call this method if your authentication token is stored to a session.
*
* @param SessionAuthenticationStrategyInterface $sessionStrategy

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

no need for this one :)

/**
* Call this method if your authentication token is stored to a session.
*
* @param SessionAuthenticationStrategyInterface $sessionStrategy

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

to be removed

/**
* Call this method if your authentication token is stored to a session.
*
* @param SessionAuthenticationStrategyInterface $sessionStrategy

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

boo

/**
* Call this method if your authentication token is stored to a session.
*
* @param SessionAuthenticationStrategyInterface $sessionStrategy

This comment has been minimized.

@nicolas-grekas
@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 7, 2018

Ok, CI is happy!

To summarize: the only downside to this PR is on a technical level: the fact that I'm using setter injection everywhere to avoid trying to add this dependency to the constructor (when every class already has optional deps). I've made the setters final, so, if we want to change this in the future, we could decide to do that with less trouble.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 7, 2018

LGTM. I'd just suggest moving the final note to an annotation, as real final tokens block legit usages (e.g. lazy proxies) and we try to avoid them here usually.

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 7, 2018

Done!

I'll make a separate PR for 3.4 for UsernamePasswordJsonAuthenticationListener, but it won't block this (that is on my list). - see #27556

@@ -18,7 +18,7 @@
"require": {
"php": ">=5.3.9",
"ext-xml": "*",
"symfony/security": "^2.8.41|^3.4.11",
"symfony/security": "^2.8.42|^3.4.12",

This comment has been minimized.

@weaverryan

weaverryan Jun 8, 2018

Member

I bumped both versions - pretty sure that's correct :)

@chalasr

chalasr approved these changes Jun 8, 2018

I would just make the setSessionAuthenticationStrategy() internal to not introduce any new public api in a patch and just fix our own listeners until this reaches master. 👍 anyways

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 9, 2018

I would just make the setSessionAuthenticationStrategy() internal to not introduce any new public api in a patch and just fix our own listeners until this reaches master. 👍 anyways

We could do this, but technically speaking, if anyone uses the components, then they would need to use these methods to migrate the session. So, I'm not sure that we can

@fabpot

fabpot approved these changes Jun 10, 2018

with minor comment

/**
* Call this method if your authentication token is stored to a session.
*
* @final since version 2.8

This comment has been minimized.

@fabpot

fabpot Jun 10, 2018

Member

"since version 2.8" should be removed. I don't think we are doing that anywhere else.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 10, 2018

Member

We do when we want to hint that the @final has been added after the method has been introduced. i.e. when it's deprecated to extend from it.
Which is not the case here, so yes, this has to be removed.

This comment has been minimized.

@fabpot
@fabpot

This comment has been minimized.

Member

fabpot commented Jun 10, 2018

Thank you @weaverryan.

@fabpot fabpot merged commit cca73bb into symfony:2.8 Jun 10, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 10, 2018

bug #27452 Avoid migration on stateless firewalls (weaverryan)
This PR was squashed before being merged into the 2.8 branch (closes #27452).

Discussion
----------

Avoid migration on stateless firewalls

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | Related to #27395
| License       | MIT
| Doc PR        | symfony/symfony-docs#9860

This is a proof-of-concept. Once we agree / are happy, I need to add this to all of the other authentication mechanisms that recently got the session migration code & add tests.

Basically, this avoids migrating the session if the firewall is stateless. There were 2 options to do this:

A) Make the `SessionAuthenticationStrategy` aware of all stateless firewalls. **This is the current approach**
or
B) Make each individual authentication listener aware whether or not *its* firewall is stateless.

Commits
-------

cca73bb Avoid migration on stateless firewalls

fabpot added a commit that referenced this pull request Jun 10, 2018

bug #27556 Avoiding session migration for stateless firewall Username…
…PasswordJsonAuthenticationListener (weaverryan)

This PR was squashed before being merged into the 3.4 branch (closes #27556).

Discussion
----------

Avoiding session migration for stateless firewall UsernamePasswordJsonAuthenticationListener

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | Related to #27395
| License       | MIT
| Doc PR        | symfony/symfony-docs#9860

This is the sister PR to #27452, which covered all the other authentication listeners.

Commits
-------

c06f322 Avoiding session migration for stateless firewall UsernamePasswordJsonAuthenticationListener

chalasr added a commit that referenced this pull request Jun 12, 2018

bug #27581 Fix bad method call with guard authentication + session mi…
…gration (weaverryan)

This PR was squashed before being merged into the 2.8 branch (closes #27581).

Discussion
----------

Fix bad method call with guard authentication + session migration

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no (but there needs to be on master)
| Tests pass?   | yes
| Fixed tickets | #27577
| License       | MIT
| Doc PR        | n/a

I messed up #27452 :/. Guard is the one class where the session migration is not on the listener, it's on the handler. The tricky part is that there is only ONE handler (unlike listeners where there is 1 listener per firewall). That means that implementing a session migration strategy that avoids stateless firewalls was a bit more tricky: I could only think to inject a map into `GuardAuthenticationHandler`. On the bright side, this also fixes session migration (not happening) when people call the `authenticateUserAndHandleSuccess()` method directly.

On master, we'll need to add a deprecation to make the 3rd argument of `authenticateWithToken()` required - it's optional now for BC. We may also need to re-order the constructor args.

I DID test this in a real 2.8 project, to make sure that things were properly wired up. Apologies for not doing that for the other PR.

Cheers!

Commits
-------

2c0ac93 Fix bad method call with guard authentication + session migration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment