Bug 20489 - Prevent DB user login
Summary: Prevent DB user login
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 20580
Blocks: 20877
  Show dependency treegraph
 
Reported: 2018-03-28 17:29 UTC by Jonathan Druart
Modified: 2018-06-04 18:44 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
It is no longer possible to use the database user (defined in koha-conf.xml) to login into Koha. You should first create a superlibrarian patron and use it for logging in. See the script misc/devel/create_superlibrarian.pl


Attachments
Bug 20489: Prevent DB user login (9.21 KB, patch)
2018-03-28 17:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20489: Remove warnings from the interface (5.33 KB, patch)
2018-03-28 17:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20489: Remove warnings from the interface (3.95 KB, patch)
2018-03-29 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20489: Prevent DB user login (8.72 KB, patch)
2018-04-02 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20489: Remove warnings from the interface (3.95 KB, patch)
2018-04-02 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20489: Prevent DB user login (8.74 KB, patch)
2018-04-03 04:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20489: Remove warnings from the interface (3.97 KB, patch)
2018-04-03 04:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20489: Prevent DB user login (8.83 KB, patch)
2018-04-13 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20489: Remove warnings from the interface (4.07 KB, patch)
2018-04-13 10:07 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 Jonathan Druart 2018-03-28 17:29:53 UTC
Since we have the onboarding tool that create a superlibrarian patron as well as a misc script (misc/devel/create_superlibrarian.pl), we could reject login with the DB user.
Comment 1 Jonathan Druart 2018-03-28 17:44:48 UTC
Created attachment 73400 [details] [review]
Bug 20489: Prevent DB user login
Comment 2 Jonathan Druart 2018-03-28 17:45:46 UTC
Something like that, would you agree guys?
Comment 3 Jonathan Druart 2018-03-28 17:49:36 UTC
Created attachment 73402 [details] [review]
Bug 20489: Remove warnings from the interface
Comment 4 Marcel de Rooy 2018-03-29 07:36:21 UTC
(In reply to Jonathan Druart from comment #2)
> Something like that, would you agree guys?

Sounds good. Second patch contains other stuff too.
Comment 5 Kyle M Hall 2018-03-29 11:04:11 UTC
(In reply to Marcel de Rooy from comment #4)
> (In reply to Jonathan Druart from comment #2)
> > Something like that, would you agree guys?
> 
> Sounds good. Second patch contains other stuff too.

Agreed!
Comment 6 Jonathan Druart 2018-03-29 14:00:10 UTC
Created attachment 73425 [details] [review]
Bug 20489: Remove warnings from the interface
Comment 7 Jonathan Druart 2018-03-29 14:00:58 UTC
(In reply to Marcel de Rooy from comment #4)
> Second patch contains other stuff too.

Indeed, fixed! I was testing the installer part.
Comment 8 Jonathan Druart 2018-03-29 14:01:44 UTC
No test plan provided, use your knowledge and imagination to know if these patches are relevant and enough.
Comment 9 M. Tompsett 2018-04-01 15:35:24 UTC
Tested:
Staff
OPAC
Self-checkout
And fresh install

Only the fresh install allowed the system log in, as desired.
Comment 10 M. Tompsett 2018-04-01 15:49:18 UTC
Attempted steps to go to urls which should trigger modified code in both logged in and logged out states. Failures and successes as expected.
Comment 11 M. Tompsett 2018-04-01 15:50:30 UTC
Comment on attachment 73400 [details] [review]
Bug 20489: Prevent DB user login

Review of attachment 73400 [details] [review]:
-----------------------------------------------------------------

::: acqui/neworderempty.pl
@@ +203,5 @@
>          my $order_patron = Koha::Patrons->find( $order_user_id );
>          push @order_users, $order_patron if $order_patron;
>      }
> +    if ( $data->{subscriptionid} ) {
> +        $template->param( subscriptionid => $data->{subscriptionid} );

This tweak is the one I don't understand.
Comment 12 Jonathan Druart 2018-04-02 14:35:30 UTC
Created attachment 73528 [details] [review]
Bug 20489: Prevent DB user login
Comment 13 Jonathan Druart 2018-04-02 14:35:33 UTC
Created attachment 73529 [details] [review]
Bug 20489: Remove warnings from the interface
Comment 14 Jonathan Druart 2018-04-02 14:36:13 UTC
(In reply to M. Tompsett from comment #11)
> This tweak is the one I don't understand.

It was not related to this patch.
Comment 15 M. Tompsett 2018-04-03 04:05:56 UTC
Created attachment 73543 [details] [review]
Bug 20489: Prevent DB user login

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 M. Tompsett 2018-04-03 04:05:59 UTC
Created attachment 73544 [details] [review]
Bug 20489: Remove warnings from the interface

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 17 M. Tompsett 2018-04-03 04:06:44 UTC
See comment #9 and comment #10 for how I tested.
Comment 18 Marcel de Rooy 2018-04-13 09:12:17 UTC
QA: Looking here
Comment 19 Marcel de Rooy 2018-04-13 09:53:56 UTC
Is the term Basic installation complete? from onboarding still valid?
Since you cannot login anymore, you will have to go further..
Comment 20 Marcel de Rooy 2018-04-13 10:04:44 UTC
die "A patron with cardnumber '42' already exists" ???
From the new script misc/devel/create_superlibrarian.pl
Comment 21 Marcel de Rooy 2018-04-13 10:06:06 UTC
Just another note on that script: We should really remove the default koha/koha. Force people to enter something.
Comment 22 Marcel de Rooy 2018-04-13 10:07:13 UTC
Created attachment 74150 [details] [review]
Bug 20489: Prevent DB user login

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2018-04-13 10:07:24 UTC
Created attachment 74151 [details] [review]
Bug 20489: Remove warnings from the interface

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2018-04-13 10:08:38 UTC
In the release notes it would be good to refer to the Forgot password feature (hopefully enabled) or the script create_superlibrarian
Comment 25 Jonathan Druart 2018-04-13 13:36:50 UTC
(In reply to Marcel de Rooy from comment #21)
> Just another note on that script: We should really remove the default
> koha/koha. Force people to enter something.

I think that a blocker for this one.
Comment 26 Jonathan Druart 2018-04-20 15:49:40 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 27 Katrin Fischer 2018-04-22 08:49:21 UTC
I am not sure yet if this is the cause, but I cannot log in the OPAC with my staff user right now, which is not even superlibrarian?
Comment 28 Katrin Fischer 2018-04-22 08:58:07 UTC
Ok, reset_all fixed it, still baffled.
Comment 29 Katrin Fischer 2018-04-22 09:02:31 UTC
For other confused comments see bug 20632.
Comment 30 Katrin Fischer 2018-04-22 09:59:29 UTC
I am not sure about this one. 

Won't it remove the possibility to fix a forgotten superlibrarian password?
I know there is the script, but it's often hard to get IT run something for you, especially in smaller libraries.

Have updates been tested? I don't see that in comment#9.

What happens with libraries that didn't add a superlibrarian user in the past (pre-onboarding)?

>In the release notes it would be good to refer to the Forgot password feature >(hopefully enabled) or the script create_superlibrarian

The reset password feature won't ever be used by all libraries. For example libraries using LDAP, Shibboleth or CAS won't ever use it.

Also, why is there no proper test plan or nice description on the commit messages? (looking at you, Jonathan, this time! ;) )
Comment 31 Jonathan Druart 2018-04-23 14:50:41 UTC
(In reply to Katrin Fischer from comment #30)
> I am not sure about this one. 
> 
> Won't it remove the possibility to fix a forgotten superlibrarian password?
> I know there is the script, but it's often hard to get IT run something for
> you, especially in smaller libraries.

What is sure is that it put too much work on dev side, see related bug reports.
We discussed that several times and I understood there was a general agreement on removing this "feature".
It's a long standing problem and we will need to re-educate the users/sysadmin to use other tools, like the script to create real user accounts.
I think we will certainly have to provide more tools to replace this workaround.

> Have updates been tested? I don't see that in comment#9.

Marcel and I did.

> What happens with libraries that didn't add a superlibrarian user in the
> past (pre-onboarding)?

They will have to create a superlibrarian user.

> >In the release notes it would be good to refer to the Forgot password feature >(hopefully enabled) or the script create_superlibrarian
> 
> The reset password feature won't ever be used by all libraries. For example
> libraries using LDAP, Shibboleth or CAS won't ever use it.

I do not understand what it means.

> Also, why is there no proper test plan or nice description on the commit
> messages? (looking at you, Jonathan, this time! ;) )

I told it on comment 8, the idea was to let testers think about what I forgot.
Comment 32 Katrin Fischer 2018-04-23 14:54:25 UTC
If you use LDAP, you don't store patron's passwords, so you won't turn on the password reset feature. And you will turn off the password change option in the OPAC, because it will give people wrong ideas :)

Could the superlibrarian script be extended a bit (separate bug) to allow for changing passwords from command line?