Skip to content

Commit d19ceac

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
authored
Update missing RFC parts for user event tacking (#7213)
1 parent 2d36ca7 commit d19ceac

5 files changed

Lines changed: 144 additions & 94 deletions

File tree

‎dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecorator.java‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package datadog.trace.bootstrap.instrumentation.decorator;
22

33
import static datadog.trace.api.UserEventTrackingMode.DISABLED;
4+
import static datadog.trace.api.UserEventTrackingMode.SAFE;
45

56
import datadog.trace.api.Config;
67
import datadog.trace.api.UserEventTrackingMode;
@@ -9,10 +10,17 @@
910
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
1011
import datadog.trace.bootstrap.instrumentation.api.Tags;
1112
import java.util.Map;
13+
import java.util.regex.Pattern;
1214
import javax.annotation.Nonnull;
1315

1416
public class AppSecUserEventDecorator {
1517

18+
private static final String NUMBER_PATTERN = "[0-9]+";
19+
private static final String UUID_PATTERN =
20+
"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}";
21+
private static final Pattern SAFE_USER_ID_PATTERN =
22+
Pattern.compile(NUMBER_PATTERN + "|" + UUID_PATTERN, Pattern.CASE_INSENSITIVE);
23+
1624
public boolean isEnabled() {
1725
if (!ActiveSubsystems.APPSEC_ACTIVE) {
1826
return false;
@@ -41,9 +49,7 @@ public void onLoginSuccess(String userId, Map<String, String> metadata) {
4149
return;
4250
}
4351

44-
if (userId != null) {
45-
segment.setTagTop("usr.id", userId);
46-
}
52+
onUserId(segment, "usr.id", userId);
4753
onEvent(segment, "users.login.success", metadata);
4854
}
4955

@@ -53,10 +59,7 @@ public void onLoginFailure(String userId, Map<String, String> metadata) {
5359
return;
5460
}
5561

56-
if (userId != null) {
57-
segment.setTagTop("appsec.events.users.login.failure.usr.id", userId);
58-
}
59-
62+
onUserId(segment, "appsec.events.users.login.failure.usr.id", userId);
6063
onEvent(segment, "users.login.failure", metadata);
6164
}
6265

@@ -66,9 +69,7 @@ public void onSignup(String userId, Map<String, String> metadata) {
6669
return;
6770
}
6871

69-
if (userId != null) {
70-
segment.setTagTop("usr.id", userId);
71-
}
72+
onUserId(segment, "usr.id", userId);
7273
onEvent(segment, "users.signup", metadata);
7374
}
7475

@@ -87,6 +88,18 @@ private void onEvent(@Nonnull TraceSegment segment, String eventName, Map<String
8788
}
8889
}
8990

91+
private void onUserId(final TraceSegment segment, final String tag, final String userId) {
92+
if (userId == null) {
93+
return;
94+
}
95+
UserEventTrackingMode mode = Config.get().getAppSecUserEventsTrackingMode();
96+
if (mode == SAFE && !SAFE_USER_ID_PATTERN.matcher(userId).matches()) {
97+
// do not set the user id if not numeric or UUID
98+
return;
99+
}
100+
segment.setTagTop(tag, userId);
101+
}
102+
90103
protected TraceSegment getSegment() {
91104
return AgentTracer.get().getTraceSegment();
92105
}

‎dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/AppSecUserEventDecoratorTest.groovy‎

Lines changed: 59 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
package datadog.trace.bootstrap.instrumentation.decorator
22

3+
import datadog.trace.api.Config
34
import datadog.trace.api.internal.TraceSegment
45
import datadog.trace.bootstrap.ActiveSubsystems
56
import datadog.trace.test.util.DDSpecification
67

8+
import static datadog.trace.api.UserEventTrackingMode.DISABLED
9+
import static datadog.trace.api.UserEventTrackingMode.EXTENDED
10+
import static datadog.trace.api.UserEventTrackingMode.SAFE
711
import static datadog.trace.api.config.AppSecConfig.APPSEC_AUTOMATED_USER_EVENTS_TRACKING
812

913
class AppSecUserEventDecoratorTest extends DDSpecification {
@@ -27,20 +31,26 @@ class AppSecUserEventDecoratorTest extends DDSpecification {
2731
def decorator = newDecorator()
2832

2933
when:
30-
decorator.onSignup('user', ['key1': 'value1', 'key2': 'value2'])
34+
decorator.onSignup(user, ['key1': 'value1', 'key2': 'value2'])
3135

3236
then:
33-
1 * traceSegment.setTagTop('_dd.appsec.events.users.signup.auto.mode', modeTag)
37+
1 * traceSegment.setTagTop('_dd.appsec.events.users.signup.auto.mode', mode)
3438
1 * traceSegment.setTagTop('appsec.events.users.signup.track', true, true)
3539
1 * traceSegment.setTagTop('asm.keep', true)
36-
1 * traceSegment.setTagTop('usr.id', 'user')
37-
1 * traceSegment.setTagTop('appsec.events.users.signup', ['key1':'value1', 'key2':'value2'])
40+
if (setUser) {
41+
1 * traceSegment.setTagTop('usr.id', user)
42+
}
43+
1 * traceSegment.setTagTop('appsec.events.users.signup', ['key1': 'value1', 'key2': 'value2'])
3844
0 * _
3945

4046
where:
41-
mode | modeTag
42-
'safe' | 'SAFE'
43-
'extended' | 'EXTENDED'
47+
mode | user | setUser
48+
'safe' | 'user' | false
49+
'safe' | '1234' | true
50+
'safe' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
51+
'extended' | 'user' | true
52+
'extended' | '1234' | true
53+
'extended' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
4454
}
4555

4656
def "test onLoginSuccess [#mode]"() {
@@ -49,44 +59,54 @@ class AppSecUserEventDecoratorTest extends DDSpecification {
4959
def decorator = newDecorator()
5060

5161
when:
52-
decorator.onLoginSuccess('user', ['key1': 'value1', 'key2': 'value2'])
62+
decorator.onLoginSuccess(user, ['key1': 'value1', 'key2': 'value2'])
5363

5464
then:
55-
1 * traceSegment.setTagTop('_dd.appsec.events.users.login.success.auto.mode', modeTag)
65+
1 * traceSegment.setTagTop('_dd.appsec.events.users.login.success.auto.mode', mode)
5666
1 * traceSegment.setTagTop('appsec.events.users.login.success.track', true, true)
5767
1 * traceSegment.setTagTop('asm.keep', true)
58-
1 * traceSegment.setTagTop('usr.id', 'user')
59-
1 * traceSegment.setTagTop('appsec.events.users.login.success', ['key1':'value1', 'key2':'value2'])
68+
if (setUser) {
69+
1 * traceSegment.setTagTop('usr.id', user)
70+
}
71+
1 * traceSegment.setTagTop('appsec.events.users.login.success', ['key1': 'value1', 'key2': 'value2'])
6072
0 * _
6173

6274
where:
63-
mode | modeTag
64-
'safe' | 'SAFE'
65-
'extended' | 'EXTENDED'
75+
mode | user | setUser
76+
'safe' | 'user' | false
77+
'safe' | '1234' | true
78+
'safe' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
79+
'extended' | 'user' | true
80+
'extended' | '1234' | true
81+
'extended' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
6682
}
6783

68-
def "test onLoginFailed #description [#mode]"() {
84+
def "test onLoginFailed [#mode]"() {
6985
setup:
7086
injectSysConfig(APPSEC_AUTOMATED_USER_EVENTS_TRACKING, mode)
7187
def decorator = newDecorator()
7288

7389
when:
74-
decorator.onLoginFailure('user', ['key1': 'value1', 'key2': 'value2'])
90+
decorator.onLoginFailure(user, ['key1': 'value1', 'key2': 'value2'])
7591

7692
then:
77-
1 * traceSegment.setTagTop('_dd.appsec.events.users.login.failure.auto.mode', modeTag)
93+
1 * traceSegment.setTagTop('_dd.appsec.events.users.login.failure.auto.mode', mode)
7894
1 * traceSegment.setTagTop('appsec.events.users.login.failure.track', true, true)
7995
1 * traceSegment.setTagTop('asm.keep', true)
80-
1 * traceSegment.setTagTop('appsec.events.users.login.failure.usr.id', 'user')
81-
1 * traceSegment.setTagTop('appsec.events.users.login.failure', ['key1':'value1', 'key2':'value2'])
96+
if (setUser) {
97+
1 * traceSegment.setTagTop('appsec.events.users.login.failure.usr.id', user)
98+
}
99+
1 * traceSegment.setTagTop('appsec.events.users.login.failure', ['key1': 'value1', 'key2': 'value2'])
82100
0 * _
83101

84102
where:
85-
mode | modeTag | description
86-
'safe' | 'SAFE' | 'with existing user'
87-
'safe' | 'SAFE' | 'user doesn\'t exist'
88-
'extended' | 'EXTENDED' | 'with existing user'
89-
'extended' | 'EXTENDED' | 'user doesn\'t exist'
103+
mode | user | setUser
104+
'safe' | 'user' | false
105+
'safe' | '1234' | true
106+
'safe' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
107+
'extended' | 'user' | true
108+
'extended' | '1234' | true
109+
'extended' | '591dc126-8431-4d0f-9509-b23318d3dce4' | true
90110
}
91111

92112
def "test onUserNotFound [#mode]"() {
@@ -102,9 +122,7 @@ class AppSecUserEventDecoratorTest extends DDSpecification {
102122
0 * _
103123

104124
where:
105-
mode | modeTag
106-
'safe' | 'SAFE'
107-
'extended' | 'EXTENDED'
125+
mode << ['safe', 'extended']
108126
}
109127

110128
def "test isEnabled (appsec = #appsec, mode = #mode)"() {
@@ -119,14 +137,21 @@ class AppSecUserEventDecoratorTest extends DDSpecification {
119137
then:
120138
enabled == result
121139
140+
and:
141+
Config.get().getAppSecUserEventsTrackingMode() == expectedMode
142+
122143
where:
123-
appsec | mode | result
124-
false | "disabled" | false
125-
false | "safe" | false
126-
false | "extended" | false
127-
true | "disabled" | false
128-
true | "safe" | true
129-
true | "extended" | true
144+
appsec | mode | result | expectedMode
145+
false | "disabled" | false | DISABLED
146+
false | "safe" | false | SAFE
147+
false | "1" | false | SAFE
148+
false | "true" | false | SAFE
149+
false | "extended" | false | EXTENDED
150+
true | "disabled" | false | DISABLED
151+
true | "safe" | true | SAFE
152+
true | "1" | true | SAFE
153+
true | "true" | true | SAFE
154+
true | "extended" | true | EXTENDED
130155
}
131156
132157
def newDecorator() {

‎dd-java-agent/instrumentation/spring-security-5/src/main/java17/datadog/trace/instrumentation/springsecurity5/SpringSecurityUserEventDecorator.java‎

Lines changed: 31 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,10 @@ public void onSignup(UserDetails user, Throwable throwable) {
4040
}
4141

4242
UserEventTrackingMode mode = Config.get().getAppSecUserEventsTrackingMode();
43-
String userId = null;
43+
String userId = user.getUsername();
4444
Map<String, String> metadata = null;
4545

46-
if (mode == EXTENDED && user != null) {
47-
userId = user.getUsername();
48-
46+
if (mode == EXTENDED) {
4947
metadata = new HashMap<>();
5048
metadata.put("enabled", String.valueOf(user.isEnabled()));
5149
metadata.put(
@@ -71,44 +69,36 @@ public void onLogin(Authentication authentication, Throwable throwable, Authenti
7169
return;
7270
}
7371

74-
String userId = null;
75-
76-
if (mode == EXTENDED) {
77-
userId = authentication.getName();
78-
}
79-
80-
if (mode != DISABLED) {
81-
if (throwable == null && result != null && result.isAuthenticated()) {
82-
Map<String, String> metadata = null;
83-
Object principal = result.getPrincipal();
84-
if (principal instanceof User) {
85-
User user = (User) principal;
86-
metadata = new HashMap<>();
87-
metadata.put("enabled", String.valueOf(user.isEnabled()));
88-
metadata.put(
89-
"authorities",
90-
user.getAuthorities().stream()
91-
.map(Object::toString)
92-
.collect(Collectors.joining(",")));
93-
metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired()));
94-
metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked()));
95-
metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired()));
96-
}
97-
98-
onLoginSuccess(userId, metadata);
99-
} else if (throwable != null) {
100-
// On bad password, throwable would be
101-
// org.springframework.security.authentication.BadCredentialsException,
102-
// on user not found, throwable can be BadCredentials or
103-
// org.springframework.security.core.userdetails.UsernameNotFoundException depending on the
104-
// internal setting
105-
// hideUserNotFoundExceptions (or a custom AuthenticationProvider implementation overriding
106-
// this).
107-
// This would be the ideal place to check whether the user exists or not, but we cannot do
108-
// so reliably yet.
109-
// See UsernameNotFoundExceptionInstrumentation for more details.
110-
onLoginFailure(userId, null);
72+
String userId = result != null ? result.getName() : authentication.getName();
73+
74+
if (throwable == null && result != null && result.isAuthenticated()) {
75+
Map<String, String> metadata = null;
76+
Object principal = result.getPrincipal();
77+
if (principal instanceof User) {
78+
User user = (User) principal;
79+
metadata = new HashMap<>();
80+
metadata.put("enabled", String.valueOf(user.isEnabled()));
81+
metadata.put(
82+
"authorities",
83+
user.getAuthorities().stream().map(Object::toString).collect(Collectors.joining(",")));
84+
metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired()));
85+
metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked()));
86+
metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired()));
11187
}
88+
89+
onLoginSuccess(userId, metadata);
90+
} else if (throwable != null) {
91+
// On bad password, throwable would be
92+
// org.springframework.security.authentication.BadCredentialsException,
93+
// on user not found, throwable can be BadCredentials or
94+
// org.springframework.security.core.userdetails.UsernameNotFoundException depending on the
95+
// internal setting
96+
// hideUserNotFoundExceptions (or a custom AuthenticationProvider implementation overriding
97+
// this).
98+
// This would be the ideal place to check whether the user exists or not, but we cannot do
99+
// so reliably yet.
100+
// See UsernameNotFoundExceptionInstrumentation for more details.
101+
onLoginFailure(userId, null);
112102
}
113103
}
114104

‎dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
124124
response.body().string() == REGISTER.body
125125
!span.getTags().isEmpty()
126126
span.getTag("appsec.events.users.signup.track") == true
127-
span.getTag("_dd.appsec.events.users.signup.auto.mode") == 'EXTENDED'
127+
span.getTag("_dd.appsec.events.users.signup.auto.mode") == 'extended'
128128
span.getTag("usr.id") == 'admin'
129129
span.getTag("appsec.events.users.signup")['enabled'] == 'true'
130130
span.getTag("appsec.events.users.signup")['authorities'] == 'ROLE_USER'
@@ -150,7 +150,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
150150
response.body().string() == LOGIN.body
151151
!span.getTags().isEmpty()
152152
span.getTag("appsec.events.users.login.failure.track") == true
153-
span.getTag("_dd.appsec.events.users.login.failure.auto.mode") == 'EXTENDED'
153+
span.getTag("_dd.appsec.events.users.login.failure.auto.mode") == 'extended'
154154
span.getTag("appsec.events.users.login.failure.usr.exists") == false
155155
span.getTag("appsec.events.users.login.failure.usr.id") == 'not_existing_user'
156156
}
@@ -174,7 +174,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
174174
response.body().string() == LOGIN.body
175175
!span.getTags().isEmpty()
176176
span.getTag("appsec.events.users.login.failure.track") == true
177-
span.getTag("_dd.appsec.events.users.login.failure.auto.mode") == 'EXTENDED'
177+
span.getTag("_dd.appsec.events.users.login.failure.auto.mode") == 'extended'
178178
// TODO: Ideally should be `false` but we have no reliable method to detect it it is just absent. See APPSEC-12765.
179179
span.getTag("appsec.events.users.login.failure.usr.exists") == null
180180
span.getTag("appsec.events.users.login.failure.usr.id") == 'admin'
@@ -200,7 +200,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
200200
response.body().string() == LOGIN.body
201201
!span.getTags().isEmpty()
202202
span.getTag("appsec.events.users.login.success.track") == true
203-
span.getTag("_dd.appsec.events.users.login.success.auto.mode") == 'EXTENDED'
203+
span.getTag("_dd.appsec.events.users.login.success.auto.mode") == 'extended'
204204
span.getTag("usr.id") == 'admin'
205205
span.getTag("appsec.events.users.login.success")['credentialsNonExpired'] == 'true'
206206
span.getTag("appsec.events.users.login.success")['accountNonExpired'] == 'true'

0 commit comments

Comments
 (0)