https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- We have a lot of patrons whose userid and cardnumber are the same, so I'd want to preserve that. But it does make sense to prevent a userid/cardnumber that belongs to the cardnumber/userid of another borrower... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Nick Clemens from comment #0)

Currently Koha allows a user to sign in with either their cardnumber or userid

Confusion can arise when a user has a username that is another users cardnumber, and vice versa

We should check uniqueness across both of these fields.

We had definitely problems with that in the past and had solved it. Could this be a regression? For our libraries Joubu's solution would not work. We have all different cases: * Libraries where the self check reads the chip number that is long, complicated and unknown to the user. They user the userid for logging into the OPAC. * Libraries that don't use the userid at all, but only use the cardnumber Koha has always supported both - I think there is no step back from that. But uniqueness must be enforced. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #2)

Can you remind me the good reason behind the 2 login fields? :)

If you want to use cardnumber to login why not set userid=cardnumber and always use userid as login?

Like Katrin was saying, I think the cardnumber is more for machines, and userid is more for humans. At my local public library, they used to only have cardnumber for logging in, but then they created "alias" which is similar to userid I think. When I log in to my account, I use that human-friendly alias/userid, but I validate myself on the self-checkout using my card. (As I say that, I realize that the Koha self-checkout could have an option to show both cardnumber and userid logins... as you might've forgotten your card but might have your userid/password...) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=14323, | |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36575 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #6)

Could we consider adding CHECK constraints or add/update TRIGGERS at the DB level?

I think it's worth exploring for sure. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #8)

(In reply to David Cook from comment #7)

...

(In reply to Tomás Cohen Arazi (tcohen) from comment #6)

...

Could we consider adding CHECK constraints or add/update TRIGGERS at the DB level?

I think it's worth exploring for sure.

Got it implemented. School pick time got in the middle. Will submit first thing in the morning for y’all to ideate around it.

Cool :D -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #10 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 183664 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183664&action=edit Bug 33905: Add database triggers to prevent cardnumber/userid interchange This patch adds database triggers to prevent users from having cardnumbers that match other users' userids and vice versa. This prevents potential security issues and user confusion where a patron might accidentally or maliciously use another patron's cardnumber as their userid. The solution implementation includes: * Database triggers: - trg_borrowers_cardnumber_userid_insert: Prevents INSERT operations that would create conflicts - trg_borrowers_cardnumber_userid_update: Prevents UPDATE operations that would create conflicts * Exception-style error messages: - Koha::Exceptions::Patron::CardnumberMatchesUserID: When cardnumber matches another user's userid - Koha::Exceptions::Patron::UserIDMatchesCardnumber: When userid matches another user's cardnumber * New trigger_exists() utility function in C4::Installer: - Uses standard information_schema.triggers table for portability - Returns 1 if trigger exists, 0 if it doesn't - Includes proper POD documentation - Available for other atomicupdates to use * Database structure updates: - atomicupdate for existing installations - kohastructure.sql updates for new installations It uses database triggers instead of CHECK constraints because CHECK constraints with subqueries are not portable between MySQL/MariaDB and PostgreSQL. The exception messages follow Koha's exception naming patterns, making them easy to catch and handle in application code. TODO: * Exception handling is not implemented in Koha::Patron->store() * Exceptions are not defined yet * There's already Koha::Patron->has_valid_userid which should be adjusted to not look for duplicates and its tests should be adapted. * Some existing tests should fail and will need tweaks * I haven't checked if the TRIGGER definitions fail if existing inconsistencies are found. We should probably add a check in the atomicupdate before an attempt to change the DB. Test plan: 1. Apply the patches 2. Update the DB structure: $ ktd --shell k$ updatedatabase => SUCCESS: It works :-D 3. Run: k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t 4. Verify that attempts to create conflicting cardnumber/userid combinations throw the appropriate exceptions: - Koha::Exceptions::Patron::CardnumberMatchesUserID - Koha::Exceptions::Patron::UserIDMatchesCardnumber 5. Verify that valid operations still work normally 6. Test fresh installations to ensure triggers are created: k$ reset_all => SUCCESS: Triggers are created 7. Sign off :-D -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #11 from Owen Leonard <oleonard@myacpl.org> ---

k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t

I don't see this file -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #10)

It uses database triggers instead of CHECK constraints because CHECK constraints with subqueries are not portable between MySQL/MariaDB and PostgreSQL.

I think that one of these days we need to admit that Koha will only ever work with MySQL/MariaDB. I think we still have quite a few MySQLisms and introduce new ones sometimes too. That said, just because MySQLisms exist doesn't mean that we have to add more. It's an interesting point avoiding CHECK with subqueries because of lack of portability... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- When it comes to database triggers, we do need to be mindful of transactions and potential database deadlocks. That said, they can be difficult to diagnose, so easier said than done... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #15)

When it comes to database triggers, we do need to be mindful of transactions and potential database deadlocks. That said, they can be difficult to diagnose, so easier said than done...

You know... the more I think about it... the more I'm worried about this, since these triggers will fire for all updates to "borrowers", and there are a lot of different ways that borrowers can be updated (as we have 84 columns in that table). If userid and cardnumber were in a separate table, I'd be less worried. But yeah... I work on a non-Koha database with a fair number of triggers and deadlocks are a pain. Unfortunately, my concern is difficult to prove with a single user or a simple test script. It only becomes apparent when you start having concurrent users trying to modify the same data. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Username and cardnumber |A patron's username and |should be unique |cardnumber must not be used |respectively |by another patron in any | |permutation --- Comment #19 from David Cook <dcook@prosentient.com.au> --- Changing the title since "Username and cardnumber should be unique respectively" is already technically true. They are unique respectively. It's actually much more complicated than that... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- Technically, Patron A's cardnumber being used for Patron B's userid, or Patron A's userid being used for Patron B's cardnumber is only a real problem in two scenarios: 1. They share the same password 2. A cardnumber or userid is used for looking up a patron without the patron authenticating that cardnumber/userid with their password In theory, the second scenario should be mostly solved by bug 36575 although there are other places in Koha where we look up users with cardnumber, which is a terrible idea... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #22 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #21)

...

But... that would also be a big breaking change...

We are now having THE conversation, and we know how it looks to use the triggers. I will give the separate table a try tomorrow

Well... I thought I was on-board for the database trigger, but now I'm having doubts. We could try the "borrower_identifiers" option, and reset userid wherever it matches a cardnumber. In theory, that should work, and it shouldn't actually break anything... because the identifier itself will be preserved in someone's cardnumber. But it would be a significant change. Maybe we just try out the DB trigger based solution, since it's easy to add, and if it is a problem - it's also easy to remove. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183664|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 183687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183687&action=edit Bug 33905: Add database triggers to prevent cardnumber/userid interchange This patch adds database triggers to prevent users from having cardnumbers that match other users' userids and vice versa. This prevents potential security issues and user confusion where a patron might accidentally or maliciously use another patron's cardnumber as their userid. The solution implementation includes: * Database triggers: - trg_borrowers_cardnumber_userid_insert: Prevents INSERT operations that would create conflicts - trg_borrowers_cardnumber_userid_update: Prevents UPDATE operations that would create conflicts * Exception-style error messages: - Koha::Exceptions::Patron::CardnumberMatchesUserID: When cardnumber matches another user's userid - Koha::Exceptions::Patron::UserIDMatchesCardnumber: When userid matches another user's cardnumber * New trigger_exists() utility function in C4::Installer: - Uses standard information_schema.triggers table for portability - Returns 1 if trigger exists, 0 if it doesn't - Includes proper POD documentation - Available for other atomicupdates to use * Database structure updates: - atomicupdate for existing installations - kohastructure.sql updates for new installations It uses database triggers instead of CHECK constraints because CHECK constraints with subqueries are not portable between MySQL/MariaDB and PostgreSQL. The exception messages follow Koha's exception naming patterns, making them easy to catch and handle in application code. TODO: * Exception handling is not implemented in Koha::Patron->store() * Exceptions are not defined yet * There's already Koha::Patron->has_valid_userid which should be adjusted to not look for duplicates and its tests should be adapted. * Some existing tests should fail and will need tweaks * I haven't checked if the TRIGGER definitions fail if existing inconsistencies are found. We should probably add a check in the atomicupdate before an attempt to change the DB. Test plan: 1. Apply the patches 2. Update the DB structure: $ ktd --shell k$ updatedatabase => SUCCESS: It works :-D 3. Run: k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t 4. Verify that attempts to create conflicting cardnumber/userid combinations throw the appropriate exceptions: - Koha::Exceptions::Patron::CardnumberMatchesUserID - Koha::Exceptions::Patron::UserIDMatchesCardnumber 5. Verify that valid operations still work normally 6. Test fresh installations to ensure triggers are created: k$ reset_all => SUCCESS: Triggers are created 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #26)

Please test the backup/restore procedure to make sure the triggers are copied (regarding the doc it's enabled by default so it should be ok).

That's a good idea. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 Katie Bliss <kebliss@dmpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kebliss@dmpl.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #29 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #28)

I still think dcook has a point on this generating a couple extra queries on each INSERT/UPDATE. Having a separate table with a simple UNIQUE constraint could work too, but would imply breaking reports and I've heard of people wanting userid=cardnumber in some scenarios which wouldn't work well with the UNIQUE constraint (i.e. we would end up doing it in the code again or having the need of a trigger).

After some chatting on Mattermost and some braining, I think I might have a new idea which leverages both DB referential integrity and uniqueness while also relying on application code for the actual logic. Basically, the borrowers table stays essentially the same, but we change the constraints on the cardnumber and userid. We also create a new "identifiers" table (the name can be something else I don't care), which has a UNIQUE constraint on the identifier, so that the particular identifier can only exist 1 time in that table. The cleverness here is that the borrowers table can reference that unique identifier more than once for the *same* borrowernumber. I'll post the SQL in a separate comment. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #31 from David Cook <dcook@prosentient.com.au> --- That's the enforcement at the DB level. Of course, it's also going to take some Perl changes. For existing users, it's not a big drama, because we already have their borrowernumber, so we could just extract the cardnumber and userid and insert/update the test_identifiers table. For new users... we'd have to extract the cardnumber and userid before storage and then insert/update after we get the borrowernumber. So this could be a bit tricky... but it could be doable. The DB constraints are strong, and the application code could be flexible. It also protects us from irresponsible plugins and rogue sysadmins. -- Anyway, food for thought. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 --- Comment #33 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #32)

Oh my idea would actually cover off another scenario that Kyle raised previously. I'll find the ticket for that...

Oh maybe not as I'm not differentiating between types of identifier in that test_identifiers table although that could potentially be done... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion -- You are receiving this mail because: You are watching all bug changes.