Page MenuHomePhabricator

Login button shouldn't be displayed if no input fields are there
Closed, ResolvedPublic

Description

Currently, if you enable GoogleLogin and disable all other primaryauth providers (such as LocalPasswordAuthenticationProvider and the TemporarayPasswordAuthenticationProvider) you get a login form like this:

The "Log in" button is pretty useless, as it always returns "Credentials could not be authenticated". So the form is now more confusing as helping, as the only way to login is with the GoogleLogin button.

Personally, I'm not sure, how or if we could fix this easily, because it seems there is a variaty of possible input options, some may need this button, some may not. But probably we could, maybe, check, if all AuthenticationRequests return buttons only (as far as I can imagine, this would represent providers that need these buttons only?!). Or maybe someone else has a better idea :]

Event Timeline

Florian created this task.Jul 27 2016, 7:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 27 2016, 7:09 PM
Tgr added a comment.EditedJul 27 2016, 7:24 PM

AuthManagerSpecialPage::needsSubmitButton() is supposed to be doing that. It is confused by the rememberMe field in this case; not sure what to do about that.

Hmm, right. The problem: AuthManager sets the "remember me" option automatically (and internally), so you can't configure it to don't do that (and Remember me triggers a login button, as it's a form input rather than a submit value) :/

Tgr added a comment.EditedJul 27 2016, 7:41 PM

Isn't that the right thing to do, though? Even if you use GoogleLogin you still need to know what cookie lifetime the user wants.

What probably should be fixed is showing that checkbox when $wgExtendedLoginCookieExpiration is not set. That should be doable by changing getRememberUserDuration() in CookieSessionProvider and CentralAuthSessionProvider to return null instead of the normal cookie lifetime in that case (which it should be doing anyway according to its documentation). That will in effect disable RememberMeAuthenticationRequest.

Florian added a comment.EditedJul 27 2016, 7:42 PM

Maybe we can move the "internal" fields of AuthManager into the AuthManager default config array, maybe with a key like "internal" and an array or into a separate configuration variable, so that the user can configure it (if he knows what he do) and so the config isn't overwritten, if the AuthManager config is set. In this way a user can configure what other requests should be added to a form of a specific action, e.g. login, or what fields shouldn't be there?

Edited after reading @Tgr's comment:
Even if I've the remember me checkbox, I don't need the login button, as it's still useless, so we should think about to not add the login button, if no other input field is there :)

Tgr added a comment.Jul 27 2016, 7:45 PM

Maybe we can move the "internal" fields of AuthManager into the AuthManager default config array, maybe with a key like "internal" and an array or into a separate configuration variable, so that the user can configure it (if he knows what he do) and so the config isn't overwritten, if the AuthManager config is set. In this way a user can configure what other requests should be added to a form of a specific action, e.g. login, or what fields shouldn't be there?

I'd rather not. If someone is really desperate they can always use the AuthChangeFormFields hook.

Tgr added a comment.Jul 27 2016, 7:47 PM

Even if I've the remember me checkbox, I don't need the login button, as it's still useless, so we should think about to not add the login button, if no other input field is there :)

I guess LoginSignupSpecialPage could override needsSubmitButton() and add special handling for the remember me checkbox.

Florian claimed this task.Jul 27 2016, 7:53 PM

Sounds good!

Change 301431 had a related patch set uploaded (by Florianschmidtwelzow):
Special:UserLogin: Don't show login button with rememberMe checkbox only

https://gerrit.wikimedia.org/r/301431

Change 303012 had a related patch set uploaded (by Gergő Tisza):
AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED

https://gerrit.wikimedia.org/r/303012

Change 303012 merged by jenkins-bot:
AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED

https://gerrit.wikimedia.org/r/303012

Change 301431 merged by jenkins-bot:
Special:UserLogin: Don't show login button when not required

https://gerrit.wikimedia.org/r/301431

Florian closed this task as Resolved.Aug 22 2016, 10:11 PM

Change 313588 had a related patch set uploaded (by Cicalese):
Merge "AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED" into REL1_27

https://gerrit.wikimedia.org/r/313588

Change 313596 had a related patch set uploaded (by Cicalese):
Merge "Special:UserLogin: Don't show login button when not required" into REL1_27

https://gerrit.wikimedia.org/r/313596

Change 313596 merged by jenkins-bot:
Special:UserLogin: Don't show login button when not required

https://gerrit.wikimedia.org/r/313596

Change 313588 merged by jenkins-bot:
AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED

https://gerrit.wikimedia.org/r/313588