Sentry Spring Boot Starter. #517
Conversation
…figuration properties.
|
I figured out how to set up annotation processing 🎉 |
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java
Outdated
Show resolved
Hide resolved
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryProperties.java
Show resolved
Hide resolved
...ot-starter/src/main/java/io/sentry/spring/boot/SentryRequestHttpServletRequestProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
happy where this is going @maciejwalkowiak
left a few comments though.
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryProperties.java
Show resolved
Hide resolved
...ot-starter/src/main/java/io/sentry/spring/boot/SentryRequestHttpServletRequestProcessor.java
Show resolved
Hide resolved
| private static @NotNull User toUser( | ||
| final @Nullable User existingUser, final @NotNull HttpServletRequest request) { | ||
| final User user = existingUser != null ? existingUser : new User(); | ||
| user.setIpAddress(toIpAddress(request)); |
There was a problem hiding this comment.
I'm afraid that this would require the send_default_pii flag to be added on options, which is disabled by default.
https://docs.sentry.io/error-reporting/configuration/?platform=python
There was a problem hiding this comment.
Seems like it is working for me
There was a problem hiding this comment.
but that's the problem, the SDK should only capture (automatically) PII if the flag send_default_pii is enabled, as we don't even have the flag, sending this info would be a PII problem, basically, to capture and send the IP address should only be possible if the user has enabled this flag like sentry.sendDefaultPII=true, as we don't even have/expose this flag yet, I'd say that we should skip the IP address for now (maybe just commenting the code and adding a TODO).
or to implement the flag on this or another PR.
There was a problem hiding this comment.
Ah ok, understood. I imagine username could be also considered PII. In case of Spring integration, this filter has to be added explicitly by the user as we are not able to autoconfigure it. Maybe it's ok then to stay as it's just an opt-in option?
There was a problem hiding this comment.
yeah usually those things are opt-in.
the thing is, we need to make it opt-in when we are able to collect this info automatically eg IP Address, if the user depends on the customer setting like Sentry.setUser(...) then it's fine, I'm just not sure if the IP address here is set by the customer or Spring offers this for free, that's the question that should be answered.
There was a problem hiding this comment.
We either take the ip address from the X-FORWARDED-FOR header set by load balancers or directly from HttpServletRequest object. But in order to use it, users have to explicitly create an instance of this filter and add it to Spring Security configuration (example https://github.com/getsentry/sentry-android/pull/517/files#diff-6c5b2fc2c37b9e5c8bdbdc86be9588ccR30)
There was a problem hiding this comment.
What @marandaneto said.
Could you please add Boolean sendDefaultPii to SentryOptions and checking for that before adding any of this user data by default?
There was a problem hiding this comment.
Ok, in this case it is enough to add it to SentryProperties but I am wondering if we shouldn't erase any personal information somewhere deeper in sentry-core if this flag is set to false?
There was a problem hiding this comment.
that's a good point, if the flag is a safeguard to not even set it, I don't see how we could do differently other than doing the check guard in all the setters/all the integrations.
the other option would be if setIpAddress gets a boolean to set it or not so at least we don't repeat the same if everywhere.
we could start doing this check guard in here and if we see the need for other places, we just refactor it?!
the other option would be, we always set it, if possible, and we could create a PiiEventProcessor that reads the flag and erase the values if set, it'd be an elegant way of solving it, but this allows the user to set it in the first place, I don't know if this is fine or not. cc @bruno-garcia
There was a problem hiding this comment.
We don't have any PII to be erased yet. We delayed adding the flag until the first one popped up.
Happy to merge this and add that later
maciejwalkowiak
changed the title
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private static @NotNull String toIpAddress(final @NotNull HttpServletRequest request) { | ||
| final String ipAddress = request.getHeader("X-FORWARDED-FOR"); |
There was a problem hiding this comment.
Should this be opt-in?
Would technically expose the server to report a fake client IP if the request was sent to the server by the client with this (no-proxy) set?
Also, I believe the value here can be a list (chained-proxy servers) so might require some more handling.
| id("org.springframework.boot") version "2.3.3.RELEASE" | ||
| id("io.spring.dependency-management") version "1.0.10.RELEASE" |
There was a problem hiding this comment.
would be possible to reuse the Config class like the SDK itself? they are probably the same groups/modules/versions
There was a problem hiding this comment.
this build.gradle.kts is kept as close as possible to one generated using https://start.spring.io - I think if we don't touch it much it will be easier to understand for users what exactly needs to be set up to use Sentry.
There was a problem hiding this comment.
I get your point, but the thing is, the samples on this repo are to easier the development/testing/debugging, not really customer samples, they still could learn from it, but that's not the main reason, we have eg https://github.com/getsentry/examples/tree/master/java
we'd be updating the examples after publishing these new integrations and there we'd keep as close as possible.
There was a problem hiding this comment.
Ok, got it. Would you also like to put all the dependencies from sample application to Config class?
There was a problem hiding this comment.
for me, it only makes sense the ones that are also used in the SDK itself, the dependencies of the samples that are only used in the samples, its fine to be hardcoded.
if it happens that a dependency of a sample is hardcoded but still share a version that is in the config file, then I'd do a string template like "dependency$Config.sharedVersion" so we know that if we update something, it's done only in one place, this is just a nit about DRY though.
|
|
||
| tasks.withType<KotlinCompile> { | ||
| kotlinOptions { | ||
| freeCompilerArgs = listOf("-Xjsr305=strict") |
There was a problem hiding this comment.
why do we need this? just curiosity
There was a problem hiding this comment.
It's also added by start.spring.io - i think it's meant to tell Kotlin to respect Spring Framework nullability related annotations.
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryRequestFilter.java
Show resolved
Hide resolved
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryRequestFilter.java
Show resolved
Hide resolved
sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryProperties.java
Show resolved
Hide resolved
3 participants
📢 Type of change
📜 Description
Sentry Spring Boot Starter providing configuration via Spring Boot
@ConfigurationProperties, automatically setting HTTP request details onSentryEvent#request, setting user details onSentryEvent#user.💡 Motivation and Context
In conjunction with
sentry-logbackenables users to utilise Sentry with close to zero extra effort.💚 How did you test it?
📝 Checklist
🔮 Next steps
Currently we have only Spring MVC integration (and indirectly Spring Security through getting user data from
Principal). I think this PR should not contain anything more.We can consider other integrations as separate tasks & PRs.