Bug 33905 - A patron's username and cardnumber must each not be used by another patron in any permutation
Summary: A patron's username and cardnumber must each not be used by another patron in...
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-06 13:39 UTC by Nick Clemens (kidclamp)
Modified: 2025-10-07 11:13 UTC (History)
9 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Crowdfunding goal: 0
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 33905: Add database triggers to prevent cardnumber/userid interchange (13.70 KB, patch)
2025-07-01 14:10 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 33905: Add database triggers to prevent cardnumber/userid interchange (13.75 KB, patch)
2025-07-02 10:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.
Comment 28 Tomás Cohen Arazi (tcohen) 2025-10-01 18:47:33 UTC
(In reply to Martin Renvoize (ashimema) from comment #25)
> 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 was doing some ideating on the INSERT case where the borrowernumber (AUTO_INCREMENT) is still not generated. I will clean that as it is useless and should be on the UPDATE case.

> I've signed off to keep it moving, but the TODO stands I reckon before we
> can processed with QA

Of course. If there's agreement on this kind of approach I'll finish it.

I believe this is crucial to start cleaning our code and set a precedent on better coding practices once we agree on using triggers like this.

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).
Comment 29 David Cook 2025-10-06 23:22:35 UTC
(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.
Comment 30 David Cook 2025-10-06 23:25:03 UTC
CREATE TABLE test_identifiers (
    borrowernumber int(11) NOT NULL,
    identifier varchar(75) NOT NULL,
    CONSTRAINT t_ids UNIQUE (identifier),
    PRIMARY KEY (borrowernumber,identifier)
);

CREATE TABLE test_borrowers (
    borrowernumber int(11) NOT NULL AUTO_INCREMENT,
    cardnumber varchar(32) DEFAULT NULL,
    userid  varchar(75) DEFAULT NULL,
    CONSTRAINT t_cardnumber FOREIGN KEY (borrowernumber,cardnumber) REFERENCES test_identifiers (borrowernumber,identifier),
    CONSTRAINT t_userid FOREIGN KEY (borrowernumber,userid) REFERENCES test_identifiers (borrowernumber,identifier)
);

insert into test_borrowers (cardnumber,userid) VALUES (null,null);
insert into test_borrowers (cardnumber,userid) VALUES (null,null);
insert into test_borrowers (cardnumber,userid) VALUES (null,null);

--


I've made the SQL as simple as possible, so that you can easily try it out with some basic SQL in KTD. 

After running the above SQL, you can generate your identifiers:

insert into test_identifiers (borrowernumber,identifier) VALUES (1,'test');
insert into test_identifiers (borrowernumber,identifier) VALUES (1,'test2');

update test_borrowers set userid = 'test' where borrowernumber = 1
update test_borrowers set cardnumber = 'test' where borrowernumber = 1
update test_borrowers set userid = 'test2' where borrowernumber = 1

You can see here that the userid and cardnumber can be the same or different.

If you try to update borrowernumber 2 to use any of these identifiers, you'll find that it fails the DB constraint.
Comment 31 David Cook 2025-10-06 23:27:52 UTC
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.
Comment 32 David Cook 2025-10-07 00:51:46 UTC
Oh my idea would actually cover off another scenario that Kyle raised previously. I'll find the ticket for that...
Comment 33 David Cook 2025-10-07 00:57:56 UTC
(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...
Comment 34 Jonathan Druart 2025-10-07 07:46:43 UTC
(In reply to David Cook from comment #30)
> CREATE TABLE test_identifiers (
>     borrowernumber int(11) NOT NULL,
>     identifier varchar(75) NOT NULL,
>     CONSTRAINT t_ids UNIQUE (identifier),
>     PRIMARY KEY (borrowernumber,identifier)
> );
> 
> CREATE TABLE test_borrowers (
>     borrowernumber int(11) NOT NULL AUTO_INCREMENT,
>     cardnumber varchar(32) DEFAULT NULL,
>     userid  varchar(75) DEFAULT NULL,
>     CONSTRAINT t_cardnumber FOREIGN KEY (borrowernumber,cardnumber)
> REFERENCES test_identifiers (borrowernumber,identifier),
>     CONSTRAINT t_userid FOREIGN KEY (borrowernumber,userid) REFERENCES
> test_identifiers (borrowernumber,identifier)
> );

Great idea, I love it!