|
Description
Magnus Enger
2015-05-29 12:59:58 UTC
Yep cookies are tied to domains not ports. This will be a tricky one to fix it's essentially the same as an user. IE any user can do this if they have an opac login and the wrong permissions. The only difference is the sco user will definitely have circ permissions Could it be possible to not use the general login functionality of Koha for AutoSelfCheckID? I guess that means not to set the same cookie as the regular login, for this user? That way it should not be possible to carry the login over to other parts of Koha. Why not adding a new permission? It could solve easily the problem I think. We could even add a check on the permission page not to allow this new permission with others (and/or warn on the about page). I suppose the user used to access SCO is not used as a real user. *** Bug 10799 has been marked as a duplicate of this bug. *** ping? (In reply to Jonathan Druart from comment #3) > Why not adding a new permission? > It could solve easily the problem I think. Yeah, that sounds like it might be a nice way to solve it! Created attachment 42229 [details] [review] Bug 14298: Add the new 'self_checkout' permissions There is a security issue in the self checkout module. The user used to check items out must have the circulate => circulate_remaining_permissions permissions. So even if a user does not have a login/password or a barcode he cans access to the circulation module of the intranet. Imagine if the sco patron used is a superlibrarian... This patch set will change the behavior and adds a new permission to access to the sco module (circulate => self_checkout). This permission should be the only one defined for this patron. IMPORTANT NOTE: Hopefully, this only works if both interfaces use the same domains (but different ports). Test plan: 0/ Does not apply this patch set 1/ Create a patron with the circulate => circulate_remaining_permissions and some others. Note his userid/pwd (later 'sco/sco'). Turn on WebBasedSelfCheck and AutoSelfCheckAllowed Fill the AutoSelfCheckID and AutoSelfCheckPass wich 'sco' and 'sco' 2/ Log you out from the OPAC and the intranet 3/ Go on the sco page 4/ Note that your are automatically logged in 5/ Go on the circulation module on the intranet side 6/ Oops 7/ Apply this patch 8/ Execute the updatedatabase 9/ Note that the sco user only has the new permission circulate => self_checkout, others have been removed 10/ Try to reproduce the issue, it should not access anything on the intranet side 11/ Confirm that there is no regression in the sco module Created attachment 42230 [details] [review] Bug 14298: Use the new permission in the sco scripts Created attachment 42231 [details] [review] Bug 14298: Add warning to the about page if the sco user does not have correct permissions I am not familiar enough with this module to be confident with these patches, please be involved! Bug 10799 fixes the OPAC side of this. I am testing the new permission fixes the staff side. Created attachment 42249 [details] [review] Bug 14298: Add the new 'self_checkout' permissions There is a security issue in the self checkout module. The user used to check items out must have the circulate => circulate_remaining_permissions permissions. So even if a user does not have a login/password or a barcode he cans access to the circulation module of the intranet. Imagine if the sco patron used is a superlibrarian... This patch set will change the behavior and adds a new permission to access to the sco module (circulate => self_checkout). This permission should be the only one defined for this patron. IMPORTANT NOTE: Hopefully, this only works if both interfaces use the same domains (but different ports). Test plan: 0/ Does not apply this patch set 1/ Create a patron with the circulate => circulate_remaining_permissions and some others. Note his userid/pwd (later 'sco/sco'). Turn on WebBasedSelfCheck and AutoSelfCheckAllowed Fill the AutoSelfCheckID and AutoSelfCheckPass wich 'sco' and 'sco' 2/ Log you out from the OPAC and the intranet 3/ Go on the sco page 4/ Note that your are automatically logged in 5/ Go on the circulation module on the intranet side 6/ Oops 7/ Apply this patch 8/ Execute the updatedatabase 9/ Note that the sco user only has the new permission circulate => self_checkout, others have been removed 10/ Try to reproduce the issue, it should not access anything on the intranet side 11/ Confirm that there is no regression in the sco module Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Works well no regressions, changes the permissions appropriately. Created attachment 42250 [details] [review] Bug 14298: Use the new permission in the sco scripts Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Note without this patch, you won't be able to access the SCO side. So don't panic if you cant use SCO with just the first patch Created attachment 42251 [details] [review] Bug 14298: Add warning to the about page if the sco user does not have correct permissions Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Note: Made a little change, changed He should have, to They should have Thanks for the fix and the signoff! This works alright, but please add the permission to the translated installers before this is being pushed so we don't create inconsistencies. Created attachment 42511 [details] [review] Bug 14298: Add perms for other languages (In reply to Katrin Fischer from comment #17) > This works alright, but please add the permission to the translated > installers before this is being pushed so we don't create inconsistencies. I didn't forget that, it was completely expected. I always wait for someone to confirm the wording. Created attachment 42536 [details] [review] [PASSED QA] Bug 14298: Add the new 'self_checkout' permissions There is a security issue in the self checkout module. The user used to check items out must have the circulate => circulate_remaining_permissions permissions. So even if a user does not have a login/password or a barcode he cans access to the circulation module of the intranet. Imagine if the sco patron used is a superlibrarian... This patch set will change the behavior and adds a new permission to access to the sco module (circulate => self_checkout). This permission should be the only one defined for this patron. IMPORTANT NOTE: Hopefully, this only works if both interfaces use the same domains (but different ports). Test plan: 0/ Does not apply this patch set 1/ Create a patron with the circulate => circulate_remaining_permissions and some others. Note his userid/pwd (later 'sco/sco'). Turn on WebBasedSelfCheck and AutoSelfCheckAllowed Fill the AutoSelfCheckID and AutoSelfCheckPass wich 'sco' and 'sco' 2/ Log you out from the OPAC and the intranet 3/ Go on the sco page 4/ Note that your are automatically logged in 5/ Go on the circulation module on the intranet side 6/ Oops 7/ Apply this patch 8/ Execute the updatedatabase 9/ Note that the sco user only has the new permission circulate => self_checkout, others have been removed 10/ Try to reproduce the issue, it should not access anything on the intranet side 11/ Confirm that there is no regression in the sco module Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Works well no regressions, changes the permissions appropriately. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 42537 [details] [review] [PASSED QA] Bug 14298: Use the new permission in the sco scripts Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Note without this patch, you won't be able to access the SCO side. So don't panic if you cant use SCO with just the first patch Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 42538 [details] [review] [PASSED QA] Bug 14298: Add warning to the about page if the sco user does not have correct permissions Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Note: Made a little change, changed He should have, to They should have Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 42539 [details] [review] [PASSED QA] Bug 14298: Add perms for other languages Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to 3.20.x will be in 3.20.4 Patches pushed to master. Thanks Jonathan! Created attachment 45136 [details] [review] Bug 14298: Do not insert the permission if already exists. To avoid a warning on updating from 3.20.x to 3.22, use "insert ignore into". This patch is not useful for 3.20.x. Created attachment 45139 [details] [review] [SIGNED OFF] Bug 14298: Do not insert the permission if already exists. To avoid a warning on updating from 3.20.x to 3.22, use "insert ignore into". This patch is not useful for 3.20.x. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Followup pushed to master. Thanks Jonathan! Comment on attachment 42536 [details] [review] [PASSED QA] Bug 14298: Add the new 'self_checkout' permissions Bringing the old patches back |