Bug 24065 - Shibboleth should fail the login if matchpoint is not unique
Summary: Shibboleth should fail the login if matchpoint is not unique
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-19 12:19 UTC by Nick Clemens
Modified: 2019-11-21 11:36 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 24065: Fail shib login if multiple users matched (3.73 KB, patch)
2019-11-19 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24065: Fail shib login if multiple users matched (3.78 KB, patch)
2019-11-19 14:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 24065: Fail shib login if multiple users matched (3.87 KB, patch)
2019-11-21 08:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-11-19 12:19:30 UTC
The shib config allows you to use any patron field for matching. Email is sometimes used, but Koha does not enforce uniqueness here.

We call 'find' to get the patron, this will throw an error, but not fail

We should 'search' instead and fail explicitly if there is more than one patron
Comment 1 Nick Clemens 2019-11-19 13:03:00 UTC
Created attachment 95546 [details] [review]
Bug 24065: Fail shib login if multiple users matched

Ideally you could test against active shib, but is a small code change and
covered by tests and should be readable

To test:
prove -v t/Auth_with_shibboleth.t
Comment 2 Liz Rea 2019-11-19 14:52:30 UTC
Created attachment 95562 [details] [review]
Bug 24065: Fail shib login if multiple users matched

Ideally you could test against active shib, but is a small code change and
covered by tests and should be readable

To test:
prove -v t/Auth_with_shibboleth.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 3 Marcel de Rooy 2019-11-21 08:14:40 UTC
Created attachment 95648 [details] [review]
Bug 24065: Fail shib login if multiple users matched

Ideally you could test against active shib, but is a small code change and
covered by tests and should be readable

To test:
prove -v t/Auth_with_shibboleth.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2019-11-21 08:24:00 UTC
I still do not understand why we stick to Test::DBIx::Class to test changes like this. Obviously we need a borrowers database here ;)
Even more, if we use C4::Context in a t script (like this one), it makes not much sense.
The theoretical benefit of Test::DBIx::Class is imo not in balance with its *horrible* installation..
Comment 5 Martin Renvoize 2019-11-21 11:36:58 UTC
Nice work!

Pushed to master for 19.11.00