diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecorator.java index 951a9b8e28a..a55d7dd80fd 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecorator.java @@ -1,6 +1,7 @@ package datadog.trace.bootstrap.instrumentation.decorator; import static datadog.trace.api.UserEventTrackingMode.DISABLED; +import static datadog.trace.api.UserEventTrackingMode.SAFE; import datadog.trace.api.Config; import datadog.trace.api.UserEventTrackingMode; @@ -9,10 +10,17 @@ import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.api.Tags; import java.util.Map; +import java.util.regex.Pattern; import javax.annotation.Nonnull; public class AppSecUserEventDecorator { + private static final String NUMBER_PATTERN = "[0-9]+"; + private static final String UUID_PATTERN = + "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"; + private static final Pattern SAFE_USER_ID_PATTERN = + Pattern.compile(NUMBER_PATTERN + "|" + UUID_PATTERN, Pattern.CASE_INSENSITIVE); + public boolean isEnabled() { if (!ActiveSubsystems.APPSEC_ACTIVE) { return false; @@ -41,9 +49,7 @@ public void onLoginSuccess(String userId, Map metadata) { return; } - if (userId != null) { - segment.setTagTop("usr.id", userId); - } + onUserId(segment, "usr.id", userId); onEvent(segment, "users.login.success", metadata); } @@ -53,10 +59,7 @@ public void onLoginFailure(String userId, Map metadata) { return; } - if (userId != null) { - segment.setTagTop("appsec.events.users.login.failure.usr.id", userId); - } - + onUserId(segment, "appsec.events.users.login.failure.usr.id", userId); onEvent(segment, "users.login.failure", metadata); } @@ -66,9 +69,7 @@ public void onSignup(String userId, Map metadata) { return; } - if (userId != null) { - segment.setTagTop("usr.id", userId); - } + onUserId(segment, "usr.id", userId); onEvent(segment, "users.signup", metadata); } @@ -87,6 +88,18 @@ private void onEvent(@Nonnull TraceSegment segment, String eventName, Map metadata = null; - if (mode == EXTENDED && user != null) { - userId = user.getUsername(); - + if (mode == EXTENDED) { metadata = new HashMap<>(); metadata.put("enabled", String.valueOf(user.isEnabled())); metadata.put( @@ -71,44 +69,36 @@ public void onLogin(Authentication authentication, Throwable throwable, Authenti return; } - String userId = null; - - if (mode == EXTENDED) { - userId = authentication.getName(); - } - - if (mode != DISABLED) { - if (throwable == null && result != null && result.isAuthenticated()) { - Map metadata = null; - Object principal = result.getPrincipal(); - if (principal instanceof User) { - User user = (User) principal; - metadata = new HashMap<>(); - metadata.put("enabled", String.valueOf(user.isEnabled())); - metadata.put( - "authorities", - user.getAuthorities().stream() - .map(Object::toString) - .collect(Collectors.joining(","))); - metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired())); - metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked())); - metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired())); - } - - onLoginSuccess(userId, metadata); - } else if (throwable != null) { - // On bad password, throwable would be - // org.springframework.security.authentication.BadCredentialsException, - // on user not found, throwable can be BadCredentials or - // org.springframework.security.core.userdetails.UsernameNotFoundException depending on the - // internal setting - // hideUserNotFoundExceptions (or a custom AuthenticationProvider implementation overriding - // this). - // This would be the ideal place to check whether the user exists or not, but we cannot do - // so reliably yet. - // See UsernameNotFoundExceptionInstrumentation for more details. - onLoginFailure(userId, null); + String userId = result != null ? result.getName() : authentication.getName(); + + if (throwable == null && result != null && result.isAuthenticated()) { + Map metadata = null; + Object principal = result.getPrincipal(); + if (principal instanceof User) { + User user = (User) principal; + metadata = new HashMap<>(); + metadata.put("enabled", String.valueOf(user.isEnabled())); + metadata.put( + "authorities", + user.getAuthorities().stream().map(Object::toString).collect(Collectors.joining(","))); + metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired())); + metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked())); + metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired())); } + + onLoginSuccess(userId, metadata); + } else if (throwable != null) { + // On bad password, throwable would be + // org.springframework.security.authentication.BadCredentialsException, + // on user not found, throwable can be BadCredentials or + // org.springframework.security.core.userdetails.UsernameNotFoundException depending on the + // internal setting + // hideUserNotFoundExceptions (or a custom AuthenticationProvider implementation overriding + // this). + // This would be the ideal place to check whether the user exists or not, but we cannot do + // so reliably yet. + // See UsernameNotFoundExceptionInstrumentation for more details. + onLoginFailure(userId, null); } } diff --git a/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy index 53c2756f64d..c25ec3a0bfa 100644 --- a/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy @@ -124,7 +124,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest