yii\web\User::loginRequired does not cause redirect for AJAX requests by default in Yii 2.0.8

[dmitry-kulikov]

What steps will reproduce the problem?

Install Yii 2 Advanced Application Template, run built-in PHP server, go to http://localhost:8080/backend/web/index.php?r=site%2Flogin as unauthenticated user, execute in browser console $.ajax('http://localhost:8080/backend/web/index.php');. Note that visiting of page http://localhost:8080/backend/web/index.php requires authentication.

What is the expected result?

Redirect to Login page.

What do you get instead?

403 (Forbidden) is shown in browser console, no redirect.

Additional info

Q	A
Yii version	2.0.8
PHP version	PHP 5.5.9-1ubuntu4.16
Operating system	Linux Ubuntu 14.04

In described situation Yii 2.0.7 does redirect to Login page, Yii 2.0.8 does not. I'm not sure whether it is bug or planned change of default behavior. If it was planned then probably makes sense to clarify it https://github.com/yiisoft/yii2/blob/master/framework/UPGRADE.md#upgrade-from-yii-207.
Old behavior can be returned by specifying false for parameter $checkAcceptHeader of yii\web\User::loginRequired() or by adding '*/*' to yii\web\User::$acceptableRedirectTypes.

[SamMousa]

Okay, I confirmed the behavior.

Redirecting the "main" page when an AJAX request returns a 302 is not standard, it is implemented by Yii: https://github.com/yiisoft/yii2/blob/master/framework/assets/yii.js#L326

From 2.0.8 onwards, as is listed in the documentation for loginRequired(), if a redirect is not applicable an 403 is thrown:

/**
     * Redirects the user browser to the login page.
     *
     * Before the redirection, the current URL (if it's not an AJAX url) will be kept as [[returnUrl]] so that
     * the user browser may be redirected back to the current page after successful login.
     *
     * Make sure you set [[loginUrl]] so that the user browser can be redirected to the specified login URL after
     * calling this method.
     *
     * Note that when [[loginUrl]] is set, calling this method will NOT terminate the application execution.
     *
     * @param boolean $checkAjax whether to check if the request is an AJAX request. When this is true and the request
     * is an AJAX request, the current URL (for AJAX request) will NOT be set as the return URL.
     * @param boolean $checkAcceptHeader whether to check if the request accepts HTML responses. Defaults to `true`. When this is true and
     * the request does not accept HTML responses the current URL will not be SET as the return URL. Also instead of
     * redirecting the user an ForbiddenHttpException is thrown. This parameter is available since version 2.0.8.
     * @return Response the redirection response if [[loginUrl]] is set
     * @throws ForbiddenHttpException the "Access Denied" HTTP exception if [[loginUrl]] is not set or a redirect is
     * not applicable.
     * @see checkAcceptHeader
     */

So in my opinion the default behavior was changed, and this change was planned. (Note I'm not in the Yii team).
If it was planned I guess the upgrade notes should be updated; if it was not planned then changing the default value of loginRequired() would revert to the old behavior, but also implicitly disable the new functionality for all request checked via the AccessControl filter.

We could add "*/*" to yii\web\User::$acceptableRedirectTypes, as @dmitry-kulikov mentioned. This has some downsides as well; for example: $.getJSON('/') will send this header:

Accept:application/json, text/javascript, */*; q=0.01

In this case '*/*' is acceptable, but it is definitely not desirable.

The best solution I see is separately checking if '*/*' is the only acceptable type in checkRedirectAcceptable(). If it is and $acceptableRedirectTypes is not empty then we can return true.

[dmitry-kulikov]

@SamMousa

The best solution I see is separately checking if '*/*' is the only acceptable type in checkRedirectAcceptable(). If it is and $acceptableRedirectTypes is not empty then we can return true.

Looks too complicated for me.
I think work of loginRequired should not depend on ajax dataType. At least by default. Although I can imagine situation when loginRequired should work different for $.ajax(); and $.getJSON(), I don't think it is self-evident. I did a little bit more testing:
$.ajax('http://localhost:8080/backend/web/index.php'); - no redirect;
$.ajax('http://localhost:8080/backend/web/index.php', {dataType: 'html'}); - redirect;
$.ajax('http://localhost:8080/backend/web/index.php', {dataType: 'json'}); - no redirect.
It is not surprising if you know about yii\web\User::$acceptableRedirectTypes, but adds more magic.

[SamMousa]

It might look complicated at first; but it will restore the default behavior from 2.0.7 while at the same time providing the more sensible behavior when the text/html mime type is not accepted.

So from a developer perspective no change except configuration is required.

[SamMousa]

We could also consider adding an extra header.

1. 302 redirect is not valid for AJAX requests.
2. Yii uses an X-Redirect header to manually redirect via javascript.

How about for this specific case we add an X-Login header while sending the 403 status?
The javascript can then be extended to check for that header and perform a redirect.

Advantages:

• Redirect still works.
• Other clients can detect a login is required and choose to redirect the user or just open the login page in a new screen.

Note: adding an X-Redirect header would work as well but is not as clean in my opinion.

[SDKiller]

...for this specific case we add an X-Login header while sending the 403 status?

Why not use 401 Unauthorized header instead of inventing custom X-headers?

[SamMousa]

401 is for http basic authentication
On May 14, 2016 11:16 AM, "Serge Postrash" notifications@github.com wrote:

...for this specific case we add an X-Login header while sending the 403
status?

Why not use 401 Unauthorized header instead of inventing custom X-headers?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#11523 (comment)

[SamMousa]

Your fix will not result in expected behavior for json requests; see my earlier comment regarding $.getJSON.

[SilverFire]

@Faryshta thank you for bringing my attention, I've overseen your point. Fixed in 0ff6eeb

[Faryshta]

@SilverFire I think you confused me 😸