Summary: | check_cookie_auth overwrites interface set by get_template_and_user | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Authentication | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | 1joynelson, bjorn.nylen, david, dcook, didier.gautheron, dpavlin, emily.lamancusa, fridolin.somers, jonathan.druart, laurent.ducos, lucas, m.de.rooy, martin.renvoize, nick, tomascohen |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30109 |
||
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
|
Circulation function: | |||
Attachments: |
Bug 33879: Do no longer overwrite interface in cookie_auth
Bug 33879: Add unit test to show problem Bug 33879: Do no longer overwrite interface in cookie_auth Bug 33879: Do no longer overwrite interface in check_cookie_auth Bug 33879: Add unit test to show problem Bug 33879: Do no longer overwrite interface in check_cookie_auth Bug 33879: Add unit test to show problem Bug 33879: Do no longer overwrite interface in check_cookie_auth Bug 33879: Add unit test to show problem Bug 33879: Do no longer overwrite interface in check_cookie_auth Bug 33879: Add unit test to show problem Bug 33879: Do no longer overwrite interface in check_cookie_auth |
Description
Marcel de Rooy
2023-06-01 12:11:29 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? Created attachment 151928 [details] [review] Bug 33879: Do no longer overwrite interface in 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 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Still looking at unit test. 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> Created attachment 151932 [details] [review] Bug 33879: Do no longer overwrite interface in 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> 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> It's late and I am tired but isn't this a security bug? Moving to security as a precaution. (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. That's the description of the test: 'cookie_auth did use interface from cookie' It's confusing IMO. (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. 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> 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> 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> 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> 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. (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 ! 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> 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> 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> 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> 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 Pushed to master for 23.11. Nice work everyone, thanks! Thanks for all the hard work! Pushed to 23.05.x for the next release Nice work everyone! Pushed to oldstable for 22.11.x *** Bug 32965 has been marked as a duplicate of this bug. *** |