NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Security] Make security.providers optional#26787

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromMatTheCat:ticket_21998

Apr 19, 2018

Merged

[Security] Make security.providers optional#26787

fabpot merged 1 commit intosymfony:masterfromMatTheCat:ticket_21998

Apr 19, 2018

Conversation

Copy link

Contributor

MatTheCat commentedApr 4, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21998
License	MIT

Don't really know if it's viable but I just hit#21998 so I would like to tackle this.

carsonbot added Status: Needs Review Security Feature labels

Apr 4, 2018

nicolas-grekas added this to the4.1 milestone

Apr 4, 2018

Copy link

ContributorAuthor

MatTheCat commentedApr 6, 2018

@chalasr @stof do you think this can cause issues?

Also I don't know how to provide a proper error message in case a provider is needed but none is configured.

Copy link

Member

chalasr commentedApr 6, 2018•
edited
Loading

Very complex topic, looks like you like them :) (#24805)

@weaverryan and me have this in mind for a while, the issue is that actually pretty much all security listeners are requiring a user provider for user loading/refreshment.
This misses new test case(s) (authentication with no configured provider), adding them will better answer your question.

Copy link

ContributorAuthor

MatTheCat commentedApr 6, 2018•
edited
Loading

Yes the way we'll authenticate our users makes me discover some edge cases!

the issue is that actually pretty much all security listeners are requiring a user provider for user loading/refreshment.

I don't understand how this is an issue?

chalasr reviewed

Apr 6, 2018

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php Outdated

		/**
		* @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
		* @expectedExceptionMessage Not configuring explicitly the provider for the "http_basic" listener on "ambiguous" firewall is ambiguous as there is more than one registered provider.
		* @expectedException \TypeError

Copy link

Member

chalasrApr 6, 2018

Choose a reason for hiding this comment

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

getting aTypeError when not setting the provider isn't fine to me as this aims to make it optional

Copy link

ContributorAuthor

MatTheCatApr 6, 2018

Choose a reason for hiding this comment

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

Yes I agree we need a proper error but I don't know when to throw it.

Copy link

ContributorAuthor

MatTheCat commentedApr 6, 2018

I created a dummy user provider whose sole purpose is to throw an exception, WDYT?

Copy link

Member

weaverryan commentedApr 9, 2018

Huh, a dummy provider is a really clever idea - it makes this very feasible (and sure, we could try to rip out the guts of the user provider system later... but that will be super hard and will probably never happen).

I like this!

Copy link

ContributorAuthor

MatTheCat commentedApr 9, 2018

Do we need a functional test where authentication is done without user provider?

Copy link

Member

chalasr commentedApr 12, 2018

@MatTheCat The config test added here should be enough

Copy link

ContributorAuthor

MatTheCat commentedApr 13, 2018

Are we good to go then or are some things missing?

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 17, 2018

Copy link

ContributorAuthor

MatTheCat commentedApr 17, 2018

Is it alright to letBadMethodCallExceptions empty?

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Apr 17, 2018

fabpot requested changes

Apr 17, 2018

View reviewed changes

src/Symfony/Component/Security/Core/User/MissingUserProvider.php Outdated

		thrownewInvalidConfigurationException(sprintf(
		'"%s" firewall requires a user provider but none was defined.',
		$firewall
		));

Copy link

Member

fabpotApr 17, 2018

Choose a reason for hiding this comment

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

small thing: should be on one line.

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated

		$container->setDefinition(
		$userProvider,
		(newChildDefinition('security.user.provider.missing'))->replaceArgument(0,$id)
		);

Copy link

Member

fabpotApr 17, 2018

Choose a reason for hiding this comment

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

CS fix: should be on one line.

Copy link

ContributorAuthor

MatTheCatApr 17, 2018

Choose a reason for hiding this comment

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

Oh okay.

chalasr reviewed

Apr 17, 2018

View reviewed changes

src/Symfony/Component/Security/Core/User/MissingUserProvider.php Outdated

		/**
		* @param string $firewall the firewall missing a provider
		*/
		publicfunction__construct($firewall)

Copy link

Member

chalasrApr 17, 2018

Choose a reason for hiding this comment

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

string typehint

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Apr 18, 2018

chalasr approved these changes

Apr 18, 2018

View reviewed changes

fabpot approved these changes

Apr 19, 2018

View reviewed changes

[Security] Make security.providers optional

ee54bfa

Copy link

Member

fabpot commentedApr 19, 2018

Thank you@MatTheCat.

fabpot merged commitee54bfa intosymfony:master

Apr 19, 2018

fabpot added a commit that referenced this pull request

Apr 19, 2018

feature#26787[Security] Make security.providers optional (MatTheCat)

8c4fd12

This PR was squashed before being merged into the 4.1-dev branch (closes#26787).Discussion----------[Security] Make security.providers optional| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21998| License       | MITDon't really know if it's viable but I just hit#21998 so I would like to tackle this.Commits-------ee54bfa [Security] Make security.providers optional