Skip to content

[SecurityBundle] Allow accessing remember_me services via FirewallMap #19819

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SecurityBundle] Allow accessing remember_me services via FirewallMap #19819

wants to merge 1 commit into from

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Sep 1, 2016 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #3137, #11158 (not sure)
License MIT
Doc PR todo

This allows to retrieve the RememberMeServices service of the current firewall using:

FirwallMap::getRememberMeServices(Request $request) : RememberMeServicesInterface

An alternative could be to create a public alias of the service.
It seems there are use cases, see the ticket and the ones mentioning it (some from FOSUserBundle).

HeahDude
HeahDude reviewed Sep 1, 2016
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php
public function getListeners(Request $request)
{
$context = $this->getContext($request);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about caching contexts by requests using a SplObjectStorage?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be a good candidate I think!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #20196

@linaori
Copy link
Contributor

linaori commented Sep 2, 2016

So does this encourage developers to avoid the security configuration to do it programmatically outside of the firewall? If so, sounds like a bad idea. All authentication related stuff should go through the firewall. If this is a DX issue, maybe that should be solved instead?

@chalasr
Copy link
Member Author

chalasr commented Sep 2, 2016 •
edited
Loading

@iltar That's not the goal of this PR, although you're right, it can be a side effect.
I didn't just create a public alias of this service nor propose a LoginManager as FOSUB did it for avoiding wrong usages by accessing the service of any firewall . The most relevant use case to me is when using third party libraries for authenticating users such as OAuth, where it can be useful to access the remember me services for persistence. ATM I don't see any alternative for fixing this issue while being restrictive on this point. Any clue?

@linaori
Copy link
Contributor

linaori commented Sep 2, 2016

I don't know any alternative so I guess this is the way to go for the case at hand

@chalasr
Copy link
Member Author

chalasr commented Sep 14, 2016 •
edited
Loading

@iltar Just re-read you.

to avoid the security configuration

This allows to access the remember_me services of the current firewall, involving to have this one secured, so it requires configuration as well as per the classic way (at least setting up a remember_me, which needs a form-positioned security factory configured, a user provider, etc.).
So this just allows to be authenticated by requesting a different endpoint than the login one, e.g. from a registration confirmation sent by email, which is IMHO useful.

BTW, Guard already provides a way to authenticate users programatically (which, contrarily to this, doesn't seem to require any configuration) via GuardAuthenticatorHandler::authenticateUserAndHandleSuccess().

This PR could give a structured way to solve the related issues, and it is IMHO a nice have.

lemoinem
lemoinem suggested changes Sep 28, 2016
View reviewed changes
Copy link
Contributor

@lemoinem lemoinem left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the contribution. If you could take a look at my comment regarding the tests...

If you could also provide a docs PR, I think we would be good to go to Reviewed status.

src/Symfony/Bundle/SecurityBundle/Tests/Security/FirewallContextTest.php
$context = new FirewallContext($listeners, $exceptionListener, $rememberMeServices);

$this->assertEquals(array($listeners, $exceptionListener), $context->getContext());
$this->assertEquals($rememberMeServices, $context->getRememberMeServices());
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't assertSame be better in these cases?

@chalasr
Copy link
Member Author

chalasr commented Sep 28, 2016 •
edited
Loading

@lemoinem I updated the test according to your suggestion.

About the doc PR, I would like to have at least a thought from the core team before submitting it. Doc PRs being most often created after merging the code PR, it should not be a problem for marking this as reviewed.

Thank you for your review.

Status: needs review

lemoinem
lemoinem approved these changes Sep 28, 2016
View reviewed changes
Copy link
Contributor

@lemoinem lemoinem left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@chalasr chalasr mentioned this pull request Oct 10, 2016
Merged
fabpot added a commit that referenced this pull request Oct 12, 2016
@fabpot
minor #20196 [SecurityBundle] Cache contexts per request in FirewallM…
bf2ff76 
…ap (chalasr)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[SecurityBundle] Cache contexts per request in FirewallMap

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19819 (comment)
| License       | MIT
| Doc PR        | n/a

From @HeahDude in #19819 (comment), I propose to store and retrieve `Context` objects per `Request` using `SplObjectStorage`.

At the moment, contexts are consumed by the `Symfony\Components\Security\Http\Firewall` class only, but they could be indirectly by end users if #19490 and/or #19819 come to be merged.

Commits
-------

ffacec1 [SecurityBundle] Cache contexts per request in FirewallMap
@chalasr chalasr force-pushed the rememberme_access branch 2 times, most recently from 528ddae to 5853fdd Compare October 12, 2016 09:23
@chalasr chalasr force-pushed the rememberme_access branch 4 times, most recently from f8cc3a0 to 501f04d Compare November 4, 2016 21:40
@chalasr chalasr mentioned this pull request Nov 5, 2016
Closed
@chalasr
Copy link
Member Author

chalasr commented Nov 5, 2016

Not ready as is.
Status: needs work

@chalasr
Copy link
Member Author

chalasr commented Nov 11, 2016

Closing this as it doesn't look the best way to fix the related tickets.

@chalasr chalasr closed this Nov 11, 2016
@chalasr chalasr deleted the rememberme_access branch November 11, 2016 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemoinem lemoinem lemoinem approved these changes

Assignees
No one assigned
Labels
Feature SecurityBundle Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @linaori @lemoinem @HeahDude @carsonbot