Bug 33879 - check_cookie_auth overwrites interface set by get_template_and_user
Summary: check_cookie_auth overwrites interface set by get_template_and_user
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
: 32965 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-06-01 12:11 UTC by Marcel de Rooy
Modified: 2023-11-27 19:53 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes an issue with recording the interface for the log viewer where installations run the OPAC and staff interface on the same domain name. Before this patch, if a user logged into the OPAC and then went to the staff interface and performed a logable action (such as a checkout), the interface in the log was incorrectly recorded as the OPAC.
Version(s) released in:
23.11.00,23.05.02,22.11.08


Attachments
Bug 33879: Do no longer overwrite interface in cookie_auth (1.72 KB, patch)
2023-06-01 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Add unit test to show problem (2.23 KB, patch)
2023-06-01 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in cookie_auth (1.74 KB, patch)
2023-06-01 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in check_cookie_auth (1.74 KB, patch)
2023-06-01 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Add unit test to show problem (2.30 KB, patch)
2023-06-13 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in check_cookie_auth (1.74 KB, patch)
2023-06-13 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33879: Add unit test to show problem (2.40 KB, patch)
2023-06-14 09:11 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in check_cookie_auth (1.84 KB, patch)
2023-06-14 09:13 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 33879: Add unit test to show problem (2.45 KB, patch)
2023-06-14 23:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in check_cookie_auth (1.89 KB, patch)
2023-06-14 23:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33879: Add unit test to show problem (2.51 KB, patch)
2023-06-27 13:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33879: Do no longer overwrite interface in check_cookie_auth (1.95 KB, patch)
2023-06-27 13:07 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 Marcel de Rooy 2023-06-01 12:11:29 UTC
From bug 33608
Comment 1 Marcel de Rooy 2023-06-01 12:52:34 UTC
Use case
If you logged in via OPAC (sharing same domain with intranet), and then switch to staff side, get_template_and_user gets the correct interface from the staff script but later on checkauth is called and then check_cookie_auth. The last one may override the interface with the interface from the cookie (opac in our example).

This is confusing.

What would be the side-effects if we decide not to overwrite there but only fill if we have nothing?
Comment 2 Marcel de Rooy 2023-06-01 13:14:12 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2023-06-01 13:14:39 UTC
Still looking at unit test.
Comment 4 Marcel de Rooy 2023-06-01 13:52:00 UTC
Created attachment 151931 [details] [review]
Bug 33879: Add unit test to show problem

Test plan:
Run t/db_dependent/Auth.t without second patch.
Should fail:
    #          got: 'opac'
    #     expected: 'intranet'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-06-01 13:52:03 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2023-06-01 13:52:29 UTC
Created attachment 151933 [details] [review]
Bug 33879: Do no longer overwrite interface in check_cookie_auth

This will only have effect on installations running OPAC and staff
on the same domain name. In that case an OPAC cookie still allows
you to access intranet, and v.v.

Test plan:
Repeat the following steps WITHOUT this patch and WITH it.
Login via OPAC.
Go to staff. Perform an action that logs the interface in e.g. the
statistics table, like a checkout.
Inspect interface in the corresponding table. Observe difference
that this patch makes.

With this patch:
Run t/db_dependent/Auth.t. Should pass again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Jonathan Druart 2023-06-01 15:30:02 UTC
It's late and I am tired but isn't this a security bug?
Moving to security as a precaution.
Comment 8 Marcel de Rooy 2023-06-02 06:20:57 UTC
(In reply to Jonathan Druart from comment #7)
> It's late and I am tired but isn't this a security bug?
> Moving to security as a precaution.

Thanks for being cautious. I dont think however that this is needed. Sharing a cookie is a known thing. This patch deals only with the innocent interface attribute.
Comment 9 Jonathan Druart 2023-06-06 09:13:42 UTC
That's the description of the test: 'cookie_auth did use interface from cookie'

It's confusing IMO.
Comment 10 Marcel de Rooy 2023-06-13 14:32:55 UTC
(In reply to Jonathan Druart from comment #9)
> That's the description of the test: 'cookie_auth did use interface from
> cookie'
> 
> It's confusing IMO.

Yes, it could certainly be a bit more clear. Second try on the way.
Comment 11 Marcel de Rooy 2023-06-13 14:37:28 UTC
Created attachment 152300 [details] [review]
Bug 33879: Add unit test to show problem

Test plan:
Run t/db_dependent/Auth.t without second patch.
Should fail:
    #          got: 'opac'
    #     expected: 'intranet'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2023-06-13 14:37:31 UTC
Created attachment 152301 [details] [review]
Bug 33879: Do no longer overwrite interface in check_cookie_auth

This will only have effect on installations running OPAC and staff
on the same domain name. In that case an OPAC cookie still allows
you to access intranet, and v.v.

Test plan:
Repeat the following steps WITHOUT this patch and WITH it.
Login via OPAC.
Go to staff. Perform an action that logs the interface in e.g. the
statistics table, like a checkout.
Inspect interface in the corresponding table. Observe difference
that this patch makes.

With this patch:
Run t/db_dependent/Auth.t. Should pass again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Björn Nylén 2023-06-14 09:11:25 UTC
Created attachment 152328 [details] [review]
Bug 33879: Add unit test to show problem

Test plan:
Run t/db_dependent/Auth.t without second patch.
Should fail:
    #          got: 'opac'
    #     expected: 'intranet'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 14 Björn Nylén 2023-06-14 09:13:39 UTC
Created attachment 152329 [details] [review]
Bug 33879: Do no longer overwrite interface in check_cookie_auth

This will only have effect on installations running OPAC and staff
on the same domain name. In that case an OPAC cookie still allows
you to access intranet, and v.v.

Test plan:
Repeat the following steps WITHOUT this patch and WITH it.
Login via OPAC.
Go to staff. Perform an action that logs the interface in e.g. the
statistics table, like a checkout.
Inspect interface in the corresponding table. Observe difference
that this patch makes.

With this patch:
Run t/db_dependent/Auth.t. Should pass again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 15 Björn Nylén 2023-06-14 09:20:18 UTC
Good thing, we serve intranet and opac on same host with different ports so this will be useful. Was hoping it would solve Bug 31990 too but that is apparently a different issue.

Failed a bit when attaching signed off patches but it looks correct now.
Comment 16 Marcel de Rooy 2023-06-14 11:10:10 UTC
(In reply to Björn Nylén from comment #15)
> Good thing, we serve intranet and opac on same host with different ports so
> this will be useful. Was hoping it would solve Bug 31990 too but that is
> apparently a different issue.
> 
> Failed a bit when attaching signed off patches but it looks correct now.

Yes, patches are fine. Thanks !
Comment 17 David Nind 2023-06-14 23:46:35 UTC
Created attachment 152366 [details] [review]
Bug 33879: Add unit test to show problem

Test plan:
Run t/db_dependent/Auth.t without second patch.
Should fail:
    #          got: 'opac'
    #     expected: 'intranet'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2023-06-14 23:46:39 UTC
Created attachment 152367 [details] [review]
Bug 33879: Do no longer overwrite interface in check_cookie_auth

This will only have effect on installations running OPAC and staff
on the same domain name. In that case an OPAC cookie still allows
you to access intranet, and v.v.

Test plan:
Repeat the following steps WITHOUT this patch and WITH it.
Login via OPAC.
Go to staff. Perform an action that logs the interface in e.g. the
statistics table, like a checkout.
Inspect interface in the corresponding table. Observe difference
that this patch makes.

With this patch:
Run t/db_dependent/Auth.t. Should pass again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Martin Renvoize 2023-06-27 13:07:47 UTC
Created attachment 152735 [details] [review]
Bug 33879: Add unit test to show problem

Test plan:
Run t/db_dependent/Auth.t without second patch.
Should fail:
    #          got: 'opac'
    #     expected: 'intranet'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-06-27 13:07:50 UTC
Created attachment 152736 [details] [review]
Bug 33879: Do no longer overwrite interface in check_cookie_auth

This will only have effect on installations running OPAC and staff
on the same domain name. In that case an OPAC cookie still allows
you to access intranet, and v.v.

Test plan:
Repeat the following steps WITHOUT this patch and WITH it.
Login via OPAC.
Go to staff. Perform an action that logs the interface in e.g. the
statistics table, like a checkout.
Inspect interface in the corresponding table. Observe difference
that this patch makes.

With this patch:
Run t/db_dependent/Auth.t. Should pass again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2023-06-27 13:09:23 UTC
Whilst we continue to support same domain using different ports for staff and opac I think this change makes a lot of sense..

Patch works as described, tests are included and passing and the qa scripts are happy.

No regressions found in manual testing either.. though I'd will be paying attention throughout the cycle just in case there's some unforeseen issue present.

Passing QA
Comment 22 Tomás Cohen Arazi 2023-07-05 15:19:34 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 23 Martin Renvoize 2023-07-17 15:35:48 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 24 Matt Blenkinsop 2023-07-18 13:28:24 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 25 Emily Lamancusa 2023-11-27 19:53:34 UTC
*** Bug 32965 has been marked as a duplicate of this bug. ***