[Bug 31393] New: shibboleth broken since migration from XML::Simple
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Bug ID: 31393 Summary: shibboleth broken since migration from XML::Simple Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Authentication Assignee: koha-bugs@lists.koha-community.org Reporter: didier.gautheron@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: dpavlin@rot13.org Hi, shibboleth can use content in its koha-conf.xml mapping but content key is optimized out if it's the only key before #28278 : koha-conf <branchcode content="foo"/> perl: 'branchcode' => { 'content' => 'foo' }, Now <branchcode content="foo"/> perl: 'branchcode' => 'foo', It's faster but it breaks C4/Auth_with_shibboleth.pm while ( my ( $key, $entry ) = each %{$config->{'mapping'}} ) { if ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) { $borrower{$key} = ( $entry->{'is'} && $ENV{"HTTP_" . uc($entry->{'is'}) } ) || $entry->{'content'} || ''; } else { $borrower{$key} = ( $entry->{'is'} && $ENV{ $entry->{'is'} } ) || $entry->{'content'} || ''; } } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|shibboleth broken since |Shibboleth broken since |migration from XML::Simple |migration from XML::Simple -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com, | |matthias.meusburger@biblibr | |e.com --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Can someone using Shibboleth chime in here? It's marked enhancement, but that seems wrong if things are totally broken? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- <branchcode content="foo"/> doesn't look like valid configuration to me. It should be something like <branchcode content="foo" is="branch"/> or <branchcode content="foo" is="branch"></branchcode> If you have the "is" attribute, then there wouldn't be a problem. That said, I don't get why bug 28278 has any handling for the "content" attribute... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #3 from Matthias Meusburger <matthias.meusburger@biblibre.com> --- Hi, Actually, having only "content" or only "is" is valid. "is" will get the value from the matching Shibboleth attribute. "content" is a fixed string. If you use both, Koha will first check if there is an attribute matching "is", and then use "content": C4/Auth_with_shibboleth.pm, line 131: $borrower{$key} = ( $entry->{'is'} && $ENV{"HTTP_" . uc($entry->{'is'}) } ) || $entry->{'content'} || ''; -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |28278 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- (In reply to Matthias Meusburger from comment #3)
Hi,
Actually, having only "content" or only "is" is valid.
"is" will get the value from the matching Shibboleth attribute.
"content" is a fixed string.
Ah that's good to know. Thanks for the correction. That would explain too why I haven't noticed the problem yet as well. -- I still think we should look at moving away from koha-conf.xml anyway. But I suppose we have to fix this in the meantime... I took a look at the code from bug 28278 and I have no idea why it removes elements with an attribute of "content"... Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28278 [Bug 28278] Improve $KOHA_CONF parsing speed by using XML::LibXML -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #4)
I took a look at the code from bug 28278 and I have no idea why it removes elements with an attribute of "content"...
Of course, as soon as I look at it again, the answer looks obvious. At a glance, it looks like we should change "content" to "_content" in _read_from_dom_node. But I haven't actually written that out or tried it yet. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- Created attachment 152609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152609&action=edit Bug 31393: Use _content as a placeholder when parsing koha-conf.xml This patch uses '_content' instead of 'content' as a placeholder when parsing koha-conf.xml, so that elements with a "content" attribute don't get parsed incorrectly (like with shibboleth config). Test plan: 0. Apply patch 1. echo 'flush_all' | nc -q 1 memcached 11211 2. koha-plack --reload kohadev 3. Add Shibboleth config to 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 --> <branchcode content="foo"/> </mapping> </shibboleth> 4. vi Koha/Config.pm 5. Dump out the $config from the read_from_file() function 6. Note that the following is shown: 'branchcode' => { 'content' => 'foo' } 7. Note that the following is NOT shown: 'branchcode' => 'foo' 8. git restore Koha/Config.pm 9. Rejoice! For bonus points, you can actually do a full SAML test and make sure the Shibboleth integration works as expected -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- Voila! Not bad for 15 minutes. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Shibboleth broken since |Koha::Config->read_from_fil |migration from XML::Simple |e incorrectly parses | |elements with 1 attribute | |named" content" (Shibboleth | |config) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #8 from David Cook <dcook@prosentient.com.au> --- Hopefully this patch doesn't break a case where there really is a config item that only has an attribute of "content" that should be treated as if it didn't have an attribute. CCing Julian Maurice in case I've missed something. But I think "content" was just used as a placeholder so changing it to "_content" should be perfectly fine. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #9 from Julian Maurice <julian.maurice@biblibre.com> --- Hi David, Have you checked that the 'content' key is not used anywhere else ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #10 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Julian Maurice from comment #9)
Hi David,
Have you checked that the 'content' key is not used anywhere else ?
It looks like it's used in C4::Creators::PDF::Font -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #9)
Hi David,
Have you checked that the 'content' key is not used anywhere else ?
(In reply to David Cook from comment #8)
Hopefully this patch doesn't break a case where there really is a config item that only has an attribute of "content" that should be treated as if it didn't have an attribute.
Not really. That's why I said hopefully ;). I did briefly look at fonts but didn't really look into it deeply. (In reply to Julian Maurice from comment #10)
It looks like it's used in C4::Creators::PDF::Font
That's interesting. It does look like this change would break that: 'ttf' => { 'font' => [ { 'type' => 'TR', '_content' => '/usr/share/fonts/truetype/dejavu/DejaVuSerif.ttf' }, -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Also possibly some Zebra stuff: 'listen' => { 'biblioserver' => { '_content' => 'unix:/var/run/koha/kohadev/bibliosocket' }, 'authorityserver' => { '_content' => 'unix:/var/run/koha/kohadev/authoritysocket' } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|dcook@prosentient.com.au |koha-bugs@lists.koha-commun | |ity.org --- Comment #13 from David Cook <dcook@prosentient.com.au> --- Looks like the algorithm in bug 28278 will need to be enhanced a bit further. Going to reset the Assignee to default. Hopefully Julian has an idea for this one. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- I can't find a proper documentation of the 'content' attribute in Shibboleth config, except in https://wiki.koha-community.org/w/index.php?title=Shibboleth_Configuration but it was added long after the patch made it into master. The original bug (bug 12026) doesn't mention it. Are we sure we are supposed to use <branchcode content="foo" is="branchcode" /> and not <branchcode is="branchcode">foo</branchcode> ? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152609|0 |1 is obsolete| | --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 152685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152685&action=edit Bug 31393: Koha::Config: handle the special case for 'content' attribute <key content="value"></key> was wrongly parsed as { key => 'value' } whereas it should be { key => { content => 'value' } } The 'content' attribute is used in shibboleth config Test plan: 1 Run `prove t/Koha/Config.t` -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #16 from Julian Maurice <julian.maurice@biblibre.com> --- Still unsure about comment 14, but either way this patch should fix the problem. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152685|0 |1 is obsolete| | --- Comment #17 from David Nind <david@davidnind.com> --- Created attachment 157768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157768&action=edit Bug 31393: Koha::Config: handle the special case for 'content' attribute <key content="value"></key> was wrongly parsed as { key => 'value' } whereas it should be { key => { content => 'value' } } The 'content' attribute is used in shibboleth config Test plan: 1 Run `prove t/Koha/Config.t` Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Assignee|koha-bugs@lists.koha-commun |julian.maurice@biblibre.com |ity.org | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Julian: please fill Assignee. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | Status|Signed Off |BLOCKED --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I have the feeling that we are trying to solve a Shibboleth issue here on a general level, risking to break other uses where 'content' is involved. The fact that _read_from_dom_node uses 'content' to pass results in a recursive way makes things a bit harder here. It makes using content sort of reserved keyword. Could be done better. But out of scope here. Would it be easier to use _content in Shibboleth for now? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #21 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Marcel de Rooy from comment #20)
I have the feeling that we are trying to solve a Shibboleth issue here on a general level, risking to break other uses where 'content' is involved.
The fact that _read_from_dom_node uses 'content' to pass results in a recursive way makes things a bit harder here. It makes using content sort of reserved keyword. Could be done better. But out of scope here.
With bug 28278 I tried to replicate the behavior of XML::Simple, where 'content' is kind of a reserved keyword. I'm not saying it's the best behavior, but it should explain why it's done this way. The patch on this bug makes Koha::Config behave more closely to what XML::Simple was doing.
Would it be easier to use _content in Shibboleth for now? Do you mean changing the attribute name in $KOHA_CONF from content to _content ?
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Julian Maurice from comment #21)
(In reply to Marcel de Rooy from comment #20)
Would it be easier to use _content in Shibboleth for now? Do you mean changing the attribute name in $KOHA_CONF from content to _content ?
Which has its drawbacks of course ;) But yes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Marcel de Rooy from comment #22)
Which has its drawbacks of course ;) But yes.
A big drawback in my opinion: asking everyone using Shibboleth to update their configuration file, while there is a patch available that works and don't require configuration changes. I don't understand. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Julian Maurice from comment #23)
(In reply to Marcel de Rooy from comment #22)
Which has its drawbacks of course ;) But yes.
A big drawback in my opinion: asking everyone using Shibboleth to update their configuration file, while there is a patch available that works and don't require configuration changes. I don't understand.
Will have another look. If it 'works', is the crux, yes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157768|0 |1 is obsolete| | --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 158060 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158060&action=edit Bug 31393: Koha::Config: handle the special case for 'content' attribute <key content="value"></key> was wrongly parsed as { key => 'value' } whereas it should be { key => { content => 'value' } } The 'content' attribute is used in shibboleth config Test plan: 1 Run `prove t/Koha/Config.t` Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Reading back, I saw that there was use of _content elsewhere instead of content. I do not really like the handling of 'content' in read_from_dom_node. This feels more like a hack. But I wont block it. We have some other exceptions in this code btw. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.06 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable --- Comment #28 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00,23.05.06 |23.11.00,23.05.06,22.11.12 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31393 --- Comment #29 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org