Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SecurityStrategy logs twice #334

Closed
JanWaller opened this issue Aug 7, 2018 · 10 comments · Fixed by #437
Closed

SecurityStrategy logs twice #334

JanWaller opened this issue Aug 7, 2018 · 10 comments · Fixed by #437
Milestone

Comments

@JanWaller
Copy link

If I add a second LogBook Filter with Strategy.SECURITY, my calls resulting in 401 get logged twice. Once unfiltered with the whole body by the normal filter and once by the security filter.

I use Spring Boot without Spring Security.
What am I missing?

@JanWaller
Copy link
Author

To clarify, I had an error with filter ordering. So my filter causing the 401 response was to low of precedence.

However, with the automated error handling of Spring Boot, the problem remains.
Here, my filter won't be called (I want the "no auth" error message to be visible without auth). So it will be logged twice again. Once by the normal strategy and once by the security strategy.

@whiskeysierra
Copy link
Collaborator

Can you try to remove spring-security from your classpath? That should fix it.

Proper fix within Logbook would be to not rely on @ConditionalOnClass:

We should rather use @ConditionalOnBean.

@whiskeysierra
Copy link
Collaborator

To clarify, I had an error with filter ordering. So my filter causing the 401 response was to low of precedence.

Our setup assumes that the security filter runs in between our two logbook filters, iirc. If your application logic or a filter further down the chain causes 401 then you will see the output twice. There is no easy fix that I can think of right now.

@JanWaller
Copy link
Author

JanWaller commented Aug 7, 2018

I actually have no Spring Security, so I added
@bean
@ConditionalOnProperty(name = "logbook.filter.enabled", havingValue = "true", matchIfMissing = true)
@ConditionalOnMissingBean(name = "unauthorizedLogbookFilter")
public FilterRegistrationBean unauthorizedLogbookFilter(final Logbook logbook) {
final Filter filter = new LogbookFilter(logbook, Strategy.SECURITY);
final FilterRegistrationBean registration = new FilterRegistrationBean<>(filter);
registration.setName("unauthorizedLogbookFilter");
registration.setDispatcherTypes(REQUEST, ASYNC, ERROR);
registration.setOrder(-100); // lower number then spring security (0)
return registration;
}
to my own configuration.

I like the idea of the security strategy to skip the body for 401 responses.
So my custom security filter causes 401 with the same precedence as spring security would.
For the actual request everything looks fine. However, the 401 causes a redirect to /error, which is again logged by logbook. This final logging happens twice. Once by both strategies.

@whiskeysierra
Copy link
Collaborator

However, the 401 causes a redirect to /error, which is again logged by logbook. This final logging happens twice.

Error dispatch isn't really properly supported in logbook. You try to remove the ERROR dispatcher type from your customized version. That may help.

@JanWaller
Copy link
Author

If I register both filter beans without the error dispatcher, I get the desired result

@sideisra
Copy link

sideisra commented Sep 5, 2018

I have a similar problem. Would it be a proper solution that the authorizedLogbookFilter (configured via logbook-spring-boot-starter) doesn't log any requests/responses where the response status is 401 or 403? Similar to unauthorizedLogbookFilter which only logs requests/responses with response status 401 or 403.

@whiskeysierra whiskeysierra added this to the 2.0.0 milestone Feb 21, 2019
@whiskeysierra
Copy link
Collaborator

@sideisra That requires to delay the logging of the request until the response is available. #296 would support this as a custom strategy.

@whiskeysierra
Copy link
Collaborator

For the actual request everything looks fine. However, the 401 causes a redirect to /error, which is again logged by logbook.

@JanWaller Maybe you can just exclude the /error url from Logbook?

@whiskeysierra whiskeysierra mentioned this issue Feb 22, 2019
6 tasks
@whiskeysierra
Copy link
Collaborator

If I register both filter beans without the error dispatcher, I get the desired result

This will become the default as of #437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants