Skip to content
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

[POC][Security] Introduce GuardAuthenticationManager to make guards first-class security #33558

Open
wants to merge 17 commits into
base: master
from

Conversation

@wouterj
Copy link
Member

wouterj commented Sep 11, 2019

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

This is a first PR to kick-start Security improvements. @weaverryan and I have discussed the best steps and we think these changes offer the best experience with the least amount of work for Symfony users.

The Goal

migrate everything in Security to be "Guard" instead of the current providers/listeners/handlers.

  • Each built-in "authentication system" (e.g. form_login or http_basic) would be converted to a "Guard" authenticator internally.
  • This makes internal logic equal to how users write custom security. Internals that are easy to understand result in a component that is easier to customize, use and contribute to.

The Changes

This PR creates a new AuthenticationManagerInterface that is effectively the GuardAuthenticationProvider. This (when activated) would replace the core AuthenticationProviderManager.

  • This PR contains no deprecations. This allows to continue to develop the new Security features during the 5.x release cycle without disturbing Symfony users.
    In 5.4, we can deprecate old Security and remove it in 6.0.
  • The FrameworkBundle integration supports both authentication managers. By setting the guard_authentication_manager option on a firewall to true, a users switches from old security to new security. This is only possible if all "authentication mechanisms" on that firewall have been converted to the Guard system.
  • As the same Guard AuthenticatorInterface is used, systems only supporting new security can be used in older applications through the "old Guard" integration (even back to Symfony 2.6).

Upcoming plans

After this PR (and possibly during the whole 5.x release cycle), Security needs more changes:

  • All old security providers/listeners/handlers have to be transformed to guards (see e.g. the HttpBasic refactoring in this PR).
  • The Guard sub-component needs to be integrated with Core.
  • Meanwhile, the current AuthenticatorInterface needs to be rediscussed and modified to support both HTTP and non-HTTP authentication, as well as probably non-user authentication.
  • The UserInterface needs to be tweaked (i.e. moving password related methods to a new UserWithPasswordInterface)
  • The same thing this PR does with AuthenticationManager needs to be done with Firewall as well: The logic of the Guard listener should be moved to the Firewall class
@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch 2 times, most recently from 8960e3a to b485767 Sep 11, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Sep 12, 2019
@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch from b485767 to 93ce631 Sep 15, 2019
@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Sep 15, 2019

Just note:
A user should also be able to use the provider/listener/handler/factory implementation. I love Guard but sometimes it is not enough to work with my super special authentication.

I recently had a scenario where I get a user if passed with an authenticated jwt token. I should create a user entity if it missing (by using data in auth0), if email is valid I create an authenticated token, if not, I will ask the user to login (with different authenticator).

I was not able to write a nice solution with guard for this scenario, so I had to use the “low level code”.

I’m all for promoting guard and trying to make it better, but I think it would be use the more flexible solution when needed.

@Koc

This comment has been minimized.

Copy link
Contributor

Koc commented Sep 17, 2019

Good movement, but AFIK it is impossible to define custom config nodes with guards. Should we add method similar to addConfiguration(NodeDefinition $builder) inside GuardFactoryInterface like already done in SecurityFactoryInterface?

@wouterj

This comment has been minimized.

Copy link
Member Author

wouterj commented Sep 21, 2019

I’m all for promoting guard and trying to make it better, but I think it would be use the more flexible solution when needed.

Hmm, interesting. Our assumption has always been that Guard is not less flexible than the current listener-provider solution. Guard only combines them into one class, which makes it more difficult to reuse partial systems (i.e. the DaoAuthenticationProvider).

I do not fully follow your requirements, but it would be interesting to see whether or not Guard (or a modification in Guard) could have fixed your requirements.


For now, I would say it doesn't matter as this PR is not about deprecating the old system. It's only introducing a new system next to it, to allow experimenting (and discovering exactly cases like you mentioned). For that reason, it's maybe a good idea to mark the new classes as @experimental (or do we only mark complete components as experimental?)

@fabpot @nicolas-grekas @weaverryan what should we do to get this into the next release (probably not 4.4, but 5.0?)

@wouterj

This comment has been minimized.

Copy link
Member Author

wouterj commented Sep 21, 2019

Good movement, but AFIK it is impossible to define custom config nodes with guards. Should we add method similar to addConfiguration(NodeDefinition $builder) inside GuardFactoryInterface like already done in SecurityFactoryInterface?

In order to be usable a class should as of now implement both interfaces, so the addConfiguration() method is added by the other interface. For clarity, we can add the methods to the new interface as well.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 21, 2019

Making it experimental means that it cannot be part of 4.4. But it can be part of 5.0.

@symfony symfony deleted a comment from wouterj Oct 10, 2019
@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch from 93ce631 to b21b514 Dec 13, 2019
@wouterj wouterj requested review from dunglas, lyrixx, sroze and xabbuh as code owners Dec 13, 2019
@wouterj wouterj changed the base branch from 4.4 to master Dec 14, 2019
@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch 3 times, most recently from 959fbbf to dc68936 Jan 26, 2020
if ($this->guardAuthenticationManagerEnabled) {
$authenticationManagerId = 'security.authentication.manager.guard';
$container->setAlias('security.authentication.manager', new Alias($authenticationManagerId));
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jan 31, 2020

Member

I wonder if guardAuthenticationManagerEnabled is true, if we should remove (or not load) the old system/services. Like, you're switching fully between the two systems for now. It might (?) make converting to the system easier - as once you opt into it, you are fully into it (and it's impossible to use the old system)?

@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch from 79ea1eb to 2b2d17f Feb 6, 2020
wouterj added 12 commits Jan 26, 2020
This is an iteration on the AuthenticatorInterface of the Guard, to allow more
flexibility so it can be used as a real replaced of the authentication
providers and listeners.
This is required to create the correct authenticated token in the
GuardAuthenticationManager.
This removes the introduced dependency on Guard from core. It also allows an
easier migration path, as the complete Guard subcomponent can now be deprecated
later in the 5.x life.
This allows more flexibility for the authentication manager (to e.g. implement
login throttling, easier remember me, etc). It is also a known design pattern
in Symfony HttpKernel.
This to remove confusion between the new system and Guard. When using the new
system, guard should not be installed. Guard did however influence the idea
behind the new system. Thus keeping the mentions of "guard" makes it confusing
to use the new system.
@wouterj wouterj force-pushed the wouterj:security/deprecate-providers-listeners branch from 2b2d17f to 7799078 Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.