If you enabled AutoLocation and have a branch without ip, this
triggers warnings.
Test plan:
Check logs in this situation with/without this patch by logging out
and in again on staff.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This patch also renames StaffLoginBranchBasedOnIP to
StaffLoginLibraryBasedOnIP to be consistent with our terminology
guidelines
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch sets AutoLocation to be called StaffLoginRestrictBranchByIP.
The new name is chosen to reflect the new pref StaffLoginBranchBasedOnIP.
Also this patch corrects the order of sysprefs in installer file.
To test:
Follow test plans on bug 36665 and bug 35890 and confirm that the preferences
continue to work as expected
Confirm the descriptions of the prefs in the staff interface match the behaviors expected
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This adds a sort based on branchcode, it's a fallback for an edge case that should be rare
so I think is acceptable, as long as documented.
I added test coverage, but it may no longer be possible to encounter this scenario.
System preference descriptions are updated as well.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This patch adds a new system preference StaffLoginBranchBasedOnIP which restores the behaviour before bug 35918
of using the current IP to determine the user's logged in branchcode
To test:
1 - Get your current ip
2 - Set that IP for a library in the administration section
3 - Find a user account assigned to a different library that can login to staff side
4 - Login to staff as that user, select 'My library'
5 - You are logged in to the user's branch
6 - Apply patch, restart all
7 - Log out and back in, selecting 'My library'
8 - You are logged in to the user's branch
9 - Enable new system preference StaffLoginBranchBasedOnIP
9 - Log out and back in, selecting a different branch, noting the new warning below the library selection
10 - You are logged in to the branch with the matching IP
11 - Log out and back in, selecting 'My library'
10 - You are logged in to the branch with the matching IP
11 - Change your logged in branch
12 - Verify the selection sticks and you can perform staff actions in the chosen branch
13 - Change the IP of the library to one that doesn't match yours
14 - Verify you can log out and log back in and that selected branch is respected when your IP doesn't match library IP
Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
We can get into a scenario what a user cannot stay logged in for more than a single page load.
If AutoLocation is enabled with branch IP addresses being set to a space, you will be logged out with every page load.
Test Plan:
1) Set your branch ip to a space
2) Enable AutoLocation
3) Restart all the things!
4) Log out
5) Log in
6) Browse to another page
7) You are logged out
8) Apply patch
9) Repeat 1-6
10) You are not logged out!
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
To test:
1 - Have something on the SCO page that won't load
Add to SCOUserJS:
</script>
<img src="http://localhost:8080/silk/famfamfam.png"/>
<script>
2 - Enable the SCO and SCI system preferences:
WebBasedSelfCheck: Enable
SelfCheckInModuke: Enable
AutoSelfCheckAllowed: Allow
AutoSelfCheckID / AutoSelfCheckPass to a valid user with permissions
3 - Add the self_checkin_module and self_checkout_module permissions to the AutoSelfCheck user
4 - Access the selfcheckout module https://yoursite.org/cgi-bin/koha/sco/sco-main.pl
5 - Sign in using a patron
6 - Enter an unkown barcode, I used "DONTMATTER"
7 - If not logged out, click to 'Return to account summary'
8 - You are asked to sign in again
9 - Repeat with the SCI module
10 - Apply patch
11 - Repeat, but this time you can return to account
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Move the check to C4::Auth.
Yes, it's not nice, I didn't expect to add exception to this code, and
it's adding more ugly code to get_template_and_user, but... Suggestions
welcome!
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Working on bug 31791, I found myself wondering if our current recursive
code in C4::Auth::haspermission() would allow checking AND on
subpermissions.
As it is not documented in the POD or tested, I decided to write some
unit tests for it.
It turned out it was well supported, so I decided to submit the tests,
and a small tweak in the POD to reflect that.
To test:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Auth/haspermission.t
=> SUCCESS: Tests pass! The code supports AND on subpermissions!
3. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
We deal with that in the middleware, we don't want 403.pl to early exit.
If we notice that we actually need it (for other scripts), we could
eventually add a new flag to get_template_and_user to skip the CSRF check,
or build the template without using get_template_and_user for errors/*
scripts.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The easiest here is to not empty 'op' but instead redirect to an error
page.
Minor changes: to keep the patch simple it removed the 'dev only' error and
display the error for non-dev installs. It should not be a problem
anyway and will prevent errors to be hidden in the log.
We could make KOHA_ERROR an arrayref, but later (we don't need it now
anyway).
Note that the OPAC still not benefit from a friendly specific error for
invalid token.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
GET svc/authentication will return a CSRF token in a response header
POST svc/authentication requires a CSRF token which can be sourced
from the response header of GET svc/authentication or some other
place like the meta element on a HTML page
Note: misc/migration_tools/koha-svc.pl is a simple script which
can be used to practically evaluate svc/authentication and svc/bib
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Suggestion to move the CSRF check to CGI->new so that we will check it
for every request, and it will cover svc scripts as well (they are not
using get_template_and_user).
The token will be retrieve from the param list *or the csrf_token
header* (do we want to name it x-koha-csrf-token instead?).
This will be done for *every* request that are not GET: CSRF token is now
required everywhere CGI is used (side-effects possible?).
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Hum this didn't make sense. We are not checking credentials after
checkauth.
This patch is suggesting to rename "userid" and "password" parameters
from login forms to "login_userid" and "login_password" to not interfere
with other parameters with the same name.
This looks quite correct, however I am seeing
"The form submission failed (Wrong CSRF token)."
in the log after a successful login. Which feels wrong, what's
happening?
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Not sure this is the right place in get_template_and_user
Will have to test login and 2FA
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch adds a Koha::Session module that makes it easier
to work with Koha sessions without needing the full C4::Auth module.
Test plan:
0. Apply the patch
1. Run the following unit tests:
prove ./t/db_dependent/Auth.t
prove ./t/db_dependent/Auth_with_cas.t
prove ./t/db_dependent/Koha/Session.t
2. Observe that they all pass
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
It seems safer to pass the logged in user and session info at the end of
the sub.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This code is a bit weird, its purpose it to auto select the library depending on the IP.
A problem appears if the same IP is used, then the user's choice will
might be overwritten randomly by another library.
To recreate the problem:
Turn on AutoLocation
Use koha/koha @CPL for test
And the following config:
*************************** 1. row ***************************
branchcode: CPL
branchname: Centerville
branchip: 172.18.0.1
*************************** 2. row ***************************
branchcode: FFL
branchname: Fairfield
branchip: 172.18.0.1
*************************** 3. row ***************************
branchcode: FPL
branchname: Fairview
branchip: 172.18.0.4
Connect and select CPL. Randomly FFL will be picked instead.
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested this on top of 35890 and 35904 because git bz said they were required dependencies.
Figured out the IP Koha was seeing me as coming from in /var/log/koha/kohadev/plack.log.
Added that IP to the branchip for Centerville, Fairfield and Fairview. Set AutoLocation = Yes.
After this I could recreate the problem: If i left the "Library" field in the login screen
at "My Library" I got logged into a random library selected from the three i had set
branchip for. Applying the patches fixed this, as expected.
Tests pass, with AutoLocation off.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch suggests to add a new flag do_not_print to
C4::Auth::checkauth to not print the headers and allow to test this
subroutine more easily.
We do no longer need to mock safe_exit and redirect STDOUT to test its
return values.
There are still 3 left:
1.
733 # checkauth will redirect and safe_exit if not authenticated and not authorized
=> Better to keep this one, not trivial to replace
2.
806 # This will fail on permissions
This should be replaced but testing $template->{VARS}->{nopermission}
fails, I dont' think the comment is better.
3.
828 # Patron does not have the borrowers permission
Same as 2.
2. and 3. should be investigated a bit more.
This patch also move duplicated code to set patron's password to a
subroutine set_weak_password.
Test plan:
Read the code and confirm that everything makes sense.
QA: Do you have a better way for this? Yes it's dirty!
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Adapt code to the change of return value type of checkpw
introduced in bug 34893
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This patch introduces some tests on the current (and new) behavior for
the `checkpw` function.
I needed it to better understand if an edge case was actually possible
(it wasn't).
Found a really minor annoyance for the internal check with expired
password not returning the $patron object for consistency with the other
use cases.
I think this method deserves (at least) changing the return value to a
sane data structure. But that's not target for backporting to stable
releases. So a separate bug.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Imagine we have a set of users. Some of those users have a NULL userid. We then call AuthenticatePatron from ILS-DI for a patron with a NULL userid, but a valid cardnumber. We call checkpw, which returns the cardnumber and userid. We then call Koha::Patrons->find on the userid *which is null*, meaning the borrowernumber returned is not the correct one, but instead the earliest patron inserted into the database that has a NULL userid.
Test Plan:
1) Give three patrons a userid and a password
2) From the database cli, set all patrons's userid to null
Run this query: update borrowers set userid = null;
3) Call AuthenticatePatron with username being the 1st patron cardnumber,
and password being the password you set for that patron
http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=kohacard&password=koha
4) Note you get back a borrowernumber for a different patron. Refresh the page and the number is correct.
5) Do the same with the 2nd patron. Same issue at 1st and correct number after.
6) Apply this patch
7) Restart all the things!
8) Do the same with the 3rd patron.
9) Note you get the correct borrowernumber! :D
10) prove t/Auth.t t/db_dependent/Auth_with_ldap.t t/Auth_with_shibboleth.t t/db_dependent/Auth_with_cas.t
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
To test, disable OpacPublic and ensure a call to ilsdi.pl will still return expected results from a private browser, not logged into the OPAC.
Sponsored-by: Auckland University of Technology
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Test plan:
Without this patch:
Logout from OPAC. Crash.
Try to login. Crash.
With this patch:
Enable login in tracking triggers. Clear lastseen.
Flush memcache.
Login. Check lastseen.
Logout.
Login.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
We were using a series of similarly named methods spread in distinct places
around the codebase. This combines the logic of C4::Auth::track_login_daily
and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This patch adds a trigger to every instance of track_login_daily
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This patch adds triggers to track_login_daily so that it only tracks activity when that trigger is active
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Currently the system preference intranetreadinghistory determines visibility of both circulation history and holds history tabs in the patron record. It would be helpful to allow the option of setting each of those independently.
Specifically, libraries have requested the option of being able to view the holds history in a patron record without having to enable viewing of the circulation history.
Test Plan:
1) Apply this patch
2) Restart all the things!
3) Run updatadatabase.pl
4) Verify the new syspref intranetReadingHistoryHolds has the same value
as the existing syspref intranetreadinghistory
5) Disable intranetreadinghistory, enable intranetReadingHistoryHolds
6) Verify you can view a patron's holds history but not reading history
Signed-off-by: Sam Lau <samalau@gmail.com>
JD amended patch:
* renamed syspref intranetReadingHistoryHolds => IntranetReadingHistoryHolds
* tidy
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Bug 32721: (QA follow-up) Rename fields to opac*
This patch updates the field names to reflect that they're OPAC
related.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Bug 32721: (QA follow-up) Fix rebase errors
We let some superflous template params creep back in during a rebase
somewhere.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This patches addresses issues raised by the QA tests. It also adds a missed import of the Branches file in the document head
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Bug 32721: (QA follow-up) Add missing imports
Missing imports added in three template files
Exec flag added to atomic update file
Tinymce imports removed
A new bug will be created to move codemirror into an inc file at latest
version
Test plan as before
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Bug 32721: Tidy up - duplicate fetching of userjs and css
Currently UserJS and UserCSS is injected into the template as a parameter through Auth.pm but is then fetched using Koha.Preference() in the template. This patch tidies this up by removing the parameters from Auth.pm
Test plan as per first commit
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This patch sets the $auth_state to failed when changing auth sessions,
so that the new login attempt gets processed correctly (instead
of skipping the authorization step).
Test plan:
0. Apply the patch
1. koha-plack --reload kohadev
2. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl
3. Log in as an OPAC user with 0 permissions
4. Note the auth screen "Error: You do not have permission to access this page"
5. Click "Log in"
6. Note that you're still shown a login screen (and that you've been logged out of
your previous authenticated session)
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This change adds a mfa_range configuration option for TOTP
to koha-conf.xml, and overrides the "verify" method from
Auth::GoogleAuth in order to provide a new default for "range"
Test plan:
0. Apply the patch
1. koha-plack --restart kohadev
2. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=TwoFactorAuthentication
3. Change the syspref to "Enable"
4. Go to
http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
5. Click "More" and "Manage two-factor authentication"
6. Register using an app
7. In an Incognito window, go to
http://localhost:8081/cgi-bin/koha/mainpage.pl
8. Sign in with the "koha" user
9. Note down a code from your Authenticator app
10. Wait until after 60 seconds and try it
11. Note it says "Invalid two-factor code"
12. Try a new code from the app
13. Note that it works
14. Add <mfa_range>10</mfa_range> to /etc/koha/sites/kohadev/koha-conf.xml
15. Clear memcached and koha-plack --restart kohadev
16. Sign in with the "koha" user
17. Note down a code from your Authenticator app
18. Wait 4 minutes and then try it
19. Note that it works
20. Disable your two-factor authentication and click to re-enable it
21. Use a code older than 60 seconds when registering for the two
factor authentication
22. Note that the code works
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
