Bug 12930 - Web installer does not show login errors
Summary: Web installer does not show login errors
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Alex Buckley
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-16 14:20 UTC by Owen Leonard
Modified: 2017-05-03 04:27 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12930 - Implemented error message display for invalid username or password on web installer login (2.55 KB, patch)
2017-04-01 21:20 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 12930 - Followup removing empty else branch (655 bytes, patch)
2017-04-03 07:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 12930 - Implemented error message display for invalid username or password on web installer login (2.82 KB, patch)
2017-04-03 18:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12930 - Followup removing empty else branch (789 bytes, patch)
2017-04-03 18:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12930 - Implemented invalid_username_or_password template variable being handed to Auth.tt. Removed indentation changes and modification to return value of checkpw (968 bytes, patch)
2017-04-13 02:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 12930: Web installer does not show login errors (1.49 KB, patch)
2017-04-13 06:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12930: Web installer does not show login errors (1.59 KB, patch)
2017-04-14 11:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2014-09-16 14:20:44 UTC
The web installer login template has markup for handling the same kinds of errors the regular login template does (nopermission, timed_out, different_ip, wrongip, invalid_username_or_password) but the script doesn't seem to be written to handle those errors.

For instance: Submitting the wrong username and password doesn't result in an error message. The page simply reloads.
Comment 1 Alex Buckley 2017-04-01 21:20:24 UTC
Created attachment 61768 [details] [review]
Bug 12930 - Implemented error message display for invalid username or password on web installer login

Now InstallAuth.pm hands the invalid_username_or_password variable to
the template file auth.tt if the user entered an invalid username or
password and this is displayed to the user
Comment 2 Alex Buckley 2017-04-02 00:54:42 UTC
The patch I have attached displays an error message if the user writes in an invalid username and/or password. 

The InstallAuth.pm module does not contain subroutines to check for permissions, session timeout, or if the ip address is different even though the markup checking for these in the auth.tt has been implemented. 

Regarding the nopermission check, as I understand it the only password that is valid to be written into this installer login is the database admin credentials and so I don't see why we need the nopermission check in the markup (as is required for the staff interface login).


Test plan:
1. Restart memcached: sudo service memcached restart

2. Drop and recreate the database

3. Login with these pretend credentials: 
username: fred 
password: password

4. Notice the page refreshes and you get no "invalid username or password" error displayed

5. Apply the patch attached to this bug

6. Try logging in with the same username and password specified in step 3

7. Notice the page refreshes and a "invalid username or password' error message is displayed 

8. Now login with the Koha database administrator account and notice you are logged in successfully
Comment 3 Marc Véron 2017-04-02 18:11:14 UTC
Hi Alex
I do not understand the empty else branch in 
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61768&action=diff#a/C4/InstallAuth.pm_sec5

Marc
Comment 4 Alex Buckley 2017-04-03 07:34:43 UTC
Created attachment 61777 [details] [review]
Bug 12930 - Followup removing empty else branch
Comment 5 Alex Buckley 2017-04-03 07:37:09 UTC
Thanks for noticing that Marc, I forgot to remove it. The followup patch I have just attached removes it
Comment 6 Marc Véron 2017-04-03 18:39:08 UTC
Created attachment 61807 [details] [review]
Bug 12930 - Implemented error message display for invalid username or password on web installer login

Now InstallAuth.pm hands the invalid_username_or_password variable to
the template file auth.tt if the user entered an invalid username or
password and this is displayed to the user

I tested both patches together by direkctly calling
[MY SERVER]/cgi-bin/koha/installer/install.pl?step=1&op=updatestructure
Works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 7 Marc Véron 2017-04-03 18:39:59 UTC
Created attachment 61808 [details] [review]
Bug 12930 - Followup removing empty else branch

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 8 Jonathan Druart 2017-04-11 22:25:06 UTC
Alex, it seems that the only changes you made are:

+    if ($info{'invalid_username_or_password'} == 1) {
+        $template->param( 'invalid_username_or_password' => $info{'invalid_username_or_password'});
+    }

If it is right I would prefer a single patch with only the needed changes instead of indentation changes and modification of the return value of checkpw.
Comment 9 Alex Buckley 2017-04-13 02:35:57 UTC
Created attachment 62116 [details] [review]
Bug 12930 - Implemented invalid_username_or_password template variable being handed to Auth.tt. Removed indentation changes and modification to return value of checkpw
Comment 10 Alex Buckley 2017-04-13 02:39:08 UTC
(in response to Jonathan Druart in comment 8)
I have implemented a single patch meeting the requests you asked in comment 8. The comment 2 test plan is still applicable.
Comment 11 Marc Véron 2017-04-13 06:35:46 UTC
Created attachment 62117 [details] [review]
Bug 12930: Web installer does not show login errors

Implemented invalid_username_or_password template
variable being handed to Auth.tt. Removed indentation
changes and modification to return value of checkpw

Test plan:
See comment #2

Alternative test plan:
- Clear session cookies or close / open your browser
- Go to  [MY SERVER]/cgi-bin/koha/installer/install.pl
- Verify that message appears if wrong username/pw provided
- Verify that you can log in with databas administrator account

Tested with alternative test plan, works as expected. Title and
description in commit message adapted.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2017-04-13 06:37:27 UTC
Changed title and text of commit message, obsoleted previous patch, signed off.
Comment 13 Marcel de Rooy 2017-04-14 11:18:29 UTC
Created attachment 62166 [details] [review]
Bug 12930: Web installer does not show login errors

Implemented invalid_username_or_password template
variable being handed to Auth.tt. Removed indentation
changes and modification to return value of checkpw

Test plan:
See comment #2

Alternative test plan:
- Clear session cookies or close / open your browser
- Go to  [MY SERVER]/cgi-bin/koha/installer/install.pl
- Verify that message appears if wrong username/pw provided
- Verify that you can log in with databas administrator account

Tested with alternative test plan, works as expected. Title and
description in commit message adapted.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Kyle M Hall 2017-04-14 14:42:20 UTC
Pushed to master for 17.05, thanks Alex!
Comment 15 Katrin Fischer 2017-04-16 12:42:43 UTC
This patch has been pushed to 16.11.x and will be in 16.11.07.
Comment 16 Julian Maurice 2017-04-21 11:12:56 UTC
Pushed to 3.22.x for 3.22.20
Comment 17 Mason James 2017-05-03 04:27:14 UTC
Pushed to 16.05.x, for 16.05.12 release