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

[Security] Make security.providers optional #26787

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
6 participants
@MatTheCat
Contributor

MatTheCat commented Apr 4, 2018

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.

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 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.

@chalasr

This comment has been minimized.

Member

chalasr commented Apr 6, 2018

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.

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 6, 2018

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?

@@ -166,8 +166,7 @@ public function testPerListenerProvider()
}
/**
* @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

This comment has been minimized.

@chalasr

chalasr Apr 6, 2018

Member

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

This comment has been minimized.

@MatTheCat

MatTheCat Apr 6, 2018

Contributor

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

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 6, 2018

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

@weaverryan

This comment has been minimized.

Member

weaverryan commented Apr 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!

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 9, 2018

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

@chalasr

This comment has been minimized.

Member

chalasr commented Apr 12, 2018

@MatTheCat The config test added here should be enough

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 13, 2018

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

@MatTheCat

This comment has been minimized.

Contributor

MatTheCat commented Apr 17, 2018

Is it alright to let BadMethodCallExceptions empty?

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

This comment has been minimized.

@fabpot

fabpot Apr 17, 2018

Member

small thing: should be on one line.

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

This comment has been minimized.

@fabpot

fabpot Apr 17, 2018

Member

CS fix: should be on one line.

This comment has been minimized.

@MatTheCat

MatTheCat Apr 17, 2018

Contributor

Oh okay.

/**
* @param string $firewall the firewall missing a provider
*/
public function __construct($firewall)

This comment has been minimized.

@chalasr

chalasr Apr 17, 2018

Member

string typehint

@fabpot

fabpot approved these changes Apr 19, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 19, 2018

Thank you @MatTheCat.

@fabpot fabpot merged commit ee54bfa into symfony:master Apr 19, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 19, 2018

feature #26787 [Security] Make security.providers optional (MatTheCat)
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       | MIT

Don'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

@MatTheCat MatTheCat deleted the MatTheCat:ticket_21998 branch Apr 19, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment