Bug 37247 - On subscriptions operation allowed without authentication
Summary: On subscriptions operation allowed without authentication
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Bugs List
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-03 14:53 UTC by Fridolin Somers
Modified: 2024-08-01 15:43 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
24.11.00,24.05.02,23.11.07,23.05.13,22.11.19
Version(s) released in:
Circulation function:


Attachments
Bug 37247: Fix subscriptions operation allowed without authentication (2.90 KB, patch)
2024-07-04 14:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 37247: Fix subscriptions operation allowed without authentication (2.96 KB, patch)
2024-07-04 20:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 37247: Fix display of "closed" (4.59 KB, patch)
2024-07-05 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37247: Fix display of "closed" (4.65 KB, patch)
2024-07-10 22:22 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 37247: Fix subscriptions operation allowed without authentication (3.02 KB, patch)
2024-07-11 09:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37247: Fix display of "closed" (4.71 KB, patch)
2024-07-11 09:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37247: [23.11.x] Fix subscriptions operation allowed without authentication (3.01 KB, patch)
2024-07-22 12:42 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 37247: [23.11.x] Fix display of "closed" (4.86 KB, patch)
2024-07-22 12:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 37247: [23.05.x] Fix subscriptions operation allowed without authentication (3.01 KB, patch)
2024-07-24 08:25 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 37247: [23.05.x] Fix display of "closed" (4.24 KB, patch)
2024-07-24 08:25 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 37247: [23.05.x] Send Koha::Subscription to template (1.58 KB, patch)
2024-07-24 08:25 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2024-07-03 14:53:08 UTC
On subscriptions, operations close and reopen can be launched without authentication :

For example in 22.11 :

Find an existing open subscription, ie subscriptionid=3.
Call : https://<staff interface>/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=3&op=close

See :
https://git.koha-community.org/Koha-community/Koha/src/branch/22.11.x/serials/subscription-detail.pl#L42

The operations close and reopen are before get_template_and_user().

In >= 22.11 there is CSRF but maybe still an issue.
Comment 1 Fridolin Somers 2024-07-03 15:22:35 UTC
We must also move :

my $subscription = Koha::Subscriptions->find( $subscriptionid );

I'll create a patch soon
Comment 2 David Cook 2024-07-04 00:13:57 UTC
Woof. Good catch.
Comment 3 Fridolin Somers 2024-07-04 14:26:36 UTC
Created attachment 168504 [details] [review]
Bug 37247: Fix subscriptions operation allowed without authentication

Move close and reopen after get_template_and_user().
Also move Koha::Subscriptions->find(), not a good idea to run DB queries
before authentication.

Test plan :
1) Apply patch
2) Authenticate to staff interface
3) Go to an existing open subscription
4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed
9) Check you can close and reopen subscription
Comment 4 Chris Cormack 2024-07-04 20:50:48 UTC
Created attachment 168513 [details] [review]
Bug 37247: Fix subscriptions operation allowed without authentication

Move close and reopen after get_template_and_user().
Also move Koha::Subscriptions->find(), not a good idea to run DB queries
before authentication.

Test plan :
1) Apply patch
2) Authenticate to staff interface
3) Go to an existing open subscription
4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed
9) Check you can close and reopen subscription

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 5 Jonathan Druart 2024-07-05 12:50:20 UTC
Created attachment 168535 [details] [review]
Bug 37247: Fix display of "closed"

The subscription was not shown as closed after we closed it.
This is because "closed" is not passed to the template.
It seems more reliable to rely on the subscription object (that is passed to both
serials/serials-collection.tt and serials/subscription-detail.tt, the
others are not showing the Reopen/Close buttons)

Also fetch the subscription object after and reopen/close it to display
accurate values.
Comment 6 Jonathan Druart 2024-07-10 14:16:28 UTC
Upping severity to get attention.
Comment 7 Chris Cormack 2024-07-10 22:22:00 UTC
Created attachment 168769 [details] [review]
Bug 37247: Fix display of "closed"

The subscription was not shown as closed after we closed it.
This is because "closed" is not passed to the template.
It seems more reliable to rely on the subscription object (that is passed to both
serials/serials-collection.tt and serials/subscription-detail.tt, the
others are not showing the Reopen/Close buttons)

Also fetch the subscription object after and reopen/close it to display
accurate values.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 8 Martin Renvoize 2024-07-11 09:53:40 UTC
Created attachment 168782 [details] [review]
Bug 37247: Fix subscriptions operation allowed without authentication

Move close and reopen after get_template_and_user().
Also move Koha::Subscriptions->find(), not a good idea to run DB queries
before authentication.

Test plan :
1) Apply patch
2) Authenticate to staff interface
3) Go to an existing open subscription
4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed
9) Check you can close and reopen subscription

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2024-07-11 09:53:43 UTC
Created attachment 168783 [details] [review]
Bug 37247: Fix display of "closed"

The subscription was not shown as closed after we closed it.
This is because "closed" is not passed to the template.
It seems more reliable to rely on the subscription object (that is passed to both
serials/serials-collection.tt and serials/subscription-detail.tt, the
others are not showing the Reopen/Close buttons)

Also fetch the subscription object after and reopen/close it to display
accurate values.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-07-11 09:54:48 UTC
Code change makes sense, no regressions were found in manual testing, QA scripts happy.

Passing QA
Comment 11 wainuiwitikapark 2024-07-18 05:26:15 UTC
Patch doesn't apply cleanly for 23.05.x, could this please be rebased?
Comment 12 Martin Renvoize 2024-07-18 05:48:53 UTC
Surely this should be pushed to 24.05 first and you cherry pick from there for 23.11 and then 23.05 as and when required.

I really don't understand how people are doing security bugs now, there seems to be no process being followed.
Comment 13 Frédéric Demians 2024-07-19 05:00:18 UTC
> I really don't understand how people are doing security bugs now, there
> seems to be no process being followed.

Me neither
Comment 14 Fridolin Somers 2024-07-22 12:42:38 UTC
Created attachment 169282 [details] [review]
Bug 37247: [23.11.x] Fix subscriptions operation allowed without authentication

Move close and reopen after get_template_and_user().
Also move Koha::Subscriptions->find(), not a good idea to run DB queries
before authentication.

Test plan :
1) Apply patch
2) Authenticate to staff interface
3) Go to an existing open subscription
4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed
9) Check you can close and reopen subscription

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Fridolin Somers 2024-07-22 12:43:54 UTC
Created attachment 169283 [details] [review]
Bug 37247: [23.11.x] Fix display of "closed"

The subscription was not shown as closed after we closed it.
This is because "closed" is not passed to the template.
It seems more reliable to rely on the subscription object (that is passed to both
serials/serials-collection.tt and serials/subscription-detail.tt, the
others are not showing the Reopen/Close buttons)

Also fetch the subscription object after and reopen/close it to display
accurate values.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Fridolin Somers 2024-07-22 12:45:00 UTC
23.11.x patches a bit different because no 'cud-'.
Comment 17 wainuiwitikapark 2024-07-23 04:04:38 UTC
I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly but I was able to rebase it - however, I still cannot close and reopen the subscription reliably
Comment 18 wainuiwitikapark 2024-07-23 23:57:54 UTC
(In reply to wainuiwitikapark from comment #17)
> I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly
> but I was able to rebase it - however, I still cannot close and reopen the
> subscription reliably

The first patch works, except I have to choose "Close" and "Reopen" twice for them to work
Comment 19 wainuiwitikapark 2024-07-24 01:44:55 UTC
(In reply to wainuiwitikapark from comment #18)
> (In reply to wainuiwitikapark from comment #17)
> > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly
> > but I was able to rebase it - however, I still cannot close and reopen the
> > subscription reliably
> 
> The first patch works, except I have to choose "Close" and "Reopen" twice
> for them to work

The second patch doesn't seem to work, I have tried rebasing it a few different ways
Comment 20 Fridolin Somers 2024-07-24 08:24:53 UTC
(In reply to wainuiwitikapark from comment #19)
> (In reply to wainuiwitikapark from comment #18)
> > (In reply to wainuiwitikapark from comment #17)
> > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly
> > > but I was able to rebase it - however, I still cannot close and reopen the
> > > subscription reliably
> > 
> > The first patch works, except I have to choose "Close" and "Reopen" twice
> > for them to work
> 
> The second patch doesn't seem to work, I have tried rebasing it a few
> different ways

I've rebased on 23.05.x and compared with 23.11.x

I think I found the cause

In 23.05.x the Koha::Subscription object in not sent to template => I create a follo-wup for this branch.
Comment 21 Fridolin Somers 2024-07-24 08:25:33 UTC
Created attachment 169452 [details] [review]
Bug 37247: [23.05.x] Fix subscriptions operation allowed without authentication

Move close and reopen after get_template_and_user().
Also move Koha::Subscriptions->find(), not a good idea to run DB queries
before authentication.

Test plan :
1) Apply patch
2) Authenticate to staff interface
3) Go to an existing open subscription
4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed
9) Check you can close and reopen subscription

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Fridolin Somers 2024-07-24 08:25:36 UTC
Created attachment 169453 [details] [review]
Bug 37247: [23.05.x] Fix display of "closed"

The subscription was not shown as closed after we closed it.
This is because "closed" is not passed to the template.
It seems more reliable to rely on the subscription object (that is passed to both
serials/serials-collection.tt and serials/subscription-detail.tt, the
others are not showing the Reopen/Close buttons)

Also fetch the subscription object after and reopen/close it to display
accurate values.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Fridolin Somers 2024-07-24 08:25:39 UTC
Created attachment 169454 [details] [review]
Bug 37247: [23.05.x] Send Koha::Subscription to template
Comment 24 wainuiwitikapark 2024-07-24 23:42:23 UTC
(In reply to Fridolin Somers from comment #20)
> (In reply to wainuiwitikapark from comment #19)
> > (In reply to wainuiwitikapark from comment #18)
> > > (In reply to wainuiwitikapark from comment #17)
> > > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly
> > > > but I was able to rebase it - however, I still cannot close and reopen the
> > > > subscription reliably
> > > 
> > > The first patch works, except I have to choose "Close" and "Reopen" twice
> > > for them to work
> > 
> > The second patch doesn't seem to work, I have tried rebasing it a few
> > different ways
> 
> I've rebased on 23.05.x and compared with 23.11.x
> 
> I think I found the cause
> 
> In 23.05.x the Koha::Subscription object in not sent to template => I create
> a follo-wup for this branch.

Thanks so much Fridolin!

Unfortunately with these patches, when do the following testing:

4) Open a new browser tab and use it to log-out
5) Go to first tab and click on 'Close'
6) You get login page
7) Authenticate
8) Check subscription is not closed

The subscription is closed when I log back in
Comment 25 wainuiwitikapark 2024-07-25 02:16:00 UTC
(In reply to wainuiwitikapark from comment #24)
> (In reply to Fridolin Somers from comment #20)
> > (In reply to wainuiwitikapark from comment #19)
> > > (In reply to wainuiwitikapark from comment #18)
> > > > (In reply to wainuiwitikapark from comment #17)
> > > > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly
> > > > > but I was able to rebase it - however, I still cannot close and reopen the
> > > > > subscription reliably
> > > > 
> > > > The first patch works, except I have to choose "Close" and "Reopen" twice
> > > > for them to work
> > > 
> > > The second patch doesn't seem to work, I have tried rebasing it a few
> > > different ways
> > 
> > I've rebased on 23.05.x and compared with 23.11.x
> > 
> > I think I found the cause
> > 
> > In 23.05.x the Koha::Subscription object in not sent to template => I create
> > a follow-up for this branch.
> 
> Thanks so much Fridolin!
> 
> Unfortunately with these patches, when do the following testing:
> 
> 4) Open a new browser tab and use it to log-out
> 5) Go to first tab and click on 'Close'
> 6) You get login page
> 7) Authenticate
> 8) Check subscription is not closed
> 
> The subscription is closed when I log back in

Apologies, Chris helped me understand how to test better.

I had to login in the new tab not the original one.

Can confirm the 23.05 patches work, thanks Fridolin!
Comment 26 Lucas Gass 2024-07-25 15:21:38 UTC
This has been backported to 24.05.x-security branch for 24.05.02
Comment 27 Katrin Fischer 2024-08-01 15:37:37 UTC
Pushed for 24.11!

Well done everyone, thank you!