From 143305e3ff6f582157182a557056bbf6bf90f373 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Wed, 30 Dec 2020 14:15:21 +0100 Subject: [PATCH 1/4] Send user.ip_address = {{ auto }} when sendDefaultPii is true --- CHANGELOG.md | 1 + .../java/io/sentry/MainEventProcessor.java | 16 +++++++++ .../java/io/sentry/MainEventProcessorTest.kt | 36 ++++++++++++++++++- 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9c50978838..c26231f7c95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * Fix: Initialize Logback after context refreshes (#1129) * Ref: Return NoOpTransaction instead of null (#1126) * Fix: Do not crash when passing null values to @Nullable methods, eg User and Scope +* Enhancement: Send user.ip_address = {{ auto }} when sendDefaultPii is true # 4.0.0-alpha.2 diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index c260c67301c..703d8588fef 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -1,6 +1,7 @@ package io.sentry; import io.sentry.protocol.SentryException; +import io.sentry.protocol.User; import io.sentry.util.ApplyScopeUtils; import io.sentry.util.Objects; import java.util.ArrayList; @@ -19,6 +20,12 @@ public final class MainEventProcessor implements EventProcessor { */ private static final String DEFAULT_ENVIRONMENT = "production"; + /** + * Default value for {@link User#getIpAddress()} set when event does not have user and ip address + * set and when {@link SentryOptions#isSendDefaultPii()} is set to true. + */ + private static final String DEFAULT_IP_ADDRESS = "{{auto}}"; + private final @NotNull SentryOptions options; private final @NotNull SentryThreadFactory sentryThreadFactory; private final @NotNull SentryExceptionFactory sentryExceptionFactory; @@ -123,5 +130,14 @@ private void processNonCachedEvent(final @NotNull SentryEvent event) { event.setThreads(sentryThreadFactory.getCurrentThread()); } } + if (options.isSendDefaultPii()) { + if (event.getUser() == null) { + final User user = new User(); + user.setIpAddress(DEFAULT_IP_ADDRESS); + event.setUser(user); + } else if (event.getUser().getIpAddress() == null) { + event.getUser().setIpAddress(DEFAULT_IP_ADDRESS); + } + } } } diff --git a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index 5481d482aa1..ca898018bc9 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -4,6 +4,7 @@ import com.nhaarman.mockitokotlin2.mock import io.sentry.hints.ApplyScopeData import io.sentry.hints.Cached import io.sentry.protocol.SdkVersion +import io.sentry.protocol.User import java.lang.RuntimeException import kotlin.test.Test import kotlin.test.assertEquals @@ -25,10 +26,13 @@ class MainEventProcessorTest { version = "1.2.3" } } - fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map = emptyMap()): MainEventProcessor { + fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map = emptyMap(), sendDefaultPii: Boolean? = null): MainEventProcessor { sentryOptions.isAttachThreads = attachThreads sentryOptions.isAttachStacktrace = attachStackTrace sentryOptions.environment = environment + if (sendDefaultPii != null) { + sentryOptions.isSendDefaultPii = sendDefaultPii + } tags.forEach { sentryOptions.setTag(it.key, it.value) } return MainEventProcessor(sentryOptions) } @@ -191,6 +195,36 @@ class MainEventProcessorTest { assertEquals("production", event.environment) } + @Test + fun `when event does not have ip address set and sendDefaultPii is set to true, sets "{{auto}}" as the ip address`() { + val sut = fixture.getSut(sendDefaultPii = true) + val event = SentryEvent() + sut.process(event, null) + assertNotNull(event.user) + assertEquals("{{auto}}", event.user.ipAddress) + } + + @Test + fun `when event has ip address set and sendDefaultPii is set to true, keeps original ip address`() { + val sut = fixture.getSut(sendDefaultPii = true) + val event = SentryEvent() + event.user = User() + event.user.ipAddress = "192.168.0.1" + sut.process(event, null) + assertNotNull(event.user) + assertEquals("192.168.0.1", event.user.ipAddress) + } + + @Test + fun `when event does not have ip address set and sendDefaultPii is set to false, does not set ip address`() { + val sut = fixture.getSut(sendDefaultPii = false) + val event = SentryEvent() + event.user = User() + sut.process(event, null) + assertNotNull(event.user) + assertNull(event.user.ipAddress) + } + @Test fun `when event has environment set, does not overwrite environment`() { val sut = fixture.getSut(environment = null) From b560520cad4a25c9aafd41a0a600a314ec915113 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 4 Jan 2021 21:05:16 +0100 Subject: [PATCH 2/4] Unset default ip address in Spring integration. --- .../spring/boot/SentryAutoConfiguration.java | 2 +- sentry-spring/api/sentry-spring.api | 2 +- .../spring/SentryInitBeanPostProcessor.java | 2 +- .../SentryUserProviderEventProcessor.java | 15 +++- .../SentryUserProviderEventProcessorTest.kt | 73 +++++++++++++++++-- sentry/api/sentry.api | 1 + .../java/io/sentry/MainEventProcessor.java | 12 +-- 7 files changed, 91 insertions(+), 16 deletions(-) diff --git a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java index 6f185053da1..d33fc58f4d6 100644 --- a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java +++ b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java @@ -80,7 +80,7 @@ static class HubConfiguration { sentryUserProviders.forEach( sentryUserProvider -> options.addEventProcessor( - new SentryUserProviderEventProcessor(sentryUserProvider))); + new SentryUserProviderEventProcessor(options, sentryUserProvider))); transportGate.ifAvailable(options::setTransportGate); transport.ifAvailable(options::setTransport); inAppPackagesResolver.resolveInAppIncludes().forEach(options::addInAppInclude); diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api index fa588a9076c..ddadac07274 100644 --- a/sentry-spring/api/sentry-spring.api +++ b/sentry-spring/api/sentry-spring.api @@ -53,7 +53,7 @@ public abstract interface class io/sentry/spring/SentryUserProvider { } public final class io/sentry/spring/SentryUserProviderEventProcessor : io/sentry/EventProcessor { - public fun (Lio/sentry/spring/SentryUserProvider;)V + public fun (Lio/sentry/SentryOptions;Lio/sentry/spring/SentryUserProvider;)V public fun getSentryUserProvider ()Lio/sentry/spring/SentryUserProvider; public fun process (Lio/sentry/SentryEvent;Ljava/lang/Object;)Lio/sentry/SentryEvent; } diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java index e7a0e51d18a..2feded61580 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java @@ -28,7 +28,7 @@ public Object postProcessAfterInitialization( .forEach( sentryUserProvider -> options.addEventProcessor( - new SentryUserProviderEventProcessor(sentryUserProvider))); + new SentryUserProviderEventProcessor(options, sentryUserProvider))); } Sentry.init(options); } diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryUserProviderEventProcessor.java b/sentry-spring/src/main/java/io/sentry/spring/SentryUserProviderEventProcessor.java index f52c48b4af6..f44a17ffda3 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryUserProviderEventProcessor.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryUserProviderEventProcessor.java @@ -1,7 +1,9 @@ package io.sentry.spring; import io.sentry.EventProcessor; +import io.sentry.MainEventProcessor; import io.sentry.SentryEvent; +import io.sentry.SentryOptions; import io.sentry.protocol.User; import io.sentry.util.Objects; import java.util.Map; @@ -12,9 +14,12 @@ import org.jetbrains.annotations.Nullable; public final class SentryUserProviderEventProcessor implements EventProcessor { + private final @NotNull SentryOptions options; private final @NotNull SentryUserProvider sentryUserProvider; - public SentryUserProviderEventProcessor(final @NotNull SentryUserProvider sentryUserProvider) { + public SentryUserProviderEventProcessor( + final @NotNull SentryOptions options, final @NotNull SentryUserProvider sentryUserProvider) { + this.options = Objects.requireNonNull(options, "options is required"); this.sentryUserProvider = Objects.requireNonNull(sentryUserProvider, "sentryUserProvider is required"); } @@ -39,6 +44,14 @@ public SentryEvent process(final @NotNull SentryEvent event, final @Nullable Obj } event.setUser(existingUser); } + if (options.isSendDefaultPii()) { + final User existingUser = event.getUser(); + if (existingUser != null + && MainEventProcessor.DEFAULT_IP_ADDRESS.equals(existingUser.getIpAddress())) { + // unset {{auto}} as it would set the server's ip address as a user ip address + existingUser.setIpAddress(null); + } + } return event; } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt index 5694f52ab3d..94a5a1f10b8 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt @@ -1,17 +1,29 @@ package io.sentry.spring import io.sentry.SentryEvent +import io.sentry.SentryOptions import io.sentry.protocol.User import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull +import kotlin.test.assertNull class SentryUserProviderEventProcessorTest { + class Fixture { + + fun getSut(isSendDefaultPii: Boolean = false, userProvider: () -> User?): SentryUserProviderEventProcessor { + val options = SentryOptions() + options.isSendDefaultPii = isSendDefaultPii + return SentryUserProviderEventProcessor(options, userProvider) + } + } + + private val fixture = Fixture() + @Test fun `when event user is null, provider user data is set`() { - - val processor = SentryUserProviderEventProcessor { + val processor = fixture.getSut { val user = User() user.username = "john.doe" user.id = "user-id" @@ -35,7 +47,7 @@ class SentryUserProviderEventProcessorTest { @Test fun `when event user is empty, provider user data is set`() { - val processor = SentryUserProviderEventProcessor { + val processor = fixture.getSut { val user = User() user.username = "john.doe" user.id = "user-id" @@ -60,7 +72,7 @@ class SentryUserProviderEventProcessorTest { @Test fun `when processor returns empty User, user data is not changed`() { - val processor = SentryUserProviderEventProcessor { + val processor = fixture.getSut { val user = User() user } @@ -86,7 +98,7 @@ class SentryUserProviderEventProcessorTest { @Test fun `when processor returns null, user data is not changed`() { - val processor = SentryUserProviderEventProcessor { + val processor = fixture.getSut { null } @@ -111,7 +123,7 @@ class SentryUserProviderEventProcessorTest { @Test fun `merges user#others with existing user#others set on SentryEvent`() { - val processor = SentryUserProviderEventProcessor { + val processor = fixture.getSut { val user = User() user.others = mapOf("key" to "value") user @@ -127,4 +139,53 @@ class SentryUserProviderEventProcessorTest { assertNotNull(result.user) assertEquals(mapOf("key" to "value", "new-key" to "new-value"), result.user.others) } + + @Test + fun `when isSendDefaultPii is true and user is not set, user remains null`() { + val processor = fixture.getSut(isSendDefaultPii = true) { + null + } + + val event = SentryEvent() + event.user = null + + val result = processor.process(event, null) + + assertNotNull(result) + assertNull(result.user) + } + + @Test + fun `when isSendDefaultPii is true and user is set with custom ip address, user ip is unchanged`() { + val processor = fixture.getSut(isSendDefaultPii = true) { + null + } + + val event = SentryEvent() + val user = User() + user.ipAddress = "192.168.0.1" + event.user = user + + val result = processor.process(event, null) + + assertNotNull(result) + assertEquals(user.ipAddress, result.user.ipAddress) + } + + @Test + fun `when isSendDefaultPii is true and user is set with {{auto}} ip address, user ip is set to null`() { + val processor = fixture.getSut(isSendDefaultPii = true) { + null + } + + val event = SentryEvent() + val user = User() + user.ipAddress = "{{auto}}" + event.user = user + + val result = processor.process(event, null) + + assertNotNull(result) + assertNull(result.user.ipAddress) + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index fc39d98b396..21a128609d9 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -311,6 +311,7 @@ public abstract interface class io/sentry/Integration { } public final class io/sentry/MainEventProcessor : io/sentry/EventProcessor { + public static final field DEFAULT_IP_ADDRESS Ljava/lang/String; public fun process (Lio/sentry/SentryEvent;Ljava/lang/Object;)Lio/sentry/SentryEvent; } diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index 703d8588fef..6440a88ba88 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -15,16 +15,16 @@ public final class MainEventProcessor implements EventProcessor { /** - * Default value for {@link SentryEvent#getEnvironment()} set when both event and {@link - * SentryOptions} do not have the environment field set. + * Default value for {@link User#getIpAddress()} set when event does not have user and ip address + * set and when {@link SentryOptions#isSendDefaultPii()} is set to true. */ - private static final String DEFAULT_ENVIRONMENT = "production"; + public static final String DEFAULT_IP_ADDRESS = "{{auto}}"; /** - * Default value for {@link User#getIpAddress()} set when event does not have user and ip address - * set and when {@link SentryOptions#isSendDefaultPii()} is set to true. + * Default value for {@link SentryEvent#getEnvironment()} set when both event and {@link + * SentryOptions} do not have the environment field set. */ - private static final String DEFAULT_IP_ADDRESS = "{{auto}}"; + private static final String DEFAULT_ENVIRONMENT = "production"; private final @NotNull SentryOptions options; private final @NotNull SentryThreadFactory sentryThreadFactory; From 37735b86168838533b290ad38290a86bff4f8031 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 4 Jan 2021 21:05:47 +0100 Subject: [PATCH 3/4] Set default user id if user without id is set on the event. --- .../core/DefaultAndroidEventProcessor.java | 5 ++++- .../core/DefaultAndroidEventProcessorTest.kt | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java index 76ded9ff373..0f19ac1aada 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java @@ -149,8 +149,11 @@ public DefaultAndroidEventProcessor( } // userId should be set even if event is Cached as the userId is static and won't change anyway. - if (event.getUser() == null) { + final User user = event.getUser(); + if (user == null) { event.setUser(getDefaultUser()); + } else if (user.getId() == null) { + user.setId(getDeviceId()); } if (event.getContexts().getDevice() == null) { diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt index 3004a0faa2b..97821fe137c 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt @@ -182,9 +182,10 @@ class DefaultAndroidEventProcessorTest { } @Test - fun `When user is already set, do not overwrite it`() { + fun `When user with id is already set, do not overwrite it`() { val processor = DefaultAndroidEventProcessor(context, fixture.options.logger, fixture.buildInfo) val user = User() + user.id = "user-id" var event = SentryEvent().apply { setUser(user) } @@ -193,6 +194,18 @@ class DefaultAndroidEventProcessorTest { assertSame(user, event.user) } + @Test + fun `When user without id is set, user id is applied`() { + val processor = DefaultAndroidEventProcessor(context, fixture.options.logger, fixture.buildInfo) + val user = User() + var event = SentryEvent().apply { + setUser(user) + } + event = processor.process(event, null) + assertNotNull(event.user) + assertNotNull(event.user.id) + } + @Test fun `Executor service should be called on ctor`() { val processor = DefaultAndroidEventProcessor(context, fixture.options.logger, fixture.buildInfo) From f7d952cd36e9c911ce95f9b2a30bad65422a429f Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 4 Jan 2021 21:11:03 +0100 Subject: [PATCH 4/4] Update changelog. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcb5a882f5d..1ec6671b8bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ * Fix: Initialize Logback after context refreshes (#1129) * Ref: Return NoOpTransaction instead of null (#1126) * Fix: Do not crash when passing null values to @Nullable methods, eg User and Scope -* Enhancement: Send user.ip_address = {{ auto }} when sendDefaultPii is true +* Enhancement: Send user.ip_address = {{auto}} when sendDefaultPii is true * Fix: Resolving dashed properties from external configuration * Feat: Read `uncaught.handler.enabled` property from the external configuration