Bug 31393 - Koha::Config->read_from_file incorrectly parses elements with 1 attribute named" content" (Shibboleth config)
Summary: Koha::Config->read_from_file incorrectly parses elements with 1 attribute nam...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 28278
Blocks:
  Show dependency treegraph
 
Reported: 2022-08-18 12:01 UTC by Didier Gautheron
Modified: 2023-11-13 15:29 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06,22.11.12


Attachments
Bug 31393: Use _content as a placeholder when parsing koha-conf.xml (2.34 KB, patch)
2023-06-23 03:01 UTC, David Cook
Details | Diff | Splinter Review
Bug 31393: Koha::Config: handle the special case for 'content' attribute (2.86 KB, patch)
2023-06-26 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 31393: Koha::Config: handle the special case for 'content' attribute (2.91 KB, patch)
2023-10-24 23:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 31393: Koha::Config: handle the special case for 'content' attribute (3.01 KB, patch)
2023-10-30 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Didier Gautheron 2022-08-18 12:01:16 UTC
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'} || '';
        }
    }
Comment 1 Katrin Fischer 2023-06-17 11:16:45 UTC
Can someone using Shibboleth chime in here? 

It's marked enhancement, but that seems wrong if things are totally broken?
Comment 2 David Cook 2023-06-19 02:17:33 UTC
<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...
Comment 3 Matthias Meusburger 2023-06-21 13:31:14 UTC
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'} || '';
Comment 4 David Cook 2023-06-23 02:42:10 UTC
(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"...
Comment 5 David Cook 2023-06-23 02:45:36 UTC
(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.
Comment 6 David Cook 2023-06-23 03:01:54 UTC
Created attachment 152609 [details] [review]
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
Comment 7 David Cook 2023-06-23 03:02:19 UTC
Voila! Not bad for 15 minutes.
Comment 8 David Cook 2023-06-23 03:06:13 UTC
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.
Comment 9 Julian Maurice 2023-06-23 06:19:56 UTC
Hi David,

Have you checked that the 'content' key is not used anywhere else ?
Comment 10 Julian Maurice 2023-06-23 06:23:02 UTC
(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
Comment 11 David Cook 2023-06-23 06:38:13 UTC
(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'
                                               },
Comment 12 David Cook 2023-06-23 06:39:05 UTC
Also possibly some Zebra stuff:

  'listen' => {
                        'biblioserver' => {
                                            '_content' => 'unix:/var/run/koha/kohadev/bibliosocket'
                                          },
                        'authorityserver' => {
                                               '_content' => 'unix:/var/run/koha/kohadev/authoritysocket'
                                             }
Comment 13 David Cook 2023-06-23 06:45:40 UTC
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.
Comment 14 Julian Maurice 2023-06-23 08:01:38 UTC
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>

?
Comment 15 Julian Maurice 2023-06-26 13:45:21 UTC
Created attachment 152685 [details] [review]
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`
Comment 16 Julian Maurice 2023-06-26 13:46:49 UTC
Still unsure about comment 14, but either way this patch should fix the problem.
Comment 17 David Nind 2023-10-24 23:50:30 UTC
Created attachment 157768 [details] [review]
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>
Comment 18 Marcel de Rooy 2023-10-27 07:29:27 UTC
Julian: please fill Assignee.
Comment 19 Marcel de Rooy 2023-10-27 07:31:05 UTC
Looking here
Comment 20 Marcel de Rooy 2023-10-27 09:15:48 UTC
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?
Comment 21 Julian Maurice 2023-10-27 09:59:49 UTC
(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 ?
Comment 22 Marcel de Rooy 2023-10-27 10:06:12 UTC
(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.
Comment 23 Julian Maurice 2023-10-27 11:46:57 UTC
(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.
Comment 24 Marcel de Rooy 2023-10-30 08:56:59 UTC
(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.
Comment 25 Marcel de Rooy 2023-10-30 10:48:59 UTC
Created attachment 158060 [details] [review]
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>
Comment 26 Marcel de Rooy 2023-10-30 10:52:12 UTC
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.
Comment 27 Tomás Cohen Arazi 2023-10-30 12:03:06 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 28 Fridolin Somers 2023-11-08 08:15:35 UTC
Pushed to 23.05.x for 23.05.06
Comment 29 Matt Blenkinsop 2023-11-13 15:29:09 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x