Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parameter name for SAML authn delegation logout call #6315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+30 −15
Open

Fix parameter name for SAML authn delegation logout call #6315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/cas-server-core-api-protocol/src/main/java/org/apereo/cas/CasProtocolConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ public interface CasProtocolConstants {
*/
String PARAMETER_PROXY_GRANTING_TICKET_URL = "pgtUrl";

/**
* Constant representing the logout parameter in the request.
*/
String PARAMETER_LOGOUT_REQUEST = "logoutRequest";

/* CAS Protocol Error Codes. **/

/**
Expand Down
18 changes: 11 additions & 7 deletions core/cas-server-core-logout-api/src/main/java/org/apereo/cas/logout/LogoutHttpMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.apereo.cas.logout;

import org.apereo.cas.CasProtocolConstants;
import org.apereo.cas.services.RegisteredService;
import org.apereo.cas.util.http.HttpMessage;

import lombok.Getter;
Expand All @@ -20,21 +22,23 @@
@Getter
public class LogoutHttpMessage extends HttpMessage {

/**
* The parameter name that contains the logout request.
*/
public static final String LOGOUT_REQUEST_PARAMETER = "logoutRequest";

@Serial
private static final long serialVersionUID = 399581521957873727L;

public LogoutHttpMessage(final URL url, final String message, final boolean asynchronous) {
private final String logoutParameter;

public LogoutHttpMessage(final RegisteredService registeredService, final URL url, final String message, final boolean asynchronous) {
super(url, message, asynchronous);
setContentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE);
if ("SAML2 Service Provider".equals(registeredService.getFriendlyName())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.logoutParameter = "SAMLRequest";
} else {
this.logoutParameter = CasProtocolConstants.PARAMETER_LOGOUT_REQUEST;
}
}

@Override
protected String formatOutputMessageInternal(final String message) {
return LOGOUT_REQUEST_PARAMETER + '=' + super.formatOutputMessageInternal(message);
return logoutParameter + '=' + super.formatOutputMessageInternal(message);
}
}
2 changes: 1 addition & 1 deletion ...ut-api/src/main/java/org/apereo/cas/logout/slo/BaseSingleLogoutServiceMessageHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,6 @@ protected boolean sendMessageToEndpoint(final LogoutHttpMessage msg,
* @return the logout http message to send
*/
protected LogoutHttpMessage getLogoutHttpMessageToSend(final SingleLogoutRequestContext request, final SingleLogoutMessage logoutMessage) {
return new LogoutHttpMessage(request.getLogoutUrl(), logoutMessage.getPayload(), this.asynchronous);
return new LogoutHttpMessage(request.getRegisteredService(), request.getLogoutUrl(), logoutMessage.getPayload(), this.asynchronous);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will fix that.

}
}
7 changes: 5 additions & 2 deletions core/cas-server-core-logout/src/test/java/org/apereo/cas/logout/LogoutHttpMessageTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.apereo.cas.logout;

import org.apereo.cas.CasProtocolConstants;
import org.apereo.cas.services.CasRegisteredService;

import lombok.val;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
Expand All @@ -18,7 +21,7 @@
class LogoutHttpMessageTests {
@Test
void verifyOperation() throws Throwable {
val message = new LogoutHttpMessage(new URI("https://github.com").toURL(), "LogoutMessage", false);
assertTrue(message.getMessage().startsWith(LogoutHttpMessage.LOGOUT_REQUEST_PARAMETER));
val message = new LogoutHttpMessage(new CasRegisteredService(), new URI("https://github.com").toURL(), "LogoutMessage", false);
assertTrue(message.getMessage().startsWith(CasProtocolConstants.PARAMETER_LOGOUT_REQUEST));
}
}
2 changes: 1 addition & 1 deletion ...t-actions-core/src/main/java/org/apereo/cas/web/flow/logout/FrontChannelLogoutAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ protected Event doInternalExecute(final RequestContext context) {
.map(Unchecked.function(handler -> handler.createSingleLogoutMessage(url)))
.forEach(logoutMessage -> {
LOGGER.debug("Front-channel logout message to send to [{}] is [{}]", url.getLogoutUrl(), logoutMessage);
val msg = new LogoutHttpMessage(url.getLogoutUrl(), logoutMessage.getPayload(), true);
val msg = new LogoutHttpMessage(url.getRegisteredService(), url.getLogoutUrl(), logoutMessage.getPayload(), true);
logoutUrls.put(url, msg);
url.setStatus(LogoutRequestStatus.SUCCESS);
url.getService().setLoggedOutAlready(true);
Expand Down
7 changes: 5 additions & 2 deletions ...rc/main/java/org/apereo/cas/web/flow/actions/logout/DelegatedSaml2ClientLogoutAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package org.apereo.cas.web.flow.actions.logout;

import org.apereo.cas.CasProtocolConstants;
import org.apereo.cas.authentication.principal.ClientCredential;
import org.apereo.cas.logout.LogoutHttpMessage;
import org.apereo.cas.logout.slo.SingleLogoutRequestExecutor;
import org.apereo.cas.support.pac4j.authentication.DelegatedAuthenticationClientLogoutRequest;
import org.apereo.cas.support.saml.SamlProtocolConstants;
import org.apereo.cas.ticket.InvalidTicketException;
import org.apereo.cas.ticket.TicketGrantingTicket;
import org.apereo.cas.ticket.TransientSessionTicket;
Expand Down Expand Up @@ -91,6 +92,8 @@ protected Event doExecuteInternal(final RequestContext requestContext) throws Th
}

protected boolean isDirectLogoutRequest(final HttpServletRequest request) {
return HttpMethod.POST.matches(request.getMethod()) || request.getParameter(LogoutHttpMessage.LOGOUT_REQUEST_PARAMETER) != null;
return HttpMethod.POST.matches(request.getMethod())
|| request.getParameter(CasProtocolConstants.PARAMETER_LOGOUT_REQUEST) != null
|| request.getParameter(SamlProtocolConstants.PARAMETER_SAML_REQUEST) != null;
}
}
4 changes: 2 additions & 2 deletions ...4j-saml/src/test/java/org/apereo/cas/web/saml2/DelegatedSaml2ClientLogoutActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package org.apereo.cas.web.saml2;

import org.apereo.cas.CasProtocolConstants;
import org.apereo.cas.authentication.CoreAuthenticationTestUtils;
import org.apereo.cas.authentication.principal.ClientCredential;
import org.apereo.cas.logout.LogoutHttpMessage;
import org.apereo.cas.support.pac4j.authentication.DelegatedAuthenticationClientLogoutRequest;
import org.apereo.cas.test.CasTestExtension;
import org.apereo.cas.ticket.TicketFactory;
Expand Down Expand Up @@ -116,7 +116,7 @@ void verifyOperationLogoutRequestParameter() throws Exception {

val context = MockRequestContext.create(applicationContext);
context.setMethod(HttpMethod.GET);
context.setParameter(LogoutHttpMessage.LOGOUT_REQUEST_PARAMETER, "adirectlogoutrequesttotreat");
context.setParameter(CasProtocolConstants.PARAMETER_LOGOUT_REQUEST, "adirectlogoutrequesttotreat");
val webContext = new JEEContext(context.getHttpServletRequest(), context.getHttpServletResponse());
val manager = new ProfileManager(webContext, delegatedClientDistributedSessionStore);
val profile = new CommonProfile();
Expand Down