Skip to content

Commit 82417c0

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
committed
Fix system-test tests due to naming and add new missing user id metric
1 parent 23f30ad commit 82417c0

File tree

8 files changed

+135
-54
lines changed

8 files changed

+135
-54
lines changed

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -234,10 +234,6 @@ private Flow<Void> onLoginEvent(
234234
if (mode == DISABLED) {
235235
return NoopFlow.INSTANCE;
236236
}
237-
final String user = anonymizeUser(mode, originalUser);
238-
if (user == null) {
239-
return NoopFlow.INSTANCE;
240-
}
241237
final AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC);
242238
if (ctx == null) {
243239
return NoopFlow.INSTANCE;
@@ -248,29 +244,38 @@ private Flow<Void> onLoginEvent(
248244
segment.setTagTop(Tags.ASM_KEEP, true);
249245
segment.setTagTop(Tags.PROPAGATED_APPSEC, true);
250246

247+
// update span tags
248+
segment.setTagTop("appsec.events." + eventName + ".track", true, true);
249+
if (exists != null) {
250+
segment.setTagTop("appsec.events." + eventName + ".usr.exists", exists, true);
251+
}
252+
if (metadata != null && !metadata.isEmpty()) {
253+
segment.setTagTop("appsec.events." + eventName, metadata, true);
254+
}
255+
if (mode == SDK) {
256+
segment.setTagTop("_dd.appsec.events." + eventName + ".sdk", true, true);
257+
} else {
258+
segment.setTagTop("_dd.appsec.events." + eventName + ".auto.mode", mode.fullName(), true);
259+
}
260+
261+
final String user = anonymizeUser(mode, originalUser);
262+
if (user == null) {
263+
// can happen in custom events
264+
return NoopFlow.INSTANCE;
265+
}
266+
251267
// skip event if we have an SDK one
252268
if (mode != SDK) {
253269
segment.setTagTop("_dd.appsec.usr.login", user);
254270
segment.setTagTop("_dd.appsec.usr.id", user);
255-
segment.setTagTop(
256-
"_dd.appsec.events.users." + eventName + ".auto.mode", mode.fullName(), true);
257271
if (ctx.getUserLoginSource() == SDK) {
258272
return NoopFlow.INSTANCE;
259273
}
260-
} else {
261-
segment.setTagTop("_dd.appsec.events.users." + eventName + ".sdk", true, true);
262274
}
263275

264-
// update span tags
265-
segment.setTagTop("appsec.events.users." + eventName + ".usr.login", user, true);
266-
segment.setTagTop("appsec.events.users." + eventName + ".usr.id", user, true);
267-
segment.setTagTop("appsec.events.users." + eventName + ".track", true, true);
268-
if (exists != null) {
269-
segment.setTagTop("appsec.events.users." + eventName + ".usr.exists", exists, true);
270-
}
271-
if (metadata != null && !metadata.isEmpty()) {
272-
segment.setTagTop("appsec.events.users." + eventName, metadata, true);
273-
}
276+
// update user span tags
277+
segment.setTagTop("appsec.events." + eventName + ".usr.login", user, true);
278+
segment.setTagTop("appsec.events." + eventName + ".usr.id", user, true);
274279

275280
// update current context with new user login
276281
ctx.setUserLoginSource(mode);
@@ -284,9 +289,9 @@ private Flow<Void> onLoginEvent(
284289
final List<Address<?>> addresses = new ArrayList<>(3);
285290
addresses.add(KnownAddresses.USER_LOGIN);
286291
addresses.add(KnownAddresses.USER_ID);
287-
if (KnownAddresses.LOGIN_SUCCESS.getKey().endsWith(eventName)) {
292+
if (eventName.endsWith("login.success")) {
288293
addresses.add(KnownAddresses.LOGIN_SUCCESS);
289-
} else if (KnownAddresses.LOGIN_FAILURE.getKey().endsWith(eventName)) {
294+
} else if (eventName.endsWith("login.failure")) {
290295
addresses.add(KnownAddresses.LOGIN_FAILURE);
291296
}
292297
final MapDataBundle.Builder bundleBuilder =

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,7 +1092,7 @@ class GatewayBridgeSpecification extends DDSpecification {
10921092
eventDispatcher.getDataSubscribers(_) >> nonEmptyDsInfo
10931093
10941094
when:
1095-
loginEventCB.apply(ctx, mode, 'signup', null, USER_ID, metadata)
1095+
loginEventCB.apply(ctx, mode, 'users.signup', null, USER_ID, metadata)
10961096
10971097
then:
10981098
if (mode == DISABLED) {
@@ -1130,7 +1130,7 @@ class GatewayBridgeSpecification extends DDSpecification {
11301130
eventDispatcher.getDataSubscribers(_) >> nonEmptyDsInfo
11311131
11321132
when:
1133-
loginEventCB.apply(ctx, mode, 'login.success', null, USER_ID, metadata)
1133+
loginEventCB.apply(ctx, mode, 'users.login.success', null, USER_ID, metadata)
11341134
11351135
then:
11361136
if (mode == DISABLED) {
@@ -1169,7 +1169,7 @@ class GatewayBridgeSpecification extends DDSpecification {
11691169
eventDispatcher.getDataSubscribers(_) >> nonEmptyDsInfo
11701170
11711171
when:
1172-
loginEventCB.apply(ctx, mode, 'login.failure', false, USER_ID, metadata)
1172+
loginEventCB.apply(ctx, mode, 'users.login.failure', false, USER_ID, metadata)
11731173
11741174
then:
11751175
if (mode == DISABLED) {
@@ -1202,6 +1202,23 @@ class GatewayBridgeSpecification extends DDSpecification {
12021202
mode << UserIdCollectionMode.values()
12031203
}
12041204
1205+
void "test onCustomEvent (#mode)"() {
1206+
setup:
1207+
final metadata = ['key1': 'value1', 'key2': 'value2']
1208+
eventDispatcher.getDataSubscribers(_) >> nonEmptyDsInfo
1209+
1210+
when:
1211+
loginEventCB.apply(ctx, SDK, 'my.event', null, null, metadata)
1212+
1213+
then:
1214+
1 * traceSegment.setTagTop('_dd.appsec.events.my.event.sdk', true, true)
1215+
1 * traceSegment.setTagTop('appsec.events.my.event.track', true, true)
1216+
1 * traceSegment.setTagTop('appsec.events.my.event', ['key1': 'value1', 'key2': 'value2'], true)
1217+
1 * traceSegment.setTagTop('asm.keep', true)
1218+
1 * traceSegment.setTagTop('_dd.p.appsec', true)
1219+
0 * eventDispatcher.publishDataEvent
1220+
}
1221+
12051222
void "test onUserEvent (automated login events should not overwrite SDK)"() {
12061223
setup:
12071224
final firstUser = 'first-user'
@@ -1234,7 +1251,7 @@ class GatewayBridgeSpecification extends DDSpecification {
12341251
eventDispatcher.getDataSubscribers(_) >> nonEmptyDsInfo
12351252
12361253
when:
1237-
loginEventCB.apply(ctx, SDK, 'login.success', null, firstUser, null)
1254+
loginEventCB.apply(ctx, SDK, 'users.login.success', null, firstUser, null)
12381255
12391256
then:
12401257
1 * traceSegment.setTagTop('appsec.events.users.login.success.usr.login', firstUser, true)
@@ -1245,7 +1262,7 @@ class GatewayBridgeSpecification extends DDSpecification {
12451262
1 * eventDispatcher.publishDataEvent(nonEmptyDsInfo, ctx.data, _ as DataBundle, _ as GatewayContext) >> NoopFlow.INSTANCE
12461263
12471264
when:
1248-
loginEventCB.apply(ctx, IDENTIFICATION, 'login.success', null, secondUser, null)
1265+
loginEventCB.apply(ctx, IDENTIFICATION, 'users.login.success', null, secondUser, null)
12491266
12501267
then:
12511268
0 * traceSegment.setTagTop('appsec.events.users.login.success.usr.login', _, _)

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
7676
GlobalTracer.getEventTracker().trackLoginSuccessEvent('user1', ['key1': 'value1', 'key2': 'value2'])
7777
7878
then:
79-
1 * loginEvent.apply(_ as RequestContext, SDK, 'login.success', null, 'user1', ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
79+
1 * loginEvent.apply(_ as RequestContext, SDK, 'users.login.success', null, 'user1', ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
8080
0 * _
8181
}
8282
@@ -85,7 +85,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
8585
GlobalTracer.getEventTracker().trackLoginFailureEvent('user1', true, ['key1': 'value1', 'key2': 'value2'])
8686
8787
then:
88-
1 * loginEvent.apply(_ as RequestContext, SDK, 'login.failure', true, 'user1', ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
88+
1 * loginEvent.apply(_ as RequestContext, SDK, 'users.login.failure', true, 'user1', ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
8989
0 * _
9090
}
9191
@@ -142,7 +142,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
142142
143143
then:
144144
if (mode != DISABLED) {
145-
1 * loginEvent.apply(_ as RequestContext, mode, 'signup', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
145+
1 * loginEvent.apply(_ as RequestContext, mode, 'users.signup', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
146146
}
147147
0 * _
148148
@@ -156,7 +156,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
156156
157157
then:
158158
if (mode != DISABLED) {
159-
1 * loginEvent.apply(_ as RequestContext, mode, 'login.success', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
159+
1 * loginEvent.apply(_ as RequestContext, mode, 'users.login.success', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
160160
}
161161
0 * _
162162
@@ -170,7 +170,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
170170
171171
then:
172172
if (mode != DISABLED) {
173-
1 * loginEvent.apply(_ as RequestContext, mode, 'login.failure', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
173+
1 * loginEvent.apply(_ as RequestContext, mode, 'users.login.failure', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> NoopFlow.INSTANCE
174174
}
175175
0 * _
176176
@@ -259,7 +259,7 @@ class AppSecEventTrackerSpecification extends DDSpecification {
259259
void 'test blocking on a userId'() {
260260
setup:
261261
final action = new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO)
262-
loginEvent.apply(_ as RequestContext, SDK, 'login.success', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> new ActionFlow<Void>(action: action)
262+
loginEvent.apply(_ as RequestContext, SDK, 'users.login.success', null, USER_ID, ['key1': 'value1', 'key2': 'value2']) >> new ActionFlow<Void>(action: action)
263263
264264
when:
265265
tracker.onLoginSuccessEvent(SDK, USER_ID, ['key1': 'value1', 'key2': 'value2'])

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,9 @@ public void onUser(final Authentication authentication) {
138138

139139
UserIdCollectionMode mode = UserIdCollectionMode.get();
140140
String username = authentication.getName();
141+
if (missingUserId(username)) {
142+
return;
143+
}
141144
tracker.onUserEvent(mode, username);
142145
}
143146

@@ -163,6 +166,14 @@ private static String findRootAuthentication(Class<?> authentication) {
163166
return Authentication.class.getName(); // set this a default for really custom impls
164167
}
165168

169+
private static boolean missingUserId(final String username) {
170+
if (username == null || username.isEmpty()) {
171+
WafMetricCollector.get().missingUserId(LoginFramework.SPRING_SECURITY);
172+
return true;
173+
}
174+
return false;
175+
}
176+
166177
private static boolean missingUsername(final String username, final LoginEvent event) {
167178
if (username == null || username.isEmpty()) {
168179
WafMetricCollector.get().missingUserLogin(LoginFramework.SPRING_SECURITY, event);

‎internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
import static datadog.trace.api.UserIdCollectionMode.DISABLED;
44
import static datadog.trace.api.UserIdCollectionMode.SDK;
55
import static datadog.trace.api.gateway.Events.EVENTS;
6-
import static datadog.trace.api.telemetry.LoginEvent.LOGIN_FAILURE;
7-
import static datadog.trace.api.telemetry.LoginEvent.LOGIN_SUCCESS;
8-
import static datadog.trace.api.telemetry.LoginEvent.SIGN_UP;
96

107
import datadog.appsec.api.blocking.BlockingException;
118
import datadog.trace.api.EventTracker;
@@ -91,7 +88,7 @@ public void onSignupEvent(
9188
}
9289
dispatch(
9390
EVENTS.loginEvent(),
94-
(ctx, callback) -> callback.apply(ctx, mode, SIGN_UP.getSpanTag(), null, userId, metadata));
91+
(ctx, callback) -> callback.apply(ctx, mode, "users.signup", null, userId, metadata));
9592
}
9693

9794
public void onLoginSuccessEvent(
@@ -101,7 +98,7 @@ public void onLoginSuccessEvent(
10198
}
10299
dispatch(
103100
EVENTS.loginEvent(),
104-
(ctx, cb) -> cb.apply(ctx, mode, LOGIN_SUCCESS.getSpanTag(), null, userId, metadata));
101+
(ctx, cb) -> cb.apply(ctx, mode, "users.login.success", null, userId, metadata));
105102
}
106103

107104
public void onLoginFailureEvent(
@@ -114,7 +111,7 @@ public void onLoginFailureEvent(
114111
}
115112
dispatch(
116113
EVENTS.loginEvent(),
117-
(ctx, cb) -> cb.apply(ctx, mode, LOGIN_FAILURE.getSpanTag(), exists, userId, metadata));
114+
(ctx, cb) -> cb.apply(ctx, mode, "users.login.failure", exists, userId, metadata));
118115
}
119116

120117
public void onCustomEvent(

‎internal-api/src/main/java/datadog/trace/api/telemetry/LoginEvent.java

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,20 @@
11
package datadog.trace.api.telemetry;
22

33
public enum LoginEvent {
4-
LOGIN_SUCCESS("login.success", "login_success"),
5-
LOGIN_FAILURE("login.failure", "login_failure"),
4+
LOGIN_SUCCESS("login_success"),
5+
LOGIN_FAILURE("login_failure"),
66
SIGN_UP("signup");
77

88
private static final int numValues = LoginEvent.values().length;
99

10-
private final String spanTag;
11-
private final String telemetryTag;
10+
private final String tag;
1211

1312
LoginEvent(final String tag) {
14-
this(tag, tag);
13+
this.tag = tag;
1514
}
1615

17-
LoginEvent(final String spanTag, final String telemetryTag) {
18-
this.spanTag = spanTag;
19-
this.telemetryTag = telemetryTag;
20-
}
21-
22-
public String getTelemetryTag() {
23-
return telemetryTag;
24-
}
25-
26-
public String getSpanTag() {
27-
return spanTag;
16+
public String getTag() {
17+
return tag;
2818
}
2919

3020
public static int getNumValues() {

‎internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,12 @@ private WafMetricCollector() {
4242
new AtomicLongArray(RuleType.getNumValues());
4343
private static final AtomicLongArray missingUserLoginQueue =
4444
new AtomicLongArray(LoginFramework.getNumValues() * LoginEvent.getNumValues());
45+
private static final AtomicLongArray missingUserIdQueue =
46+
new AtomicLongArray(LoginFramework.getNumValues());
4547

4648
/** WAF version that will be initialized with wafInit and reused for all metrics. */
4749
private static String wafVersion = "";
50+
4851
/**
4952
* Rules version that will be updated on each wafInit and wafUpdates. This is not entirely
5053
* accurate, since wafRequest metrics might be collected for a period where a rules update happens
@@ -105,6 +108,10 @@ public void missingUserLogin(final LoginFramework framework, final LoginEvent ev
105108
framework.ordinal() * LoginEvent.getNumValues() + eventType.ordinal());
106109
}
107110

111+
public void missingUserId(final LoginFramework framework) {
112+
missingUserLoginQueue.incrementAndGet(framework.ordinal());
113+
}
114+
108115
@Override
109116
public Collection<WafMetric> drain() {
110117
if (!rawMetricsQueue.isEmpty()) {
@@ -215,7 +222,7 @@ public void prepareMetrics() {
215222
long counter = missingUserLoginQueue.getAndSet(ordinal, 0);
216223
if (counter > 0) {
217224
if (!rawMetricsQueue.offer(
218-
new MissingUserLoginMetric(counter, framework.getTag(), event.getTelemetryTag()))) {
225+
new MissingUserLoginMetric(counter, framework.getTag(), event.getTag()))) {
219226
return;
220227
}
221228
}
@@ -260,6 +267,17 @@ public MissingUserLoginMetric(long counter, String framework, String type) {
260267
}
261268
}
262269

270+
public static class MissingUserIdMetric extends WafMetric {
271+
272+
public MissingUserIdMetric(long counter, String framework) {
273+
super(
274+
"instrum.user_auth.missing_user_id",
275+
counter,
276+
"framework:" + framework,
277+
"event_type:authenticated_request");
278+
}
279+
}
280+
263281
public static class WafRequestsRawMetric extends WafMetric {
264282
public WafRequestsRawMetric(
265283
final long counter,

‎internal-api/src/test/groovy/datadog/trace/api/telemetry/WafMetricCollectorTest.groovy

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,49 @@ class WafMetricCollectorTest extends DDSpecification {
253253
}
254254
}
255255

256+
void 'test missing user id event metric'() {
257+
given:
258+
def collector = WafMetricCollector.get()
259+
final count = 6
260+
final latch = new CountDownLatch(1)
261+
final executors = Executors.newFixedThreadPool(4)
262+
final action = { LoginFramework framework ->
263+
latch.await()
264+
collector.missingUserId(framework)
265+
}
266+
267+
when:
268+
(1..count).each {
269+
executors.submit {
270+
action.call(LoginFramework.SPRING_SECURITY)
271+
}
272+
}
273+
274+
latch.countDown()
275+
executors.shutdown()
276+
final finished = executors.awaitTermination(5, TimeUnit.SECONDS)
277+
278+
then:
279+
finished
280+
collector.prepareMetrics()
281+
final drained = collector.drain()
282+
final metrics = drained.findAll {
283+
it.metricName == 'instrum.user_auth.missing_user_id'
284+
}
285+
metrics.size() == 1
286+
metrics.forEach { metric ->
287+
assert metric.namespace == 'appsec'
288+
assert metric.type == 'count'
289+
assert metric.value == count
290+
final tags = metric.tags.collectEntries {
291+
final parts = it.split(":")
292+
return [(parts[0]): parts[1]]
293+
}
294+
assert tags["framework"] == LoginFramework.SPRING_SECURITY.getTag()
295+
assert tags["event_type"] == "authenticated_request"
296+
}
297+
}
298+
256299
def "test Rasp #ruleType metrics"() {
257300
when:
258301
WafMetricCollector.get().wafInit('waf_ver1', 'rules.1')

0 commit comments

Comments
 (0)