Move authorization checks inside the Authorization class

[dreadnaut]

Originally reported on Google Code with ID 161

A few checks that have to do with authorization are spread through the code: correct
password, failed login, default password. I collected them inside the Authorization
class.

Attached is a patch agains r306, which also remove a hidden 'proc_login' field from
the login form. I'm ready to put it back where it was if it had a good reason to exist
:)

Reported by dreadnaut on 2012-11-27 20:12:22

<hr> * Attachment: auth.diff

[Christopher Kramer]

I like it. As it is security related, I'll have another look at it, but I think it looks
good at first sight.
Regarding proc_login: It looks like this was introduced to capture cases in which the
form was submitted without using the button. For example, you can submit a form using
[Enter] without using the button Logout and you will not get a $_POST['Logout']. At
least I think this is the case, I have not checked.

We should check whether other forms in phpLiteAdmin rely on using the button.

I'd use a GET parameter or only the hidden field, but not the hidden field + the button-parameter.

Reported by crazy4chrissi on 2012-11-27 20:22:45

[dreadnaut]

> Regarding proc_login: It looks like this was introduced to capture cases in
> which the form was submitted without using the button.

Maybe in older browsers? Pressing Enter seems to work fine in the latest IE/Fx/Chrome.

At that point we might change the form action to something like "PAGE?action=login"
and then redirect to the main view on success. (Similar solution could be applied to
logout, allowing for a simple link instead of a form)

[Related: a website should try not to break browser navigation (back-fwd-refresh).
Phpliteadmin suffers a bit of 'broken F5', where refreshing the current page might
re-execute some action, resulting in an error. It might take some time to "fix" this
for the whole program, if we want, but GET action+redirect would solve the problem
for login and logout.]

Reported by dreadnaut on 2012-11-27 20:57:30 - Status changed: Accepted

[Christopher Kramer]

[what you wrote in brackets is issue #104]

> Pressing Enter seems to work fine in the latest IE/Fx/Chrome.
Hmm.. I'll test this. I think it depends on what is being focused when you press enter.
I mean you can have 2 submit buttons. Which post-parameter should the browser send
in this case if you press enter while focusing a text-input-field?

Reported by crazy4chrissi on 2012-11-27 21:22:08

[dreadnaut]

My experience is consistent with the answer here:

http://stackoverflow.com/questions/925334/how-is-the-default-submit-button-on-an-html-form-determined

first submit appearing in the source code, or lowest tabindex (which are equivalent
if no tabindex are defined). There is nothing in the html specs about this, so different
browsers might do different things.

We could assume it's safe with one submit button, or that it's never safe and always
have an additional parameter.

(Thanks for pointing out #104, I hand't noticed!)

Reported by dreadnaut on 2012-11-27 21:28:43

[Christopher Kramer]

Thanks for the link.

If nothing is specified, then I would consider it to be never safe. We can never know
what some future browser will do. And there are lots of different mobile browsers and
so on.

The questioner on stackoverflow writes:
> when pressing Enter, IE uses either the first submit button or none at all depending
on conditions I haven't been able to figure out

I am sure I also stumbled across a browser someday that did not use the first button.
I just tried to figure out why this code was introduced and found issue #7.

So IE it is indeed. But not old IE6 or older but IE8 and 9, which should definitely
be supported.

So I clearly vote for "we never rely on it".

Reported by crazy4chrissi on 2012-11-27 21:51:17

[Christopher Kramer]

Just out of interest:

Reading the HTML spec carefully, I'd even say browsers using the first button do NOT
follow it strictly:
"If a form contains more than one submit button, only the activated submit button is
successful."

This leaves out cases with only 1 button of course. But if there is more than one,
using the first one really breaks this rule as the first one was NOT activated by the
user if the user presses enter. If the user activates no submit button and there is
more than one, according to HTML 4.01*, no submit button should be successful and therefore
included as a POST parameter.
And this really makes sense: If I have a huge form with multiple submit-buttons. At
the top, there is a "delete this record"-button, then lots of inputs follow and at
the bottom there is a "update this record"-Button. A user hitting enter while in a
field at the bottom of the form would never suspect that he implicitly hits the "delete
this record" from the top.

In our case we only have 1 button. I couldn't find a clear statement for this in the
spec.

* we probably better should look at the XHTML spec, which I didn't do yet

Reported by crazy4chrissi on 2012-11-27 22:04:06

[dreadnaut]

Here's an amended patch, tested with Chrome/Fx/IE7-8-9. The only difference with the
previous one is at line 2321 (136 in the diff file), where I removed the submit name,
and re-added the hidden input field.

Reported by dreadnaut on 2012-11-28 13:12:25

<hr> * Attachment: auth.diff

[dreadnaut]

Small improvement, added a boolean return value to Authorization::attemptGrant().

Reported by dreadnaut on 2012-11-28 22:19:12

<hr> * Attachment: auth.diff

[dreadnaut]

Should I go ahead with this?

I tend to wait for a review before applying patches. Is that acceptable, or should
I check the code in, and eventually retract changes?

Reported by dreadnaut on 2012-12-08 01:13:17

[Christopher Kramer]

yeah, commit it. Looks good at first sight.

Reported by crazy4chrissi on 2012-12-08 17:03:54

[dreadnaut]

Committed as r307.

Reported by dreadnaut on 2012-12-10 21:39:30 - Status changed: Fixed