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 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...
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?
(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.
(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...)
(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? The idea is that you can get your card lost or stolen, and you ask for a new one. The number in it should be different to avoid impersonation or to identify the card is no longer valid, etc.
Could we consider adding CHECK constraints or add/update TRIGGERS at the DB level?
(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.
(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.
(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
Created attachment 183664 [details] [review] 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
> k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t I don't see this file
(In reply to Owen Leonard from comment #11) > > k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t > > I don't see this file Yes, I removed it along with the exception handling code I wrote and needed more tests. The idea here is to look at the approach on the DB side and if there’s consensus, we complete this. I’ll leave it NSO to get eyes on it
(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...
I suppose 1 thing to note is that these triggers mean 1 create/update query now becomes 3 queries. Is the reason for doing the cardnumber and userid check with separate subqueries due to wanting to have more distinct error messages?
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...
To make this more DRY, I think you could probably create the checks as functions/procedure, and then reference them in the trigger.
(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.
(In reply to David Cook from comment #1) > 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 know... this problem would be simpler to solve via the schema if we eliminated the requirement that 1 user's userid and cardnumber can be the same. Because then we could just have a "borrower_identifiers" table with a unique index on "value". borrower_identifiers `value`,`type`,`borrowernumber` '42', 'cardnumber',51 'koha','userid',51 UNIQUE(value) -- But... that would also be a big breaking change...
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...
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...
(In reply to David Cook from comment #18) > (In reply to David Cook from comment #1) > > 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 know... this problem would be simpler to solve via the schema if we > eliminated the requirement that 1 user's userid and cardnumber can be the > same. > > Because then we could just have a "borrower_identifiers" table with a unique > index on "value". > > borrower_identifiers > `value`,`type`,`borrowernumber` > '42', 'cardnumber',51 > 'koha','userid',51 > UNIQUE(value) > > -- > > 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
(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.
(In reply to David Cook from comment #22) > 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. In theory... it should be a rare event that NEW.cardnumber and NEW.userid is not null for a borrowers table change, which means that the subqueries shouldn't run? You know... it'll probably be fine. The deadlocks I get on other systems are high-write high-concurrency systems. I guess I'm just trying to think of any possible pitfalls. But maybe let's not have the perfect be the enemy of the good... -- In which case, I think overall this looks pretty good.
Created attachment 183687 [details] [review] 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>
This all looks pretty solid to me, I like the use of triggers and it's introduction here is well thought out. I did wonder a little about why we need the COALESE on NEW.borrowernumber? I've signed off to keep it moving, but the TODO stands I reckon before we can processed with QA
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).
(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.