Skip to content

AuthenticationSupplier feature for Web Security#6496

Closed
sbespalov wants to merge 4 commits intospring-projects:masterfrom
sbespalov:auth-supplier
Closed

AuthenticationSupplier feature for Web Security#6496
sbespalov wants to merge 4 commits intospring-projects:masterfrom
sbespalov:auth-supplier

Conversation

@sbespalov
Copy link
Copy Markdown
Contributor

@sbespalov sbespalov commented Feb 1, 2019

This PR targets to make Authentication Filters implementation to be more "SOLID".
Currently there are set of different Atuhenticaion Filters like BasicAuthenticationFilter, DigestAuthenticationFilter etc. and most of such filters logic is almost the same:

  1. expose Authentication from request (supply);
  2. proceed filter chain, if there is no supported Authentication provided
  3. authenticate the Authentication object with AuthenticationManager, if we have supported Authentication

This PR contains following main changes:

  1. Added AuthenticationSupplier interface;
  2. Added GenericAuthenticationFilter class;
  3. Basic Authentication support implemented with BasicAuthenticationSupplier and GenericAuthenticationFilter;

If this concept will be accepted in general then additional changes can be provided.

@pivotal-issuemaster
Copy link
Copy Markdown

@sbespalov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link
Copy Markdown

@sbespalov Thank you for signing the Contributor License Agreement!

@rwinch
Copy link
Copy Markdown
Member

rwinch commented Feb 4, 2019

Thanks for the PR @sbespalov!

In general, there are too many changes in this one PR. For example, there are polish actions that add ServletException where it is not necessary.

In regards to the Supplier, I think it may be a good idea, but let's not combine AuthenticationSupplier and AuthenticationEntryPoint. If you look at the reactive bits it uses a ServerAuthenticationConverter which is closer to what I would have in mind.

I'm going to close this PR. If you are still wanting to see this support, please create a ticket for us to discuss a bit so we can be on the same page before writing more code. This will ensure you don't spend a lot of time coding to find out that we are not yet on the same page.

@rwinch rwinch closed this Feb 4, 2019
@rwinch rwinch self-assigned this Feb 4, 2019
@sbespalov
Copy link
Copy Markdown
Contributor Author

sbespalov commented Feb 5, 2019

@rwinch thanks for your response.

#6506

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 this pull request may close these issues.

4 participants