Explicit saml.metadata.providers does not replace defaults

[irstevenson]

Plugin version 3.3.1-SNAPSHOT

I've found that when you provide your own explicit saml.metadata.providers it is merged with the defaults for saml.metadata.providers. So as we know, the default config has one provider ping with a metadata file at security/idp-local.xml. Now if I have a config like:

      saml:
        metadata:
          defaultIdp: 'shib'
          providers:
            shib: 'security/metadata-dev-idp.xml'

I see some start-up logging which shows the default is still in play:

Configuring Spring Security SAML ...
Importing beans from classpath:security/springSecuritySamlBeans.xml...
Registering metadata key: ping and value: security/idp-local.xml
Registering metadata key: shib and value: security/metadata-dev-idp.xml
Sp File exists security/metadata-dev-sp.xml
...finished configuring Spring Security SAML

Looking at this I see a few things:

• I'd expect my settings to overwrite any defaults;
• I wouldn't really think there should be defaults for this - and there's a few others in this category;
• A bigger issue, the plugin should probably not include any of grails-app/conf/security/* when packaged - that stuff should only be for testing. (Except for security/springSecuritySamlBeans.xml.)

But to stay on track, a fix for this would simply be:

1. Remove the default section for providers in grails-app/conf/application.yml; and
2. In SpringSecuritySamlGrailsPlugin before the conf.saml.metadata.providers.each {} do a check to see if there are any providers, and if not report an error with config guidance and throw an exception; as well as
3. A solution to specify these values so as to ensure we can still run the test suite. (Not entirely sure if this is needed.)

[valentingoebel]

1. Yes, please remove that section.

2. A simple message to the log "No service provider metadata was configured in saml.metadata.providers" if the list is empty should be enough.

3. I can run the testsuite even with an empty plugin.yml. It's not neccessary but we could optionally migrate the unwanted settings to application.yml so that grails run-app will still work for manual testing.

[irstevenson]

Oh yes, looks like I got muddled there. These need to be removed from plugin.yml not application.yml. ;)

[irstevenson]

Note the main issue here is solved with #54

:+1:

Can close once merged.

[valentingoebel]

This has been merged.