This plugin hook allows to change patron data or define the patron based on the authenticated user. We use this hook to map the authenticated user to a patron record we migrated from the former ILS based on data from the identity provider. In Koha we use OIDC/Shibboleth for authentication but we don't have the EPPN in the former ILS and therefore not in the migrated patron record. We have written a plugin that takes the card number from the identity provider, searches a matching patron record in Koha and adds the EPPN to this patron.
Created attachment 164379 [details] [review] Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT)
Hi Raphael, you don't need to check if the plugins are enabled ie the if statement should be removed if ( C4::Context->config('enable_plugins') ) { .. } and could you please provide a rudimentary example plugin to test the hook
Hi Marc, we have added a test plugin, see attached file. This plugins write "Bug 36503: Hook test" into field "OPAC note" from the user currently logging in. To test: 1) Install and configure SSO with OIDC, see https://wiki.koha-community.org/wiki/Testing_SSO#SAML 2) Install the attached plugin AfterLogin-458d2ef.kpz 3) Login to OPAC Using SSO with user kohadev 4) Check to see "Messages for you: Bug 36503: Hook test" after Login
Created attachment 164628 [details] Plugin to test the hook
Hi Marc, if plugins are disabled and the if statement does not exist, we get following error message: "There was an error authenticating to external identity provider Can't call method "GetPlugins" on an undefined value at /kohadevbox/koha/Koha/Auth/Client.pm line 70." So the if statement is necessary.
I followed the test plan, but had to add an entry for "sso" into my "/etc/hosts": In docker-compose, keycloak runs as service name "sso" and thus is reachable from eg the OPAC via http://sso:8082. But as OAuth needs to also redirect the browser (running on my host) to the same URL, I needed to add this to the "/etc/hosts" on my host (= dev laptop): 127.0.0.1 sso Depending on your setup, you might need to do different things, just make sure that the "well_known_url" is "http://sso:8082/auth/realms/test/.well-known/openid-configuration" and that you can access this URL from your host and from inside koha testing docker (try "curl http://sso:8082", which should return some raw html like "Resource not found" and NOT "Failed to connect")
Created attachment 164893 [details] Screenshot showing that it worked! Just a small screenshot showing that the test plan worked
Also note that the unit test does not work after running the manual test plan and installing the pluging, so run the unit test before the manual test!
Created attachment 164894 [details] [review] Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at>
Hmm sounds interesting. There's all kinds of interesting/useful things you could do with a plugin hook like this. I'm not sure about the plugin method name. Despite this bug's title, I suppose you wouldn't be required to "modify" the user. This is just a method that is running after the user has been returned by a successful authentication. Hmm... I notice that there isn't a test on $patron, so the plugin author needs to make sure to add their own test to $patron to handle first time logins. I suppose that's OK, since you might want to update the $mapped_data. I wonder if the plugin name should reference OIDC/Oauth2 since at the moment it would only work for those protocols, but the hook is added to the base class Koha::Auth::Client... It seems that you'd need a different plugin hook for SAML though, so I think maybe it would be worth adding a prefix/suffix to the plugin method name... I better run, but overall I like the idea of this.
Hmm... wouldn't it make more sense to run this plugin just before returning? That way you could also have access to the $domain, and you'd have access to the default values coming from the domain in the $mapped_data. Otherwise, if you tried to change the categorycode/branchcode based on IdP data for a new patron, you wouldn't be able to do it. I won't mark as Failed QA yet, but I'm curious.
(In reply to David Cook from comment #10) > I'm not sure about the plugin method name. Despite this bug's title, I > suppose you wouldn't be required to "modify" the user. This is just a method > that is running after the user has been returned by a successful > authentication. Hmm... Yes, as this hook can do many things, I had the idea to name it "auth_client_get_user" after the namespace and method that calls it. > I wonder if the plugin name should reference OIDC/Oauth2 since at the moment > it would only work for those protocols, but the hook is added to the base > class Koha::Auth::Client... > > It seems that you'd need a different plugin hook for SAML though, so I think > maybe it would be worth adding a prefix/suffix to the plugin method name... I intentionally added the hook to the base class so that all authentication methods based on it use the same hook. You are right, currently, this is only OIDC/OAuth2, but I suppose that there will be additional authentication methods in the future. If we add some prefix/suffix to the plugin method name that indicates the authentication method, all plugins depending on that hook would have to be updated if the plugin method name changes because of additional authentication methods.
(In reply to David Cook from comment #11) > Hmm... wouldn't it make more sense to run this plugin just before returning? Maybe, but I wanted to use the update on auth parameter of the domain, so I had to update the mapped data before the call to $patron->set($mapped_data). In addition, if update on auth is true, the patron is updated in the database before the hook has the opportunity to choose another patron. > That way you could also have access to the $domain, and you'd have access to > the default values coming from the domain in the $mapped_data. > > Otherwise, if you tried to change the categorycode/branchcode based on IdP > data for a new patron, you wouldn't be able to do it. In our use case having access to the domain is not necessary, but calling the hook before $patron->…->store with the domain as an additional argument could be a good idea. Then the plugin might modify $domain->update_on_auth, $domain->default_category_id or $domain->default_library_id.
Created attachment 165466 [details] [review] Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT)
(In reply to David Cook from comment #11) > Hmm... wouldn't it make more sense to run this plugin just before returning? > > That way you could also have access to the $domain, and you'd have access to > the default values coming from the domain in the $mapped_data. > > Otherwise, if you tried to change the categorycode/branchcode based on IdP > data for a new patron, you wouldn't be able to do it. > > I won't mark as Failed QA yet, but I'm curious. Hi David, thanks for your feedback. As you can see Raphael has added a follow-up patch. We would be very glad if you could do QA and this hook would be in version 24.05 :-)
(In reply to Raphael Straub from comment #13) > In our use case having access to the domain is not necessary, but calling > the hook before $patron->…->store with the domain as an additional argument > could be a good idea. Then the plugin might modify $domain->update_on_auth, > $domain->default_category_id or $domain->default_library_id. The more I look at Koha::Auth::Client, the more I think the already existing approach has its flaws (like arbitrarily setting the branchcode and categorycode in the $mapped_data at the end of get_user()... I think that's probably a bug in the current implementation). -- Could you describe your use case more? I know someone who might be interested in this as well, so I'd like to get more feedback...
(In reply to David Cook from comment #16) > (In reply to Raphael Straub from comment #13) > > In our use case having access to the domain is not necessary, but calling > > the hook before $patron->…->store with the domain as an additional argument > > could be a good idea. Then the plugin might modify $domain->update_on_auth, > > $domain->default_category_id or $domain->default_library_id. > > The more I look at Koha::Auth::Client, the more I think the already existing > approach has its flaws (like arbitrarily setting the branchcode and > categorycode in the $mapped_data at the end of get_user()... I think that's > probably a bug in the current implementation). > > -- > > Could you describe your use case more? > > I know someone who might be interested in this as well, so I'd like to get > more feedback... Hi David, our use case is described in the Description of this bug. We use this hook to match and afterwards to update the patron record via data we get from the identity provider after the patron has authenticated. All this is done behind the scenes, the patron does not recognize this process, he just uses the normal authentication process he is used to (in the university context) We do not change the branchcode or the categorycode. This is our code : sub auth_client_get_user { my ( $self, $args ) = @_; my $logger = Koha::Logger->get(); my $mapped_data = $args->{'mapped_data'}; if ( !$args->{'patron'} ) { $logger->warn('Patron not found, find patron by card number'); $logger->warn(encode_json($mapped_data)); } if ( defined $mapped_data->{'cardnumber'} ) { # Split data (e. g. kit.edu:123456789) and set the card number. my ( $card_domain, $cardnumber ) = split( /\:/, $mapped_data->{'cardnumber'} ); $mapped_data->{'cardnumber'} = $cardnumber; $logger->warn( 'cardnumber: ' . $cardnumber ); if ( !$args->{'patron'} ) { # Find the patron by card number. my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); if ( $patrons->count == 1 ) { $args->{'patron'} = $patrons->next; $logger->warn('Patron found.'); } } } return; } If you see problems in Koha::Auth::Client this should be filed in a separate bug. We think this new hook has no negative effects on existing koha community code.
Hi Raphael, It looks like you missed Marks comment about removing the 'enable_plugins' section... in fact you can and should just use the ->call method now. my @responses = Koha::Plugins->call($method, @args); The above should suffice for a large part of your code block introduced.. it does the check for enable_plugins, find the right plugins and checked they're enabled/disabled individually and then runs their methods.. it's also allows passing by reference so you should still be able to use the passed arguments to fill the data on return.
(In reply to Martin Renvoize from comment #18) Hi Martin, > It looks like you missed Marks comment about removing the 'enable_plugins' > section... no, as Clemens wrote in comment #5 the if statement is necessary. > in fact you can and should just use the ->call method now. > > my @responses = Koha::Plugins->call($method, @args); > > The above should suffice for a large part of your code block introduced.. it > does the check for enable_plugins, find the right plugins and checked > they're enabled/disabled individually and then runs their methods.. it's > also allows passing by reference so you should still be able to use the > passed arguments to fill the data on return. Yes, I considered using Koha::Plugins->call but, as far as I understand, "call" calls the given method of all plugins that implement it in an arbitrary order. Because all such plugins could read/modify the mapped data and/or the patron, the order is important here. This is why I added the possibility to define a priority ($plugin->retrieve_data('priority')) on these plugins.
Created attachment 166050 [details] [review] Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT)
Sorry Raphael, I was distracted by all the release activity happening right now. I'm going to take a pass through this now; I didn't mean to block it. Regards the prioritising I hadn't spotted it.. we should certainly put that into 'call' at some point soon, I've been after such a feature for a while.
Created attachment 166111 [details] [review] Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166112 [details] [review] Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166113 [details] [review] Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166114 [details] [review] Bug 36503: Fix unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166115 [details] [review] Bug 36503: (follow-up) Simplify code by passing by reference
Created attachment 166116 [details] [review] Bug 36503: Move and update unit test
We could certainly test more thoroughly here and I was tempted to try and use the syntax we're moving towards for passing parameters... but I don't want to hold this up any further for now and don't have the time to work on further refinements. As such, I'm passing QA.
For reference, we've been moving to: ->call( 'hook_name', { action => 'action_name', payload => { key => value, . . . } }); See the 'before_x_action' and 'after_x_action' hooks added to Items.pm for example.
(In reply to Martin Renvoize from comment #26) > Created attachment 166115 [details] [review] [review] > Bug 36503: (follow-up) Simplify code by passing by reference Hi Martin, thanks for your work and your time! Unfortunately this patch does not work for our plugin. Our plugin sets the patron if it is undefined: if ( !$args->{'patron'} ) { # Find the patron by card number. my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); if ( $patrons->count == 1 ) { $args->{'patron'} = $patrons->next; $logger->warn('Patron found.'); } } So, with your patch, $patron is still undefined after calling the hook.
Moving to Failed QA to resolve issue pointed out by KIT
(In reply to Clemens Tubach from comment #30) > (In reply to Martin Renvoize from comment #26) > > Created attachment 166115 [details] [review] [review] [review] > > Bug 36503: (follow-up) Simplify code by passing by reference > > Hi Martin, > thanks for your work and your time! > Unfortunately this patch does not work for our plugin. > Our plugin sets the patron if it is undefined: > > if ( !$args->{'patron'} ) { > # Find the patron by card number. > my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); > if ( $patrons->count == 1 ) { > $args->{'patron'} = $patrons->next; > $logger->warn('Patron found.'); > } > } > > So, with your patch, $patron is still undefined after calling the hook. That should still work find.. have you tested?
I'll add to the unit test for you
(In reply to Martin Renvoize from comment #32) > (In reply to Clemens Tubach from comment #30) > > So, with your patch, $patron is still undefined after calling the hook. > > That should still work find.. have you tested? Yes, and it did not work.
Not forgotten this.. just iterating on the unit test and restoring the functionality you've highlighted regressed.
Created attachment 166458 [details] [review] Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166459 [details] [review] Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166460 [details] [review] Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166461 [details] [review] Bug 36503: Fix unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166462 [details] [review] Bug 36503: Move and update unit test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I stand corrected.. I've remove my patch and increased the test coverage
Thank to everyone for their perseverance :)
Hm, a little too early - Martin, can you double check? I am having a fail: prove -v /kohadevbox/koha/t/db_dependent/Koha/Auth/Client.t /kohadevbox/koha/t/db_dependent/Koha/Auth/Client.t .. 1..4 # Subtest: get_user() tests 1..5 C4::Context->userenv not defined! at /kohadevbox/koha/Koha/Patron.pm line 1869. C4::Context->userenv not defined! at /kohadevbox/koha/Koha/Patron.pm line 1869. ok 1 - Patron correctly retrieved ok 2 - Data mapped correctly ok 3 - No surname mapped ok 4 - Is the same domain # Looks like you planned 5 tests but ran 4. not ok 1 - get_user() tests
Created attachment 166531 [details] [review] Bug 36503: Move and update unit test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166532 [details] [review] Bug 36503: (follow-up) Fix tests count Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Thanks for the quick work, Tomas!
Pushed for 24.05! Well done everyone, thank you!
Please remember to update the Hooks page in the wiki!
Is it possible to backport this hook to 23.11? I have updated the Wiki page and added a text for release notes
additional_work_needed is obsolete ?
(In reply to Fridolin Somers from comment #50) > additional_work_needed is obsolete ? It was for the hooks page on the wiki, but I see it's listed there now.
(In reply to Michaela Sieber from comment #49) > Is it possible to backport this hook to 23.11? > > I have updated the Wiki page and added a text for release notes Sure, but we are close to release, I will have a look on next month release
Pushed to 23.11.x for 23.11.07
(In reply to Fridolin Somers from comment #53) > Pushed to 23.11.x for 23.11.07 Thank You!
UT depends on Bug 35536
Not backporting to 23.05.x unless requested