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

Add Generic AuthenticationFilter #6506

Closed
sbespalov opened this issue Feb 5, 2019 · 20 comments

Comments

@sbespalov
Copy link
Contributor

commented Feb 5, 2019

The proposal is to have generic Authentication filter which will be able to provide single authentication strategy regardless of authentication type.

The Servlet API based authentication process commonly consist of following stages:

  1. Expose Authentication from request (supply);
  2. If there is no supported Authentication provided then proceed filter chain;
  3. If there is supported Authentication then try to authenticate it with AuthenticationManager;
  4. If the authentication succeed then proceed filter chain with "authenticated" Authentication;
  5. If the authentication failed then provide appropriate response with AuthenticationEntryPoint;

The first stage, from above, can be done with AuthenticationSupplier interface:

public interface AuthenticationSupplier<T extends Authentication> extends AuthenticationEntryPoint {
	
	T supply(HttpServletRequest request) throws AuthenticationException;

	AuthenticationType getAuthenticationType();

}

This issue targets to clarify proper implementation previously suggested within PR #6496.

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@rwinch let me clarify why AuthenticationSupplier was extended from AuthenticationEntryPoint

but let's not combine AuthenticationSupplier and AuthenticationEntryPoint

the reason is that in case of failed authentication, which was supplied, we should commence same authentication scheme, that was supplied. So probably there is not much sense to separate this interfaces and it might be logical to have one interface for this.

@rwinch

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

Thank you for getting the conversation going. I can see value in a generic Filter. However, I think that we need to discuss this with the team as it is a big change for users (it would deprecate a lot of existing Filters). I do like the idea in principal, and it aligns with how the reactive part works.

I do think we need to separate the entry point from the supplier. The entry point is typically used to indicate that no authentication/credentials are present so prompt for some credentials so adding a supplier to the entry point seems counter intuitive to me.

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@rwinch thanks, will wait for your response

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I do think we need to separate the entry point from the supplier. The entry point is typically used to indicate that no authentication/credentials are present so prompt for some credentials so adding a supplier to the entry point seems counter intuitive to me.

In this case ExceptionTranslationFilter will use regular AuthenticationEntryPoint (BasicAuthenticationEntryPoint or any other) if there was AccessDeniedException.

I mean the case when we receive invalid credentials, provided with specific AuthenticationSupplier, then we should prompt for another credentials with same authentication type that was supplied.

so the point is that if we are able to supply authentication from request, then we also should be able to prompt appropriate authentication in response.

Maybe AuthenticationSupplier name confusing here? If so then it can be renamed to something like WebAuthenticationSupport.

@rwinch

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

That is a separate API too. In that case you use an AuthenticationFailureHandler. That handler should not be tied to how the authentication is obtained from the request.

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

yeah, but there was no AuthenticationFailureHandler interface changes proposed, this handler is just used to call appropriate AuthenticationEntryPoint.

I can try to think how to split AuthenticationSupplier from AuthenticationEntryPoint, but it's also important to know would you accept AuthenticationType and AuthenticationSupplierRegistry?

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@rwinch have you bean able to decide something on AuthenticationSupplier feature in general?

would you accept AuthenticationType and AuthenticationSupplierRegistry?

this also needs your response.

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@rwinch can you please provide a feedback, so I can proceed with this task?

@carlspring

This comment has been minimized.

Copy link

commented Mar 11, 2019

@rwinch ,

We're quite interested in contributing this work to your project and we believe that many people will benefit from @sbespalov 's changes. Could you please give us an update?

Many thanks in advance!

Martin

@carlspring

This comment has been minimized.

Copy link

commented Mar 12, 2019

@rwinch ,

In addition, we're already using this in our project (Strongbox). It was introduced via strongbox/strongbox#935.

We thought we it would be a great idea to improve the Spring Security project with these fixes, which is why @sbespalov extracted this implementation in a pull request that we would like to contribute to you guys.

We understand that these changes could cause deprecations of existing code and that if accepted, they may have to be introduced in a future major release. However, we believe this would improve the way things are currently done in Spring Security and would like to continue this conversation and understand:

  • What your concerns are
  • How to address them
  • How to get them included in an upcoming release

It would be great to hear your thoughts!

Kind regards,

Martin

@steve-todorov

This comment has been minimized.

Copy link

commented May 16, 2019

@rwinch ping, are there any updates on this? :)

@rwinch

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Thanks for the ping everyone.

I'm ok with this happening. However, we need to first decide on a design. I'd take a look at how the reactive bits work and model after that.

@carlspring

This comment has been minimized.

Copy link

commented May 18, 2019

@rwinch :

Would you mind elaborating on what you mean by that? Are you looking for some diagrams and flow charts, or what? Or, do you need to go over the code again?

@rwinch

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Thanks for reaching out @carlspring! I'd first call dibs on the work (so that others aren't duplicating efforts). I'd then look at how AuthenticationWebFilter looks and provide an outline (just method signatures) that looks similar as a proposal.

@carlspring

This comment has been minimized.

Copy link

commented May 21, 2019

@rwinch ,

Have you had a chance to look at the pull request @sbespalov submitted for this? Do you mean to say that you'll be re-implementing this, or what...? Would you mind outlining your plan, so that we could see how we can help improve the proposed pull request, if necessary?

@rwinch

This comment has been minimized.

Copy link
Member

commented May 21, 2019

My comment was confusing. Sorry for the misunderstanding.

First, I did have time to look at the PR. The feedback I provided still stands.

The security team doesn't currently have time for this issue. So if you would like to contribute it please use the following steps:

  • If you want to work on it call dibs on this issue so others aren't duplicating your efforts
  • Look at how AuthenticationWebFilter works
  • Provide an outline (just method signatures) that look similar to how AuthenticationWebFilter works as a proposal.
  • Once the outline is verified by the Security team, you can work on implementing it
  • Make sure the commits only contain necessary changes (extra noise makes it harder for us to review).
@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@rwinch I am glad we can have this conversation to find a right way implementing proposed feature.

Let me provide an outline as you requested:

  1. The solution is based on AbstractAuthenticationProcessingFilter and use its callback methods for authentication flow, follwing methods are used:

    • AbstractAuthenticationProcessingFilter.requiresAuthentication to check if request contains supported authentication and expose unauthorized Authentication object into SecurityContextHolder (the AuthenticationSupplier used here)
    • AbstractAuthenticationProcessingFilter.attemptAuthentication to authenticate the Authentication with AuthenticationManager

    The filter called GenericAuthenticationFilter as it can be used with different authentication types. (This part most likely corresponds to AuthenticationWebFilter)

  2. The main proposal is to have AuthenticationSupplier interface which extracts the Authentication object from request (this seems corresponds to ServerAuthenticationConverter, also the AuthenticationEntryPoint dropped as you declined it):

    public interface AuthenticationSupplier<T extends Authentication> {
    
        T supply(HttpServletRequest request) throws AuthenticationException;
    
            AuthenticationType getAuthenticationType();
    
    }
  3. There is also a bit difference with AuthenticationWebFilter, which maybe you will like as well. This is the AuthenticationSupplierRegistry which provides ability to use one HTTP Filter instance to authenticate different types of Authentication:

    public interface AuthenticationSupplierRegistry {
    
        public <T extends Authentication> AuthenticationSupplier<T> lookupSupplierByAuthenticationType(AuthenticationType authenticationType);
    
    }

    Above feature makes possible to change supported authentication types without changing the filter chain, so it can be safety configurable at runtime. It also makes possible to have plugable third party authentication implementations without exposing filter chain management to it. For example you can have nested Application Context in your application where the people can safely configure their own authentications without affecting on the application filter chain.

Please ask if something unclear so I can provide more details.

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@rwinch is it what you expect, or maybe something else needed?

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Unless there is compelling reason, I'd like there to be a one to one mapping with the reactive bits. This means the names should align and methods align. So more concretely, getAuthenticationType() would not exist, AuthenticationSupplier would be named AuthenticationConverter, etc.

without changing the filter chain, so it can be safety configurable at runtime

You can do this leveraging the https://github.com/spring-projects/spring-security/blob/master/core/src/main/java/org/springframework/security/authentication/AuthenticationManagerResolver.java[AuthenticationManagerResolver] which also allows looking up the AuthenticationManager via the request (this allows supporting multi tenant scenarios).

@sbespalov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

ok, so summarizing all there will be following:

  1. All new classes and interfaces will be placed under org.springframework.security.web.authentication.www package
  2. There will be AuthenticationConverter interface:
    public interface AuthenticationConverter {
    
    Authentication convert(HttpServletRequest request);
    
    }
    
    
  3. There will be GenericAuthenticationFilter extended from AbstractAuthenticationProcessingFilter which uses AuthenticationSupplier to provide unauthorized Authentication object from HttpServletRequest
  4. By default GenericAuthenticationFilter will use HttpBasicAuthenticationSupplier

@rwinch would you mind if I will create a PR with such implementation?

off topic:

You can do this leveraging the https://github.com/spring-projects/spring-security/blob/master/core/src/main/java/org/springframework/security/authentication/AuthenticationManagerResolver.java[AuthenticationManagerResolver] which also allows looking up the AuthenticationManager via the request (this allows supporting multi tenant scenarios).

seems this is not exactly the same, in terms of reactive we probably will need something like AuthenticationConverterResolver to lookup the AuthenticationConverter via the request

@rwinch rwinch self-assigned this Jul 23, 2019

@rwinch rwinch added this to the 5.2.0.M4 milestone Jul 23, 2019

@rwinch rwinch closed this in #7025 Jul 23, 2019

@rwinch rwinch changed the title AuthenticationSupplier feature for Web Security Add Generic AuthenticationFilter Jul 23, 2019

jzheaux added a commit that referenced this issue Aug 1, 2019

Polish AuthenticationFilter
Updated member variable references to be prefixed with "this.".
Fixed typo in authentication manager resolver error message.

Issue: gh-6506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.