Bug 29543 - Self-checkout allows returning everybody's loans
Summary: Self-checkout allows returning everybody's loans
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 26857 (view as bug list)
Depends on:
Blocks: 28735 30045 30199
  Show dependency treegraph
 
Reported: 2021-11-20 20:34 UTC by Joonas Kylmälä
Modified: 2022-04-28 12:52 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25


Attachments
Bug 29543: Remove borrower variable (5.81 KB, patch)
2022-01-05 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Remove inputfocus variable (1.38 KB, patch)
2022-01-05 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Add JWT token handling (5.57 KB, patch)
2022-01-05 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Enforce authentication for self-checkout (7.59 KB, patch)
2022-01-05 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Prevent user to checkin or renew items they don't own (3.77 KB, patch)
2022-01-05 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Remove borrower variable (5.87 KB, patch)
2022-01-05 15:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: Remove inputfocus variable (1.43 KB, patch)
2022-01-05 15:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: Add JWT token handling (5.62 KB, patch)
2022-01-05 15:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: Enforce authentication for self-checkout (7.64 KB, patch)
2022-01-05 15:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: Prevent user to checkin or renew items they don't own (3.82 KB, patch)
2022-01-05 15:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin (1.35 KB, patch)
2022-01-05 15:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: Add Mojo::JWT dependency (694 bytes, patch)
2022-01-05 15:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Set autocomplete off for SCO login fields (1.82 KB, patch)
2022-01-05 16:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29543: [19.11] Enforce authentication for self-checkout (21.95 KB, patch)
2022-01-07 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29543: Remove borrower variable (5.92 KB, patch)
2022-01-08 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Remove inputfocus variable (1.49 KB, patch)
2022-01-08 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Add JWT token handling (5.67 KB, patch)
2022-01-08 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Enforce authentication for self-checkout (7.70 KB, patch)
2022-01-08 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Prevent user to checkin or renew items they don't own (3.88 KB, patch)
2022-01-08 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin (1.41 KB, patch)
2022-01-08 20:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Add Mojo::JWT dependency (749 bytes, patch)
2022-01-08 20:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: Set autocomplete off for SCO login fields (1.87 KB, patch)
2022-01-08 20:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29543: [19.11] Enforce authentication for self-checkout (22.06 KB, patch)
2022-01-21 01:45 UTC, wainuiwitikapark
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-11-20 20:34:30 UTC
All patrons have the ability to return everybody's loans using the OPAC self-checkout method! It should be possible only to return the loans of the authenticated patron.

To reproduce:

1) Login to SCO: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
2) Open Firefox developer console
3) Type some random barcode to the field "Scan a new item or enter its barcode: " and click "submit"
4) Right click on the dev console's network tab the sco-main.pl request and select "Edit and Resend". Then change the request contents to something like  
barcode=XXXXXXXX&op=returnbook&patronid=23529000591678&newissues=
5) Send the modified request via dev console and notice you have succesfully returned the item
Comment 1 Joonas Kylmälä 2021-11-20 20:57:54 UTC
Actually you don't even need to login at all, you can simply go to the following url unlogged:

localhost:8080/cgi-bin/koha/sco/sco-main.pl?barcode=39999000013733&op=returnbook&patronid=42&newissues=

and it works.

On a futher though, the bug here is actually only problematic in the above case, as the scenario where a patron can return everybody's books is probably possible by just physically going in to the library if they are using self-checkout machines over SIP. However, here the situation is different, the Koha OPAC might be exposed to the internet and given you can set SelfCheckoutByLogin to require authentication and now in the above case that is not required bad things might happen because now anyone on the internet can come and return everybody's books, so even if you had only trusted patrons in the library and thought you had configured that only those can do the returns that is not the case here.
Comment 2 Fridolin Somers 2021-12-02 01:01:00 UTC
Should we check that logged-in patron is the issuer of the document ?

We may also check if patron can renew an issue from someone else.
Comment 3 Katrin Fischer 2021-12-06 11:57:14 UTC
(In reply to Fridolin Somers from comment #2)
> Should we check that logged-in patron is the issuer of the document ?
> 
> We may also check if patron can renew an issue from someone else.

I am not sure if this will work for all libraries. Sometimes someone might want to return something for someone else, I know that we had that happen with students a lot. Checking issuer = logged in, would break that use case.

Is the person returning the item logged? Then we could find out if there was any abuse.
Comment 4 Jonathan Druart 2022-01-05 11:50:44 UTC
It worst than that, you can access personal info: /cgi-bin/koha/sco/sco-main.pl?patronid=23529001000463

You must turn off this feature ASAP on all your sites if the self-checkout is enabled.
Comment 5 Jonathan Druart 2022-01-05 14:09:53 UTC
(In reply to Katrin Fischer from comment #3)
> (In reply to Fridolin Somers from comment #2)
> > Should we check that logged-in patron is the issuer of the document ?
> > 
> > We may also check if patron can renew an issue from someone else.
> 
> I am not sure if this will work for all libraries. Sometimes someone might
> want to return something for someone else, I know that we had that happen
> with students a lot. Checking issuer = logged in, would break that use case.

Is this actually valid? The SCO UI does not allow you to checkin an item that you don't have in your account. It's only possible if you "hack" the url.
Comment 6 Jonathan Druart 2022-01-05 14:13:18 UTC
Created attachment 129022 [details] [review]
Bug 29543: Remove borrower variable

It's not needed, we have $patron
Comment 7 Jonathan Druart 2022-01-05 14:13:23 UTC
Created attachment 129023 [details] [review]
Bug 29543: Remove inputfocus variable

It's not used in template
Comment 8 Jonathan Druart 2022-01-05 14:13:26 UTC
Created attachment 129024 [details] [review]
Bug 29543: Add JWT token handling

Mojo::JWT is installed already, it's not a new dependency.
We need a way to send the patron a token when it's correctly logged in,
and not assumed it's logged in only if patronid is passed
Comment 9 Jonathan Druart 2022-01-05 14:13:30 UTC
Created attachment 129025 [details] [review]
Bug 29543: Enforce authentication for self-checkout

The self-checkout feature is assuming a patron is logged in if patronid
is passed. It also assumes that "We're in a controlled environment; we
trust the user", which is terribly wrong!

This patch is suggesting to generate a JSON Web Token (JWT) to store in
a cookie and only allow action (renew, check in/out) is the token is
valid. The token is only generated once the user has been authenticated
And is removed when the user finish the session/logout.

Test plan:
You must know exactly how the self-checkout feature works to test this patch.
The 4 following sysprefs must be tested:
 SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass
Confirm that you can renew, checkin for the items you own, and checkout new items.
Confirm that you are not allowed to access other account's info.
Comment 10 Katrin Fischer 2022-01-05 14:14:26 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Katrin Fischer from comment #3)
> > (In reply to Fridolin Somers from comment #2)
> > > Should we check that logged-in patron is the issuer of the document ?
> > > 
> > > We may also check if patron can renew an issue from someone else.
> > 
> > I am not sure if this will work for all libraries. Sometimes someone might
> > want to return something for someone else, I know that we had that happen
> > with students a lot. Checking issuer = logged in, would break that use case.
> 
> Is this actually valid? The SCO UI does not allow you to checkin an item
> that you don't have in your account. It's only possible if you "hack" the
> url.

Hm, I am not sure. But I think we need to probably check in 2 places:

* the separate sci feature that requires you to authenticate first (SelfCheckInModule)
* the in-built checkin feature in the sco screens (SCOAllowCheckin)

If it wasn't possible so far (and nooone missed it) then I think we could definitely do the stricter check.
Comment 11 Jonathan Druart 2022-01-05 14:14:58 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Katrin Fischer from comment #3)
> > (In reply to Fridolin Somers from comment #2)
> > > Should we check that logged-in patron is the issuer of the document ?
> > > 
> > > We may also check if patron can renew an issue from someone else.
> > 
> > I am not sure if this will work for all libraries. Sometimes someone might
> > want to return something for someone else, I know that we had that happen
> > with students a lot. Checking issuer = logged in, would break that use case.
> 
> Is this actually valid? The SCO UI does not allow you to checkin an item
> that you don't have in your account. It's only possible if you "hack" the
> url.

This needs to be confirmed. Unless I am missing something we should not allow people to checkin items they don't own (the UI does not allow that), and so we should make sure the "logged in user" is owning the item.
Comment 12 Jonathan Druart 2022-01-05 14:17:58 UTC
This is what I get when I enter a checked out barcode:

"""
Item cannot be checked out.

Sorry, this item cannot be checked out at this station.

This item is checked out to someone else. Please see a member of the library staff.
"""
Comment 13 Jonathan Druart 2022-01-05 14:29:00 UTC
Created attachment 129026 [details] [review]
Bug 29543: Prevent user to checkin or renew items they don't own

Checkin or renew must be restricted to the items they own.

Test plan:
Create an item with barcode bc_1
Check it in to user A
Login to SCO with user B
Get the token using the browser dev tool, from the cookie
Hit (replace $JWT)
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1

You should see an error message
Comment 14 Jonathan Druart 2022-01-05 15:28:35 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 129024 [details] [review] [review]
> Bug 29543: Add JWT token handling
> 
> Mojo::JWT is installed already, it's not a new dependency.

To be confirmed!
It was installed on D10, but Nick told me he had to install it on D11.

At least it's package in all versions:

    buster (oldstable) (perl): JSON Web Token the Mojo way
    0.08-1: all
    bullseye (stable) (perl): JSON Web Token the Mojo way
    0.09-1: all
    bookworm (testing) (perl): JSON Web Token the Mojo way
    0.09-1: all
    sid (unstable) (perl): JSON Web Token the Mojo way
    0.09-1: all
Comment 15 Nick Clemens 2022-01-05 15:33:49 UTC
Created attachment 129031 [details] [review]
Bug 29543: Remove borrower variable

It's not needed, we have $patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2022-01-05 15:33:53 UTC
Created attachment 129032 [details] [review]
Bug 29543: Remove inputfocus variable

It's not used in template

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2022-01-05 15:33:56 UTC
Created attachment 129033 [details] [review]
Bug 29543: Add JWT token handling

Mojo::JWT is installed already, it's not a new dependency.
We need a way to send the patron a token when it's correctly logged in,
and not assumed it's logged in only if patronid is passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2022-01-05 15:34:00 UTC
Created attachment 129034 [details] [review]
Bug 29543: Enforce authentication for self-checkout

The self-checkout feature is assuming a patron is logged in if patronid
is passed. It also assumes that "We're in a controlled environment; we
trust the user", which is terribly wrong!

This patch is suggesting to generate a JSON Web Token (JWT) to store in
a cookie and only allow action (renew, check in/out) is the token is
valid. The token is only generated once the user has been authenticated
And is removed when the user finish the session/logout.

Test plan:
You must know exactly how the self-checkout feature works to test this patch.
The 4 following sysprefs must be tested:
 SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass
Confirm that you can renew, checkin for the items you own, and checkout new items.
Confirm that you are not allowed to access other account's info.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2022-01-05 15:34:03 UTC
Created attachment 129035 [details] [review]
Bug 29543: Prevent user to checkin or renew items they don't own

Checkin or renew must be restricted to the items they own.

Test plan:
Create an item with barcode bc_1
Check it in to user A
Login to SCO with user B
Get the token using the browser dev tool, from the cookie
Hit (replace $JWT)
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1

You should see an error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2022-01-05 15:34:07 UTC
Created attachment 129036 [details] [review]
Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin

This updates the language to warn users of risk if using cardnumber for login and auto-self-check is enabled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Jonathan Druart 2022-01-05 15:38:07 UTC
Created attachment 129037 [details] [review]
Bug 29543: Add Mojo::JWT dependency
Comment 22 Jonathan Druart 2022-01-05 15:38:46 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 129037 [details] [review] [review]
> Bug 29543: Add Mojo::JWT dependency

Mason, we will need that in all stables if this solution is accepted.
Comment 23 Nick Clemens 2022-01-05 16:15:52 UTC
Created attachment 129039 [details] [review]
Bug 29543: Set autocomplete off for SCO login fields

Cardnumber already had it set, adding for username and password
Comment 24 Victor Grousset/tuxayo 2022-01-07 00:05:58 UTC
Backportability down to oldoldstable (20.11) looks good. The conflicts are simple.

However for oldoldoldold stable (19.11) there is one tricky with commit "Remove borrower variable" because some code for renewing has moved (or maybe it was just removed?). So care must be given in case 19.11 has additional code using $borrower.
Comment 25 Jonathan Druart 2022-01-07 09:24:30 UTC
Created attachment 129133 [details] [review]
Bug 29543: [19.11] Enforce authentication for self-checkout

The self-checkout feature is assuming a patron is logged in if patronid
is passed. It also assumes that "We're in a controlled environment; we
trust the user", which is terribly wrong!

This patch is suggesting to generate a JSON Web Token (JWT) to store in
a cookie and only allow action (renew, check in/out) is the token is
valid. The token is only generated once the user has been authenticated
And is removed when the user finish the session/logout.

Test plan:
You must know exactly how the self-checkout feature works to test this patch.
The 4 following sysprefs must be tested:
 SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass
Confirm that you can renew, checkin for the items you own, and checkout new items.
Confirm that you are not allowed to access other account's info.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Remove borrower variable

It's not needed, we have $patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Remove inputfocus variable

It's not used in template

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Add JWT token handling

Mojo::JWT is installed already, it's not a new dependency.
We need a way to send the patron a token when it's correctly logged in,
and not assumed it's logged in only if patronid is passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Prevent user to checkin or renew items they don't own

Checkin or renew must be restricted to the items they own.

Test plan:
Create an item with barcode bc_1
Check it in to user A
Login to SCO with user B
Get the token using the browser dev tool, from the cookie
Hit (replace $JWT)
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1

You should see an error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin

This updates the language to warn users of risk if using cardnumber for login and auto-self-check is enabled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Add Mojo::JWT dependency

Bug 29543: Set autocomplete off for SCO login fields

Cardnumber already had it set, adding for username and password
Comment 26 Jonathan Druart 2022-01-07 09:24:59 UTC
(In reply to Jonathan Druart from comment #25)
> Created attachment 129133 [details] [review] [review]
> Bug 29543: [19.11] Enforce authentication for self-checkout

Patches adjusted and squashed for 19.11.x
Comment 27 Katrin Fischer 2022-01-08 20:08:31 UTC
While trying to get the "Edit and resend" step done I noticed the following message:

Cookie “JWT” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Is this something we could/should improve on in this patch set?
Comment 28 Katrin Fischer 2022-01-08 20:17:36 UTC
Created attachment 129210 [details] [review]
Bug 29543: Remove borrower variable

It's not needed, we have $patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2022-01-08 20:17:41 UTC
Created attachment 129211 [details] [review]
Bug 29543: Remove inputfocus variable

It's not used in template

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2022-01-08 20:17:46 UTC
Created attachment 129212 [details] [review]
Bug 29543: Add JWT token handling

Mojo::JWT is installed already, it's not a new dependency.
We need a way to send the patron a token when it's correctly logged in,
and not assumed it's logged in only if patronid is passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2022-01-08 20:17:51 UTC
Created attachment 129213 [details] [review]
Bug 29543: Enforce authentication for self-checkout

The self-checkout feature is assuming a patron is logged in if patronid
is passed. It also assumes that "We're in a controlled environment; we
trust the user", which is terribly wrong!

This patch is suggesting to generate a JSON Web Token (JWT) to store in
a cookie and only allow action (renew, check in/out) is the token is
valid. The token is only generated once the user has been authenticated
And is removed when the user finish the session/logout.

Test plan:
You must know exactly how the self-checkout feature works to test this patch.
The 4 following sysprefs must be tested:
 SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass
Confirm that you can renew, checkin for the items you own, and checkout new items.
Confirm that you are not allowed to access other account's info.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2022-01-08 20:17:56 UTC
Created attachment 129214 [details] [review]
Bug 29543: Prevent user to checkin or renew items they don't own

Checkin or renew must be restricted to the items they own.

Test plan:
Create an item with barcode bc_1
Check it in to user A
Login to SCO with user B
Get the token using the browser dev tool, from the cookie
Hit (replace $JWT)
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1

You should see an error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2022-01-08 20:18:01 UTC
Created attachment 129215 [details] [review]
Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin

This updates the language to warn users of risk if using cardnumber for login and auto-self-check is enabled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2022-01-08 20:18:06 UTC
Created attachment 129216 [details] [review]
Bug 29543: Add Mojo::JWT dependency

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2022-01-08 20:18:11 UTC
Created attachment 129217 [details] [review]
Bug 29543: Set autocomplete off for SCO login fields

Cardnumber already had it set, adding for username and password

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2022-01-08 20:21:23 UTC
I've only tested the patch set for master, not for 19.11 yet. 

If there is a nice way to have a 19.11 env with koha-testing-docker, please let me know!
Comment 37 Jonathan Druart 2022-01-10 10:00:30 UTC
(In reply to Katrin Fischer from comment #27)
> While trying to get the "Edit and resend" step done I noticed the following
> message:
> 
> Cookie “JWT” will be soon rejected because it has the “SameSite” attribute
> set to “None” or an invalid value, without the “secure” attribute. To know
> more about the “SameSite“ attribute, read
> https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite
> 
> Is this something we could/should improve on in this patch set?

The warning certainly appears for other cookies as well.
Comment 38 Jonathan Druart 2022-01-10 10:01:49 UTC
(In reply to Katrin Fischer from comment #36)
> I've only tested the patch set for master, not for 19.11 yet. 
> 
> If there is a nice way to have a 19.11 env with koha-testing-docker, please
> let me know!

You can test stable branches doing:

% cd /path/to/koha
% git checkout origin/19.11.x
% cd /path/to/ktd
% git checkout origin/19.11

then run your usual ktd commands.
Comment 39 Victor Grousset/tuxayo 2022-01-12 00:33:30 UTC
(In reply to Jonathan Druart from comment #26)
> Patches adjusted and squashed for 19.11.x

Thanks a lot Jonathan :)



Tested on 20.11.x (superficial, I don't know exactly how self-checkout works)
- can't get anymore personal data using /cgi-bin/koha/sco/sco-main.pl?patronid=SOME_PATRON_CARD_NUMBER
- renewal until limit
- check-out
- check-in
- print receipt and end session
Comment 40 wainuiwitikapark 2022-01-18 02:38:42 UTC
(In reply to Victor Grousset/tuxayo from comment #39)
> (In reply to Jonathan Druart from comment #26)
> > Patches adjusted and squashed for 19.11.x
> 
> Thanks a lot Jonathan :)
> 
> 
> 
> Tested on 20.11.x (superficial, I don't know exactly how self-checkout works)
> - can't get anymore personal data using
> /cgi-bin/koha/sco/sco-main.pl?patronid=SOME_PATRON_CARD_NUMBER
> - renewal until limit
> - check-out
> - check-in
> - print receipt and end session

If this has been tested on 19.11.x already can I backport it?
Comment 41 Victor Grousset/tuxayo 2022-01-19 00:08:17 UTC
I think we can already backport and test. Until someone tells us RMaints to check more stuff than comment 39 or do themselves thorougher tests. We can assume it's enough.
Comment 42 Victor Grousset/tuxayo 2022-01-19 00:13:17 UTC
Backported: Pushed to 20.11.x-security branch for 20.11.14
Comment 43 wainuiwitikapark 2022-01-19 23:05:02 UTC
When applied to 19.11.x, it is not allowing any check-ins, check-outs and renewals. When clicking these things, the self-checkout module just logs the user out.

/cgi-bin/koha/sco/sco-main.pl?patronid=SOME_PATRON_CARD_NUMBER takes it to the login of self-checkout
Comment 44 Jonathan Druart 2022-01-20 08:41:03 UTC
(In reply to wainuiwitikapark from comment #43)
> When applied to 19.11.x, it is not allowing any check-ins, check-outs and
> renewals. When clicking these things, the self-checkout module just logs the
> user out.
> 
> /cgi-bin/koha/sco/sco-main.pl?patronid=SOME_PATRON_CARD_NUMBER takes it to
> the login of self-checkout

I've tried on v19.11.24+Bug 29543: [19.11] Enforce authentication for self-checkout
and it's working as expected.
Can you detail what you have in the related syspref?
 AutoSelfCheckAllowed  SelfCheckoutByLogin
Comment 45 wainuiwitikapark 2022-01-21 01:18:44 UTC
(In reply to Jonathan Druart from comment #44)
> (In reply to wainuiwitikapark from comment #43)
> > When applied to 19.11.x, it is not allowing any check-ins, check-outs and
> > renewals. When clicking these things, the self-checkout module just logs the
> > user out.
> > 
> > /cgi-bin/koha/sco/sco-main.pl?patronid=SOME_PATRON_CARD_NUMBER takes it to
> > the login of self-checkout
> 
> I've tried on v19.11.24+Bug 29543: [19.11] Enforce authentication for
> self-checkout
> and it's working as expected.
> Can you detail what you have in the related syspref?
>  AutoSelfCheckAllowed  SelfCheckoutByLogin

Oh awesome thanks Jonathan.

I have now seemed to update something on my devbox that now has got it working properly so all seems fine.

Is this able to be backported during string freeze?
Comment 46 Victor Grousset/tuxayo 2022-01-21 01:38:14 UTC
> Is this able to be backported during string freeze?

My guess is that string freeze can't be a thing for security patches. As our current processes it must cause untranslated strings on release unfortunately.
Because to be translatable a string change must be in the public branch during the string freeze. Which would disclose the security patch.
Comment 47 wainuiwitikapark 2022-01-21 01:45:47 UTC
Created attachment 129679 [details] [review]
Bug 29543: [19.11] Enforce authentication for self-checkout

The self-checkout feature is assuming a patron is logged in if patronid
is passed. It also assumes that "We're in a controlled environment; we
trust the user", which is terribly wrong!

This patch is suggesting to generate a JSON Web Token (JWT) to store in
a cookie and only allow action (renew, check in/out) is the token is
valid. The token is only generated once the user has been authenticated
And is removed when the user finish the session/logout.

Test plan:
You must know exactly how the self-checkout feature works to test this patch.
The 4 following sysprefs must be tested:
 SelfCheckoutByLogin, AutoSelfCheckAllowed, AutoSelfCheckID, AutoSelfCheckPass
Confirm that you can renew, checkin for the items you own, and checkout new items.
Confirm that you are not allowed to access other account's info.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Remove borrower variable

It's not needed, we have $patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Remove inputfocus variable

It's not used in template

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Add JWT token handling

Mojo::JWT is installed already, it's not a new dependency.
We need a way to send the patron a token when it's correctly logged in,
and not assumed it's logged in only if patronid is passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Prevent user to checkin or renew items they don't own

Checkin or renew must be restricted to the items they own.

Test plan:
Create an item with barcode bc_1
Check it in to user A
Login to SCO with user B
Get the token using the browser dev tool, from the cookie
Hit (replace $JWT)
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=renew&barcode=bc_1
    /cgi-bin/koha/sco/sco-main.pl?jwt=$JWT&op=returnbook&barcode=bc_1

You should see an error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: (follow-up) Add a warning to SelfCheckoutByLogin

This updates the language to warn users of risk if using cardnumber for login and auto-self-check is enabled

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 29543: Add Mojo::JWT dependency

Bug 29543: Set autocomplete off for SCO login fields

Cardnumber already had it set, adding for username and password

Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Comment 48 wainuiwitikapark 2022-01-21 01:49:23 UTC
(In reply to Victor Grousset/tuxayo from comment #46)
> > Is this able to be backported during string freeze?
> 
> My guess is that string freeze can't be a thing for security patches. As our
> current processes it must cause untranslated strings on release
> unfortunately.
> Because to be translatable a string change must be in the public branch
> during the string freeze. Which would disclose the security patch.

Ah true, thanks for that Victor!

Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 49 Andrew Fuerste-Henry 2022-01-26 17:01:04 UTC
Pushed to 21.05.x security for 21.05.09
Comment 50 Marcel de Rooy 2022-02-08 13:35:17 UTC
Question when looking in sco-main

my $jwt = $query->cookie('JWT');
if ($op eq "logout") {
    $template->param( loggedout => 1 );
    $query->param( patronlogin => undef, patronpw => undef );
    undef $jwt;
}

Shouldnt we do some cleaning up here and just exit ?
Comment 51 Marcel de Rooy 2022-02-08 13:37:32 UTC
Hmm. Why are we doing this in sco-main:

L47
my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
So the CGISESSID cookie is used here and we check permissions.

L371
$cookie = $query->cookie(     -name => 'JWT',
We create a new cookie JWT. But ignore the session cookie.

L381
output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 };
We output now without the CGISESSID, only JWT.
Why dont you pass the session cookie?
You could pass something like [ $cookie1, $cookie2 ] ?
Comment 52 Marcel de Rooy 2022-02-08 13:39:31 UTC
L362
            csrf_token           => Koha::Token->new->generate_csrf( { session_id => scalar $query->cookie('CGISESSID') . $patron->cardnumber, id => $patron->userid } ),

Where is the corresponding csrf check?
Comment 53 Marcel de Rooy 2022-02-08 14:55:58 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2022-02-08 15:26:10 UTC
$cookie = $query->cookie(
    -name => 'JWT',
    -value => $jwt // '',
    -expires => $jwt ? '+1d' : '',

Expiry should be shorter and in line with the expiry for sco.
Comment 55 Jonathan Druart 2022-02-08 15:27:46 UTC
(In reply to Marcel de Rooy from comment #50)
> Question when looking in sco-main
> 
> my $jwt = $query->cookie('JWT');
> if ($op eq "logout") {
>     $template->param( loggedout => 1 );
>     $query->param( patronlogin => undef, patronpw => undef );
>     undef $jwt;
> }
> 
> Shouldnt we do some cleaning up here and just exit ?

exit and what? Here we are going to display the login form, that's the expected behaviour

(In reply to Marcel de Rooy from comment #51)
> Hmm. Why are we doing this in sco-main:
> 
> L47
> my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
> So the CGISESSID cookie is used here and we check permissions.

We check authentication for the staff member (who has logged in, or the credential from the sysprefs)

> L371
> $cookie = $query->cookie(     -name => 'JWT',
> We create a new cookie JWT. But ignore the session cookie.

The JWT is used to authorised the OPAC user that has been authenticate in the previous step (cardnumber or login+password, depending on SelfCheckoutByLogin.

> L381
> output_html_with_http_headers $query, $cookie, $template->output, undef, {
> force_no_caching => 1 };
> We output now without the CGISESSID, only JWT.
> Why dont you pass the session cookie?
> You could pass something like [ $cookie1, $cookie2 ] ?

I think CGISESSID is in the CGI object already.

(In reply to Marcel de Rooy from comment #52)
> L362
>             csrf_token           => Koha::Token->new->generate_csrf( {
> session_id => scalar $query->cookie('CGISESSID') . $patron->cardnumber, id
> => $patron->userid } ),
> 
> Where is the corresponding csrf check?

in sco-patron-image.pl

(In reply to Marcel de Rooy from comment #53)
> Iiuc JWT is not encrypted. So this may be a bit more secure, but could be
> improved.

It is encoded using a secret, see Koha::Token::_gen_jwt
Comment 56 Jonathan Druart 2022-02-08 15:29:11 UTC
(In reply to Marcel de Rooy from comment #54)
> $cookie = $query->cookie(
>     -name => 'JWT',
>     -value => $jwt // '',
>     -expires => $jwt ? '+1d' : '',
> 
> Expiry should be shorter and in line with the expiry for sco.

Yes, correct. That could be shorter.
Comment 57 Marcel de Rooy 2022-02-08 15:52:59 UTC
(In reply to Jonathan Druart from comment #55)
> (In reply to Marcel de Rooy from comment #50)
> > Why dont you pass the session cookie?
> > You could pass something like [ $cookie1, $cookie2 ] ?
> 
> I think CGISESSID is in the CGI object already.

Cookies need to be sent out. If they have changed, they will only be updated at client side when you sent them explicitly to the output subs.
output_html_with_http_headers and friends actually assume that it is the authentication cookie; so you are cheating here.

> (In reply to Marcel de Rooy from comment #53)
> > Iiuc JWT is not encrypted. So this may be a bit more secure, but could be
> > improved.
> 
> It is encoded using a secret, see Koha::Token::_gen_jwt

OK. It is bit hidden but I found it in the _default_params sub.
Comment 58 Magnus Enger 2022-04-28 12:52:34 UTC
*** Bug 26857 has been marked as a duplicate of this bug. ***