Bug 31393

Can someone using Shibboleth chime in here? 

It's marked enhancement, but that seems wrong if things are totally broken?

<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...

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'} || '';

(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"...

(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.

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

Voila! Not bad for 15 minutes.

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.

Hi David,

Have you checked that the 'content' key is not used anywhere else ?

(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

(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'
                                               },

Also possibly some Zebra stuff:

  'listen' => {
                        'biblioserver' => {
                                            '_content' => 'unix:/var/run/koha/kohadev/bibliosocket'
                                          },
                        'authorityserver' => {
                                               '_content' => 'unix:/var/run/koha/kohadev/authoritysocket'
                                             }

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.

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>

?

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`

Still unsure about comment 14, but either way this patch should fix the problem.

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>

Julian: please fill Assignee.

Looking here

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?

(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 ?

(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.

(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.

(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.

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>

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.

Pushed to master for 23.11.

Nice work everyone, thanks!

Pushed to 23.05.x for 23.05.06

Nice work everyone!

Pushed to oldstable for 22.11.x