From bb960ed20aaec5c0818b8c3fd49a177981bb5ed6 Mon Sep 17 00:00:00 2001 From: sahandilshan Date: Sat, 19 Aug 2023 21:57:14 +0530 Subject: [PATCH] Fix review suggestions --- .../identity/sso/saml/SAMLSSOConstants.java | 59 ++++++++----------- .../IdPInitSSOAuthnRequestProcessor.java | 6 +- .../SPInitLogoutRequestProcessor.java | 33 ++++++----- .../saml/servlet/SAMLSSOProviderServlet.java | 51 +++++++++++----- .../identity/sso/saml/SAMLSSOServiceTest.java | 5 +- 5 files changed, 86 insertions(+), 68 deletions(-) diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/SAMLSSOConstants.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/SAMLSSOConstants.java index 8a4795625..2abd36acb 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/SAMLSSOConstants.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/SAMLSSOConstants.java @@ -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 { @@ -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 { diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/IdPInitSSOAuthnRequestProcessor.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/IdPInitSSOAuthnRequestProcessor.java index 100e99b88..c3310bff7 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/IdPInitSSOAuthnRequestProcessor.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/IdPInitSSOAuthnRequestProcessor.java @@ -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) @@ -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); diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/SPInitLogoutRequestProcessor.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/SPInitLogoutRequestProcessor.java index 858aff6fd..8aad9c9d7 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/SPInitLogoutRequestProcessor.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/processors/SPInitLogoutRequestProcessor.java @@ -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; @@ -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; @@ -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 { @@ -126,21 +129,20 @@ public SAMLSSOReqValidationResponseDTO process(LogoutRequest logoutRequest, Stri for (Function> validator : logoutRequestValidators) { ValidationResult 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 validationResult = @@ -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); diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/servlet/SAMLSSOProviderServlet.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/servlet/SAMLSSOProviderServlet.java index 48f6740ef..408d9081d 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/servlet/SAMLSSOProviderServlet.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/servlet/SAMLSSOProviderServlet.java @@ -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); @@ -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(); @@ -799,6 +802,10 @@ private void handleSPInitSSO(HttpServletRequest req, HttpServletResponse resp, setSPAttributeToRequest(req, signInRespDTO.getIssuer(), SAMLSSOUtil.getTenantDomainFromThreadLocal()); if (!signInRespDTO.isLogOutReq()) { // an 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 { @@ -813,6 +820,10 @@ private void handleSPInitSSO(HttpServletRequest req, HttpServletResponse resp, signInRespDTO.getAssertionConsumerURL(), req, resp); } } else { // a 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 { @@ -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); @@ -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); @@ -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; @@ -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); diff --git a/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/SAMLSSOServiceTest.java b/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/SAMLSSOServiceTest.java index eea19d3c3..29a385df4 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/SAMLSSOServiceTest.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/SAMLSSOServiceTest.java @@ -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; @@ -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") @@ -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" +