Monitoring plugin doesn't log remote user

Description

Hi,

We are testing the monitoring plugin in GeoServer 2.8.3 and noticed that the user executing the request (RemoteUser) was never logged.
We noticed this behaviour with users from the default XML service and with users from LDAP (using LDAP authentication provider).

We've investigated this a bit further.
Both users from the XML service as users from LDAP implement org.springframework.security.core.userdetails.UserDetails.

In MonitorFilter (https://github.com/geoserver/geoserver/blob/2.8.3/src/extension/monitor/core/src/main/java/org/geoserver/monitor/MonitorFilter.java) however the remote user is only set when auth.getPrincipal() is an instance of org.springframework.security.core.userdetails.User, which is not the case:

if (SecurityContextHolder.getContext() != null
&& SecurityContextHolder.getContext().getAuthentication() != null) {
Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if (auth.getPrincipal() != null && auth.getPrincipal() instanceof User) {
data.setRemoteUser(((User)auth.getPrincipal()).getUsername());
}
}

Changing this code to check whether auth.getPrincipal() is an instance of org.springframework.security.core.userdetails.UserDetails and casting to this class instead would probably solve the issue.

Best regards,
Tim.

Environment

None

Activity

Show:
Andrea Aime
May 10, 2016, 6:22 PM

Wondering, given you already had a look in the code, if there is no one else able to fix it quickly, any chance you can make a pull request to fix it?
Code contribution rules here: https://github.com/geoserver/geoserver/blob/master/CONTRIBUTING.md

Jeroen Dries
May 10, 2016, 7:34 PM

Hi, yes we could do a pr, but would you need some kind of test with that? We would only replace the instanceof check, with a check against a more generic interface, so this would be a minimal change with a minimal impact. We can also do a manual test on our development infrastructure.

Andrea Aime
May 11, 2016, 1:58 PM

Adding a test is the only line of defence against eventual future regressions (e.g., might happen next time we upgrade Spring).
I believe you can try adding a simple test to https://github.com/geoserver/geoserver/blob/master/src/extension/monitor/core/src/test/java/org/geoserver/monitor/MonitorFilterTest.java
by setting (and making sure to reset) and autheticantion object of your choice.

Jan Van den bosch
May 19, 2016, 7:03 AM
Nuno Oliveira
May 20, 2016, 1:16 PM
Edited
Fixed

Assignee

Nuno Oliveira

Reporter

Tim Vander Borght

Triage

None

Fix versions

Affects versions

Components

Priority

Medium