Skip to content

[Security] deprecate non UserInterface users #34909

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

Closed
wants to merge 1 commit into from

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Dec 10, 2019

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? yes
Tickets #34723 (comment)
License MIT
Doc PR

@@ -76,6 +76,10 @@ public function setUser($user)
throw new \InvalidArgumentException('$user must be an instanceof UserInterface, an object implementing a __toString method, or a primitive string.');
}

if (!$user instanceof UserInterface) {
@trigger_error(sprintf('Storing a user not implementing the %s is deprecated since Symfony 5.1.', UserInterface::class));
Copy link
Contributor

@linaori linaori Dec 10, 2019

Choose a reason for hiding this comment

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

Wouldn't this cause deprecations on anonymous authentications? The AnonymousToken also calls setUser(). As proposed back in #3697, perhaps an AnonymousUser would make sense? This could be a stale object always returning the same information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for me pushed code should just be the starting point. Actually, I was a bit surprised that our code did not trigger any deprecations. Might be an indicator that we do miss some tests (the anonymous user is one of the examples where I did expect the new deprecation to be triggered).

Copy link
Contributor

Choose a reason for hiding this comment

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

my proposal applied to ControllerTrait::getUser(), to keep it consistent with Security::getUser().

no need to patch TokenInterface IMHO, as it's low-level API, unless we decide to refactor everything now..? 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is good to make the usage consistent for now. At a later stage, I hope we can get rid of the UserInterface inside the token altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ro0NL IMO it would be too confusing if the controller method behaved differently than the token storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's confusing for Security::getUser to behave different already.

They way i see it is, Security/ControllerTrait::getUser() is high-level API, designed around UserInterface|null.

Whereas TokenInterface::getUser(), the low-level API, is designed around object|string|null.

So the high level API is a subset.

fabpot added a commit that referenced this pull request Apr 21, 2020
…s" first-class security (wouterj)

This PR was squashed before being merged into the 5.1-dev branch.

Discussion
----------

[Security] AuthenticatorManager to make "authenticators" first-class security

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

The tl;dr
---

The old authentication listener + authentication provider system was replaced by a new "authenticator" system (similar to Guard authentication). All existing "auth systems" (e.g. `form_login` are now written as an "authenticator" in core).

Instead of each "authentication system" registering its own listener in the `Firewall`, there is now only one listener: `AuthenticatorManagerListener`

* `Firewall` -> executes `AuthenticatorManagerListener`
* `AuthenticatorManagerListener` -> calls `AuthenticatorManager`
* `AuthenticatorManager` -> calls each authenticator

This PR contains *no deprecations* and the "new system" is *marked as experimental*. This allows to continue to develop the new Security system during the 5.x release cycle without disturbing Symfony users. In 5.4, we can deprecate "old" Security and remove it completely in 6.0.

Important Decisions
---

* A) **The new authentication manager - `AuthenticatorManager` - now dispatches 3 important "hook" events**:

  * `VerifyAuthenticatorCredentialsEvent`: occurs at the point when a "password" needs to be checked. Allows us to centralize password checking, CSRF validation, password upgrading and the "user checker" logic.
  * `LoginSuccessEvent`: Dispatched after a successful authentication. E.g. used by remember me listener.
  * `LoginFailedEvent`: Dispatched after an unsuccessful authentication. Also used by remember me (and in theory could be used for login throttling).

* B) **`getCredentials()`, `getUser()` and `checkCredentials()` methods from old Guard are gone: their logic is centralized**.
   Authenticators now have an `authenticate(Request $request): PassportInterface` method. A passport contains the user object, the credentials and any other add-in Security badges (e.g. CSRF):

   ```php
   public function authenticate(Request $request): PassportInterface
   {
       return new Passport(
           $user,
           new PasswordCredentials($request->get('_password')),
           [
               new CsrfBadge($request->get('_token'))
           ]
       );
   }
   ```

   All badges (including the credentials) need to be resolved by listeners to `VerifyAuthenticatorCredentialsEvent`. There is build-in core support for the following badges/credentials:

   * `PasswordCredentials`: validated using the password encoder factory
   * `CustomCredentials`: allows a closure to do credentials checking
   * `CsrfTokenBadge`: automatic CSRF token verification
   * `PasswordUpgradeBadge`: enables password migration
   * `RememberMeBadge`: enables remember-me support for this authenticator

* C) **`AuthenticatorManager` contains all logic to authenticate**
  As authenticators always relate to HTTP, the `AuthenticatorManager` contains all logic to authenticate. It has three methods, the most important two are:

  * `authenticateRequest(Request $request): TokenInterface`: Doing what is previously done by a listener and an authentication provider;
  * `authenticateUser(UserInterface $user, AuthenticatorInterface $authenticator, Request $request, array $badges = [])` for manual login in e.g. a controller.

* D) **One AuthenticatorManager per firewall**
  In the old system, there was 1 authentication manager containing all providers and each firewall had a specific firewall listener. In the new system, each firewall has a specific authentication manager.

* E) **Pre-authentication tokens are dropped.**
  As everything is now handled inside `AuthenticatorManager` and everything is stored in the Security `Passport`, there was no need for a token anymore (removing lots of confusion about what information is inside the token).

  This change deprecates 2 authentication calls: one in `AuthorizationChecker#isGranted()` and one in `AccessListener`.  These seem now to be mis-used to reload users (e.g. re-authenticate the user after you change their roles). This (some "way" to change a user's roles *without* logging them out) needs to be "fixed"/added in another PR.

* F) **The remember me service now uses *all* user providers**
  Previously, only user providers of authentication providers listening on that firewall were used. This change is due to practical reasons and we don't think it is common to have 2 user providers supporting the same user instance. In any case, you can always explicitly configure the user provider under `remember_me`.

* G) **Auth Providers No Longer Clear the Token on Auth Failure**
  Previously, authentication providers did `$this->tokenStorage->setToken(null)` upon authentication failure. This is not yet implemented: our reasoning is that if you've authenticated successfully using e.g. the login form, why should you be logged out if you visit the same login form and enter wrong credentials?
  The pre-authenticated authenticators are an exception here, they do reset the token upon authentication failure, just like the old system.

* H) **CSRF Generator Service ID No Longer Configurable**
  The old Form login authentication provider allowed you to configure the CSRF generator service ID. This is no longer possible with the automated CSRF listener. This feature was introduced in the first CSRF commit and didn't get any updates ever since, so we don't think this feature is required. This could also be accomplished by checking CSRF manually in your authenticator, instead of using the automated check.

Future Considerations
---

* Remove Security sub-components: Move CSRF to `Symfony\Component\Csrf` (just like mime); Deprecated Guard; Put HTTP + Core as `symfony/security`. This means moving the new classes to `Symfony\Component\Security`

* Convert LDAP to the new system

* This is fixed (and merged) by #36243 <s>There is a need for some listeners to listen for events on one firewall, but not another (e.g. `RememberMeListener`). This is now fixed by checking the `$providerKey`. We thought it might be nice to introduce a feature to the event dispatcher:</s>

  * <s>Create one event dispatcher per firewall;</s>
  * <s>Extend the `kernel.event_subscriber` tag, so that you can optionally specify the dispatcher service ID (to allow listening on events for a specific dispatcher);</s>
  * <s>Add a listener that always also triggers the events on the main event dispatcher, in case you want a listener that is listening on all firewalls.</s>

* Drop the `AnonymousToken` and `AnonymousAuthenticator`: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (`null`). This require changes in the `AccessListener` and `AuthorizationChecker` and probably also a new Security attribute (to replace `IS_AUTHENTICATED_ANONYMOUSLY`). Related issues: #34909, #30609

> **How to test**
> 1. Install the Symfony demo application (or any Symfony application)
> 2. Clone my Symfony fork (`git clone git@github.com:wouterj/symfony`) and checkout my branch (`git checkout security/deprecate-providers-listeners`)
> 3. Use the link utility to link my fork to the Symfony application: `/path/to/symfony-fork/link /path/to/project`
> 4. Enable the new system by setting `security.enable_authenticator_manager` to `true`

Commits
-------

b1e040f Rename providerKey to firewallName for more consistent naming
50224aa Introduce Passport & Badges to extend authenticators
9ea32c4 Also use authentication failure/success handlers in FormLoginAuthenticator
0fe5083 Added JSON login authenticator
7ef6a7a Use the firewall event dispatcher
95edc80 Added pre-authenticated authenticators (X.509 & REMOTE_USER)
f5e11e5 Reverted changes to the Guard component
ba3754a Differentiate between interactive and non-interactive authenticators
6b9d78d Added tests
59f49b2 Rename AuthenticatingListener
60d396f Added automatically CSRF protected authenticators
bf1a452 Merge AuthenticatorManager and AuthenticatorHandler
44cc76f Use one AuthenticatorManager per firewall
09bed16 Only load old manager if new system is disabled
ddf430f Added remember me functionality
1c810d5 Added support for lazy firewalls
7859977 Removed all mentions of 'guard' in the new system
999ec27 Refactor to an event based authentication approach
b14a5e8 Moved new authenticator to the HTTP namespace
b923e4c Enabled remember me for the GuardManagerListener
873b949 Mark new core authenticators as experimental
4c06236 Fixes after testing in Demo application
fa4b3ec Implemented password migration for the new authenticators
5efa892 Create a new core AuthenticatorInterface
5013258 Add provider key in PreAuthenticationGuardToken
526f756 Added GuardManagerListener
a172bac Added FormLogin and Anonymous authenticators
9b7fddd Integrated GuardAuthenticationManager in the SecurityBundle
a6890db Created HttpBasicAuthenticator and some Guard traits
c321f4d Created GuardAuthenticationManager to make Guard first-class Security
fabpot added a commit that referenced this pull request May 3, 2020
…m (wouterj)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Security] Removed anonymous in the new security system

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

This was one of the "Future considerations" of #33558:

> Drop the AnonymousToken and AnonymousAuthenticator: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (null). This require changes in the AccessListener and AuthorizationChecker and probably also a new Security attribute (to replace IS_AUTHENTICATED_ANONYMOUSLY). Related issues: #34909, #30609

This new experimental system is probably a once-in-a-lifetime change to make this change. @weaverryan and I have had some brainstorming about this. Some reasons why we think it makes 100% sense to do this change:

* From a Security perspective, **a user that is not authenticated is similar to an "unknown" user**: They both have no rights at all.
* **The higher level consequences of the AnonymousToken are confusing and inconsistent**:
  * It's hard to explain people new to Symfony Security that not being logged in still means you're authenticated within the Symfony app
  * To counter this, some higher level APIs explicitly mark anonymous tokens as not being authenticated, see e.g. the [`is_authenticated()` expression language function](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/ExpressionLanguageProvider.php#L33-L37)
  * The anonymous authentication resulted in the `IS_AUTHENTICATED` security attribute being removed from #35854, as there was no clear consensus on what its meaning should be
* **Spring Security, which is where this originated from, makes Anonymous a very special case**:

  > Finally, there is an AnonymousAuthenticationFilter, which is chained after the normal authentication mechanisms and automatically adds an AnonymousAuthenticationToken to the SecurityContextHolder if there is no existing Authentication held there.
  >
  > Note that there is no real conceptual difference between a user who is “anonymously authenticated” and an unauthenticated user. Spring Security's anonymous authentication just gives you a more convenient way to configure your access-control attributes. Calls to servlet API calls such as getCallerPrincipal, for example, will still return null even though there is actually an anonymous authentication object in the SecurityContextHolder.
* Symfony uses AnonymousToken much more than "just for convience in access-control attributes". **Removing anonymous tokens allows us to move towards only allowing `UserInterface` users**: #34909

---

Removing anonymous tokens do have an impact on `AccessListener` and `AuthorizationChecker`. These currently throw an exception if there is no token in the storage, instead of treating them like "unknown users" (i.e. no roles). See #30609 on a RFC about removing this exception. We can also see e.g. the [Twig `is_granted()` function explicitly catching this exception](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/SecurityExtension.php#L37-L52).

* **To make the changes in `AccessListener` and `AuthorizationChecker` BC, a flag has been added - default enabled - to throw an exception when no token is present** (which is automatically disabled when the new system is used). In Symfony 5.4 (or whenever the new system is no longer experimental), we can deprecate this flag and in 6.0 we can never throw the exception anymore.
* **`anonymous: lazy` has been deprecated in favor of `{ anonymous: true, lazy: true }`** This fixes the dependency on `AnonymousFactory` from the `SecurityExtension` and allows removing the `anonymous` option.
* **Introduced `PUBLIC_ACCESS` Security attribute** as alternative of `IS_AUTHENTICATED_ANONYMOUSLY`. Both work in the new system, the latter only triggers a deprecation notice (but may be usefull to allow switching back and forth between old and new system).

cc @javiereguiluz you might be interested, as I recently talked with you about this topic

Commits
-------

ac84a6c Removed AnonymousToken from the authenticator system
@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

@wouterj Can you have a look at this one and decide what to do?

@wouterj
Copy link
Member

wouterj commented Aug 13, 2020

I think this is a nice experiment to continue (but then only for the new system). The new system has a couple changes making this PR a lot less complicated:

  • No more anonymous;
  • Tokens are only for authenticated users (so no need for string anymore).

In my opinion, UserInterface should not be there in 6.0, we should instead create a new interface (removing salt, password and maybe renaming getUsername to a more describtive name). However, that requires making all users the same interface first (the goal of this PR). This PR also allows us to find a place where we can make future user interface changes (i.e. trigger deprecations when using the old user interface).

TL;DR: My idea of a roadmap:

  1. Continue this PR forcing all users to implement UserInterface;
  2. Create a modern user interface and add a BC layer in the places modified by this PR.

@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

Making it on the new security system only is fine by me.

@xabbuh Do you want to rebase on current master to benefit from the latest changes we made (like removing the anonymous token)?

@fabpot fabpot closed this Oct 7, 2020
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

fabpot added a commit that referenced this pull request Aug 13, 2021
… users, and token credentials (wouterj)

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

Discussion
----------

[Security] Deprecate AnonymousToken, non-UserInterface users, and token credentials

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

This is a continuation of `@xabbuh`'s experiment in #34909 and `@chalasr`'s work in #42050. This hopefully is the last cleanup of `TokenInterface`:

* As tokens now always represent an authenticated user (and no longer e.g. the "username" input of the form), we can finally remove the weird `string|\Stringable` union from `Token::getUser()` and other helper methods and require a user to be an instance of `UserInterface`.
* For the same reason, we can also deprecate token credentials. I didn't deprecate `Token::eraseCredentials()` as this is still used to remove credentials from `UserInterface`.
* Meanwhile, this also deprecated the `AnonymousToken`, which we forgot in 5.3. This token is not used anymore in the new system (anonymous does no longer exists). This was also the only token in core that didn't fulfill the `UserInterface` requirement for authenticated tokens.

Commits
-------

44b843a [Security] Deprecate AnonymousToken, non-UserInterface users, and token credentials
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.

7 participants