Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[SecurityBundle] Move Anonymous DI integration to new AnonymousFactory#33503

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4fromwouterj:security/add-anonymous-factory
Sep 11, 2019

Conversation

@wouterj
Copy link
Member

@wouterjwouterj commentedSep 8, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PRn/a

For some reason, all security authentication providers/listeners have aSecurityFactory 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 newAnonymousFactory.

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 theSecurityExtension in the future.

@wouterjwouterj changed the title[Security] Move Anonymous DI integration to new AnonymousFactory[SecurityBundle] Move Anonymous DI integration to new AnonymousFactorySep 8, 2019
@wouterjwouterjforce-pushed thesecurity/add-anonymous-factory branch 2 times, most recently frome152c11 to4be4d94CompareSeptember 8, 2019 14:04
@wouterjwouterjforce-pushed thesecurity/add-anonymous-factory branch from4be4d94 to0da2761CompareSeptember 8, 2019 14:32
@nicolas-grekasnicolas-grekas added this to thenext milestoneSep 8, 2019
Copy link
Contributor

@linaorilinaori left a comment

Choose a reason for hiding this comment

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

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 ifafter this factory, a custom factory is added?

@wouterj
Copy link
MemberAuthor

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
Copy link
Member

Thank you@wouterj.

fabpot added a commit that referenced this pull requestSep 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/aFor 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
@fabpotfabpot merged commit0da2761 intosymfony:4.4Sep 11, 2019
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
@wouterjwouterj deleted the security/add-anonymous-factory branchNovember 24, 2019 16:30
fabpot added a commit that referenced this pull requestNov 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#33503Commits-------0950cfb [SecurityBundle] Don't require a user provider for the anonymous listener
martijnboers added a commit to martijnboers/symfony that referenced this pull requestDec 3, 2019
chalasr pushed a commit that referenced this pull requestDec 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       | MITIt 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 resultsCommits-------8d850d2 When set, get secret from config variable
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull requestDec 3, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@linaorilinaorilinaori left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@wouterj@fabpot@linaori@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp