From 325183a1431351f5e8054e15e7ec9ea4e7eed620 Mon Sep 17 00:00:00 2001 From: Bill Dunn Date: Wed, 1 May 2024 13:09:35 +0100 Subject: [PATCH 1/4] ATO-585: Add user ID (`subjectId`), when it is available, to the AUTH_LOGOUT_SUCCESS audit event --- .../oidc/lambda/LogoutHandler.java | 35 ++++++--- .../oidc/lambda/LogoutHandlerTest.java | 71 ++++++++++++++----- .../shared/services/LogoutService.java | 30 +++++--- .../shared/services/LogoutServiceTest.java | 45 ++++++++---- 4 files changed, 132 insertions(+), 49 deletions(-) diff --git a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java index 084b8e6629..b96f71dba8 100644 --- a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java +++ b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java @@ -97,6 +97,13 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE () -> sessionService.getSessionFromSessionCookie(input.getHeaders())); attachSessionToLogsIfExists(sessionFromSessionCookie, input.getHeaders()); + Optional subjectId = Optional.empty(); + if (sessionFromSessionCookie.isPresent()) { + subjectId = + Optional.ofNullable( + sessionFromSessionCookie.get().getInternalCommonSubjectIdentifier()); + } + Map queryStringParameters = input.getQueryStringParameters(); if (queryStringParameters == null || queryStringParameters.isEmpty()) { LOG.info("Returning default logout as no input parameters"); @@ -104,7 +111,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE Optional.empty(), input, Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } Optional state = Optional.ofNullable(queryStringParameters.get("state")); @@ -115,7 +123,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE state, input, Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } LOG.info("ID token hint is present"); @@ -131,7 +140,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE OAuth2Error.INVALID_REQUEST_CODE, "unable to validate id_token_hint"), input, Optional.empty(), - Optional.empty()); + Optional.empty(), + subjectId); } Optional audience; @@ -145,7 +155,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE new ErrorObject(OAuth2Error.INVALID_REQUEST_CODE, "invalid id_token_hint"), input, Optional.empty(), - Optional.empty()); + Optional.empty(), + subjectId); } if (audience.isEmpty()) { @@ -153,7 +164,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE Optional.empty(), input, Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } final String clientID = audience.get(); @@ -167,7 +179,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE new ErrorObject(OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), input, Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } Optional postLogoutRedirectUri = @@ -179,7 +192,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE state, input, Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } if (!postLogoutRedirectUriInClientReg(postLogoutRedirectUri, clientRegistry)) { @@ -190,7 +204,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE "client registry does not contain post_logout_redirect_uri"), input, Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie)); + getSessionAndDestroyIfExists(sessionFromSessionCookie), + subjectId); } if (sessionFromSessionCookie.isPresent()) { @@ -214,6 +229,7 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE Optional.empty(), input, Optional.of(clientID), + Optional.empty(), Optional.empty())); } } @@ -277,6 +293,7 @@ private APIGatewayProxyResponseEvent logout( Optional.empty(), input, Optional.of(clientID), - Optional.of(session.getSessionId())); + Optional.of(session.getSessionId()), + Optional.ofNullable(session.getInternalCommonSubjectIdentifier())); } } diff --git a/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java b/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java index f613861c62..830f998dbe 100644 --- a/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java +++ b/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java @@ -107,9 +107,9 @@ void setUp() throws JOSEException, ParseException { logoutService); when(configurationService.getDefaultLogoutURI()).thenReturn(DEFAULT_LOGOUT_URI); when(configurationService.getInternalSectorUri()).thenReturn(INTERNAL_SECTOR_URI); - when(logoutService.generateLogoutResponse(any(), any(), any(), any(), any(), any())) + when(logoutService.generateLogoutResponse(any(), any(), any(), any(), any(), any(), any())) .thenReturn(new APIGatewayProxyResponseEvent()); - when(logoutService.generateErrorLogoutResponse(any(), any(), any(), any(), any())) + when(logoutService.generateErrorLogoutResponse(any(), any(), any(), any(), any(), any())) .thenReturn(new APIGatewayProxyResponseEvent()); when(context.getAwsRequestId()).thenReturn("aws-session-id"); @@ -128,7 +128,10 @@ class Session { @BeforeEach void sessionExistsSetup() { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); idTokenHint = signedIDToken.serialize(); when(dynamoClientService.getClient("client-id")) .thenReturn(Optional.of(createClientRegistry())); @@ -161,7 +164,8 @@ void shouldDeleteSessionAndRedirectToClientLogoutUriForValidLogoutRequest() { Optional.empty(), event, audience, - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); verify(cloudwatchMetricsService).incrementLogout(Optional.of("client-id")); } @@ -190,7 +194,8 @@ void shouldNotThrowWhenTryingToDeleteClientSessionWhichHasExpired() { Optional.empty(), event, audience, - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); verify(cloudwatchMetricsService).incrementLogout(Optional.of("client-id")); } @@ -205,7 +210,11 @@ void shouldNotThrowWhenTryingToDeleteClientSessionWhichHasExpired() { verify(logoutService, times(1)).destroySessions(session); verify(logoutService) .generateDefaultLogoutResponse( - Optional.empty(), event, Optional.empty(), Optional.of(SESSION_ID)); + Optional.empty(), + event, + Optional.empty(), + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); } @Test @@ -230,6 +239,7 @@ void shouldRedirectToClientLogoutUriWhenNoCookieExists() { Optional.empty(), event, audience, + Optional.empty(), Optional.empty()); verifyNoInteractions(cloudwatchMetricsService); } @@ -239,7 +249,10 @@ void shouldRedirectToClientLogoutUriWhenNoCookieExists() { class IdToken { @Test void shouldRedirectToDefaultLogoutUriForValidLogoutRequestWithNoTokenHint() { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); APIGatewayProxyRequestEvent event = generateRequestEvent( Map.of( @@ -256,7 +269,8 @@ void shouldRedirectToDefaultLogoutUriForValidLogoutRequestWithNoTokenHint() { Optional.of(STATE.toString()), event, Optional.empty(), - Optional.of(session.getSessionId())); + Optional.of(session.getSessionId()), + Optional.of(SUBJECT.getValue())); } @Test @@ -276,13 +290,17 @@ void shouldRedirectToDefaultLogoutUriForValidLogoutRequestWithNoTokenHint() { Optional.of(STATE.toString()), event, Optional.empty(), + Optional.empty(), Optional.empty()); } @Test void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenSignatureIdTokenIsInvalid() throws JOSEException { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); ECKey ecSigningKey = new ECKeyGenerator(Curve.P_256).algorithm(JWSAlgorithm.ES256).generate(); SignedJWT signedJWT = @@ -310,7 +328,8 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenSignatureIdTokenIsInval "unable to validate id_token_hint"), event, Optional.empty(), - Optional.empty()); + Optional.empty(), + Optional.of(SUBJECT.getValue())); verifyNoInteractions(cloudwatchMetricsService); } } @@ -321,7 +340,10 @@ class ClientIdAndPostLogoutRedirectUri { @Test void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenClientIsNotFoundInClientRegistry() throws JOSEException, ParseException { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); ECKey ecSigningKey = new ECKeyGenerator(Curve.P_256).algorithm(JWSAlgorithm.ES256).generate(); SignedJWT signedJWT = @@ -348,7 +370,8 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenClientIsNotFoundInClien OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), event, signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.of(session.getSessionId())); + Optional.of(session.getSessionId()), + Optional.of(SUBJECT.getValue())); verifyNoInteractions(cloudwatchMetricsService); } @@ -380,13 +403,17 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenClientIsNotFoundInClien OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), event, signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), + Optional.empty(), Optional.empty()); verifyNoInteractions(cloudwatchMetricsService); } @Test void shouldRedirectToDefaultUriWhenLogoutRedirectUriIsMissing() { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); var idTokenHint = signedIDToken.serialize(); session.getClientSessions().add(CLIENT_SESSION_ID); @@ -407,7 +434,8 @@ void shouldRedirectToDefaultUriWhenLogoutRedirectUriIsMissing() { Optional.of(STATE.toString()), event, audience, - Optional.of(session.getSessionId())); + Optional.of(session.getSessionId()), + Optional.of(SUBJECT.getValue())); } @Test @@ -427,14 +455,21 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM verify(logoutService, times(0)).destroySessions(session); verify(logoutService) .generateDefaultLogoutResponse( - Optional.of(STATE.toString()), event, audience, Optional.empty()); + Optional.of(STATE.toString()), + event, + audience, + Optional.empty(), + Optional.empty()); } @Test void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenLogoutUriInRequestDoesNotMatchClientRegistry() throws JOSEException, ParseException { - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); ECKey ecSigningKey = new ECKeyGenerator(Curve.P_256).algorithm(JWSAlgorithm.ES256).generate(); SignedJWT signedJWT = @@ -463,7 +498,8 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM "client registry does not contain post_logout_redirect_uri"), event, signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.of(session.getSessionId())); + Optional.of(session.getSessionId()), + Optional.of(SUBJECT.getValue())); verifyNoInteractions(cloudwatchMetricsService); } @@ -497,6 +533,7 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM "client registry does not contain post_logout_redirect_uri"), event, signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), + Optional.empty(), Optional.empty()); verifyNoInteractions(cloudwatchMetricsService); } diff --git a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java index 1b6122a530..d4c1e0bca5 100644 --- a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java +++ b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java @@ -67,7 +67,11 @@ public APIGatewayProxyResponseEvent handleAccountInterventionLogout( AccountIntervention intervention) { destroySessions(session); return generateAccountInterventionLogoutResponse( - input, clientId, session.getSessionId(), intervention); + input, + clientId, + session.getSessionId(), + intervention, + session.getInternalCommonSubjectIdentifier()); } public void destroySessions(Session session) { @@ -97,7 +101,8 @@ public APIGatewayProxyResponseEvent generateErrorLogoutResponse( ErrorObject errorObject, APIGatewayProxyRequestEvent input, Optional clientId, - Optional sessionId) { + Optional sessionId, + Optional subjectId) { LOG.info( "Generating Logout Error Response with code: {} and description: {}", errorObject.getCode(), @@ -108,14 +113,16 @@ public APIGatewayProxyResponseEvent generateErrorLogoutResponse( Optional.of(errorObject), input, clientId, - sessionId); + sessionId, + subjectId); } public APIGatewayProxyResponseEvent generateDefaultLogoutResponse( Optional state, APIGatewayProxyRequestEvent input, Optional clientId, - Optional sessionId) { + Optional sessionId, + Optional subjectId) { LOG.info("Generating default Logout Response"); sessionId.ifPresent(t -> cloudwatchMetricsService.incrementLogout(clientId)); return generateLogoutResponse( @@ -124,7 +131,8 @@ public APIGatewayProxyResponseEvent generateDefaultLogoutResponse( Optional.empty(), input, clientId, - sessionId); + sessionId, + subjectId); } public APIGatewayProxyResponseEvent generateLogoutResponse( @@ -133,7 +141,8 @@ public APIGatewayProxyResponseEvent generateLogoutResponse( Optional errorObject, APIGatewayProxyRequestEvent input, Optional clientId, - Optional sessionId) { + Optional sessionId, + Optional subjectId) { LOG.info("Generating Logout Response using URI: {}", logoutUri); URIBuilder uriBuilder = new URIBuilder(logoutUri); state.ifPresent(s -> uriBuilder.addParameter("state", s)); @@ -153,7 +162,8 @@ public APIGatewayProxyResponseEvent generateLogoutResponse( .withIpAddress(extractIpAddress(input)) .withPersistentSessionId( extractPersistentIdFromCookieHeader(input.getHeaders())) - .withSessionId(sessionId.orElse(null)); + .withSessionId(sessionId.orElse(null)) + .withUserId(subjectId.orElse(null)); auditService.submitAuditEvent(LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), user); @@ -165,7 +175,8 @@ private APIGatewayProxyResponseEvent generateAccountInterventionLogoutResponse( APIGatewayProxyRequestEvent input, String clientId, String sessionId, - AccountIntervention intervention) { + AccountIntervention intervention, + String subjectId) { URI redirectURI; if (intervention.getBlocked()) { redirectURI = configurationService.getAccountStatusBlockedURI(); @@ -184,6 +195,7 @@ private APIGatewayProxyResponseEvent generateAccountInterventionLogoutResponse( Optional.empty(), input, Optional.of(clientId), - Optional.of(sessionId)); + Optional.of(sessionId), + Optional.ofNullable(subjectId)); } } diff --git a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java index fe6a3b7e5a..ecbe23e7b8 100644 --- a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java +++ b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java @@ -102,11 +102,18 @@ public class LogoutServiceTest { private Optional audience; private Session session; private LogoutService logoutService; - private final TxmaAuditUser user = + private final TxmaAuditUser auditUser = TxmaAuditUser.user() .withIpAddress(IP_ADDRESS) .withSessionId(SESSION_ID) - .withPersistentSessionId(PERSISTENT_SESSION_ID); + .withPersistentSessionId(PERSISTENT_SESSION_ID) + .withUserId(SUBJECT.getValue()); + private final TxmaAuditUser auditUserWhenNoCookie = + TxmaAuditUser.user() + .withIpAddress(IP_ADDRESS) + .withSessionId(SESSION_ID) + .withPersistentSessionId(null) + .withUserId(null); @BeforeEach void setup() throws JOSEException, ParseException { @@ -141,7 +148,10 @@ void setup() throws JOSEException, ParseException { SignedJWT idToken = SignedJWT.parse(signedIDToken.serialize()); audience = idToken.getJWTClaimsSet().getAudience().stream().findFirst(); - session = generateSession().setEmailAddress(EMAIL); + session = + generateSession() + .setEmailAddress(EMAIL) + .setInternalCommonSubjectIdentifier(SUBJECT.getValue()); } @AfterEach @@ -159,9 +169,10 @@ void successfullyReturnsClientLogoutResponse() { Optional.empty(), event, Optional.of(audience.get()), - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); assertThat(response, hasStatus(302)); assertThat( @@ -176,9 +187,10 @@ void successfullyReturnsDefaultLogoutResponseWithoutStateWhenStateIsAbsent() { Optional.empty(), event, Optional.of(audience.get()), - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService).incrementLogout(Optional.of(CLIENT_ID)); assertThat(response, hasStatus(302)); @@ -194,9 +206,10 @@ void successfullyReturnsDefaultLogoutResponseWithStateWhenStateIsPresent() { Optional.of(STATE.getValue()), event, Optional.of(audience.get()), - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService).incrementLogout(Optional.of(CLIENT_ID)); assertThat(response, hasStatus(302)); @@ -213,9 +226,10 @@ void successfullyReturnsErrorLogoutResponse() throws URISyntaxException { new ErrorObject(OAuth2Error.INVALID_REQUEST_CODE, "invalid session"), event, Optional.empty(), - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.of(SUBJECT.getValue())); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, AuditService.UNKNOWN, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, AuditService.UNKNOWN, auditUser); verifyNoInteractions(cloudwatchMetricsService); assertThat(response, hasStatus(302)); @@ -240,7 +254,7 @@ void destroysSessionsAndReturnsAccountInterventionLogoutResponseWhenAccountIsBlo verify(clientSessionService).deleteStoredClientSession(session.getClientSessions().get(0)); verify(sessionService).deleteSessionFromRedis(session.getSessionId()); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService) .incrementLogout(Optional.of(CLIENT_ID), Optional.of(intervention)); @@ -261,7 +275,7 @@ void destroysSessionsAndReturnsAccountInterventionLogoutResponseWhenAccountIsSus verify(clientSessionService).deleteStoredClientSession(session.getClientSessions().get(0)); verify(sessionService).deleteSessionFromRedis(session.getSessionId()); - verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, user); + verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService) .incrementLogout(Optional.of(CLIENT_ID), Optional.of(intervention)); @@ -288,10 +302,13 @@ void shouldDeleteSessionFromRedisWhenNoCookieExists() { Optional.empty(), input, Optional.empty(), - Optional.of(SESSION_ID)); + Optional.of(SESSION_ID), + Optional.empty()); verify(sessionService, times(0)).deleteSessionFromRedis(SESSION_ID); verifyNoInteractions(cloudwatchMetricsService); + verify(auditService) + .submitAuditEvent(LOG_OUT_SUCCESS, AuditService.UNKNOWN, auditUserWhenNoCookie); } @Test From b16bf1a33abee4055f57b47c6c0bb8033005ce75 Mon Sep 17 00:00:00 2001 From: Bill Dunn Date: Thu, 2 May 2024 15:33:27 +0100 Subject: [PATCH 2/4] ATO-585: Generate `auditUser` in LogoutHandler rather than passing params to LogoutService and generating there --- .../oidc/lambda/LogoutHandler.java | 86 +++++++--------- .../oidc/lambda/LogoutHandlerTest.java | 99 ++++++++----------- .../shared/services/LogoutService.java | 75 +++++--------- .../shared/services/LogoutServiceTest.java | 28 ++---- 4 files changed, 112 insertions(+), 176 deletions(-) diff --git a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java index b96f71dba8..36da504cd7 100644 --- a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java +++ b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java @@ -10,6 +10,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.ThreadContext; +import uk.gov.di.orchestration.audit.TxmaAuditUser; import uk.gov.di.orchestration.shared.entity.ClientRegistry; import uk.gov.di.orchestration.shared.entity.Session; import uk.gov.di.orchestration.shared.helpers.CookieHelper; @@ -29,11 +30,13 @@ import static uk.gov.di.orchestration.shared.helpers.AuditHelper.attachTxmaAuditFieldFromHeaders; import static uk.gov.di.orchestration.shared.helpers.InstrumentationHelper.segmentedFunctionCall; +import static uk.gov.di.orchestration.shared.helpers.IpAddressHelper.extractIpAddress; import static uk.gov.di.orchestration.shared.helpers.LogLineHelper.LogFieldName.CLIENT_ID; import static uk.gov.di.orchestration.shared.helpers.LogLineHelper.LogFieldName.CLIENT_SESSION_ID; import static uk.gov.di.orchestration.shared.helpers.LogLineHelper.LogFieldName.GOVUK_SIGNIN_JOURNEY_ID; import static uk.gov.di.orchestration.shared.helpers.LogLineHelper.attachLogFieldToLogs; import static uk.gov.di.orchestration.shared.helpers.LogLineHelper.attachSessionIdToLogs; +import static uk.gov.di.orchestration.shared.helpers.PersistentIdHelper.extractPersistentIdFromCookieHeader; public class LogoutHandler implements RequestHandler { @@ -98,33 +101,36 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE attachSessionToLogsIfExists(sessionFromSessionCookie, input.getHeaders()); Optional subjectId = Optional.empty(); + Optional sessionId = Optional.empty(); if (sessionFromSessionCookie.isPresent()) { subjectId = Optional.ofNullable( sessionFromSessionCookie.get().getInternalCommonSubjectIdentifier()); + sessionId = Optional.ofNullable(sessionFromSessionCookie.get().getSessionId()); } + var auditUser = + TxmaAuditUser.user() + .withIpAddress(extractIpAddress(input)) + .withPersistentSessionId( + extractPersistentIdFromCookieHeader(input.getHeaders())) + .withSessionId(sessionId.orElse(null)) + .withUserId(subjectId.orElse(null)); + Map queryStringParameters = input.getQueryStringParameters(); if (queryStringParameters == null || queryStringParameters.isEmpty()) { LOG.info("Returning default logout as no input parameters"); + getSessionAndDestroyIfExists(sessionFromSessionCookie); return logoutService.generateDefaultLogoutResponse( - Optional.empty(), - input, - Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + Optional.empty(), auditUser, Optional.empty()); } Optional state = Optional.ofNullable(queryStringParameters.get("state")); Optional idTokenHint = Optional.ofNullable(queryStringParameters.get("id_token_hint")); if (idTokenHint.isEmpty()) { - return logoutService.generateDefaultLogoutResponse( - state, - input, - Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + getSessionAndDestroyIfExists(sessionFromSessionCookie); + return logoutService.generateDefaultLogoutResponse(state, auditUser, Optional.empty()); } LOG.info("ID token hint is present"); @@ -138,10 +144,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE Optional.empty(), new ErrorObject( OAuth2Error.INVALID_REQUEST_CODE, "unable to validate id_token_hint"), - input, - Optional.empty(), - Optional.empty(), - subjectId); + auditUser, + Optional.empty()); } Optional audience; @@ -153,19 +157,14 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE return logoutService.generateErrorLogoutResponse( Optional.empty(), new ErrorObject(OAuth2Error.INVALID_REQUEST_CODE, "invalid id_token_hint"), - input, - Optional.empty(), - Optional.empty(), - subjectId); + auditUser, + Optional.empty()); } if (audience.isEmpty()) { + getSessionAndDestroyIfExists(sessionFromSessionCookie); return logoutService.generateDefaultLogoutResponse( - Optional.empty(), - input, - Optional.empty(), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + Optional.empty(), auditUser, Optional.empty()); } final String clientID = audience.get(); @@ -174,13 +173,12 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE Optional clientRegistry = dynamoClientService.getClient(clientID); if (clientRegistry.isEmpty()) { LOG.warn("Client not found in ClientRegistry"); + getSessionAndDestroyIfExists(sessionFromSessionCookie); return logoutService.generateErrorLogoutResponse( state, new ErrorObject(OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), - input, - Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + auditUser, + Optional.of(clientID)); } Optional postLogoutRedirectUri = @@ -188,24 +186,20 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE if (postLogoutRedirectUri.isEmpty()) { LOG.info( "post_logout_redirect_uri is NOT present in logout request. Generating default logout response"); + getSessionAndDestroyIfExists(sessionFromSessionCookie); return logoutService.generateDefaultLogoutResponse( - state, - input, - Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + state, auditUser, Optional.of(clientID)); } if (!postLogoutRedirectUriInClientReg(postLogoutRedirectUri, clientRegistry)) { + getSessionAndDestroyIfExists(sessionFromSessionCookie); return logoutService.generateErrorLogoutResponse( state, new ErrorObject( OAuth2Error.INVALID_REQUEST_CODE, "client registry does not contain post_logout_redirect_uri"), - input, - Optional.of(clientID), - getSessionAndDestroyIfExists(sessionFromSessionCookie), - subjectId); + auditUser, + Optional.of(clientID)); } if (sessionFromSessionCookie.isPresent()) { @@ -217,7 +211,7 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE clientID, postLogoutRedirectUri.get(), state, - input)); + auditUser)); } else { return segmentedFunctionCall( @@ -227,10 +221,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE URI.create(postLogoutRedirectUri.get()), state, Optional.empty(), - input, - Optional.of(clientID), - Optional.empty(), - Optional.empty())); + auditUser, + Optional.of(clientID))); } } @@ -283,17 +275,11 @@ private APIGatewayProxyResponseEvent logout( String clientID, String uri, Optional state, - APIGatewayProxyRequestEvent input) { + TxmaAuditUser auditUser) { segmentedFunctionCall("destroySessions", () -> logoutService.destroySessions(session)); cloudwatchMetricsService.incrementLogout(Optional.of(clientID)); return logoutService.generateLogoutResponse( - URI.create(uri), - state, - Optional.empty(), - input, - Optional.of(clientID), - Optional.of(session.getSessionId()), - Optional.ofNullable(session.getInternalCommonSubjectIdentifier())); + URI.create(uri), state, Optional.empty(), auditUser, Optional.of(clientID)); } } diff --git a/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java b/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java index 830f998dbe..7a45d37e86 100644 --- a/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java +++ b/oidc-api/src/test/java/uk/gov/di/authentication/oidc/lambda/LogoutHandlerTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import uk.gov.di.orchestration.audit.TxmaAuditUser; import uk.gov.di.orchestration.shared.entity.ClientRegistry; import uk.gov.di.orchestration.shared.helpers.CookieHelper; import uk.gov.di.orchestration.shared.helpers.IdGenerator; @@ -47,6 +48,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; +import static uk.gov.di.orchestration.shared.helpers.IpAddressHelper.extractIpAddress; +import static uk.gov.di.orchestration.shared.helpers.PersistentIdHelper.extractPersistentIdFromCookieHeader; import static uk.gov.di.orchestration.sharedtest.helper.RequestEventHelper.contextWithSourceIp; import static uk.gov.di.orchestration.sharedtest.logging.LogEventMatcher.withMessageContaining; @@ -107,9 +110,9 @@ void setUp() throws JOSEException, ParseException { logoutService); when(configurationService.getDefaultLogoutURI()).thenReturn(DEFAULT_LOGOUT_URI); when(configurationService.getInternalSectorUri()).thenReturn(INTERNAL_SECTOR_URI); - when(logoutService.generateLogoutResponse(any(), any(), any(), any(), any(), any(), any())) + when(logoutService.generateLogoutResponse(any(), any(), any(), any(), any())) .thenReturn(new APIGatewayProxyResponseEvent()); - when(logoutService.generateErrorLogoutResponse(any(), any(), any(), any(), any(), any())) + when(logoutService.generateErrorLogoutResponse(any(), any(), any(), any())) .thenReturn(new APIGatewayProxyResponseEvent()); when(context.getAwsRequestId()).thenReturn("aws-session-id"); @@ -162,10 +165,8 @@ void shouldDeleteSessionAndRedirectToClientLogoutUriForValidLogoutRequest() { CLIENT_LOGOUT_URI, Optional.of(STATE.toString()), Optional.empty(), - event, - audience, - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + getAuditUser(event), + audience); verify(cloudwatchMetricsService).incrementLogout(Optional.of("client-id")); } @@ -192,10 +193,8 @@ void shouldNotThrowWhenTryingToDeleteClientSessionWhichHasExpired() { CLIENT_LOGOUT_URI, Optional.of(STATE.toString()), Optional.empty(), - event, - audience, - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + getAuditUser(event), + audience); verify(cloudwatchMetricsService).incrementLogout(Optional.of("client-id")); } @@ -210,11 +209,7 @@ void shouldNotThrowWhenTryingToDeleteClientSessionWhichHasExpired() { verify(logoutService, times(1)).destroySessions(session); verify(logoutService) .generateDefaultLogoutResponse( - Optional.empty(), - event, - Optional.empty(), - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + Optional.empty(), getAuditUser(event), Optional.empty()); } @Test @@ -237,10 +232,8 @@ void shouldRedirectToClientLogoutUriWhenNoCookieExists() { CLIENT_LOGOUT_URI, Optional.of(STATE.getValue()), Optional.empty(), - event, - audience, - Optional.empty(), - Optional.empty()); + getAuditUserWhenNoCookie(event), + audience); verifyNoInteractions(cloudwatchMetricsService); } } @@ -266,11 +259,7 @@ void shouldRedirectToDefaultLogoutUriForValidLogoutRequestWithNoTokenHint() { verify(logoutService, times(1)).destroySessions(session); verify(logoutService) .generateDefaultLogoutResponse( - Optional.of(STATE.toString()), - event, - Optional.empty(), - Optional.of(session.getSessionId()), - Optional.of(SUBJECT.getValue())); + Optional.of(STATE.toString()), getAuditUser(event), Optional.empty()); } @Test @@ -288,9 +277,7 @@ void shouldRedirectToDefaultLogoutUriForValidLogoutRequestWithNoTokenHint() { verify(logoutService) .generateDefaultLogoutResponse( Optional.of(STATE.toString()), - event, - Optional.empty(), - Optional.empty(), + getAuditUserWhenNoCookie(event), Optional.empty()); } @@ -326,10 +313,8 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenSignatureIdTokenIsInval new ErrorObject( OAuth2Error.INVALID_REQUEST_CODE, "unable to validate id_token_hint"), - event, - Optional.empty(), - Optional.empty(), - Optional.of(SUBJECT.getValue())); + getAuditUser(event), + Optional.empty()); verifyNoInteractions(cloudwatchMetricsService); } } @@ -368,10 +353,8 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenClientIsNotFoundInClien Optional.of(STATE.getValue()), new ErrorObject( OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), - event, - signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.of(session.getSessionId()), - Optional.of(SUBJECT.getValue())); + getAuditUser(event), + signedJWT.getJWTClaimsSet().getAudience().stream().findFirst()); verifyNoInteractions(cloudwatchMetricsService); } @@ -401,10 +384,8 @@ void shouldRedirectToDefaultLogoutUriWithErrorMessageWhenClientIsNotFoundInClien Optional.of(STATE.getValue()), new ErrorObject( OAuth2Error.UNAUTHORIZED_CLIENT_CODE, "client not found"), - event, - signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.empty(), - Optional.empty()); + getAuditUserWhenNoCookie(event), + signedJWT.getJWTClaimsSet().getAudience().stream().findFirst()); verifyNoInteractions(cloudwatchMetricsService); } @@ -431,11 +412,7 @@ void shouldRedirectToDefaultUriWhenLogoutRedirectUriIsMissing() { verify(logoutService, times(1)).destroySessions(session); verify(logoutService) .generateDefaultLogoutResponse( - Optional.of(STATE.toString()), - event, - audience, - Optional.of(session.getSessionId()), - Optional.of(SUBJECT.getValue())); + Optional.of(STATE.toString()), getAuditUser(event), audience); } @Test @@ -456,10 +433,8 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM verify(logoutService) .generateDefaultLogoutResponse( Optional.of(STATE.toString()), - event, - audience, - Optional.empty(), - Optional.empty()); + getAuditUserWhenNoCookie(event), + audience); } @Test @@ -496,10 +471,8 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM new ErrorObject( OAuth2Error.INVALID_REQUEST_CODE, "client registry does not contain post_logout_redirect_uri"), - event, - signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.of(session.getSessionId()), - Optional.of(SUBJECT.getValue())); + getAuditUser(event), + signedJWT.getJWTClaimsSet().getAudience().stream().findFirst()); verifyNoInteractions(cloudwatchMetricsService); } @@ -531,10 +504,8 @@ void shouldNotTryToDeleteSessionWhenSessionDoesNotExistWhileLogoutRedirectUriIsM new ErrorObject( OAuth2Error.INVALID_REQUEST_CODE, "client registry does not contain post_logout_redirect_uri"), - event, - signedJWT.getJWTClaimsSet().getAudience().stream().findFirst(), - Optional.empty(), - Optional.empty()); + getAuditUserWhenNoCookie(event), + signedJWT.getJWTClaimsSet().getAudience().stream().findFirst()); verifyNoInteractions(cloudwatchMetricsService); } } @@ -592,4 +563,20 @@ private void setUpClientSession(String clientSessionId, String clientId) { when(dynamoClientService.getClient(clientId)) .thenReturn(Optional.of(new ClientRegistry().withClientID(clientId))); } + + private TxmaAuditUser getAuditUser(APIGatewayProxyRequestEvent event) { + return TxmaAuditUser.user() + .withIpAddress(extractIpAddress(event)) + .withPersistentSessionId(extractPersistentIdFromCookieHeader(event.getHeaders())) + .withSessionId(session.getSessionId()) + .withUserId(session.getInternalCommonSubjectIdentifier()); + } + + private TxmaAuditUser getAuditUserWhenNoCookie(APIGatewayProxyRequestEvent event) { + return TxmaAuditUser.user() + .withIpAddress(extractIpAddress(event)) + .withPersistentSessionId(extractPersistentIdFromCookieHeader(event.getHeaders())) + .withSessionId(null) + .withUserId(null); + } } diff --git a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java index d4c1e0bca5..c75b4f0fda 100644 --- a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java +++ b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java @@ -65,13 +65,17 @@ public APIGatewayProxyResponseEvent handleAccountInterventionLogout( APIGatewayProxyRequestEvent input, String clientId, AccountIntervention intervention) { + + var auditUser = + TxmaAuditUser.user() + .withIpAddress(extractIpAddress(input)) + .withPersistentSessionId( + extractPersistentIdFromCookieHeader(input.getHeaders())) + .withSessionId(session.getSessionId()) + .withUserId(session.getInternalCommonSubjectIdentifier()); + destroySessions(session); - return generateAccountInterventionLogoutResponse( - input, - clientId, - session.getSessionId(), - intervention, - session.getInternalCommonSubjectIdentifier()); + return generateAccountInterventionLogoutResponse(auditUser, clientId, intervention); } public void destroySessions(Session session) { @@ -99,10 +103,8 @@ public void destroySessions(Session session) { public APIGatewayProxyResponseEvent generateErrorLogoutResponse( Optional state, ErrorObject errorObject, - APIGatewayProxyRequestEvent input, - Optional clientId, - Optional sessionId, - Optional subjectId) { + TxmaAuditUser auditUser, + Optional clientId) { LOG.info( "Generating Logout Error Response with code: {} and description: {}", errorObject.getCode(), @@ -111,38 +113,30 @@ public APIGatewayProxyResponseEvent generateErrorLogoutResponse( configurationService.getDefaultLogoutURI(), state, Optional.of(errorObject), - input, - clientId, - sessionId, - subjectId); + auditUser, + clientId); } public APIGatewayProxyResponseEvent generateDefaultLogoutResponse( - Optional state, - APIGatewayProxyRequestEvent input, - Optional clientId, - Optional sessionId, - Optional subjectId) { + Optional state, TxmaAuditUser auditUser, Optional clientId) { LOG.info("Generating default Logout Response"); - sessionId.ifPresent(t -> cloudwatchMetricsService.incrementLogout(clientId)); + if (auditUser.sessionId() != null) { + cloudwatchMetricsService.incrementLogout(clientId); + } return generateLogoutResponse( configurationService.getDefaultLogoutURI(), state, Optional.empty(), - input, - clientId, - sessionId, - subjectId); + auditUser, + clientId); } public APIGatewayProxyResponseEvent generateLogoutResponse( URI logoutUri, Optional state, Optional errorObject, - APIGatewayProxyRequestEvent input, - Optional clientId, - Optional sessionId, - Optional subjectId) { + TxmaAuditUser auditUser, + Optional clientId) { LOG.info("Generating Logout Response using URI: {}", logoutUri); URIBuilder uriBuilder = new URIBuilder(logoutUri); state.ifPresent(s -> uriBuilder.addParameter("state", s)); @@ -157,26 +151,15 @@ public APIGatewayProxyResponseEvent generateLogoutResponse( throw new RuntimeException("Unable to build URI"); } - var user = - TxmaAuditUser.user() - .withIpAddress(extractIpAddress(input)) - .withPersistentSessionId( - extractPersistentIdFromCookieHeader(input.getHeaders())) - .withSessionId(sessionId.orElse(null)) - .withUserId(subjectId.orElse(null)); - - auditService.submitAuditEvent(LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), user); + auditService.submitAuditEvent( + LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), auditUser); return generateApiGatewayProxyResponse( 302, "", Map.of(ResponseHeaders.LOCATION, uri.toString()), null); } private APIGatewayProxyResponseEvent generateAccountInterventionLogoutResponse( - APIGatewayProxyRequestEvent input, - String clientId, - String sessionId, - AccountIntervention intervention, - String subjectId) { + TxmaAuditUser auditUser, String clientId, AccountIntervention intervention) { URI redirectURI; if (intervention.getBlocked()) { redirectURI = configurationService.getAccountStatusBlockedURI(); @@ -190,12 +173,6 @@ private APIGatewayProxyResponseEvent generateAccountInterventionLogoutResponse( cloudwatchMetricsService.incrementLogout(Optional.of(clientId), Optional.of(intervention)); return generateLogoutResponse( - redirectURI, - Optional.empty(), - Optional.empty(), - input, - Optional.of(clientId), - Optional.of(sessionId), - Optional.ofNullable(subjectId)); + redirectURI, Optional.empty(), Optional.empty(), auditUser, Optional.of(clientId)); } } diff --git a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java index ecbe23e7b8..f80ac11cb7 100644 --- a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java +++ b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java @@ -167,10 +167,8 @@ void successfullyReturnsClientLogoutResponse() { CLIENT_LOGOUT_URI, Optional.of(STATE.getValue()), Optional.empty(), - event, - Optional.of(audience.get()), - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + auditUser, + Optional.of(audience.get())); verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); @@ -184,11 +182,7 @@ void successfullyReturnsClientLogoutResponse() { void successfullyReturnsDefaultLogoutResponseWithoutStateWhenStateIsAbsent() { APIGatewayProxyResponseEvent response = logoutService.generateDefaultLogoutResponse( - Optional.empty(), - event, - Optional.of(audience.get()), - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + Optional.empty(), auditUser, Optional.of(audience.get())); verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService).incrementLogout(Optional.of(CLIENT_ID)); @@ -203,11 +197,7 @@ void successfullyReturnsDefaultLogoutResponseWithoutStateWhenStateIsAbsent() { void successfullyReturnsDefaultLogoutResponseWithStateWhenStateIsPresent() { APIGatewayProxyResponseEvent response = logoutService.generateDefaultLogoutResponse( - Optional.of(STATE.getValue()), - event, - Optional.of(audience.get()), - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + Optional.of(STATE.getValue()), auditUser, Optional.of(audience.get())); verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, CLIENT_ID, auditUser); verify(cloudwatchMetricsService).incrementLogout(Optional.of(CLIENT_ID)); @@ -224,10 +214,8 @@ void successfullyReturnsErrorLogoutResponse() throws URISyntaxException { logoutService.generateErrorLogoutResponse( Optional.empty(), new ErrorObject(OAuth2Error.INVALID_REQUEST_CODE, "invalid session"), - event, - Optional.empty(), - Optional.of(SESSION_ID), - Optional.of(SUBJECT.getValue())); + auditUser, + Optional.empty()); verify(auditService).submitAuditEvent(LOG_OUT_SUCCESS, AuditService.UNKNOWN, auditUser); verifyNoInteractions(cloudwatchMetricsService); @@ -300,9 +288,7 @@ void shouldDeleteSessionFromRedisWhenNoCookieExists() { CLIENT_LOGOUT_URI, Optional.of(STATE.getValue()), Optional.empty(), - input, - Optional.empty(), - Optional.of(SESSION_ID), + auditUserWhenNoCookie, Optional.empty()); verify(sessionService, times(0)).deleteSessionFromRedis(SESSION_ID); From 7a425e0bf1c6bb06b9845ca0c0b0214ad0dc33cc Mon Sep 17 00:00:00 2001 From: Bill Dunn Date: Tue, 7 May 2024 10:11:43 +0100 Subject: [PATCH 3/4] ATO-585: Add RP pairwise ID when it is available to AUTH_LOG_OUT_SUCCESS audit event --- .../shared/services/LogoutService.java | 58 +++++++++++++++++-- .../shared/services/LogoutServiceTest.java | 36 +++++++++++- 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java index c75b4f0fda..6c617aa1ee 100644 --- a/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java +++ b/orchestration-shared/src/main/java/uk/gov/di/orchestration/shared/services/LogoutService.java @@ -8,8 +8,11 @@ import org.apache.logging.log4j.Logger; import uk.gov.di.orchestration.audit.TxmaAuditUser; import uk.gov.di.orchestration.shared.entity.AccountIntervention; +import uk.gov.di.orchestration.shared.entity.ClientRegistry; import uk.gov.di.orchestration.shared.entity.ResponseHeaders; import uk.gov.di.orchestration.shared.entity.Session; +import uk.gov.di.orchestration.shared.entity.UserProfile; +import uk.gov.di.orchestration.shared.helpers.ClientSubjectHelper; import java.net.URI; import java.net.URISyntaxException; @@ -20,6 +23,7 @@ import static uk.gov.di.orchestration.shared.helpers.ApiGatewayResponseHelper.generateApiGatewayProxyResponse; import static uk.gov.di.orchestration.shared.helpers.IpAddressHelper.extractIpAddress; import static uk.gov.di.orchestration.shared.helpers.PersistentIdHelper.extractPersistentIdFromCookieHeader; +import static uk.gov.di.orchestration.shared.services.AuditService.MetadataPair.pair; public class LogoutService { @@ -32,6 +36,7 @@ public class LogoutService { private final AuditService auditService; private final CloudwatchMetricsService cloudwatchMetricsService; private final BackChannelLogoutService backChannelLogoutService; + private final DynamoService dynamoService; public LogoutService(ConfigurationService configurationService) { this.configurationService = configurationService; @@ -41,6 +46,7 @@ public LogoutService(ConfigurationService configurationService) { this.auditService = new AuditService(configurationService); this.cloudwatchMetricsService = new CloudwatchMetricsService(); this.backChannelLogoutService = new BackChannelLogoutService(configurationService); + this.dynamoService = new DynamoService(configurationService); } public LogoutService( @@ -50,7 +56,8 @@ public LogoutService( ClientSessionService clientSessionService, AuditService auditService, CloudwatchMetricsService cloudwatchMetricsService, - BackChannelLogoutService backChannelLogoutService) { + BackChannelLogoutService backChannelLogoutService, + DynamoService dynamoService) { this.configurationService = configurationService; this.sessionService = sessionService; this.dynamoClientService = dynamoClientService; @@ -58,6 +65,7 @@ public LogoutService( this.auditService = auditService; this.cloudwatchMetricsService = cloudwatchMetricsService; this.backChannelLogoutService = backChannelLogoutService; + this.dynamoService = dynamoService; } public APIGatewayProxyResponseEvent handleAccountInterventionLogout( @@ -151,9 +159,7 @@ public APIGatewayProxyResponseEvent generateLogoutResponse( throw new RuntimeException("Unable to build URI"); } - auditService.submitAuditEvent( - LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), auditUser); - + sendAuditEvent(clientId, auditUser); return generateApiGatewayProxyResponse( 302, "", Map.of(ResponseHeaders.LOCATION, uri.toString()), null); } @@ -175,4 +181,48 @@ private APIGatewayProxyResponseEvent generateAccountInterventionLogoutResponse( return generateLogoutResponse( redirectURI, Optional.empty(), Optional.empty(), auditUser, Optional.of(clientId)); } + + public Optional getRpPairwiseId(String subject, String clientId) { + try { + if (subject == null || clientId == null) { + LOG.warn("User or client ID is null while getting RP pairwise ID for audit event"); + return Optional.empty(); + } + UserProfile userProfile = dynamoService.getUserProfileFromSubject(subject); + Optional client = dynamoClientService.getClient(clientId); + if (client.isEmpty()) { + LOG.warn("Client not found while getting RP pairwise ID for audit event"); + return Optional.empty(); + } + return Optional.of( + ClientSubjectHelper.getSubject( + userProfile, + client.get(), + dynamoService, + configurationService.getInternalSectorUri()) + .getValue()); + } catch (Exception e) { + LOG.warn("Exception caught while getting RP pairwise ID for audit event"); + return Optional.empty(); + } + } + + private void sendAuditEvent(Optional clientId, TxmaAuditUser auditUser) { + if (clientId.isPresent()) { + Optional rpPairwiseId = getRpPairwiseId(auditUser.userId(), clientId.get()); + if (rpPairwiseId.isPresent()) { + auditService.submitAuditEvent( + LOG_OUT_SUCCESS, + clientId.orElse(AuditService.UNKNOWN), + auditUser, + pair("rpPairwiseId", rpPairwiseId.get())); + } else { + auditService.submitAuditEvent( + LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), auditUser); + } + } else { + auditService.submitAuditEvent( + LOG_OUT_SUCCESS, clientId.orElse(AuditService.UNKNOWN), auditUser); + } + } } diff --git a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java index f80ac11cb7..b5524a846a 100644 --- a/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java +++ b/orchestration-shared/src/test/java/uk/gov/di/orchestration/shared/services/LogoutServiceTest.java @@ -26,7 +26,9 @@ import uk.gov.di.orchestration.shared.entity.ClientSession; import uk.gov.di.orchestration.shared.entity.ResponseHeaders; import uk.gov.di.orchestration.shared.entity.Session; +import uk.gov.di.orchestration.shared.entity.UserProfile; import uk.gov.di.orchestration.shared.entity.VectorOfTrust; +import uk.gov.di.orchestration.shared.helpers.ClientSubjectHelper; import uk.gov.di.orchestration.shared.helpers.IdGenerator; import uk.gov.di.orchestration.shared.helpers.IpAddressHelper; import uk.gov.di.orchestration.shared.helpers.PersistentIdHelper; @@ -34,6 +36,7 @@ import java.net.URI; import java.net.URISyntaxException; +import java.nio.ByteBuffer; import java.text.ParseException; import java.time.LocalDateTime; import java.util.List; @@ -65,6 +68,7 @@ public class LogoutServiceTest { private final DynamoClientService dynamoClientService = mock(DynamoClientService.class); private final ClientSessionService clientSessionService = mock(ClientSessionService.class); private final AuditService auditService = mock(AuditService.class); + private final DynamoService dynamoService = mock(DynamoService.class); private final APIGatewayProxyRequestEvent event = mock(APIGatewayProxyRequestEvent.class); @@ -75,6 +79,7 @@ public class LogoutServiceTest { private static MockedStatic ipAddressHelper; private static MockedStatic persistentIdHelper; + private static MockedStatic clientSubjectHelper; private static final State STATE = new State(); private static final String INTERNAL_SECTOR_URI = "https://test.account.gov.uk"; @@ -98,6 +103,9 @@ public class LogoutServiceTest { private static final String ENVIRONMENT = "test"; + private static final UserProfile USER_PROFILE = + new UserProfile().withSubjectID("any").withSalt(ByteBuffer.allocateDirect(12345)); + private SignedJWT signedIDToken; private Optional audience; private Session session; @@ -119,6 +127,7 @@ public class LogoutServiceTest { void setup() throws JOSEException, ParseException { ipAddressHelper = mockStatic(IpAddressHelper.class); persistentIdHelper = mockStatic(PersistentIdHelper.class); + clientSubjectHelper = mockStatic(ClientSubjectHelper.class); when(IpAddressHelper.extractIpAddress(any())).thenReturn(IP_ADDRESS); when(PersistentIdHelper.extractPersistentIdFromCookieHeader(event.getHeaders())) .thenReturn(PERSISTENT_SESSION_ID); @@ -138,7 +147,8 @@ void setup() throws JOSEException, ParseException { clientSessionService, auditService, cloudwatchMetricsService, - backChannelLogoutService); + backChannelLogoutService, + dynamoService); ECKey ecSigningKey = new ECKeyGenerator(Curve.P_256).algorithm(JWSAlgorithm.ES256).generate(); @@ -158,6 +168,7 @@ void setup() throws JOSEException, ParseException { void teardown() { ipAddressHelper.close(); persistentIdHelper.close(); + clientSubjectHelper.close(); } @Test @@ -336,6 +347,29 @@ void throwsWhenGenerateAccountInterventionLogoutResponseCalledInappropriately() assertEquals("Account status must be blocked or suspended", exception.getMessage()); } + @Test + void includesRpPairwiseIdInLogOutSuccessAuditEventWhenItIsAvailable() { + Subject rpSubject = new Subject(); + when(dynamoClientService.getClient(CLIENT_ID)) + .thenReturn(Optional.of(new ClientRegistry().withClientID(CLIENT_ID))); + when(dynamoService.getUserProfileFromSubject(any())).thenReturn(USER_PROFILE); + when(ClientSubjectHelper.getSubject(any(), any(), any(), any())).thenReturn(rpSubject); + + logoutService.generateLogoutResponse( + CLIENT_LOGOUT_URI, + Optional.of(STATE.getValue()), + Optional.empty(), + auditUser, + Optional.of(audience.get())); + + verify(auditService) + .submitAuditEvent( + LOG_OUT_SUCCESS, + CLIENT_ID, + auditUser, + AuditService.MetadataPair.pair("rpPairwiseId", rpSubject.getValue())); + } + private Session generateSession() { return new Session(SESSION_ID).addClientSession(CLIENT_SESSION_ID); } From 3fec8b038dfd7e1450c0b65061fe33d306614f42 Mon Sep 17 00:00:00 2001 From: Bill Dunn Date: Tue, 7 May 2024 16:26:42 +0100 Subject: [PATCH 4/4] ATO-585: Get `subjectId` and `sessionId` using `map()` --- .../di/authentication/oidc/lambda/LogoutHandler.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java index 36da504cd7..4fa985b2e8 100644 --- a/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java +++ b/oidc-api/src/main/java/uk/gov/di/authentication/oidc/lambda/LogoutHandler.java @@ -100,14 +100,8 @@ public APIGatewayProxyResponseEvent logoutRequestHandler(APIGatewayProxyRequestE () -> sessionService.getSessionFromSessionCookie(input.getHeaders())); attachSessionToLogsIfExists(sessionFromSessionCookie, input.getHeaders()); - Optional subjectId = Optional.empty(); - Optional sessionId = Optional.empty(); - if (sessionFromSessionCookie.isPresent()) { - subjectId = - Optional.ofNullable( - sessionFromSessionCookie.get().getInternalCommonSubjectIdentifier()); - sessionId = Optional.ofNullable(sessionFromSessionCookie.get().getSessionId()); - } + var subjectId = sessionFromSessionCookie.map(Session::getInternalCommonSubjectIdentifier); + var sessionId = sessionFromSessionCookie.map(Session::getSessionId); var auditUser = TxmaAuditUser.user()