Skip to content

Commit

Permalink
Fix review suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@sahandilshan
sahandilshan committed Aug 19, 2023
1 parent d9159b1 commit bb960ed
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 68 deletions.
59 changes: 26 additions & 33 deletions ...n.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/SAMLSSOConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,39 +119,6 @@ public class SAMLSSOConstants {
private SAMLSSOConstants() {
}

/**
* Constants related to log management.
*/
public static class LogConstants {

public static final String SAML_INBOUND_SERVICE = "saml-inbound-service";

/**
* Define action IDs for diagnostic logs.
*/
public static class ActionIDs {

public static final String PROCESS_SAML_LOGOUT = "process-saml-logout";
public static final String SAML_REQUEST_VALIDATION = "saml-request-validation";
public static final String SAML_LOGOUT_PROCESSING = "saml-logout-processing";
public static final String PROCESS_SAML_REQUEST = "process-saml-request";
public static final String HAND_OVER_TO_FRAMEWORK = "hand-over-to-framework";
}

/**
* Define common and reusable Input keys for diagnostic logs.
*/
public static class InputKeys {

public static final String SAML_REQUEST = "saml request";
public static final String ISSUER = "issuer";
public static final String CONSUMER_URL = "consumer url";
public static final String AUTH_MODE = "auth mode";
public static final String ASSERTION_URL = "assertion url";
public static final String QUERY_STRING = "query string";
}
}


public enum QueryParameter {

Expand Down Expand Up @@ -241,6 +208,32 @@ public static class LogConstants {

public static final String CREATE_SAML_APPLICATION = "CREATE SAML APPLICATION";
public static final String DELETE_SAML_APPLICATION = "DELETE SAML APPLICATION";
public static final String SAML_INBOUND_SERVICE = "saml-inbound-service";

/**
* Define action IDs for diagnostic logs.
*/
public static class ActionIDs {

public static final String PROCESS_SAML_LOGOUT = "process-saml-logout";
public static final String SAML_REQUEST_VALIDATION = "saml-request-validation";
public static final String SAML_LOGOUT_PROCESSING = "saml-logout-processing";
public static final String PROCESS_SAML_REQUEST = "process-saml-request";
public static final String HAND_OVER_TO_FRAMEWORK = "hand-over-to-framework";
}

/**
* Define common and reusable Input keys for diagnostic logs.
*/
public static class InputKeys {

public static final String SAML_REQUEST = "saml request";
public static final String ISSUER = "issuer";
public static final String CONSUMER_URL = "consumer url";
public static final String AUTH_MODE = "auth mode";
public static final String ASSERTION_URL = "assertion url";
public static final String QUERY_STRING = "query string";
}
}

public static class SingleLogoutCodes {
Expand Down
6 changes: 3 additions & 3 deletions ...in/java/org/wso2/carbon/identity/sso/saml/processors/IdPInitSSOAuthnRequestProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public SAMLSSORespDTO process(SAMLSSOAuthnReqDTO authnReqDTO, String sessionId,
if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, SAML_REQUEST_VALIDATION);
diagnosticLogBuilder.resultMessage("Validating SP initiated SAML Authentication Request.")
diagnosticLogBuilder.resultMessage("Validating IDP initiated SAML Authentication Request.")
.inputParam(SAML_REQUEST, authnReqDTO.getRequestMessageString())
.inputParam("auth mode", authMode)
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
Expand Down Expand Up @@ -219,8 +219,8 @@ public SAMLSSORespDTO process(SAMLSSOAuthnReqDTO authnReqDTO, String sessionId,
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, SAML_REQUEST_VALIDATION);
diagnosticLogBuilder.resultMessage("SAML Request validation successful.")
.inputParam("consumer url", samlssoRespDTO.getAssertionConsumerURL())
.inputParam(LogConstants.InputKeys.USER_ID, samlssoRespDTO.getSubject().getUserId())
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.CONSUMER_URL,
samlssoRespDTO.getAssertionConsumerURL())
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER, authnReqDTO.getIssuer())
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION);
Expand Down
33 changes: 18 additions & 15 deletions .../main/java/org/wso2/carbon/identity/sso/saml/processors/SPInitLogoutRequestProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.commons.lang.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.opensaml.saml.saml2.core.Issuer;
import org.opensaml.saml.saml2.core.LogoutRequest;
import org.opensaml.saml.saml2.core.LogoutResponse;
import org.wso2.carbon.context.PrivilegedCarbonContext;
Expand Down Expand Up @@ -49,6 +50,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;

import static org.wso2.carbon.identity.sso.saml.SAMLSSOConstants.LogConstants.ActionIDs.SAML_LOGOUT_PROCESSING;
Expand Down Expand Up @@ -102,9 +104,10 @@ public SAMLSSOReqValidationResponseDTO process(LogoutRequest logoutRequest, Stri
SAML_INBOUND_SERVICE, SAML_LOGOUT_PROCESSING);
diagnosticLogBuilder.resultMessage("Processing SP initiated logout request.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER, logoutRequest.getIssuer().getValue())
.inputParam("reason", logoutRequest.getReason());
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS);
Optional.ofNullable(logoutRequest).map(LogoutRequest::getIssuer).map(Issuer::getValue)
.ifPresent(issuerValue -> diagnosticLogBuilder.inputParam(
SAMLSSOConstants.LogConstants.InputKeys.ISSUER, issuerValue));
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
try {
Expand All @@ -126,21 +129,20 @@ public SAMLSSOReqValidationResponseDTO process(LogoutRequest logoutRequest, Stri
for (Function<LogoutRequest, ValidationResult<SAMLSSOReqValidationResponseDTO>> validator :
logoutRequestValidators) {
ValidationResult<SAMLSSOReqValidationResponseDTO> validationResult = validator.apply(logoutRequest);
if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, SAML_LOGOUT_PROCESSING);
diagnosticLogBuilder.resultMessage("Validating logout request.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.INTERNAL_SYSTEM)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER,
logoutRequest.getIssuer().getValue())
.inputParam("validator name", validator.getClass().getName())
.inputParam("validation result", validationResult.getValidationStatus());
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
if (!validationResult.getValidationStatus()) {
return validationResult.getValue();
}
}
if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, SAML_LOGOUT_PROCESSING);
diagnosticLogBuilder.resultMessage("Logout request validation successful.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.INTERNAL_SYSTEM);
Optional.ofNullable(logoutRequest).flatMap(request -> Optional.ofNullable(request.getIssuer()))
.ifPresent(issuer -> diagnosticLogBuilder.inputParam(
SAMLSSOConstants.LogConstants.InputKeys.ISSUER, issuer.getValue()));
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}

// Validate whether we have principle session.
ValidationResult<SAMLSSOReqValidationResponseDTO> validationResult =
Expand Down Expand Up @@ -222,7 +224,8 @@ public SAMLSSOReqValidationResponseDTO process(LogoutRequest logoutRequest, Stri
diagnosticLogBuilder.resultMessage("Successfully processed SP initiated logout request.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER, reqValidationResponseDTO.getIssuer())
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER,
reqValidationResponseDTO.getIssuer())
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.CONSUMER_URL,
reqValidationResponseDTO.getAssertionConsumerURL());
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
Expand Down
51 changes: 35 additions & 16 deletions ....saml/src/main/java/org/wso2/carbon/identity/sso/saml/servlet/SAMLSSOProviderServlet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,13 +652,17 @@ private void handleIdPInitSSO(HttpServletRequest req, HttpServletResponse resp,
boolean isPost, boolean isLogout) throws UserStoreException, IdentityException,
IOException, ServletException {

DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = null;
if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, PROCESS_SAML_REQUEST);
diagnosticLogBuilder.resultMessage("Handling IdP Initiated SSO request.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(SAML_INBOUND_SERVICE, PROCESS_SAML_REQUEST);
diagnosticLogBuilder.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.QUERY_STRING, queryString);
if (isLogout) {
diagnosticLogBuilder.resultMessage("Handling IdP Initiated SLO request.");
} else {
diagnosticLogBuilder.resultMessage("Handling IdP Initiated SSO request.");
}
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
String rpSessionId = req.getParameter(MultitenantConstants.SSO_AUTH_SESSION_ID);
Expand Down Expand Up @@ -781,14 +785,13 @@ private void handleSPInitSSO(HttpServletRequest req, HttpServletResponse resp,
String samlRequest, String sessionId, boolean isPost)
throws UserStoreException, IdentityException, IOException, ServletException {

DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = null;
if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, PROCESS_SAML_REQUEST);
diagnosticLogBuilder.resultMessage("Handling SP Initiated SSO request.")
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
diagnosticLogBuilder.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION)
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.QUERY_STRING, queryString);
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
String rpSessionId = req.getParameter(MultitenantConstants.SSO_AUTH_SESSION_ID);
SAMLSSOService samlSSOService = new SAMLSSOService();
Expand All @@ -799,6 +802,10 @@ private void handleSPInitSSO(HttpServletRequest req, HttpServletResponse resp,
setSPAttributeToRequest(req, signInRespDTO.getIssuer(), SAMLSSOUtil.getTenantDomainFromThreadLocal());

if (!signInRespDTO.isLogOutReq()) { // an <AuthnRequest> received
if (LoggerUtils.isDiagnosticLogsEnabled() && diagnosticLogBuilder != null) {
diagnosticLogBuilder.resultMessage("Handling SP Initiated SSO request.");
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
if (signInRespDTO.isValid()) {
sendToFrameworkForAuthentication(req, resp, signInRespDTO, relayState, isPost);
} else {
Expand All @@ -813,6 +820,10 @@ private void handleSPInitSSO(HttpServletRequest req, HttpServletResponse resp,
signInRespDTO.getAssertionConsumerURL(), req, resp);
}
} else { // a <LogoutRequest> received
if (LoggerUtils.isDiagnosticLogsEnabled() && diagnosticLogBuilder != null) {
diagnosticLogBuilder.resultMessage("Handling SP Initiated SLO request.");
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
if (signInRespDTO.isValid()) {
sendToFrameworkForLogout(req, resp, signInRespDTO, relayState, sessionId, false, isPost);
} else {
Expand Down Expand Up @@ -1150,10 +1161,10 @@ private void handleAuthenticationReponseFromFramework(HttpServletRequest req, Ht
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.logDetailLevel(DiagnosticLog.LogDetailLevel.INTERNAL_SYSTEM)
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.ISSUER, sessionDTO.getIssuer())
.inputParam(LogConstants.InputKeys.USER, LoggerUtils.isLogMaskingEnable ?
LoggerUtils.getMaskedContent(sessionDTO.getSubject()) : sessionDTO.getSubject())
.inputParam(SAMLSSOConstants.LogConstants.InputKeys.CONSUMER_URL,
sessionDTO.getAssertionConsumerURL());
sessionDTO.getAssertionConsumerURL())
.inputParam(LogConstants.InputKeys.USER, LoggerUtils.isLogMaskingEnable ?
LoggerUtils.getMaskedContent(sessionDTO.getSubject()) : sessionDTO.getSubject());
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
String sessionDataKey = getSessionDataKey(req);
Expand Down Expand Up @@ -1721,11 +1732,6 @@ private QueryParamDTO[] getQueryParams(HttpServletRequest request) {
private void sendRequestToFramework(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {

CommonAuthenticationHandler commonAuthenticationHandler = new CommonAuthenticationHandler();

CommonAuthResponseWrapper responseWrapper = new CommonAuthResponseWrapper(response);
commonAuthenticationHandler.doGet(request, responseWrapper);

if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, HAND_OVER_TO_FRAMEWORK);
Expand All @@ -1734,6 +1740,11 @@ private void sendRequestToFramework(HttpServletRequest request, HttpServletRespo
.logDetailLevel(DiagnosticLog.LogDetailLevel.INTERNAL_SYSTEM);
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
CommonAuthenticationHandler commonAuthenticationHandler = new CommonAuthenticationHandler();

CommonAuthResponseWrapper responseWrapper = new CommonAuthResponseWrapper(response);
commonAuthenticationHandler.doGet(request, responseWrapper);

Object object = request.getAttribute(FrameworkConstants.RequestParams.FLOW_STATUS);
if (object != null) {
AuthenticatorFlowStatus status = (AuthenticatorFlowStatus) object;
Expand Down Expand Up @@ -1770,6 +1781,14 @@ private void sendRequestToFramework(HttpServletRequest request, HttpServletRespo
String type)
throws ServletException, IOException {

if (LoggerUtils.isDiagnosticLogsEnabled()) {
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
SAML_INBOUND_SERVICE, HAND_OVER_TO_FRAMEWORK);
diagnosticLogBuilder.resultMessage("Call authentication framework directly via API.")
.resultStatus(DiagnosticLog.ResultStatus.SUCCESS)
.logDetailLevel(DiagnosticLog.LogDetailLevel.INTERNAL_SYSTEM);
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
CommonAuthenticationHandler commonAuthenticationHandler = new CommonAuthenticationHandler();

CommonAuthRequestWrapper requestWrapper = new CommonAuthRequestWrapper(request);
Expand Down
5 changes: 4 additions & 1 deletion ...identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/SAMLSSOServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.testng.annotations.Test;
import org.wso2.carbon.identity.base.IdentityConstants;
import org.wso2.carbon.identity.base.IdentityException;
import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils;
import org.wso2.carbon.identity.core.model.SAMLSSOServiceProviderDO;
import org.wso2.carbon.identity.core.util.IdentityUtil;
import org.wso2.carbon.identity.sso.saml.dto.QueryParamDTO;
Expand Down Expand Up @@ -63,7 +64,7 @@
* Unit Tests for SAMLSSOService.
*/
@PrepareForTest({IdentityUtil.class, SAMLSSOUtil.class, SAMLSSOReqValidationResponseDTO.class,
SSOSessionPersistenceManager.class})
SSOSessionPersistenceManager.class, LoggerUtils.class})
public class SAMLSSOServiceTest extends PowerMockTestCase {

@DataProvider(name = "testAuthenticate")
Expand Down Expand Up @@ -330,6 +331,8 @@ public void testDoSingleLogout() throws Exception {
mockStatic(SSOSessionPersistenceManager.class);
when(SSOSessionPersistenceManager.getPersistenceManager()).thenReturn(ssoSessionPersistenceManager);
when(ssoSessionPersistenceManager.getSessionIndexFromTokenId(anyString(), anyString())).thenReturn("theSessionIndex");
mockStatic(LoggerUtils.class);
when(LoggerUtils.isDiagnosticLogsEnabled()).thenReturn(true);

SAMLSSOService samlssoService = new SAMLSSOService();
assertTrue(samlssoService.doSingleLogout("aSeesionID").isLogOutReq(), " Should return" +
Expand Down

0 comments on commit bb960ed

Please sign in to comment.