From 6ec1cda12387517853c37947321c23ff8d739194 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Fri, 24 May 2024 00:39:09 +0200 Subject: [PATCH] Sonar refactoring - AbstractUaaEvent class (#2891) Remove duplicates Added test class --- .../uaa/audit/event/AbstractUaaEvent.java | 86 +++++----- .../uaa/scim/event/GroupModifiedEvent.java | 47 ------ .../uaa/audit/event/AbstractUaaEventTest.java | 156 ++++++++++++++++++ 3 files changed, 200 insertions(+), 89 deletions(-) create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEventTest.java diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEvent.java b/server/src/main/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEvent.java index 02013bbedca..fdc3fa60c0a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEvent.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEvent.java @@ -28,6 +28,7 @@ import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; +import java.io.Serial; import java.security.Principal; import java.util.ArrayList; import java.util.Collection; @@ -46,14 +47,14 @@ public abstract class AbstractUaaEvent extends ApplicationEvent { private static final long serialVersionUID = -7639844193401892160L; - private transient final String zoneId; + private final transient String zoneId; private Authentication authentication; protected AbstractUaaEvent(Object source, String zoneId) { super(source); - if (source instanceof Authentication) { - this.authentication = (Authentication)source; + if (source instanceof Authentication authenticationSource) { + this.authentication = authenticationSource; } this.zoneId = zoneId; } @@ -89,53 +90,52 @@ public Authentication getAuthentication() { // due to some OAuth authentication scenarios which don't set it. protected String getOrigin(Principal principal) { - if (principal instanceof Authentication) { + if (principal instanceof Authentication caller) { + return getAuthenticationString(caller); + } - Authentication caller = (Authentication) principal; - StringBuilder builder = new StringBuilder(); - if (caller instanceof OAuth2Authentication) { - OAuth2Authentication oAuth2Authentication = (OAuth2Authentication) caller; - builder.append("client=").append(oAuth2Authentication.getOAuth2Request().getClientId()); - if (!oAuth2Authentication.isClientOnly()) { - builder.append(", ").append("user=").append(oAuth2Authentication.getName()); - } - } - else { - builder.append("caller=").append(caller.getName()); - } + return principal == null ? null : principal.getName(); + } - if (caller.getDetails() != null) { - builder.append(", details=("); - try { - @SuppressWarnings("unchecked") - Map map = - JsonUtils.readValue((String)caller.getDetails(), new TypeReference>(){}); - if (map.containsKey("remoteAddress")) { - builder.append("remoteAddress=").append(map.get("remoteAddress")).append(", "); - } - builder.append("type=").append(caller.getDetails().getClass().getSimpleName()); - } catch (Exception e) { - // ignore - builder.append(caller.getDetails()); - } - appendTokenDetails(caller, builder); - builder.append(")"); + private String getAuthenticationString(Authentication caller) { + StringBuilder builder = new StringBuilder(); + if (caller instanceof OAuth2Authentication oAuth2Authentication) { + builder.append("client=").append(oAuth2Authentication.getOAuth2Request().getClientId()); + if (!oAuth2Authentication.isClientOnly()) { + builder.append(", ").append("user=").append(oAuth2Authentication.getName()); } - return builder.toString(); - + } + else { + builder.append("caller=").append(caller.getName()); } - return principal == null ? null : principal.getName(); - + if (caller.getDetails() != null) { + builder.append(", details=("); + try { + @SuppressWarnings("unchecked") + Map map = + JsonUtils.readValue((String) caller.getDetails(), new TypeReference>(){}); + if (map.containsKey("remoteAddress")) { + builder.append("remoteAddress=").append(map.get("remoteAddress")).append(", "); + } + builder.append("type=").append(caller.getDetails().getClass().getSimpleName()); + } catch (Exception e) { + // ignore + builder.append(caller.getDetails()); + } + appendTokenDetails(caller, builder); + builder.append(")"); + } + return builder.toString(); } protected void appendTokenDetails(Authentication caller, StringBuilder builder) { String tokenValue = null; - if (caller instanceof UaaOauth2Authentication) { - tokenValue = ((UaaOauth2Authentication)caller).getTokenValue(); - } else if (caller.getDetails() instanceof OAuth2AuthenticationDetails) { - tokenValue = ((OAuth2AuthenticationDetails)authentication.getDetails()).getTokenValue(); + if (caller instanceof UaaOauth2Authentication uaaOauth2Authentication) { + tokenValue = uaaOauth2Authentication.getTokenValue(); + } else if (caller.getDetails() instanceof OAuth2AuthenticationDetails oAuth2AuthenticationDetails) { + tokenValue = oAuth2AuthenticationDetails.getTokenValue(); } if (hasText(tokenValue)) { if (isJwtToken(tokenValue)) { @@ -161,9 +161,10 @@ protected static Authentication getContextAuthentication() { Authentication a = SecurityContextHolder.getContext().getAuthentication(); if (a==null) { a = new Authentication() { - private static final long serialVersionUID = 1748694836774597624L; - ArrayList authorities = new ArrayList(); + @Serial + private static final long serialVersionUID = -6219210214210936383L; + ArrayList authorities = new ArrayList<>(0); @Override public Collection getAuthorities() { return authorities; @@ -191,6 +192,7 @@ public boolean isAuthenticated() { @Override public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { + // ignore } @Override diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/event/GroupModifiedEvent.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/event/GroupModifiedEvent.java index 2c7ccace145..656c784dc9a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/event/GroupModifiedEvent.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/event/GroupModifiedEvent.java @@ -23,12 +23,8 @@ import org.cloudfoundry.identity.uaa.audit.event.AbstractUaaEvent; import org.cloudfoundry.identity.uaa.util.JsonUtils; import org.springframework.security.core.Authentication; -import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.context.SecurityContextHolder; -import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; public class GroupModifiedEvent extends AbstractUaaEvent { @@ -145,47 +141,4 @@ public String toString() { '}'; } } - - protected static Authentication getContextAuthentication() { - Authentication a = SecurityContextHolder.getContext().getAuthentication(); - if (a==null) { - a = new Authentication() { - ArrayList authorities = new ArrayList(); - @Override - public Collection getAuthorities() { - return authorities; - } - - @Override - public Object getCredentials() { - return null; - } - - @Override - public Object getDetails() { - return null; - } - - @Override - public Object getPrincipal() { - return "null"; - } - - @Override - public boolean isAuthenticated() { - return false; - } - - @Override - public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { - } - - @Override - public String getName() { - return "null"; - } - }; - } - return a; - } } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEventTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEventTest.java new file mode 100644 index 00000000000..f91f78c037b --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/audit/event/AbstractUaaEventTest.java @@ -0,0 +1,156 @@ +package org.cloudfoundry.identity.uaa.audit.event; + +import org.cloudfoundry.identity.uaa.annotations.WithDatabaseContext; +import org.cloudfoundry.identity.uaa.audit.AuditEventType; +import org.cloudfoundry.identity.uaa.audit.JdbcAuditService; +import org.cloudfoundry.identity.uaa.audit.UaaAuditService; +import org.cloudfoundry.identity.uaa.oauth.UaaOauth2Authentication; +import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Authentication; +import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Request; +import org.cloudfoundry.identity.uaa.oauth.provider.authentication.OAuth2AuthenticationDetails; +import org.cloudfoundry.identity.uaa.scim.event.GroupModifiedEvent; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; + +import java.util.Map; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@WithDatabaseContext +class AbstractUaaEventTest { + + @Autowired + private JdbcTemplate jdbcTemplate; + + AbstractUaaEvent event; + UaaAuditService auditListener; + + @BeforeEach + void setUp() { + event = GroupModifiedEvent.groupCreated("group", "groupName", new String[0], "uaa"); + auditListener = new JdbcAuditService(jdbcTemplate); + } + + @AfterEach + void cleanUp() { + SecurityContextHolder.clearContext(); + } + + @Test + void process() { + event.process(auditListener); + assertNotNull(auditListener); + } + + @Test + void createAuditRecord() { + assertNotNull(event.createAuditRecord("me", AuditEventType.GroupModifiedEvent, "notuaa")); + } + + @Test + void getAuthentication() { + assertNotNull(event.getAuthentication()); + } + + @Test + void getContextAuthentication() { + Authentication authentication = AbstractUaaEvent.getContextAuthentication(); + assertNotNull(authentication); + SecurityContextHolder.getContext().setAuthentication(authentication); + String originString = event.getOrigin(authentication); + assertThat(originString, is("caller=null")); + } + + @Test + void getOrigin() { + UaaOauth2Authentication authentication = mock(UaaOauth2Authentication.class); + OAuth2Request oAuth2Request = mock(OAuth2Request.class); + when(authentication.getOAuth2Request()).thenReturn(oAuth2Request); + when(authentication.getName()).thenReturn("marissa"); + when(authentication.getDetails()).thenReturn(Map.of("misc", "somedetails", "remoteAddress", "external")); + SecurityContextHolder.getContext().setAuthentication(authentication); + String originString = event.getOrigin(authentication); + assertThat(originString, containsString("marissa")); + assertThat(originString, containsString("client=null")); + assertThat(originString, containsString("misc=somedetails")); + assertThat(originString, containsString("remoteAddress=external")); + assertThat(originString, containsString("details=({")); + } + + @Test + void getOriginNotAuthenticated() { + assertNull(event.getOrigin(null)); + } + + @Test + void getOriginDetailsParsed() { + UaaOauth2Authentication authentication = mock(UaaOauth2Authentication.class); + OAuth2Request oAuth2Request = mock(OAuth2Request.class); + when(authentication.getOAuth2Request()).thenReturn(oAuth2Request); + when(authentication.getName()).thenReturn("marissa"); + when(authentication.getDetails()).thenReturn("{\"misc\":\"somedetails\",\"remoteAddress\":\"external\"}"); + SecurityContextHolder.getContext().setAuthentication(authentication); + String originString = event.getOrigin(authentication); + assertThat(originString, containsString("marissa")); + assertThat(originString, containsString("client=null")); + assertThat(originString, not(containsString("misc=somedetails"))); + assertThat(originString, containsString("remoteAddress=external")); + assertThat(originString, not(containsString("{"))); + } + + @Test + void getAuthenticationJsonWebTokenValue() { + String originTokenString = event.getOrigin(mockAuthenticationWithToken("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJtYXJpc3NhIiwiaXNzIjoidWFhIn0.omitted")); + assertThat(originTokenString, containsString("client=clientid")); + assertThat(originTokenString, containsString("iss=uaa")); + assertThat(originTokenString, containsString("sub=marissa")); + } + + @Test + void getAuthenticationOpaqueTokenValue() { + String originTokenString = event.getOrigin(mockAuthenticationWithToken("any-value")); + assertThat(originTokenString, containsString("client=clientid")); + assertThat(originTokenString, containsString("opaque-token=present")); + assertThat(originTokenString, not(containsString("any-value"))); + } + + + @Test + void getAuthenticationTokenValueInvalid() { + String originTokenString = event.getOrigin(mockAuthenticationWithToken("fake.token.value")); + assertThat(originTokenString, containsString("client=clientid")); + assertThat(originTokenString, containsString("")); + assertThat(originTokenString, not(containsString("fake"))); + assertThat(originTokenString, not(containsString("value"))); + } + + private Authentication mockAuthenticationWithToken(String token) { + OAuth2Authentication authentication = mock(OAuth2Authentication.class); + OAuth2Request oAuth2Request = mock(OAuth2Request.class); + when(authentication.getOAuth2Request()).thenReturn(oAuth2Request); + OAuth2AuthenticationDetails auth2AuthenticationDetails = mock(OAuth2AuthenticationDetails.class); + when(authentication.getDetails()).thenReturn(auth2AuthenticationDetails); + when(authentication.isClientOnly()).thenReturn(true); + when(auth2AuthenticationDetails.getTokenValue()).thenReturn(token); + when(oAuth2Request.getClientId()).thenReturn("clientid"); + return authentication; + } + + @Test + void getIdentityZoneId() { + assertEquals("uaa", event.getIdentityZoneId()); + } +}