[Security] Fix json_login default success/failure handling #22494

chalasr · 2017-04-21T08:17:47Z

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

This makes the json_login listener default configuration stateless oriented by:

Not using the default (redirect based) failure handler, it returns a 401 (json) response containing the failure reason instead
Not using the default (redirect based) success handler, just let the original request continue instead (reaching the targeted resource without being redirected).
Setting require_previous_session to false by default (I have to set it on form-login each time I want it to be stateless)
Removing the options related to redirections (default_target_path, login_path, ...) from the listener factory, if one wants redirections then one has to write its own handlers, not the inverse

chalasr · 2017-04-21T11:59:06Z

ping @lsmith77 @dunglas

lsmith77 · 2017-04-21T16:45:57Z

...e/Tests/Functional/Bundle/JsonLoginBundle/Security/Http/JsonAuthenticationSuccessHandler.php

+{
+    public function onAuthenticationSuccess(Request $request, TokenInterface $token)
+    {
+        return new JsonResponse(array('message' => sprintf('Good game @%s!', $token->getUsername())));


do we have a standard style how we word such defaults?

this is a custom success handler fixture :)

lsmith77 · 2017-04-21T20:08:10Z

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

@@ -117,6 +118,10 @@ public function handle(GetResponseEvent $event)
            $response = $this->onFailure($request, $e);
        }

+        if (null === $response) {


what is the reason this is needed?

In case of success, the listener just lets the original request succeeds so it doesn't return any response. Should it return a default success response instead?

ah of course

lsmith77 · 2017-04-21T20:08:56Z

great! thanks for implementing this.

fabpot · 2017-04-23T22:22:02Z

Thank you @chalasr.

… (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] Fix json_login default success/failure handling | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no (master only) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22483 | License | MIT | Doc PR | n/a This makes the `json_login` listener default configuration stateless oriented by: - Not using the default (redirect based) failure handler, it returns a 401 (json) response containing the failure reason instead - Not using the default (redirect based) success handler, just let the original request continue instead (reaching the targeted resource without being redirected). - Setting `require_previous_session` to `false` by default (I have to set it on `form-login` each time I want it to be stateless) - Removing the options related to redirections (`default_target_path`, `login_path`, ...) from the listener factory, if one wants redirections then one has to write its own handlers, not the inverse Commits ------- 9749618 Fix json_login default success/failure handling

arthens · 2017-12-28T09:07:59Z

@chalasr does this mean that now success_handler (and maybe failure_handler) are required? I'm trying to enable json_login in Symfony 3.3.14 and I'm not sure if the docs are out of date, if there's a bug or if I'm misconfiguring something.

I'm copying the configuration from https://symfony.com/doc/current/security/json_login_setup.html and I'm getting the following error on successful logins:

CRITICAL: Uncaught PHP Exception LogicException: "The
controller must return a response (null given). Did you forget to add a return statement somewhere in your controller?

Which I think is because my controller is empty (as suggested by the documentation) and:

Not using the default (redirect based) success handler, just let the original request continue instead (reaching the targeted resource without being redirected).

by default there is no success handler by default.

I'm happy to submit a bug report, but I'm not sure if the bug is in the docs or in the code

chalasr · 2017-12-28T13:03:05Z

does this mean that now success_handler (and maybe failure_handler) are required?

@arthens depending on your need, yes. This made json_login suited for stateless authentication by default, meaning that you can pass credentials to each single request (not restricted to a given check_path) and, if credentials are valid, then the request continues for reaching the target controller (itself returning any resource).

To me the common workflow is to send username/password to a specific check_path only once, this check_path returning a token (e.g. bearer) itself passed to each subsequent request (validated by another security listener).
In that case, you may want to set check_path and success_handler (+ failure_handler if the default one doesn't fit, which is a 401 json response).

Docs need to be updated indeed, please submit an issue there and eventually ping me, I'll take care of the PR. Thanks!

@vincentchalamon

This PR was squashed before being merged into the 3.4 branch (closes #10467). Discussion ---------- Fix and improve JSON login docs Since symfony/symfony#22494, it's mandatory to return a valid response in the controller to prevent a 500 error. I've updated the docs accordingly, and added an example of what can be returned. This PR is similar to #9611 (that was right, sorry I just noticed). ping @vincentchalamon Commits ------- 570dce3 Fix and improve JSON login docs

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

Fix json_login default success/failure handling

9749618

chalasr force-pushed the stateless-json-login branch from f481ea2 to 9749618 Compare April 21, 2017 08:18

chalasr changed the title ~~Fix json_login default success/failure handling~~ [Security] Fix json_login default success/failure handling Apr 21, 2017

chalasr mentioned this pull request Apr 21, 2017

[Security] UsernamePasswordJsonAuthenticationListener requires custom failure handler to prevent redirect to login #22483

Closed

lsmith77 reviewed Apr 21, 2017

View reviewed changes

lsmith77 approved these changes Apr 23, 2017

View reviewed changes

fabpot merged commit 9749618 into symfony:master Apr 23, 2017

chalasr deleted the stateless-json-login branch April 24, 2017 00:05

dunglas mentioned this pull request Oct 10, 2018

Fix and improve JSON login docs symfony/symfony-docs#10467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Security] Fix json_login default success/failure handling #22494

[Security] Fix json_login default success/failure handling #22494

Uh oh!

chalasr commented Apr 21, 2017 •

edited

Loading

Uh oh!

chalasr commented Apr 21, 2017

Uh oh!

lsmith77 Apr 21, 2017

Uh oh!

chalasr Apr 21, 2017

Uh oh!

lsmith77 Apr 21, 2017

Uh oh!

chalasr Apr 21, 2017 •

edited

Loading

Uh oh!

lsmith77 Apr 23, 2017

Uh oh!

lsmith77 commented Apr 21, 2017

Uh oh!

fabpot commented Apr 23, 2017

Uh oh!

arthens commented Dec 28, 2017

Uh oh!

chalasr commented Dec 28, 2017

Uh oh!

Uh oh!

Uh oh!

[Security] Fix json_login default success/failure handling #22494

[Security] Fix json_login default success/failure handling #22494

Uh oh!

Conversation

chalasr commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr commented Apr 21, 2017

Uh oh!

lsmith77 Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

lsmith77 Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsmith77 Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

lsmith77 commented Apr 21, 2017

Uh oh!

fabpot commented Apr 23, 2017

Uh oh!

arthens commented Dec 28, 2017

Uh oh!

chalasr commented Dec 28, 2017

Uh oh!

Uh oh!

chalasr commented Apr 21, 2017 •

edited

Loading

chalasr Apr 21, 2017 •

edited

Loading