Open gabbydotCMS opened 6 years ago
gabbydotCMS
commented
6 years ago @thstave
This is part of what we are missing in the new version of the plugin:
I don't see that class anywhere in the new plugin. I'm checking how it's being used while generating the request to the IdP.
gabbydotCMS
commented
6 years ago If I'm not wrong ....
This is where the authentication request gets built:
That makes a call to getIPDSSODestination(idpConfig) here:
and that method is defined here:
which is different from its old definition:
That previous version of the plugin, made the following call to define the request binding method:
which ended up using the binding type defined here:
All that was substituted by
which only uses the property DOTCMS_SAML_BINDING_TYPE.
I hope this helps.
Gabby
gabbydotCMS
commented
6 years ago [20/06/18 14:47:14:817 EDT] INFO meta.DefaultMetaDescriptorServiceImpl: Parsing the Id Provider, with the entityId: urn:saml2:r3Jq54VOGp5mKC6:telus
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SSO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST(https://sso.connect.pingidentity.com/sso/idp/SSO.saml2?idpid=c57d9b5e-13d4-490b-ad68-95a19f65f9cd)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SSO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect(https://sso.connect.pingidentity.com/sso/idp/SSO.saml2?idpid=c57d9b5e-13d4-490b-ad68-95a19f65f9cd)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SLO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect(https://sso.connect.pingidentity.com/sso/SLO.saml2)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SLO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST(https://sso.connect.pingidentity.com/sso/SLO.saml2)
[20/06/18 14:47:15:084 EDT] DEBUG parameters.DotsamlPropertiesService: Found protocol.binding : urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact@thstave ... I changed the default value of the property dotcmsSamlProtocolBinding while you can work on adding back the enumeration that contained all the possible values:
and its usage in the latest code similar to what we had in
Please let me know if you have any doubts.
Gabby
thstave
commented
6 years ago @gabbydotCMS ... Let me start with the reader's digest version and then I will answer some of your specific concerns.
In my testing (and I just re-ran them), the system does default to using HTTP-Redirect. So I need to ask if this occurs with all IDPs? You mention a parameter change you made, can you please relay what parameter change you made?
Now I'll try and address the specific concerns you mention.
BindingType class missing. The BindingType class was relocated and is now in a different package. You can find it here: https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/d0b18650db2b26e86bc46abc2e7bc16681029a56/src/com/dotcms/plugin/saml/v3/key/BindingType.java#L8-L25
The property to override this parameter is "bindingtype" you can find it at: https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlPropertyName.java#L38-L46
You reference differences in how metadata is accessed. As part of the system migrating away from using the old 'Configuration' class to store data to now using 'IdpConfig', 'MetaDataHelper' is now used to retrieve the default data. It will check both the default data as well as the specific IdpConfig. So seeing the code change in getIPDSSODestination from:
final String redirectIdentityProviderDestinationSSOURL = configuration.getIdentityProviderDestinationSSOURL(configuration);To using MetaDataHelper is a valid change.
final String redirectIdentityProviderDestinationSSOURL = MetaDataHelper
.getIdentityProviderDestinationSSOURL(idpConfig);Simarly, 'getIdentityProviderDestinationSSOURL' method you mentioned has also been moved. It is also part of 'MetaDataHelper'. The code you mention:
final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, BindingType.REDIRECT.getBinding());is replaced by
String bindingType = DotsamlPropertiesService.getOptionString(idpConfig,
DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE);Which checks to determin if there is an override parameter and if not uses the default. The default value is defined at: https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlProperties.java#L16
I hope this answers your concerns. Please let me know what other information you need.
gabbydotCMS
commented
6 years ago @thstave,
This is the commit I made and the reason why now HTTP-Redirect is the default method now: https://github.com/dotCMS/plugin-com.dotcms.dotsaml/commit/a42ef10eb104988fda3b693153e421b898fc3a77 .
If you undo that change, you'll go back to HTTP-Artifact.
In the previous version of the code, this line made sure that the default value set on DotsamlProperties.java for the binding type got changed to BindingType.REDIRECT:
So even when the enumeration still exists, the value BindingType.REDIRECT it's not been used anymore in the method getIdentityProviderDestinationSSOURL:
Defaulting to urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact gave us troubles with at least two of the IdPs we've been testing with: Shibboleth and ADFS.
Gabby
thstave
commented
6 years ago @gabbydotCMS ... Ahhh. I think I see what you're getting at.
In the previous version
final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE,
BindingType.REDIRECT.getBinding()); BindingType.REDIRECT.getBinding() was being used as a default value. If DotSamlConstants.DOTCMS_SAML_BINDING_TYPE is not found in the override parameters use BindingType.REDIRECT.getBinding().
In this version
String bindingType = DotsamlPropertiesService.getOptionString(idpConfig,
DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE);default values are already defined and do not need to be passed as part of the call. As you already discovered they are defined in DotsamlProperties class. The reason for this was to allow Site-specific properties to be read from disk without adding another layer of conditional statements throughout the code. Site-based properties are read from disk and simply replace the hardcoded defaults. A one-time event.
So the statement above is equivalent to the psuedo old code.
final String bindingType = configuration.getStringProperty(DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE,
{Default Value defined in DotsamlProperties});To ensure 'BindingType.REDIRECT.getBinding()' is the default value instead of using
private String dotcmsSamlProtocolBinding = SAML2_REDIRECT_BINDING_URI;in DotsamlProperties - we would need to use.
private String dotcmsSamlProtocolBinding = BindingType.REDIRECT.getBinding();This would make the code equivalent. As a sample see : https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlProperties.java#L16
gabbydotCMS
commented
6 years ago @thstave .. not sure what the next step is on this one.
dotcmsSamlProtocolBinding is already set to BindingType.REDIRECT.getBinding() in the latest code that we are using to test:
So if we are already using BindingType.REDIRECT as the default value for that property, I assume I can remove the temporary patch I committed (https://github.com/dotCMS/plugin-com.dotcms.dotsaml/commit/a42ef10eb104988fda3b693153e421b898fc3a77), and we'll be using HTTP-Redirect instead of HTTP-Artifact?
thstave
commented
6 years ago @gabbydotCMS ... Your patch was the right way to go. However, I would change your patch from;
private String dotcmsSamlProtocolBinding = SAML2_REDIRECT_BINDING_URI;To
private String dotcmsSamlProtocolBinding = BindingType.REDIRECT.getBinding();for consistency. Then you should be set to go.
The previous version of the plugin used the HTTP-Redirect request binding, but we seem to be back to using HTTP-Artifact instead.
Unlike the response binding (protocol.binding), we don't have a property to override this binding, so we need to make sure is the HTTP-Redirect instead of request. ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact"