?
Referenced as Pull Request for: # 10888
avatar mahagr
mahagr
3 May 2016

Steps to reproduce the issue

Log out and enter to administrator/index.php?option=com_xxx&format=json (GET or POST)

Note: Component and view doesn't need to exist as the request gets intercepted by Joomla.

Expected result

401 Unauthorized

Actual result

500 View not found [name, type, prefix]: login, json, loginView

Additional comments

Result should be the same for all non-html views (or as long as loginView doesn't exist).

avatar mahagr mahagr - open - 3 May 2016
avatar mbabker
mbabker - comment - 3 May 2016

Believe it or not 401 is NOT the expected result because you've gotten past all the redirects and ACL checks at that point and Joomla is trying to give the user a login page. And considering Joomla's error handling layer barely handles HTML requests, can't say I'm surprised non-HTML gives something "broken".

avatar mahagr
mahagr - comment - 3 May 2016

I know how it works, I just think there should be some error catching in login controller to send out different response. After all it is unauthorized request which cannot be handled by login component.

avatar mahagr
mahagr - comment - 3 May 2016

How about something like this?

public function display($cachable = false, $urlparams = false)
{
    /*
     * Special treatment is required for this component, as this view may be called
     * after a session timeout. We must reset the view and layout prior to display
     * otherwise an error will occur.
     */
    $this->input->set('view', 'login');
    $this->input->set('layout', 'default');

    try 
    {
        parent::display();
    }
    catch (Exception $e)
    {
        if (!strpos($e->getMessage(), 'loginView'))
        {
            throw $e;
        }

        throw new RuntimeException(JText::_('JERROR_ALERTNOAUTHOR'), 401);
    }
}
avatar mbabker
mbabker - comment - 3 May 2016

The display task can't do anything useful (login/logout could conceivably). A fix though IMO shouldn't be limited to com_login (maybe if you want to change the error code it'd need to be). Globally you have this issue with all components and throwing a 500 view not found message when it should probably be 404, at least that would be a more correct code for the condition. And even by the HTTP spec 403 would be the preferred code.

I would not suggest string parsing the exception message here. If the checked conditions of the string change, it changes the required checks. If you really want this one case to change, I'd say check the request format and throw your exception before calling the parent display method.

avatar mahagr
mahagr - comment - 3 May 2016

You're right, this would be much better:

public function display($cachable = false, $urlparams = false)
{
    /*
     * Special treatment is required for this component, as this view may be called
     * after a session timeout. We must reset the view and layout prior to display
     * otherwise an error will occur.
     */
    $this->input->set('view', 'login');
    $this->input->set('layout', 'default');

    // For non-html formats we do not have login view, so just display 403 instead
    if ($this->input->get('format', 'html') !== 'html') {
        throw new RuntimeException(JText::_('JERROR_ALERTNOAUTHOR'), 403);
    }

    parent::display();
}
avatar mahagr
mahagr - comment - 3 May 2016

I wouldn't display 404 code when user hasn't been logged in. All components already show 403 if you do not have permissions to access them, regardless if the view/task exists or not.

avatar mbabker
mbabker - comment - 3 May 2016

Globally IMO a 500 code for controller/view not found isn't right and a 404
should be returned. com_content doesn't support JSON views; a 404 would
better communicate that than 500 which should mean a server error. Also
globally HTML error pages shouldn't be the error response format but that's
a B/C breaking refactoring for another year.

On Tuesday, May 3, 2016, Matias Griese notifications@github.com wrote:

I wouldn't display 404 code when user hasn't been logged in. All
components already show 403 if you do not have permissions to access them,
regardless if the view/task exists or not.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#10212 (comment)

avatar mahagr
mahagr - comment - 3 May 2016

Oh yes, I must be sleeping as I didn't pay attention that the response was really 500 -- I was just thinking this one case as 404 would be wrong for unauthorized, too.

I agree that globally 500 response is wrong and 404 would be the correct response as those are really pages which do not exist. I also think that its a bug and not API change...

But what about the above change? Its really confusing for users to start getting 500 errors if their session timed out -- 403 errors would allow me to display proper message instead of just displaying the error.

avatar mbabker
mbabker - comment - 3 May 2016

Eh, that'd work.

avatar brianteeman brianteeman - change - 3 May 2016
Category Administration
avatar brianteeman brianteeman - change - 5 May 2016
Labels Added: ?
avatar wilsonge
wilsonge - comment - 10 May 2016

If the issue is just that we don't have a json view for that page - then I think it is supposed to be a 415 Unsupported Media Type status?

avatar mahagr
mahagr - comment - 10 May 2016

I think it should be consistent with all the components. Each component starts with 403 error check if you don't have access to them -- regardless if the view exists/is supported or not. The only exception is the login screen which appears to the guests, but as login is only supported with html, I would still return 403, which would happen if login didn't intercept the request.

The best reason for returning 403 is that it can then properly be handled inside the client which is making that request. Any other code wouldn't make any sense because of it changes the behaviour of the application. Why would the same request first return 200 OK and after 15 minutes 415 Unsupported? It just doesn't make any sense from the application point of view.

I'll find some time to commit this change in a few days. I'm already running my local copy with the change implemented and the error message I get back really makes sense to me.

avatar mahagr
mahagr - comment - 21 Jun 2016

There is now pull request from this issue. Sorry about the delay.

avatar zero-24 zero-24 - change - 21 Jun 2016
Status New Closed
Closed_Date 0000-00-00 00:00:00 2016-06-21 07:44:34
Closed_By zero-24
avatar joomla-cms-bot joomla-cms-bot - change - 21 Jun 2016
Closed_Date 2016-06-21 07:44:34 2016-06-21 07:44:35
Closed_By zero-24 joomla-cms-bot
avatar joomla-cms-bot joomla-cms-bot - close - 21 Jun 2016
avatar zero-24
zero-24 - comment - 21 Jun 2016

Set to "closed" on behalf of @zero-24 by The JTracker Application at issues.joomla.org/joomla-cms/10212

avatar joomla-cms-bot joomla-cms-bot - close - 21 Jun 2016
avatar zero-24
zero-24 - comment - 21 Jun 2016

PR is: #10888 Thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10212.

Add a Comment

Login with GitHub to post a comment