Add logging

[jgrandja]

We need to add logging to allow for more efficient troubleshooting during error conditions.

[rlewczuk]

Not sure when we'll reach MVP, lack of logs hampers authorization server in real world workloads. The same can be said about audit events, albeit to lesser extent and I'm not sure if it belongs to this ticket or new one should be created.

I can try implementing at least some if it's not too early and if (as an outsider) I'm proper person for such task (this is comprehensive, I'll probably need some initial guidance that long time spring contributors already know).

[sjohnr]

@rlewczuk,

Regarding audit events, see this comment. The decision was made not to pursue that at this time as there is already a hook to hang auditing on in Spring Security. If however you are referring to something more specific, please open an issue with the details and we can discuss that.

I think we'll need to do some design work before anything can be put in code, so probably best not to start anything yet.

[ikomissarov]

What about the original request to add some logging? You don't need to do a design work for that, do you?
I was really surprised not to see a single line in logs produced by the library. Not even when I switched to TRACE level. Not even in case of some error scenario, all exceptions are just swallowed without logging.

[steinwelberg]

Unfortunately the auditing events of Spring Security are not fine-grained enough for our use case. We'd like to log details about the successful or failed Authorization requests for example. Currently, in order to achieve this, we need to override theAuthenticationSuccessHandler and AuthenticationFailureHandler used in all the filters. This is quite ugly and error prone as we'd have to manually check if the implementation in the Spring Authorization server has been changed when we are upgrading the Spring authorization server version.

I might have an idea, how to be able to achieve this. What about giving the possibility to provide a FunctionalInterface that gets executed. If no interface is provided, nothing happens. We'd need to have one for both the success and failure handlers, in order to distinguish the different scenarios.

In case you'd want to see a working proof of concept, I'm willing to provide it!

An example FunctionalInterface for an Authorization Error.

@FunctionalInterface
public interface AuthorizationErrorFunction {

  void onError(OAuth2Error error);
}

An example Authentication ErrorHandler (many things are removed to keep the code focused on the goal)

public class AuthorizationErrorResponseHandler implements AuthenticationFailureHandler {
  private final RedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
  private AuthorizationErrorFunction authorizationErrorFunction = (error) -> {}; // NoOp function.

  @Override
  public void onAuthenticationFailure(HttpServletRequest request, HttpServletResponse response, AuthenticationException exception)
      throws IOException {
    OAuth2AuthorizationCodeRequestAuthenticationException authorizationCodeRequestAuthenticationException =
        (OAuth2AuthorizationCodeRequestAuthenticationException) exception;
    OAuth2Error error = authorizationCodeRequestAuthenticationException.getError();
    OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthentication =
        authorizationCodeRequestAuthenticationException.getAuthorizationCodeRequestAuthentication();

    authorizationErrorFunction.onError(error);

    if (authorizationCodeRequestAuthentication == null ||
        !StringUtils.hasText(authorizationCodeRequestAuthentication.getRedirectUri())) {
      response.sendError(HttpStatus.BAD_REQUEST.value(), error.toString());
      return;
    }

    // Rest of failure logic
    }

  public void setAuthorizationErrorFunction(
      AuthorizationErrorFunction authorizationErrorFunction) {
    if (authorizationErrorFunction != null) {
      this.authorizationErrorFunction = authorizationErrorFunction;
    }
  }
}

[jgrandja]

@steinwelberg Thanks for your suggestion with onError(OAuth2Error error). I suspect more context information will be needed in order for the listener to take appropriate action, as OAuth2Error alone will not be enough.

I am leaning towards a pub-sub model leaving it up to the listener to do the logging or whatever it chooses to do. But we still need to decide on the design.

This work won't happen until after 0.3.0 is released as our top priority is the reference documentation.

[jgrandja]

We should consider using Spring Security Observability features.

[dciarniello]

I'd like to echo the need for logging. I'm building a POC authorization server in order to assess what it's going to take to upgrade from a highly customized version of the old authorization server (assuming that it's even possible) and running into difficulties due to the lack of error logging.

[lucwillems]

I also like to give a +1 vote for both audit and logging . one of the first thing while i making a poc for migration from old authorization server to the new code was to replacing all AuthenticationSuccessHandler and AuthenticationFailureHandler with copies and some logging to have a better idea about issues. an event driven approach for audit of success/failure with request context would be nice.

[jgrandja]

@lucwillems

an event driven approach for audit of success/failure with request context would be nice.

We are considering an event driven approach for this. We'll be looking into a design soon.

[AlTurner-MOJ]

@dciarniello sounds like we are in very similar situations, we're also building a POC to assess migration from a very customised implementation based on the now deprecated spring-security-oauth2:2.5.2.RELEASE. Would definitely benefit from some logging. Would welcome any pointers to best forums where issues around this can be discussed too? We are facing a number of issues around this that it would be really useful to discuss. Thanks all