Bug 28660 - Self checkout is not automatically logging in
Summary: Self checkout is not automatically logging in
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: Other Linux
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: David Cook
URL:
Keywords:
Depends on: 21325
Blocks: 28735
  Show dependency treegraph
 
Reported: 2021-07-02 18:32 UTC by Jeremiah
Modified: 2021-11-24 21:59 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.x


Attachments
Bug 28660: (bug 21325 follow-up) Fix auto self check (1.15 KB, patch)
2021-07-06 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28660: (bug 21325 follow-up) Fix auto self check (1.34 KB, patch)
2021-07-12 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28660: (bug 21325 follow-up) Fix auto self check (1.35 KB, patch)
2021-07-22 05:34 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremiah 2021-07-02 18:32:46 UTC
The self-checkout is not automatically logging in with the staff username and password provided. Upon entering the URL for the self-checkout system, it is still prompting for the staff username and password.

This a fresh install of Koha 21.05 on Debian 10. No errors during installation.

Things I have done to ensure everything would work:

- I have created a patron account with the self-check modules turned on for this patron and the username and password is listed in the system preferences under AutoSelfCheckAllowed, and that is turned to allow.
- The selfcheck is enabled


This is either a bug or something that I am not seeing
Comment 1 Jeremiah 2021-07-03 19:07:24 UTC
I have tested this same feature in multiple Koha servers that are running 21.05 and they are having the same issue.

When testing this with Koha servers running 20.xx it works as described.
Comment 2 David Cook 2021-07-05 05:00:04 UTC
This sounds more like a support request than a bug. 

Is the "SelfCheckoutByLogin" system preference set to "Cardnumber" or "Username and Password"?
Comment 3 Jeremiah 2021-07-05 08:03:08 UTC
I originally thought it was a support request too, however as I mentioned above, I tested different versions of Koha on different systems and found that any Koha system running 21.05 were unable to use the auto-login feature. Any version of Koha that was 20.xx did not have that problem. 

To answer the previous question, it was set to cardnumber and also username and password and neither made a difference.
Comment 4 David Cook 2021-07-05 23:39:02 UTC
(In reply to Jeremiah from comment #3)
> I originally thought it was a support request too, however as I mentioned
> above, I tested different versions of Koha on different systems and found
> that any Koha system running 21.05 were unable to use the auto-login
> feature. Any version of Koha that was 20.xx did not have that problem. 
> 
> To answer the previous question, it was set to cardnumber and also username
> and password and neither made a difference.

Ah my apologies. I must not have read your original comment carefully enough. 

Let me take a quick look to see what I can see as this would be a huge problem...
Comment 5 David Cook 2021-07-05 23:46:36 UTC
I can confirm the problem in master, so I'm raising the severity. 

I don't have time right now to investigate a fix, but it probably won't be too hard. I'll make a note to look again later.
Comment 6 Jonathan Druart 2021-07-06 12:30:22 UTC
This is caused by
  commit da9006b20dc7439259043822902e05d392df7351
  Bug 21325: Prevent authentication when sending userid and password in querystring
Comment 7 Jonathan Druart 2021-07-06 12:50:16 UTC
Created attachment 122617 [details] [review]
Bug 28660: (bug 21325 follow-up) Fix auto self check
Comment 8 Jonathan Druart 2021-07-06 12:51:20 UTC
This patch is just adding more confusion to C4::Auth and make it even more fragile. Better alternative welcomed!
Comment 9 David Cook 2021-07-06 23:18:01 UTC
(In reply to Jonathan Druart from comment #8)
> This patch is just adding more confusion to C4::Auth and make it even more
> fragile. Better alternative welcomed!

I do have an idea, but it would require a few changes. Basically, it would be performing the autoselfcheck authentication before get_template_and_user and then passing a session object into get_template_and_user() through to checkauth(). I've done the same thing on some other patches, but have cheated each time by copying code rather than refactoring, so they're paused at the moment. We could refactor though and that would open up so many auth possibilities.

But my idea will take some time and effort. Since this problem affects 21.05.x, my idea probably isn't feasible.

Maybe the safest option is to just revert bug 21325.
Comment 10 Marcel de Rooy 2021-07-07 11:06:38 UTC
    || ( C4::Context->preference('AutoSelfCheckID')
    && $q_userid eq C4::Context->preference('AutoSelfCheckID') )

Feels to me that this would need some additional checks?
Like AutoSelfCheckAllowed  enabled?
Are we here in a self checkout context? Elsewhere I see matches for the template name? Or $query->param('koha_login_context') ne 'sco' ?
Comment 11 David Cook 2021-07-07 23:45:52 UTC
(In reply to Marcel de Rooy from comment #10)
>     || ( C4::Context->preference('AutoSelfCheckID')
>     && $q_userid eq C4::Context->preference('AutoSelfCheckID') )
> 
> Feels to me that this would need some additional checks?
> Like AutoSelfCheckAllowed  enabled?

I was thinking that too. 

> Are we here in a self checkout context? 

Yes.

> Elsewhere I see matches for the
> template name? Or $query->param('koha_login_context') ne 'sco' ?

Oh interesting. I do see in sco-main.pl the following within a check for AutoSelfCheckAllowed: $query->param(-name=>'koha_login_context',-values=>['sco']);

That said, that's a user-provided value, so technically you could easily use it to circumvent the protection that we added...
Comment 12 David Cook 2021-07-07 23:59:31 UTC
Ah but that koha_login_context check *plus* Jonathan's checking the userid against the AutoSelfCheckID.. that would probably be good enough although AutoSelfCheckAllowed would probably be better compared to koha_login_context, since it's a server-side call rather than provided by the client.

Anyway, that's just splitting hairs.

But I agree with Marcel. I think we need an extra check.
Comment 13 Marcel de Rooy 2021-07-08 05:38:08 UTC
(In reply to David Cook from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > Are we here in a self checkout context? 
> 
> Yes.

Hahaha. I was aware of that. But we need to check it :)
Comment 14 David Cook 2021-07-08 23:50:13 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to David Cook from comment #11)
> > (In reply to Marcel de Rooy from comment #10)
> > > Are we here in a self checkout context? 
> > 
> > Yes.
> 
> Hahaha. I was aware of that. But we need to check it :)

Haha. I'm sorry about that! I was reading that way too literally. That's what I get for trying to multi-task. I'm so embarrassed. Sorry again!
Comment 15 Jonathan Druart 2021-07-09 06:45:32 UTC
So, what do we do?

I don't think it's a good idea to revert bug 21325 (we announced that we prevented auth by GET and now we will accept it again?).

I don't see how an additional check would be helpful, but we can add it if needed (please provide a patch).

Note that if AutoSelfCheckAllowed is off, the CGI param won't be set from sco-main.pl and so $q_userid won't be == AutoSelfCheckID.
Additionally if we are not coming from the sco page and the user CGI param is AutoSelfCheckID we are getting kicked out earlier.
Comment 16 David Cook 2021-07-12 01:28:20 UTC
At this point, since this is affecting production instances, I think just getting any fix in is probably better than "the best fix", so let's just go ahead with this one and not delay.
Comment 17 Marcel de Rooy 2021-07-12 09:31:01 UTC
Created attachment 122773 [details] [review]
Bug 28660: (bug 21325 follow-up) Fix auto self check

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Works as expected.
The need for an additional check of pref WebBasedSelfCheck is
merely theoretical.
Comment 18 Marcel de Rooy 2021-07-12 09:33:36 UTC
No more delays? To expedite the process, combining SO and QA. If the RM feels the need for another SO, change status..
Comment 19 Jonathan Druart 2021-07-12 09:37:10 UTC
We have a bit of time before the next 21.05 release, so yes I would like a separate QA stamp.

Thanks for yours, Marcel!
Comment 20 David Cook 2021-07-20 23:13:40 UTC
I am planning to QA this. Ran out of time yesterday. Should be able to today.
Comment 21 David Cook 2021-07-20 23:14:19 UTC
There's another potential SCO regression I'll be looking into later today as well... need to confirm that it's a problem in master first though.
Comment 22 David Cook 2021-07-22 05:28:05 UTC
Ok finally getting to this. It would've been good to have a test plan but I'll whip one up.
Comment 23 David Cook 2021-07-22 05:32:02 UTC
Test plan:

0. Don't apply patch yet
1. Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
2. Note that you are prompted with a regular Koha login screen
3. Apply patch
4. koha-plack --restart kohadev
5. Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
6. Note that you are prompted with a Koha Self checkout system login screen
7. Log into the self checkout and note it works
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. Log into Koha and note it works

--

Well that's good enough for me.
Comment 24 David Cook 2021-07-22 05:33:22 UTC
testing 1 commit(s) (applied to 3748efc 'ab Bug 28731: (bug 17600 follow-up) I')

Processing files before patches
|========================>| 1 / 1 (100.00%)
Processing files after patches
|========================>| 1 / 1 (100.00%)

 OK     C4/Auth.pm

Processing additional checks OK!
Comment 25 David Cook 2021-07-22 05:34:27 UTC
Created attachment 123023 [details] [review]
Bug 28660: (bug 21325 follow-up) Fix auto self check

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Works as expected.
The need for an additional check of pref WebBasedSelfCheck is
merely theoretical.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 26 David Cook 2021-07-22 05:35:18 UTC
(In reply to David Cook from comment #21)
> There's another potential SCO regression I'll be looking into later today as
> well... need to confirm that it's a problem in master first though.

The other regression I was looking for wasn't there, so must be a local problem I'm having. 

Thanks, Jonathan for this one.
Comment 27 David Cook 2021-07-22 05:58:23 UTC
(In reply to David Cook from comment #26)
> (In reply to David Cook from comment #21)
> > There's another potential SCO regression I'll be looking into later today as
> > well... need to confirm that it's a problem in master first though.
> 
> The other regression I was looking for wasn't there, so must be a local
> problem I'm having. 
> 
> Thanks, Jonathan for this one.

Actually, it's not so much a regression as a security bug. Hurray. I love these.
Comment 28 Jonathan Druart 2021-07-22 06:16:12 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 29 Kyle M Hall 2021-07-23 11:46:33 UTC
Pushed to 21.05.x for 21.05.02
Comment 30 Fridolin Somers 2021-08-02 21:43:39 UTC
Depends on Bug 21325 not in 20.11.x