Skip to content

[Security] Remove everything related to the deprecated authentication manager #41613

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

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jun 8, 2021

Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR removes all deprecations from the security packages.

Some previously deprecated methods are now marked as @internal (e.g. loadUserByUsername() in the LDAP user provider) in order to remain compatible with Symfony 5.4.

@wouterj wouterj added this to the 6.0 milestone Jun 8, 2021
@wouterj wouterj requested a review from chalasr as a code owner June 8, 2021 11:02
@carsonbot carsonbot changed the title [WIP][Security] Remove everything related to the deprecated authentication manager [Security] [WIP] Remove everything related to the deprecated authentication manager Jun 8, 2021
@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch from db6d1bc to 133675b Compare June 8, 2021 11:18
@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch from 133675b to ea1395b Compare June 8, 2021 12:28
@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch from ea1395b to c5d60fb Compare June 8, 2021 12:30
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉 Thanks Wouter!

Some minor comments below.
Let's split and share the deprecation work that needs to be done for 5.4 before merging this one.
Some minor comments

fabpot added a commit that referenced this pull request Jul 8, 2021
…ion on no token" (wouterj)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate "always authenticate" and "exception on no token"

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Ref #41613
| License       | MIT
| Doc PR        | n/a

Commits
-------

4bba287 [Security] Deprecate "always authenticate" and "exception on no token"
derrabus added a commit that referenced this pull request Jul 14, 2021
…)` (chalasr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `TokenInterface::isAuthenticated()`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | todo

From #41613 (comment)

> all unauthenticated token use-cases have been replaced with passports (and the removal of anonymous). This means that if you have a token, it should always be authenticated.

Commits
-------

33b8fbd [Security] Deprecate `TokenInterface::isAuthenticated()` and `setAuthenticated()`
symfony-splitter pushed a commit to symfony/security-guard that referenced this pull request Jul 14, 2021
…)` (chalasr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `TokenInterface::isAuthenticated()`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | todo

From symfony/symfony#41613 (comment)

> all unauthenticated token use-cases have been replaced with passports (and the removal of anonymous). This means that if you have a token, it should always be authenticated.

Commits
-------

33b8fbdd8a [Security] Deprecate `TokenInterface::isAuthenticated()` and `setAuthenticated()`
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 14, 2021
…)` (chalasr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `TokenInterface::isAuthenticated()`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | todo

From symfony/symfony#41613 (comment)

> all unauthenticated token use-cases have been replaced with passports (and the removal of anonymous). This means that if you have a token, it should always be authenticated.

Commits
-------

33b8fbdd8a [Security] Deprecate `TokenInterface::isAuthenticated()` and `setAuthenticated()`
symfony-splitter pushed a commit to symfony/monolog-bridge that referenced this pull request Jul 14, 2021
…)` (chalasr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `TokenInterface::isAuthenticated()`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | todo

From symfony/symfony#41613 (comment)

> all unauthenticated token use-cases have been replaced with passports (and the removal of anonymous). This means that if you have a token, it should always be authenticated.

Commits
-------

33b8fbdd8a [Security] Deprecate `TokenInterface::isAuthenticated()` and `setAuthenticated()`
symfony-splitter pushed a commit to symfony/security-http that referenced this pull request Jul 14, 2021
…)` (chalasr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `TokenInterface::isAuthenticated()`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | todo

From symfony/symfony#41613 (comment)

> all unauthenticated token use-cases have been replaced with passports (and the removal of anonymous). This means that if you have a token, it should always be authenticated.

Commits
-------

33b8fbdd8a [Security] Deprecate `TokenInterface::isAuthenticated()` and `setAuthenticated()`
chalasr added a commit that referenced this pull request Aug 7, 2021
…rity factories (wouterj)

This PR was merged into the 5.4 branch.

Discussion
----------

[SecurityBundle] Create a smooth upgrade path for security factories

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Ref #41613 (comment)
| License       | MIT
| Doc PR        | -

This change allows removing `SecurityFactoryInterface` in Symfony 6.

I've also changed the discrete ordering using "listener positions" to the much more common continuous ordering using priorities. I feel like priorities are much more self-explanatory.

Commits
-------

7385fd5 [SecurityBundle] Create a smooth upgrade path for security factories
fabpot added a commit that referenced this pull request Aug 8, 2021
This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate legacy signatures

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Ref #41613
| License       | MIT
| Doc PR        | n/a

Deprecates the left-over legacy constructor signatures in the Security system.

Commits
-------

bbc00c8 [Security] Deprecate legacy signatures
@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch from c5d60fb to 199c99c Compare August 12, 2021 10:31
wouterj added a commit that referenced this pull request Aug 17, 2021
…BLIC_ACCESS attribute (wouterj)

This PR was squashed before being merged into the 5.3 branch.

Discussion
----------

[Security] Fix wrong cache directive when using the new PUBLIC_ACCESS attribute

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Ref #41613 (comment)
| License       | MIT
| Doc PR        | -

`PUBLIC_ACCESS` is the new `IS_AUTHENTICATED_ANONYMOUSLY` since 5.2, but we didn't correctly check for this causing a private cache directive for a stateless page.

This PR also includes 2 changes from #42595 that could be backported to 5.3

Commits
-------

ca80ee3 [Security] Fix wrong cache directive when using the new PUBLIC_ACCESS attribute
fabpot added a commit that referenced this pull request Aug 17, 2021
…erj)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

Fix incompatibilities with upcoming security 6.0

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Hats of to the person that invented the flipped tests on a new major branch! All these incompatibility bugs were discovered by the flipped tests of #41613

Commits
-------

96532e5 [SecurityHttp] Fix incompatibility with 6.0
fb45f6b [SecurityGuard] Fix incompatibility with 6.0
d2a1abf [SecurityBundle] Fix incompatibility with 6.0
4628689 [FrameworkBundle] Fix incompatibility with 6.0
98328ad [SecurityHttp] Fix incompatibility with 6.0
9137242 [PasswordHasher] Fix incompatibility with 6.0
915f75b [MonologBridge] Fix incompatibility with 6.0
0b59bc2 [Security] Minor fixes
@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch 2 times, most recently from 1c241ff to 37ea08f Compare August 17, 2021 16:38
@wouterj wouterj changed the title [Security] [WIP] Remove everything related to the deprecated authentication manager [Security] Remove everything related to the deprecated authentication manager Aug 17, 2021
@wouterj
Copy link
Member Author

wouterj commented Aug 17, 2021

🟢 !

The remaining deps=high failures are unrelated to changes in this PR. Also updated the PR description, this is no longer a todo list :)
Happy reviewing! 😉

@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch 3 times, most recently from 026a480 to c242a87 Compare August 18, 2021 09:33
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.

Only minor things on my side after a quick review.
Thank you so much for working on this!

@wouterj wouterj force-pushed the security/remove-authenticator-deprecations branch 2 times, most recently from 4934d38 to 24ad008 Compare August 18, 2021 12:49
@nicolas-grekas nicolas-grekas force-pushed the security/remove-authenticator-deprecations branch from 24ad008 to 922c131 Compare August 18, 2021 13:00
@nicolas-grekas
Copy link
Member

Thank you @wouterj.

@nicolas-grekas nicolas-grekas merged commit 61cf95e into symfony:6.0 Aug 18, 2021
@wouterj wouterj deleted the security/remove-authenticator-deprecations branch August 18, 2021 13:20
@wouterj
Copy link
Member Author

wouterj commented Aug 18, 2021

🎉 thank you for eviewing and merging!

@fabpot fabpot mentioned this pull request Nov 5, 2021
Tobion added a commit that referenced this pull request Apr 20, 2022
This PR was merged into the 6.0 branch.

Discussion
----------

[SecurityBundle] Remove forgotten unused code

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Forgotten in #41613

Commits
-------

9d35f00 [SecurityBundle] Remove forgotten unused code
nicolas-grekas added a commit that referenced this pull request Jun 28, 2024
…ute from the XSD (MatTheCat)

This PR was merged into the 6.4 branch.

Discussion
----------

[SecurityBundle] Remove unused memory users’ `name` attribute from the XSD

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | N/A
| License       | MIT

The related config has been deprecated in #40403 and removed in #41613.

Commits
-------

656f498 [SecurityBundle] Remove unused memory users’ `name` attribute from the XSD
nicolas-grekas added a commit that referenced this pull request Jun 28, 2024
…ibute from the XSD (MatTheCat)

This PR was merged into the 6.4 branch.

Discussion
----------

[SecurityBundle] Remove unused memory users’ `name` attribute from the XSD

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix cdb8354
| License       | MIT

#57520 has been reverted because

> [The] change […] risks that someone receives errors on the next patch release if their XML config still makes use of the now removed attribute.

But nobody could use it since v6.0: as #41613 removed the BC layer, the config would crash.

Keeping `name` in the XSD for 6.4, 7.0 and 7.1 branches means people using these versions would have their IDE suggesting an attribute which would make their app crash.

Commits
-------

ed5c26c [SecurityBundle] Remove unused memory users’ `name` attribute from the XSD
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.

6 participants