Shibboleth authentication has the ability to send an arbitrary number of attributes to koha; These attributes can be used to dynamically create and update user records in koha. Our shibboleth patches should support this.
Created attachment 26769 [details] [review] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users
Shamelessly grabbed the patches from Jesse's branch.. available here: https://github.com/pianohacker/koha/commits/shibboleth-v3.12.10 Re-based against master bug 8446, but kept original authorship as Jesse
Created attachment 26788 [details] [review] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users
Created attachment 28408 [details] [review] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users
Created attachment 30351 [details] [review] [SIGNED OFF] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 30352 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account.
Created attachment 30376 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account.
Created attachment 31054 [details] [review] [SIGNED OFF] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31055 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account.
Created attachment 35551 [details] [review] [SIGNED OFF] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 35552 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account.
Rebased against master
Note to self: must add unit tests
Very useful enhancement Martin! It is exactly what we need too :) One small suggestion: An extra line that defines dbh is probably required in the beginning of checkpw_shib in case _autocreate has to be called (ie something like: my $dbh = C4::Context->dbh; ) Other than that it's working like a charm for me!
Hi Theodoros, thx for testing this! I am setting to failed QA for now, as this will need a follow-up from Martin to get further. If you want, you could also sign off his patches as they are and add a follow-up for Martin to check and sign off on.
Thanks Theodoros, Think that must have crept in amongst the various rebases/iterations.. From a quick read of the code I actually thing the $dbh param can be removed from both the calling line and the sub unless I'm mistaken as it doesn't appear to be called upon at anywhere. I'll do a quick test and a followup.
Hello Martin, If you're still looking into this bug, an additional useful enhancement would be to be able to handle custom patron attributes! We have such a case where shibboleth returns a value that should go into a custom defined patron attribute and right now -although it's included it in the <mapping> tag- it is left blank, probably because ( $entry->{'is'} && $ENV{ $entry->{'is'} } ) is false My knowledge of Koha code is very limited, but I think Auth_with_ldap.pm has such a handling, so you could take ideas from there.
Created attachment 56241 [details] [review] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users
Created attachment 56242 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account.
Created attachment 56243 [details] [review] BUG 12026: Followup Rebasing
Created attachment 57486 [details] [review] Bug 12026 Fix small errors When running tests I got two errors. Do these changes make sense?
They make perfect sense.. look like mostly rebaseing errors to me.. oops :(
Created attachment 57563 [details] [review] BUG12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 57564 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 57565 [details] [review] BUG 12026: Followup Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 57566 [details] [review] Bug 12026 Fix small errors When running tests I got two errors. Do these changes make sense?
Martin: Please answer Mirko's question on the last patch and signoff on that one. After that set status back to SO. Thanks ! Temporarily moving out of the SO queue.
(In reply to Marcel de Rooy from comment #27) > Martin: Please answer Mirko's question on the last patch and signoff on that > one. After that set status back to SO. Thanks ! > > Temporarily moving out of the SO queue. Martin replied on IRC saying they make sense.
Perfect Mirko, thanks for cleaning up the rebase. Setting back to SO.
Just to clarify.. seems I've even been running Mirko's follow-up in production here on a couple of systems since they were done.. sorry for not having signed them off.. must have slipped through the net :(
Created attachment 61268 [details] [review] BUG 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61269 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61270 [details] [review] BUG 12026: Followup Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61271 [details] [review] Bug 12026 Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Added signoff line to signify that I've checked the rebase and followup added by Mirko :)
QA: Looking here now.
Created attachment 61363 [details] [review] Bug 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61364 [details] [review] Bug 12026: [Follow up] Added Documentation Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61365 [details] [review] Bug 12026: [Followup] Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61366 [details] [review] Bug 12026: Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61367 [details] [review] Bug 12026: [QA Follow-up] POD typo and tiny change in AddMember_Opac [1] Correct xml error in POD Copy-pasting this example from Auth_with_shibboleth.pm into koha-conf.xml did not work. We need a closing tag ;) [2] If AddMember_Opac calls AddMember_Auto, there is no need to call fixup_cardnumber. Adding trivial test for AddMember_Auto in Members.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
After the first patch, test t/Auth_with_shibboleth.t fails. Without it, it passes with me. Please check.
Created attachment 61435 [details] [review] BUG 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61436 [details] [review] BUG 12026, Follow up: Added Documentation BUG 12026, Follow up: Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61437 [details] [review] BUG 12026: Followup Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61438 [details] [review] Bug 12026 Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm investigating those test failures today.. I'm totally baffled as to why ldap appears to be getting loaded by the tests after these patches, but not before.
You changed the order of the patches. And you obsoleted my corrections of the commit messages. Could you restore them?
Output of the test starts with: 1..11 not ok 1 - use C4::Auth_with_shibboleth; # Failed test 'use C4::Auth_with_shibboleth;' # at t/Auth_with_shibboleth.t line 100. # Tried to use 'C4::Auth_with_shibboleth'. # Error: No ldapserver "hostname" defined in KOHA_CONF: /etc/koha/sites/master/koha-conf.xml at /usr/share/koha/masterclone/C4/Auth_with_ldap.pm line 59, <DATA> line 751.
-use_ok('C4::Auth_with_shibboleth'); +use C4::Auth_with_shibboleth; +require_ok('C4::Auth_with_shibboleth'); This will fix the tests. Do not ask me why and prefer to signoff on bug 17600 to avoid, maybe, such craziness... See also t/db_dependent/Circulation.t, commit 3660c451a36db128c143b6388d0d1f34babff4ac for a similar issue.
Created attachment 61444 [details] [review] Bug 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61445 [details] [review] Bug 12026: [Follow up] Added Documentation Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61446 [details] [review] Bug 12026: [Followup] Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61447 [details] [review] Bug 12026: Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61448 [details] [review] Bug 12026: [QA Follow-up] POD typo and tiny change in AddMember_Opac [1] Correct xml error in POD Copy-pasting this example from Auth_with_shibboleth.pm into koha-conf.xml did not work. We need a closing tag ;) [2] If AddMember_Opac calls AddMember_Auto, there is no need to call fixup_cardnumber. Adding trivial test for AddMember_Auto in Members.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61449 [details] [review] Bug 12026: [Rebase Follow-up] - Fix Test
I've added the fix for the test as a follow-up and it also includes a tiny fix to Mirko's rebased code which I had overlooked. Not sure if we need Mirko to retest for signoff or whether we can go back to QA ?
QA: Looking here again
Created attachment 61521 [details] [review] Bug 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61522 [details] [review] Bug 12026: [Follow up] Added Documentation Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61523 [details] [review] Bug 12026: [Followup] Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61524 [details] [review] Bug 12026: Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61525 [details] [review] Bug 12026: [QA Follow-up] POD typo and tiny change in AddMember_Opac [1] Correct xml error in POD Copy-pasting this example from Auth_with_shibboleth.pm into koha-conf.xml did not work. We need a closing tag ;) [2] If AddMember_Opac calls AddMember_Auto, there is no need to call fixup_cardnumber. Adding trivial test for AddMember_Auto in Members.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61526 [details] [review] Bug 12026: [Rebase Follow-up] - Fix Test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61527 [details] [review] Bug 12026: [QA Follow-up] Fix a POD line Still refers to a removed parameter of _autocreate. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Code looks good to me. This smaller change has mostly been code reviewed by me. I tested some constructs on itself, but I do not have a shibboleth server. Since most QAers do not and it has been tested by Martin and Mirko, I take the liberty to pass QA on it now. But not without a few remarks :) Koha::Database->new()->schema()->resultset('Borrower') ->find( { $config->{matchpoint} => $match } ); Strictly speaking, you should use find with primary keys/unique constraints. It might fallback to search here depending on your matchpoint (userid has a unique constraint). =head2 _autocreate my ( $retval, $retcard, $retuserid ) = _autocreate( $config, $match ); Given a database handle [...] => Where is that database handle? It got removed after the second patch.. Fixed in a followup. checkpw_shib The change to this routine should normally be accompanied by changes in the tests. Perhaps you should add autocreate => 0 explicitly to mockedConfig in this test. I wonder if testing autocreate => 1 should be done better in a db_dependent test. Since this patch goes such a long way, I do not want to block it now. But please add a small test on a new report. Passed QA
Thanks Marcel, I'll certainly take a look at adding the requisite tests.. as you say the original work goes back so far that the tests were just not thought of at the time and not added in any of my follow-ups :(. As for the call to find.. I agree, I'll add a further patch for that along with the tests (And include in those tests a case for a double matching matchpoint) Glad to see this groundwork set passed :)
Created attachment 61596 [details] [review] Add Tests for autocreate
In the name of doing it properly.. I've added the tests here :)
I'll do the switch from find to search single in another bug as it affects actual function.
Created attachment 61598 [details] [review] Add Tests for autocreate
Created attachment 61599 [details] [review] Bug 12026: [QA Follow-up] Add Tests for autocreate
Created attachment 61675 [details] [review] Bug 12026: Adds auto-provisioning to shibboleth - Adds the option to map shibboleth attributes to koha user columns and allows mapping to auto create non-existant users Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61676 [details] [review] Bug 12026: [Follow up] Added Documentation Updated to work with match point selection - The base bug that this patch enhances has added the ability to choose your match point in the configuration file. This patch adds code to take that change into account. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61677 [details] [review] Bug 12026: [Followup] Rebasing Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61678 [details] [review] Bug 12026: Fix small errors When running tests I got two errors. Do these changes make sense? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61679 [details] [review] Bug 12026: [QA Follow-up] POD typo and tiny change in AddMember_Opac [1] Correct xml error in POD Copy-pasting this example from Auth_with_shibboleth.pm into koha-conf.xml did not work. We need a closing tag ;) [2] If AddMember_Opac calls AddMember_Auto, there is no need to call fixup_cardnumber. Adding trivial test for AddMember_Auto in Members.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61680 [details] [review] Bug 12026: [Rebase Follow-up] - Fix Test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61683 [details] [review] Bug 12026: [QA Follow-up] Fix a POD line Still refers to a removed parameter of _autocreate. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 61684 [details] [review] Bug 12026: [QA Follow-up] Add Tests for autocreate https://bugs.koha-community.org/show_bug.cgi?id=12026 Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Fixes failing test in t/Auth_with_shibboleth.t Thanks for the fix!
Pushed to master for 17.05, thanks Jesse, Martin, Mirko, Marcel!
This won't get ported back to 16.11.x as it is an enhancement.
Is there documentation about this new feature available somewhere? Is it possible to choose if you want to add a new user and update existing users like it is with LDAP? Do the attributes have to match exactly to what Koha expects (branchcode, categorycode specifically) or is there a way to do a mapping?
Hi Katrin, the setup info are inside C4/Auth_with_shibboleth.pm in POD format: Map shibboleth attributes to koha fields, and configure authentication match point in koha-conf.xml. <shibboleth> <matchpoint>userid</matchpoint> <!-- koha borrower field to match upon --> <mapping> <userid is="eduPersonID"></userid> <!-- koha borrower field to shibboleth attribute mapping --> </mapping> </shibboleth> Note: The minimum you need here is a <matchpoint> block, containing a valid column name from the koha borrowers table, and a <mapping> block containing a relation between the chosen matchpoint and the shibboleth attribute name.
Thx Zeno - I looked at the patches, but didn't spot it!
Actually, after looking at the POD, I am still not sure what is the answer to my questions.
Hi Katrin, in fact to answer your question you need to read also the code. I do some answers: >Is there documentation about this new feature available somewhere? The documentation is inside C4/Auth_with_shibboleth.pm in POD format. To see it: perldoc C4/Auth_with_shibboleth.pm >Is it possible to choose if you want to add a new user and update existing users like it is with LDAP? No. The system only adds a new user, it does not update an existing user. And it adds a new user only when, inside login process, it finds that there isn't an existing user with userid = the shibboleth attribure defined with this XML in koha-conf.xml: <matchpoint>userid</matchpoint> <mapping> <userid is="uid"></userid> </mapping> >Do the attributes have to match exactly to what Koha expects (branchcode, categorycode specifically) or is there a way to do a mapping? You can mapping using koha-conf.xml. This is an example: <matchpoint>userid</matchpoint> <mapping> <userid is="uid"></userid> <surname is="sn"></surname> <categorycode is="tip"></categorycode> <branchcode is="cod"></branchcode> <email is="uid"></email> <emailpro is="mail"></emailpro> </mapping> The <matchpoint> ... </matchpoint> and the connected attribute are clearly mandatory. Also attributes for categorycode and branchcode are mandatory, I think. Ohters are optional. Bye
You're totally right Katrin, I should expand the documentation a bit here. This patch was only ever intended to 'add new' user record (i.e to 'provision') and not to 'sync'. To be honest, I'd love to also submit a 'sync' patch and include an option to only sync certain fields but I'd likely need a bit of sponsorship to get that through. I'm happy to code it up, but it's a nightmare to get these patches through signoff and qa ;)
Created attachment 62521 [details] [review] BUG 12027: Added shibboleth authentication to the staff client - This patch adds shibboleth authentication to the staff client. - Depending upon how your url structure works, you may or may not need a second native shibboleth service provider profile configured for this to work. http://bugs.koha-community.org/show_bug.cgi?id=12026
Created attachment 62522 [details] [review] BUG 12027 [Followup]: Added missing documentation
My Shibboleth and ADFS is working if I manually created the user. it's not creating the user automatically. here is my koha-conf.xml <shibboleth> <matchpoint>userid</matchpoint> <mapping> <userid is="NameID"></userid> <surname is="givenName"></surname> <branchcode is="description"></branchcode> <categorycode is="entitlement"></categorycode> <emailpro is="mail"></emailpro> </mapping> </shibboleth> What I am missing ?
Hi Ehsan, please open a new bug report or ask on the mailing list. Bugs with patches already released should not be reopened, but you can link bugs together using 'depends on'.