[SecurityBundle] Move Anonymous DI integration to new AnonymousFactory #33503
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e152c11 to
4be4d94
Compare
4be4d94 to
0da2761
Compare
linaori
reviewed
Sep 9, 2019
There was a problem hiding this comment.
It looks like the tests are still green after changing this, so feature wise it indeed doesn't change anything. 👍 from my side as this will indeed improve the internal code and no longer handles anonymous as an exceptional case.
One question though, what if after this factory, a custom factory is added?
Given no listener/provider from the earlier factories could authenticate the request, there are 2 cases: If anonymous is enabled in the firewall, the listener/provider of that factory would not be called (anonymous is able to authenticate any request). If anonymous is disabled, it'll just be called at the end. |
fabpot
approved these changes
Sep 11, 2019
|
Thank you @wouterj. |
fabpot
added a commit
that referenced
this pull request
Sep 11, 2019
…AnonymousFactory (wouterj) This PR was merged into the 4.4 branch. Discussion ---------- [SecurityBundle] Move Anonymous DI integration to new AnonymousFactory | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | n/a For some reason, all security authentication providers/listeners have a `SecurityFactory` that adds configuration and registers the necessary services, except from anonymous security. I'm not sure why that has not been done. The only thing I can think of is making sure it is added to the end. I've added a new "internal" factory position, to make sure it is always the last registered provider and moved everything to a new `AnonymousFactory`. Nothing changes on the usage side, but it makes internal code a bit easier to understand and makes sure we don't break anything while refactoring the `SecurityExtension` in the future. Commits ------- 0da2761 Move Anonymous config to a SecurityFactory
This was referenced Nov 12, 2019
Merged
Merged
fabpot
added a commit
that referenced
this pull request
Nov 25, 2019
…nymous listener (chalasr) This PR was merged into the 4.4 branch. Discussion ---------- [SecurityBundle] Don't require a user provider for the anonymous listener | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #34504 | License | MIT | Doc PR | - Forgotten when adding the AnonymousFactory in #33503 Commits ------- 0950cfb [SecurityBundle] Don't require a user provider for the anonymous listener
martijnboers
added a commit
to martijnboers/symfony
that referenced
this pull request
Dec 3, 2019
This was incorrectly copied in PR symfony#33503
chalasr
pushed a commit
that referenced
this pull request
Dec 3, 2019
…martijnboers) This PR was merged into the 4.4 branch. Discussion ---------- [SecurityBundle] Use config variable in AnonymousFactory | Q | A | ------------- | --- | Branch? | 4.4 and 5.0 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT It looks like the `AnonymousFactory` was copied incorrectly in #33503 as it uses the old `$firewall` variable available in `SecurityExtension.php`. Changing this to `$config` yields the desired results Commits ------- 8d850d2 When set, get secret from config variable
symfony-splitter
pushed a commit
to symfony/security-bundle
that referenced
this pull request
Dec 3, 2019
This was incorrectly copied in PR symfony/symfony#33503
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some reason, all security authentication providers/listeners have a
SecurityFactorythat adds configuration and registers the necessary services, except from anonymous security. I'm not sure why that has not been done. The only thing I can think of is making sure it is added to the end.I've added a new "internal" factory position, to make sure it is always the last registered provider and moved everything to a new
AnonymousFactory.Nothing changes on the usage side, but it makes internal code a bit easier to understand and makes sure we don't break anything while refactoring the
SecurityExtensionin the future.