Move authorization checks inside the Authorization class
Issue #161
resolved
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
Comments (11)
-
-
reporter > 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
-
[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 -
reporter 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 -
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 -
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 -
reporter 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
-
reporter Small improvement, added a boolean return value to Authorization::attemptGrant().
Reported by
dreadnaut
on 2012-11-28 22:19:12<hr> * Attachment: auth.diff
-
reporter 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 -
yeah, commit it. Looks good at first sight.
Reported by
crazy4chrissi
on 2012-12-08 17:03:54 -
reporter Committed as r307.
Reported by
dreadnaut
on 2012-12-10 21:39:30 - Status changed:Fixed
- Log in to comment
Reported by
crazy4chrissi
on 2012-11-27 20:22:45