Skip to content

Dedicated authentication-handler #WiP #1212

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

Closed
wants to merge 1 commit into from
Closed

Dedicated authentication-handler #WiP #1212

wants to merge 1 commit into from

Conversation

dlehammer
Copy link

Hi spring-authorization-server gurus,

I'm probably holding it wrong, but here goes :)

My use-case is pretty straight-forward; add "security logging" as recommended by OWASP.

I've looked into the authorization-server, but didn't find logging-support that matches the recommendation - my apologies if I've missed the support.

So I've created this non-exhaustive draft to demonstrate one possible approach supporting my use-case.

Expected Behavior

Augmenting framework error-/success-handling is a 1st class citizen.

Ex. based on the drafted approach I would be able to provide my own authentication-handler ala.

...

class MyOAuth2TokenEndpointFailureHandler implements AuthenticationFailureHandler {

    OAuth2TokenEndpointFailureHandler delegate = new OAuth2TokenEndpointFailureHandler()

    @Override
    public void onAuthenticationFailure(HttpServletRequest request, HttpServletResponse response,
        AuthenticationException exception) throws IOException, ServletException {

        MySecurityLogger.log(request, response, exception);

        delegate.onAuthenticationFailure(request, response, exception);
    }
}
OAuth2TokenEndpointFilter#setAuthenticationFailureHandler(new MyOAuth2TokenEndpointFailureHandler())

As demonstrated above, this approach would support augmenting without duplicating code from the framework.

I suggest this change, from private method to dedicated class, is applied to all filter authentication-handlers - as that would support any append/augment use-case I can think of 🤓

If this approach is acceptable I would be happy to take a stab at refactoring.

Current Behavior

Currently each authentication-handler implemented by the framework is encapsulated in the class in a private method unfit for reuse.

Context

How has this issue affected you?

In-order to augment the endpoint filters, I've accumulated an undesired responsibility for maintaining a mirror of the default authentication-handlers implementation.

What are you trying to accomplish?

Augment framework behavior, optimally by appending a bit of custom configuration/code - without changing the core-behavior.

What other alternatives have you considered?

Undoing the private encapsulation at runtime via reflection, but rejected the approach as it's brittle & time-consuming.

Are you aware of any workarounds?

Unfortunately no :/

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 15, 2023
@dlehammer
Copy link
Author

This approach might also be useful for #1111 & #925 :)

@jgrandja
Copy link
Collaborator

jgrandja commented May 18, 2023

Thanks for the PR @dlehammer.

I'm not sure about exposing OAuth2TokenEndpointFailureHandler but I agree we should somehow allow for reuse of the default AuthenticationSuccessHandler and AuthenticationFailureHandler.

We're currently planning for the 1.2 release so please give me a couple of weeks and I'll get back to you on this and we'll figure out a design approach that can be applied across the Filter's.

@jgrandja jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 19, 2023
@jgrandja
Copy link
Collaborator

jgrandja commented Sep 5, 2023

@dlehammer Instead of exposing OAuth2TokenEndpointFailureHandler, we're considering adding to OAuth2TokenEndpointFilter:

public static final AuthenticationFailureHandler DEFAULT_AUTHENTICATION_FAILURE_HANDLER = OAuth2TokenEndpointFilter::sendErrorResponse;

@dlehammer
Copy link
Author

Hi @jgrandja,

Sounds like an acceptable compromise, allowing for reuse when augmenting default behaviour 👍

Please consider utilizing the approach consistently for all default-handlers, enabling support for yet unknown use-cases 🙏

@jgrandja
Copy link
Collaborator

@dlehammer We just merged gh-1384 so I'll close this PR in favour of the new OAuth2ErrorAuthenticationFailureHandler.

We will continue to work on exposing additional AuthenticationFailureHandler and AuthenticationSuccessHandler in gh-1369.

@jgrandja jgrandja closed this Oct 18, 2023
@jgrandja jgrandja self-assigned this Oct 18, 2023
@jgrandja jgrandja added status: duplicate A duplicate of another issue and removed type: enhancement A general enhancement labels Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants