Bug 33905

Summary: A patron's username and cardnumber must each not be used by another patron in any permutation
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david, dcook, jonathan.druart, kyle, martin.renvoize, sally.healey, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14323
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36575
GIT URL: Change sponsored?: ---
Patch complexity: Medium patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 40286    
Attachments: Bug 33905: Add database triggers to prevent cardnumber/userid interchange
Bug 33905: Add database triggers to prevent cardnumber/userid interchange

Description Nick Clemens (kidclamp) 2023-06-06 13:39:11 UTC
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.
Comment 1 David Cook 2023-06-07 00:21:42 UTC
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...
Comment 2 Jonathan Druart 2023-06-07 08:09:43 UTC
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?
Comment 3 Katrin Fischer 2023-06-07 09:43:23 UTC
(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.
Comment 4 David Cook 2023-06-08 01:04:22 UTC
(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...)
Comment 5 Tomás Cohen Arazi (tcohen) 2023-06-08 12:12:45 UTC
(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.
Comment 6 Tomás Cohen Arazi (tcohen) 2025-06-30 20:04:20 UTC
Could we consider adding CHECK constraints or add/update TRIGGERS at the DB level?
Comment 7 David Cook 2025-06-30 23:34:33 UTC
(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.
Comment 8 Tomás Cohen Arazi (tcohen) 2025-06-30 23:36:05 UTC
(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.
Comment 9 David Cook 2025-06-30 23:37:10 UTC
(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
Comment 10 Tomás Cohen Arazi (tcohen) 2025-07-01 14:10:02 UTC
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
Comment 11 Owen Leonard 2025-07-01 19:03:49 UTC
>   k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t

I don't see this file
Comment 12 Tomás Cohen Arazi (tcohen) 2025-07-01 21:29:29 UTC
(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
Comment 13 David Cook 2025-07-01 23:22:08 UTC
(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...
Comment 14 David Cook 2025-07-01 23:23:11 UTC
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?
Comment 15 David Cook 2025-07-01 23:24:40 UTC
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...
Comment 16 David Cook 2025-07-01 23:27:14 UTC
To make this more DRY, I think you could probably create the checks as functions/procedure, and then reference them in the trigger.
Comment 17 David Cook 2025-07-01 23:36:59 UTC
(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.
Comment 18 David Cook 2025-07-01 23:53:51 UTC
(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...
Comment 19 David Cook 2025-07-02 00:04:15 UTC
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...
Comment 20 David Cook 2025-07-02 00:07:47 UTC
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...
Comment 21 Tomás Cohen Arazi (tcohen) 2025-07-02 00:35:57 UTC
(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
Comment 22 David Cook 2025-07-02 01:04:30 UTC
(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.
Comment 23 David Cook 2025-07-02 01:09:00 UTC
(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.
Comment 24 Martin Renvoize (ashimema) 2025-07-02 10:17:39 UTC
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>
Comment 25 Martin Renvoize (ashimema) 2025-07-02 10:19:13 UTC
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
Comment 26 Jonathan Druart 2025-07-03 10:02:13 UTC
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).
Comment 27 David Cook 2025-07-04 02:12:45 UTC
(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.