Bug 18442 - Permission error when logging into staff interface as db user
Summary: Permission error when logging into staff interface as db user
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Alex Buckley
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on: 18275
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-18 02:13 UTC by Alex Buckley
Modified: 2017-04-28 14:49 UTC (History)
8 users (show)

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


Attachments
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method. (2.38 KB, patch)
2017-04-19 23:49 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method. (2.55 KB, patch)
2017-04-20 08:31 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method. (2.55 KB, patch)
2017-04-22 11:00 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18442: Implemented $userid ||= $q_userid if $return == 2; in place of $userid ||= $q_userid; (2.02 KB, patch)
2017-04-26 09:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18442: Fix DB user loggin (2.02 KB, patch)
2017-04-26 17:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18442: Add a test (2.15 KB, patch)
2017-04-26 17:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18442: Fix DB user loggin (2.08 KB, patch)
2017-04-28 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18442: Fix DB user loggin (2.08 KB, patch)
2017-04-28 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18442: Add a test (2.21 KB, patch)
2017-04-28 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-04-18 02:13:59 UTC
After a user has gone through the installer and is logging in for the first time when they enter the db user credentials it throws an error saying 'Error: You do not have permission to access this page'

If you try entering in the db users credentials a second time then you can login successfully and the staff interface will appear.

Similarly if once in the staff interface you log out then try logging back in again with the db user then you get the 'Error: You do not have permission to access this page' no matter how many times you try to log in.
However if you open the URL in a new tab the staff interface appears.
Comment 1 Chris Cormack 2017-04-19 20:22:10 UTC
I've had this occur on 16.11 also.
Comment 2 Alex Buckley 2017-04-19 23:49:03 UTC
Created attachment 62423 [details] [review]
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method.

Previously the $userid variable was undefined when it was handed to the
haspermission() method and this was causing 'Error: You do not have
permission to access this page' errors when logging into the staff
interface as database user. However if you viewed the url in a new tab
you could view the staff interface.

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt
Comment 3 Alex Buckley 2017-04-20 00:02:00 UTC
This patch does not undo the security bug fixes of bug 18275 (opac-memberentry.pl security vulnerabilities) because all tests in the unit test t/db_dependent/Auth.t run successfully with this patch applied.

See test output below:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone/t/db_dependent$ prove Auth.t
Auth.t .. ok     
All tests successful.
Files=1, Tests=21,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.12 cusr  0.08 csys =  2.23 CPU)
Result: PASS
kohadev-koha@kohadevbox:/home/vagrant/kohaclone/t/db_dependent$
Comment 4 Marc Véron 2017-04-20 08:31:07 UTC
Created attachment 62448 [details] [review]
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method.

Previously the $userid variable was undefined when it was handed to the
haspermission() method and this was causing 'Error: You do not have
permission to access this page' errors when logging into the staff
interface as database user. However if you viewed the url in a new tab
you could view the staff interface.

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 5 Jonathan Druart 2017-04-20 13:47:09 UTC
Introduced by bug 18275.
Comment 6 Marcel de Rooy 2017-04-20 13:54:34 UTC
Version should be master?
Comment 7 Jonathan Druart 2017-04-20 14:07:33 UTC
I had more or less the same fix, but it was:

  $userid ||= $q_userid if $return == 2;

It makes thing safer I *think* because it will only affect behaviours for DB user ($return == 2).
On the other hand if does not really make sense because if $return == 2, $userid should not be set.
So maybe just
  $userid ||= $q_userid;
Could be enough.
And especially it will not erase $userid if one of the checkpw calls return a userid different than the one we passed (possible??).

Martin? Tomas? Kyle?
Comment 8 Alex Buckley 2017-04-20 20:53:02 UTC
Ah yup would you like me to implement:  $userid ||= $q_userid;
Comment 9 Jonathan Druart 2017-04-21 15:40:19 UTC
(In reply to Alex Buckley from comment #8)
> Ah yup would you like me to implement:  $userid ||= $q_userid;

I'd love only 1 patch and avoid follow-up, not to make git log (more) complicated.
Comment 10 Alex Buckley 2017-04-22 11:00:39 UTC
Created attachment 62567 [details] [review]
Bug 18442 - Implemented assignment of the $userid variable before the haspermission call so that a defined $userid with a value is being passed to the haspermission() method.

Previously the $userid variable was undefined when it was handed to the
haspermission() method and this was causing 'Error: You do not have
permission to access this page' errors when logging into the staff
interface as database user. However if you viewed the url in a new tab
you could view the staff interface.

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 11 Alex Buckley 2017-04-22 11:05:31 UTC
(In  reply to Jonathan Druart in comment 9)

Hi Jonathan 

I have added your suggested alteration in my patch so now we just have a single patch attached to this bug report rather than a followup patch.

I have run Auth.t again and all tests run successfully as this test output shows:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone/t/db_dependent$ prove Auth.t
Auth.t .. ok     
All tests successful.
Files=1, Tests=21,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.33 cusr  0.11 csys =  2.47 CPU)
Result: PASS
kohadev-koha@kohadevbox:/home/vagrant/kohaclone/t/db_dependent$ exit
exit
Comment 12 Jonathan Druart 2017-04-24 13:41:47 UTC
The patch is ok to me.
I'd like opinion about

    $userid ||= $q_userid;

vs

  $userid ||= $q_userid if $return == 2;
Comment 13 Martin Renvoize 2017-04-26 08:12:14 UTC
Checking the $return value would certainly be the belt and braces approach. I like it.
Comment 14 Alex Buckley 2017-04-26 09:09:42 UTC
(In reply to Martin Renvoize in comment 13)

Hi Martin

So just to confirm you would like to see:

  $userid ||= $q_userid if $return == 2;
Comment 15 Martin Renvoize 2017-04-26 09:20:34 UTC
Correct. I'd like to see the `if $return == 2` added. I believe adding that should protect us from any unforeseen down the line action at a distance bugs creeping in.. or at least some ;).
Comment 16 Alex Buckley 2017-04-26 09:28:49 UTC
Created attachment 62710 [details] [review]
Bug 18442: Implemented $userid ||= $q_userid if $return == 2; in place of $userid ||= $q_userid;

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt
Comment 17 Alex Buckley 2017-04-26 09:30:42 UTC
(In reply to Martin Renvoize in comment 15)

Thanks for the feedback Martin
Comment 18 Tomás Cohen Arazi 2017-04-26 12:07:18 UTC
I think the patch looks ok, meaning it makes sense. But we really need a small regression test, to make sure we are actually fixing the issue in the right place and not because of a collateral change of behaviour.
And also because that's how we do it.

So, in order to help I need to ask: what is the hypothesis for the bug? Answering that will make it easier to write that regression test.
Comment 19 Jonathan Druart 2017-04-26 17:55:43 UTC
Created attachment 62738 [details] [review]
Bug 18442: Fix DB user loggin

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2017-04-26 17:55:48 UTC
Created attachment 62739 [details] [review]
Bug 18442: Add a test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Martin Renvoize 2017-04-28 14:48:06 UTC
Created attachment 62826 [details] [review]
Bug 18442: Fix DB user loggin

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2017-04-28 14:48:28 UTC
Created attachment 62827 [details] [review]
Bug 18442: Fix DB user loggin

Test plan:
1. Drop and recreate your db

2. Clear memcached

3. Go through the installer (to speed up this test plan install all
sample data so you dont have to create libraries, patron categories etc. later)

4. On the installer page login as the database user and notice that it
does not work on the first attempt ( you get 'Error: You do not have
permission to access this page')

5. Try logging in as database user for a second time and notice you are
logged in successfully this time

4. In staff interface create a patron account with superlibrarian permissions

5. Logout of the staff interface

6. Login as database user

7. Notice you cant log in. You get the 'Error:: You do not have permission to access this
page' error

8. Try a second attempt and notice you get the same error

9. Open the URL in a new tab and notice the staff interface appears
showing that you are logged in

10. log out and log back in as the superlibrarian user you created and
notice it works on first login attempt

11. Apply patch

12. Log out and try logging back in as database user and notice that you
can login successfully on first attempt

13. Repeat steps 1,2,3 and login as database user and notice the login
works on first attempt

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2017-04-28 14:48:33 UTC
Created attachment 62828 [details] [review]
Bug 18442: Add a test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2017-04-28 14:49:01 UTC
Looks solid to me, thanks for adding the test too.