Bug 31651 - Log message incorrect in Auth_with_shibboleth.pm
Summary: Log message incorrect in Auth_with_shibboleth.pm
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor
Assignee: Jacob O'Mara
QA Contact: Marcel de Rooy
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2022-09-29 10:39 UTC by Martin Renvoize
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08
Circulation function:


Attachments
BUG 31651: Replaced duplicate error message (998 bytes, patch)
2022-11-29 10:29 UTC, Jacob O'Mara
Details | Diff | Splinter Review
Bug 31651: Replaced duplicate error message (1.03 KB, patch)
2023-06-22 21:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31651: Replaced duplicate error message (1.13 KB, patch)
2023-06-23 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31651: Adjust tests (959 bytes, patch)
2023-06-29 12:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-09-29 10:39:23 UTC
There's a bad copy/paste of a log message in Auth_with_shibboleth.pm.

The second occurance of `There are several users with $config->{matchpoint} of $match, matchpoints must be unique` should really read something like `No users with $config->{matchpoint} of $match found and autocreate is disabled`
Comment 1 Jacob O'Mara 2022-11-29 10:29:00 UTC
Created attachment 144312 [details] [review]
BUG 31651: Replaced duplicate error message

The duplicated error message on ln119 has now been replaced for clarity.
Comment 2 Jacob O'Mara 2022-11-29 10:31:37 UTC
Test plan to follow....
Comment 3 Martin Renvoize 2022-12-07 10:08:42 UTC
I've set to NSO as the patch is complete.. though we should probably add a Unit test to be thorough.

Where we're struggling is how to write a test plan which could use the new --sso functionality of ktd to setup a shibboleth IdP such that this trivial patch can be proven.

CCing David whose much more familiar with KeyCloak.. I think we could do with getting a few little mini guides on how to set these things up for quick tests.
Comment 4 David Cook 2022-12-07 22:11:21 UTC
(In reply to Martin Renvoize from comment #3)
> CCing David whose much more familiar with KeyCloak.. I think we could do
> with getting a few little mini guides on how to set these things up for
> quick tests.

Sounds like a plan. I'll put it on my TODO list.
Comment 5 David Cook 2022-12-14 22:54:50 UTC
(In reply to Martin Renvoize from comment #3)
> CCing David whose much more familiar with KeyCloak.. I think we could do
> with getting a few little mini guides on how to set these things up for
> quick tests.

I have a few tasks I want to get done this week, so I think I'll try to set up those mini guides next week before going on holidays...
Comment 6 Lucas Gass 2022-12-15 16:09:03 UTC
Was going to SO here but are we waiting for unit tests?
Comment 7 David Cook 2022-12-19 23:19:58 UTC
(In reply to Martin Renvoize from comment #3)
> CCing David whose much more familiar with KeyCloak.. I think we could do
> with getting a few little mini guides on how to set these things up for
> quick tests.

I've created the wiki page https://wiki.koha-community.org/wiki/Testing_SSO

I haven't filled it in yet, but I'll see if I can get something up soon...
Comment 8 David Cook 2022-12-20 01:27:09 UTC
(In reply to David Cook from comment #7)
> I've created the wiki page https://wiki.koha-community.org/wiki/Testing_SSO
> 
> I haven't filled it in yet, but I'll see if I can get something up soon...

Apparently I'm not good at leaving things be... so I went ahead and created the guides.

The formatting needs work but I've tested both OpenID Connect and SAML with Keycloak and both worked. 

The SAML guide took a few iterations, so if something doesn't work, it's probably because I forgot to make update it. But I think it should be right.

Note that Koha forces HTTPS for SAML, which is good in theory, but not so good for testing. However, you can manually bypass this by updating the URL yourself to be HTTP only.

--

In the near future, we could probably set up koha-testing-docker to have more of this setup out of the box.
Comment 9 Lucas Gass 2023-01-09 16:56:46 UTC
(In reply to Martin Renvoize from comment #3)
> I've set to NSO as the patch is complete.. though we should probably add a
> Unit test to be thorough.

Failing this based on Martin's comments.
Comment 10 David Cook 2023-01-09 22:13:50 UTC
(In reply to Martin Renvoize from comment #3)
> I've set to NSO as the patch is complete.. though we should probably add a
> Unit test to be thorough.

Is this realistic without rewriting Auth_with_shibboleth.pm?
Comment 11 Martin Renvoize 2023-06-22 15:57:01 UTC
I think this is a trivial fix really.. we should let it through without the unit test and return to testing at a later date.
Comment 12 Martin Renvoize 2023-06-22 15:58:40 UTC
Hmm.. there's already an Auth_with_shibboleth.t test where I mocked an aweful lot.. we may be able to add tests there actually..
Comment 13 Katrin Fischer 2023-06-22 21:15:44 UTC
(In reply to Martin Renvoize from comment #12)
> Hmm.. there's already an Auth_with_shibboleth.t test where I mocked an
> aweful lot.. we may be able to add tests there actually..

While a test would be great, this looks mostly like a string change to me. I couldn't test, but all the same variables are used and the code comment is pretty clear too. I'll sign off.
Comment 14 Katrin Fischer 2023-06-22 21:17:37 UTC
Created attachment 152600 [details] [review]
Bug 31651: Replaced duplicate error message

The duplicated error message on ln119 has now been replaced for clarity.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2023-06-22 21:18:00 UTC
Comment on attachment 144312 [details] [review]
BUG 31651: Replaced duplicate error message

This actually had no sign-off yet, so mine is a SO. Also fixed commit message: BUG.. should be Bug.
Comment 16 Marcel de Rooy 2023-06-23 07:43:14 UTC
Created attachment 152620 [details] [review]
Bug 31651: Replaced duplicate error message

The duplicated error message on ln119 has now been replaced for clarity.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Tomás Cohen Arazi 2023-06-23 12:45:53 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Tomás Cohen Arazi 2023-06-29 12:38:46 UTC
Created attachment 152865 [details] [review]
Bug 31651: Adjust tests
Comment 19 Martin Renvoize 2023-07-17 12:45:25 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 20 Pedro Amorim 2023-07-18 10:50:00 UTC
Nice work everyone!

Pushed to 22.11.x for next release