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

chalasr · 2017-04-13T18:08:48Z

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

lsmith77 · 2017-04-13T18:19:38Z

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 · 2017-04-13T18:25:05Z

@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) 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 · 2017-04-13T18:28:48Z

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 · 2017-04-13T18:42:27Z

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 · 2017-04-13T18:43:22Z

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php

@@ -70,6 +73,10 @@ public function handle(GetResponseEvent $event)
        $request = $event->getRequest();
        $data = json_decode($request->getContent());


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

indeed, fixed

chalasr · 2017-04-13T18:52:10Z

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


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

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

#18952 (comment)

Yes, it was the idea.

dunglas · 2017-04-13T19:42:00Z

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

chalasr · 2017-04-13T19:46:22Z

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

lsmith77 · 2017-04-13T20:17:06Z

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 · 2017-04-13T20:22:51Z

@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 · 2017-04-13T20:32:35Z

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

chalasr · 2017-04-13T20:50:01Z

@dunglas flag added in e94b5e4

phansys

Status: Needs work.

phansys · 2017-04-14T20:16:00Z

...ny/Component/Security/Http/Tests/Firewall/UsernamePasswordJsonAuthenticationListenerTest.php

+        $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
+
+        $this->listener->handle($event);
+        $this->assertEquals('ok', $event->getResponse()->getContent());


Please, use assertSame() instead.

chalasr · 2017-04-15T11:09:01Z

@phansys fixed.

lsmith77

👍

I will propose a content type check in another PR

fabpot · 2017-04-18T22:21:42Z

Thank you @chalasr.

lsmith77 · 2017-04-19T20:24:30Z

as promised here is the follow up #22477

…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

…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

carsonbot added Status: Needs Review Bug labels Apr 13, 2017

chalasr force-pushed the json-login-checkpath branch from b0593b6 to 2b2f9fe Compare April 13, 2017 18:09

chalasr mentioned this pull request Apr 13, 2017

[Security] json_login tries to handle every request #22423

Closed

chalasr force-pushed the json-login-checkpath branch from 2b2f9fe to 7cb7a0b Compare April 13, 2017 18:18

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

chalasr force-pushed the json-login-checkpath branch 2 times, most recently from 864eb94 to 407a081 Compare April 13, 2017 18:35

dmaicher reviewed Apr 13, 2017

View reviewed changes

chalasr force-pushed the json-login-checkpath branch from 407a081 to 9d30800 Compare April 13, 2017 18:50

Add check_path option to json_login listener

7e6c261

chalasr force-pushed the json-login-checkpath branch from 9d30800 to 7e6c261 Compare April 13, 2017 18:51

chalasr commented Apr 13, 2017

View reviewed changes

dunglas approved these changes Apr 13, 2017

View reviewed changes

dmaicher approved these changes Apr 13, 2017

View reviewed changes

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

phansys suggested changes Apr 14, 2017

View reviewed changes

Flag the json_login listener as experimental

e4533f8

chalasr force-pushed the json-login-checkpath branch from e94b5e4 to e4533f8 Compare April 15, 2017 11:07

lsmith77 approved these changes Apr 17, 2017

View reviewed changes

phansys approved these changes Apr 17, 2017

View reviewed changes

fabpot closed this in 6c7bced Apr 18, 2017

chalasr deleted the json-login-checkpath branch April 19, 2017 07:32

lsmith77 mentioned this pull request Apr 19, 2017

[Security] add Request type json check in json_login #22477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chalasr commented Apr 13, 2017 •

edited

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017 •

edited

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

dmaicher Apr 13, 2017

chalasr Apr 13, 2017

chalasr Apr 13, 2017

dmaicher Apr 13, 2017

dunglas Apr 13, 2017

dunglas commented Apr 13, 2017

chalasr commented Apr 13, 2017

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

phansys left a comment

phansys Apr 14, 2017

chalasr commented Apr 15, 2017

lsmith77 left a comment •

edited by fabpot

fabpot commented Apr 18, 2017

lsmith77 commented Apr 19, 2017

		@@ -70,6 +73,10 @@ public function handle(GetResponseEvent $event)
		$request = $event->getRequest();
		$data = json_decode($request->getContent());

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

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

Conversation

chalasr commented Apr 13, 2017 • edited

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017 • edited

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

dmaicher Apr 13, 2017

Choose a reason for hiding this comment

chalasr Apr 13, 2017

Choose a reason for hiding this comment

chalasr Apr 13, 2017

Choose a reason for hiding this comment

dmaicher Apr 13, 2017

Choose a reason for hiding this comment

dunglas Apr 13, 2017

Choose a reason for hiding this comment

dunglas commented Apr 13, 2017

chalasr commented Apr 13, 2017

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

lsmith77 commented Apr 13, 2017

chalasr commented Apr 13, 2017

phansys left a comment

Choose a reason for hiding this comment

phansys Apr 14, 2017

Choose a reason for hiding this comment

chalasr commented Apr 15, 2017

lsmith77 left a comment • edited by fabpot

Choose a reason for hiding this comment

fabpot commented Apr 18, 2017

lsmith77 commented Apr 19, 2017

chalasr commented Apr 13, 2017 •

edited

chalasr commented Apr 13, 2017 •

edited

lsmith77 left a comment •

edited by fabpot