8000 [Security] Allow to set a check_path on json_login listener by chalasr · Pull Request #22425 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Allow to set a check_path on json_login listener #22425

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 2 commits into from
Closed

[Security] Allow to set a check_path on json_login listener #22425

wants to merge 2 commits into from

Conversation

chalasr
Copy link
Member
@chalasr chalasr commented Apr 13, 2017 •
edited
Loading
Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no, master only
Deprecations? no
Tests pass? yes
Fixed tickets #21948, #22423
License MIT
Doc PR n/a

The listener should allow to restrict authentication to a given check_path, as stated in the docs http://symfony.com/doc/master/security/json_login_setup.html

@chalasr chalasr force-pushed the json-login-checkpath branch from b0593b6 to 2b2f9fe Compare April 13, 2017 18:09
@chalasr chalasr mentioned this pull request Apr 13, 2017
Closed
@chalasr chalasr force-pushed the json-login-checkpath branch from 2b2f9fe to 7cb7a0b Compare April 13, 2017 18:18
@chalasr chalasr changed the title Add check_path option to json_login listener Allow to set a check_path on json_login listener Apr 13, 2017
@lsmith77
Copy link
Contributor

as pointed out in #21948 I think it makes sense to make this feature stateful and then it makes sense to support the check_path properly.

another optional thing one could add to this is a check if the Content-type of the request is a json request .. something like:

if ('json' !== $request->getRequestFormat()) {
    return;
}

@chalasr
Copy link
Member Author
chalasr commented Apr 13, 2017 •
edited
Loading

@lsmith77 I just made the check_path optional, because people may effectively use this on every request.

My personal use case is the following:
a) Send username/password to e.g. /login_check (check_path) 10000 and if everything is ok, get a token in the success response (e.g. JWT)
b) Pass the token for each request which requires one

In this case, no need for both token and username/password authentications to be stateful. What I need is to be able to define the path that this listener should cover. I think most use cases for this listener will look like mine. But for those who want their users to resend username/password on each request, why not.

@lsmith77
Copy link
Contributor

my point above regarding checking the content type is so that one could use form_login and json_login in parallel on the same routes and within the same firewall

@chalasr chalasr force-pushed the json-login-checkpath branch 2 times, most recently from 864eb94 to 407a081 Compare April 13, 2017 18:35
@chalasr
Copy link
Member Author
chalasr commented Apr 13, 2017

Sounds good to me. I think it should be discussed separately though, I'll open another PR if you don't do it before.

dmaicher
dmaicher reviewed Apr 13, 2017
View reviewed changes
src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated
@@ -70,6 +73,10 @@ public function handle(GetResponseEvent $event)
$request = $event->getRequest();
$data = json_decode($request->getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved below the check? Not that expensive but still.

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, fixed

@chalasr chalasr force-pushed the json-login-checkpath branch from 407a081 to 9d30800 Compare April 13, 2017 18:50
@chalasr chalasr force-pushed the json-login-checkpath branch from 9d30800 to 7e6c261 Compare April 13, 2017 18:51
chalasr
chalasr commented Apr 13, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/JsonLogin/config.yml
@@ -16,7 +16,7 @@ security:
pattern: ^/
anonymous: true
json_login:
check_path: /mychk
check_path: /chk
Copy link
Member Author

Choose a reason for hiding this comment

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

looks like the author expected the check_path to work already :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed 😊 I think initially it was not stateless and used the check_path:

#18952 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it was the idea.

@chalasr chalasr changed the title Allow to set a check_path on json_login listener [Security] Allow to set a check_path on json_login listener Apr 13, 2017
@dunglas
Copy link
Member
dunglas commented Apr 13, 2017

I propose to mark this feature as @experimental for now.

@chalasr
Copy link
Member Author
chalasr commented Apr 13, 2017

If we talk about the whole json_login listener, I agree. Shall I do it here?

@lsmith77
Copy link
Contributor

BTW the constructor signature now looks pretty much like that of AbstractAuthenticationListener .. with the main difference left is that the options I guess are not all the same.

@chalasr
Copy link
Member Author
chalasr commented Apr 13, 2017

@lsmith77 The abstract one offers a bit too much "stateful" stuff, its main methods couldn't be reused as is. I would keep this one standalone right now, flagging as experimental will give time to factorize as needed anyway.

@lsmith77
Copy link
Contributor

sure .. thought it might mean the abstract class should maybe be refactored at some point

@chalasr
Copy link
Member Author
chalasr commented Apr 13, 2017

@dunglas flag added in e94b5e4

phansys
phansys suggested changes Apr 14, 2017
View reviewed changes
Copy link
Contributor
@phansys phansys left a comment

Choose a reason for hiding this comment

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

Status: Needs work.

...ny/Component/Security/Http/Tests/Firewall/UsernamePasswordJsonAuthenticationListenerTest.php Outdated
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);

$this->listener->handle($event);
$this->assertEquals('ok', $event->getResponse()->getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use assertSame() instead.

@chalasr chalasr force-pushed the json-login-checkpath branch from e94b5e4 to e4533f8 Compare April 15, 2017 11:07
@chalasr
Copy link
Member Author
chalasr commented Apr 15, 2017

@phansys fixed.

lsmith77
lsmith77 approved these changes Apr 17, 2017
View reviewed changes
Copy link
Contributor
@lsmith77 lsmith77 left a comment •
edited by fabpot
Loading

Choose a reason for hiding this comment

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

👍

I will propose a content type check in another PR

@fabpot
Copy link
Member
fabpot commented Apr 18, 2017

Thank you @chalasr.

@fabpot fabpot closed this in 6c7bced Apr 18, 2017
@chalasr chalasr deleted the json-login-checkpath branch April 19, 2017 07:32
@lsmith77 lsmith77 mentioned this pull request Apr 19, 2017
Merged
@lsmith77
Copy link
Contributor

as promised here is the follow up #22477

fabpot added a commit that referenced this pull request Apr 29, 2017
@fabpot
minor #22477 [Security] add Request type json check in json_login (ls…
35608f5 
…mith77)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] add Request type json check in json_login

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no, unreleased feature
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | -

follow up to #22425 to limit the `UsernamePasswordJsonAuthenticationListener` to only requests with appropriate JSON content type.

I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered.

Commits
-------

045a36b add Request type json check in json_login
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Apr 29, 2017
@fabpot
minor #22477 [Security] add Request type json check in json_login (ls…
7075d54 
…mith77)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] add Request type json check in json_login

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no, unreleased feature
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | -

follow up to symfony/symfony#22425 to limit the `UsernamePasswordJsonAuthenticationListener` to only requests with appropriate JSON content type.

I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered.

Commits
-------

045a36b303 add Request type json check in json_login
symfony-splitter pushed a commit to symfony/security-http that referenced this pull request Apr 29, 2017
@fabpot
minor #22477 [Security] add Request type json check in json_login (ls…
9af67f5 
…mith77)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] add Request type json check in json_login

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no, unreleased feature
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | -

follow up to symfony/symfony#22425 to limit the `UsernamePasswordJsonAuthenticationListener` to only requests with appropriate JSON content type.

I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered.

Commits
-------

045a36b303 add Request type json check in json_login
symfony-splitter pushed a commit to symfony/security that referenced this pull request Apr 29, 2017
@fabpot
minor #22477 [Security] add Request type json check in json_login (ls…
16b418f 
…mith77)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] add Request type json check in json_login

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no, unreleased feature
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | -

follow up to symfony/symfony#22425 to limit the `UsernamePasswordJsonAuthenticationListener` to only requests with appropriate JSON content type.

I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered.

Commits
-------

045a36b303 add Request type json check in json_login
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

@lsmith77 lsmith77 lsmith77 approved these changes

@dmaicher dmaicher dmaicher approved these changes

@phansys phansys phansys approved these changes

Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chalasr @lsmith77 @dunglas @fabpot @dmaicher @phansys @carsonbot
0