Fix parameter name for SAML authn delegation logout call

[leleuj]

To follow up my previous PR on this topic: #6206, I realized that the logout parameter used to send the SAML request in the front-channel way was incorrect: it is "logoutRequest" instead of "SAMLRequest" (pac4j being versatile enough to accept both parameters).

This PR fixes this issue and takes it into account to consider it's a direct logout call.

[mmoayyed]

This sort of check cannot work. You either need to check and compare by type, or you need to pick a different parameter name elsewhere if the type is not statically available here.

[leleuj]

Indeed, the problem is double here: the type (SamlRegisteredService) is not available, which is quite logical in this core logout API module. Though, at the same time, the "friendly constant" is unavailable too as it's also in the SamlRegisteredService.

It should not be:

public class SamlRegisteredService extends BaseWebBasedRegisteredService {

    /**
     * Service definition friendly name.
     */
    public static final String FRIENDLY_NAME = "SAML2 Service Provider";

I think the constant should be somewhere else, in some core module, shouldn't it?

BTW, thinking about it a little more and considering your other comment, maybe I should just add the logoutParameter in the HttpLogoutMessage and keep the logic outside as this class just conveys information.

[mmoayyed]

I am opposed to doing this sort of thing with a text comparison; it's much safer to do this based on the type of service, and hopefully in a dedicated component in the saml module that does have access to the right APIs. I am guessing in some extension of BaseSingleLogoutServiceMessageHandler, since that is where you mainly need to check for the type and create the right message.

[leleuj]

I fully agree, but currently, the FrontChannelLogoutAction has no visibility on the SamlRegisteredService. So for the cas-server-support-actions-core module, I should add an "implementation" dependency of cas-server-support-saml-idp-core?

[mmoayyed]

No. That should not be necessary.

Unless I am missing something I am seeing that you are mainly doing this:

return new LogoutHttpMessage(request.getRegisteredService(), request.getLogoutUrl(), logoutMessage.getPayload(), this.asynchronous);

...in BaseSingleLogoutServiceMessageHandler.

Are you able to override that line in SamlIdPSingleLogoutServiceMessageHandler and create a dedicated message there? SamlRegisteredService can only be available if the SAML2 IDP module is on, and I think that handler should fit the bill. Does that make sense?

[leleuj]

Yes, but there is also a new LogoutHttpMessage call in the FrontChannelLogoutAction component: https://github.com/apereo/cas/blob/master/support/cas-server-support-actions-core/src/main/java/org/apereo/cas/web/flow/logout/FrontChannelLogoutAction.java#L68

So if I pass: request.getRegisteredService() to new HttpLogoutMessage, I need to be able to compute that it's a SmlRegisteredService in LogoutHttpMessage, which is not currently possible as the cas-server-core-logout-api has no visibility on the SAML stuffs.

Or if I directly pass the logoutParameter to new HttpLogoutMessage, I need to be able to compute that it's a SmlRegisteredService in FrontChannelLogoutAction to determine the logout parameter, which is not currently possible as the cas-server-support-actions-core has no visibility on the SAML stuffs.

In both options, I'm unable to compute if it's a SamlRegisteredService and to know what is the value of the logout parameter ("SAMLRequest" or "logoutRequest").

[mmoayyed]

Please revert this, and do the check instead in SamlIdPSingleLogoutServiceMessageHandler.

[leleuj]

Yes, I will fix that.