| Summary: | Log message incorrect in Auth_with_shibboleth.pm | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Martin Renvoize (ashimema) <martin.renvoize> |
| Component: | Authentication | Assignee: | Jacob O'Mara <jacob.omara> |
| Status: | RESOLVED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
| Severity: | minor | ||
| Priority: | P5 - low | CC: | dcook, dpavlin, jacob.omara, lucas, m.de.rooy |
| Version: | unspecified | Keywords: | Academy |
| Hardware: | All | ||
| OS: | All | ||
| 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
Bug 31651: Replaced duplicate error message Bug 31651: Replaced duplicate error message Bug 31651: Adjust tests |
||
Created attachment 144312 [details] [review] BUG 31651: Replaced duplicate error message The duplicated error message on ln119 has now been replaced for clarity. Test plan to follow.... 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. (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. (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... Was going to SO here but are we waiting for unit tests? (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... (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. (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. (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? 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. 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.. (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. 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 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. 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> Pushed to master for 23.11. Nice work everyone, thanks! Created attachment 152865 [details] [review] Bug 31651: Adjust tests Thanks for all the hard work! Pushed to 23.05.x for the next release Nice work everyone! Pushed to 22.11.x for next release |
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`