Add Generic AuthenticationFilter #7025

sbespalov · 2019-06-20T03:46:19Z

This PR contains AuthenticationWebFilter concept implementation for regular Spring Security Filter Chain:

AuthenticationConverter interface (aligned to ServerAuthenticationConverter)
GenericAuthenticationFilter implementation (aligned to AuthenticationWebFilter)
BasicAuthenticationConverter implementation (aligned to ServerHttpBasicAuthenticationConverter)

@rwinch the AbstractAuthenticationProcessingFilter.requiresAuthentication() method modified here to throw exceptions because these exceptions declared for AbstractAuthenticationProcessingFilter.unsuccessfulAuthentication method, which need to be called within GenericAuthenticationFilter.requiresAuthentication in case of AuthenticationConverter.convert decides to throw the AuthenticationException. So this seems necessary to have this change.

sbespalov · 2019-06-26T06:45:05Z

@rwinch can you please have a look at this PR?

rwinch

Thanks for the PR @sbespalov!

I have added comments inline. Please also

add @since to each new class, interface, method
add tests for the changes

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java

...c/main/java/org/springframework/security/web/authentication/www/AuthenticationConverter.java

...n/java/org/springframework/security/web/authentication/www/BasicAuthenticationConverter.java

...n/java/org/springframework/security/web/authentication/www/AuthenticationTokenConverter.java

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

sbespalov · 2019-07-05T03:38:03Z

@rwinch thanks for review

requested changes done

add @since to each new class, interface, method

done

add tests for the changes

done

rwinch

Thanks for the updates I have responded inline.

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

sbespalov · 2019-07-11T01:31:32Z

@rwinch thanks for fast review, requested changes done

.../main/java/org/springframework/security/web/authentication/AuthenticationSuccessHandler.java

rwinch

Thanks for the updates. I just now noticed that we need to improve the package structure some. Please refer inline for the details.

rwinch · 2019-07-16T17:14:41Z

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.authentication.www;


Since this is generic, the package should not be in the www package. It should be moved up a package.

rwinch · 2019-07-16T17:14:54Z

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

+ * @author Sergey Bespalov
+ * @since 5.2.0
+ */
+public class GenericAuthenticationFilter extends OncePerRequestFilter {


Please rename to AuthenticationFilter

rwinch · 2019-07-16T17:29:56Z

...c/main/java/org/springframework/security/web/authentication/www/AuthenticationConverter.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.authentication.www;


Since this is generic, the package should not be in the www package. It should be moved up a package

rwinch · 2019-07-16T17:30:06Z

.../springframework/security/web/authentication/www/AuthenticationEntryPointFailureHandler.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.web.authentication.www;


Since this is generic, the package should not be in the www package. It should be moved up a package

rwinch · 2019-07-16T17:37:36Z

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

+public class GenericAuthenticationFilter extends OncePerRequestFilter {
+
+	private RequestMatcher requestMatcher = AnyRequestMatcher.INSTANCE;
+	private AuthenticationConverter authenticationConverter = new BasicAuthenticationConverter();


Since this will be moved up a package, we cannot use BasicAuthenticationConverter in this class otherwise we will have package tangles.

A few solutions that we could use:

Add a new converter named FormLoginAuthenticationConverter in org.springframework.security.web.authentication that is the default value. This is my preferred approach. If we do this, then UsernamePasswordAuthenticationFilter would use FormLoginAuthenticationConverter.

Require AuthenticationConverter to be injected. While this solution is a smaller change, I don't like that the user is required to inject the converter when we could reasonably default it.

done with option 2.

For not to make this PR grows too much I can create a follow-up issue to provide FormLoginAuthenticationConverter as default.

rwinch · 2019-07-16T17:51:35Z

...in/java/org/springframework/security/web/authentication/www/GenericAuthenticationFilter.java

+	private RequestMatcher requestMatcher = AnyRequestMatcher.INSTANCE;
+	private AuthenticationConverter authenticationConverter = new BasicAuthenticationConverter();
+	private AuthenticationSuccessHandler successHandler = new SavedRequestAwareAuthenticationSuccessHandler();
+	private AuthenticationFailureHandler failureHandler = new AuthenticationEntryPointFailureHandler(


Since this will be moved up a package, we cannot use BasicAuthenticationEntryPoint in this class otherwise we will have package tangles.

A few solutions that we could use:

Use HttpStatusEntryPoint or LoginUrlAuthenticationEntryPoint. I think that the HttpStatusEntryPoint makes more sense since we don't know if there is anything in the app to handle the URL, but I am open to discussion on this.

Require AuthenticationFailureHandler to be injected. While this solution is a smaller change, I don't like that the user is required to inject the converter when we could reasonably default it.

done with HttpStatusEntryPoint as default.

sbespalov · 2019-07-17T02:31:00Z

@rwinch packages fixed as you requested.

rwinch · 2019-07-23T22:33:21Z

Thanks @sbespalov! This is now merged into master

This reverts to the old behavior from BasicAuthenticationFilter. Specifically, if a token has an empty password, it still parses a username and an empty String password. Issue gh-7025

This reverts to the old behavior from BasicAuthenticationFilter. Specifically, if a token has an empty password, it still parses a username and an empty String password. Issue spring-projectsgh-7025

carlspring · 2019-09-06T08:18:37Z

Hi!

When could we expect this to be released and in what version?

jzheaux · 2019-09-09T14:28:49Z

@carlspring thanks for asking. You can see on the right-hand side of the description a section called "Milestones". This ticket was resolved for the 5.2.0.M4 milestone, so it's available there and later versions beyond that.

The next GA version is 5.2.0, and you can follow the release date via the Milestones page.

carlspring · 2019-09-09T14:42:05Z

Thanks for confirming!

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 20, 2019

rwinch requested changes Jun 27, 2019

View reviewed changes

rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 27, 2019

rwinch self-assigned this Jun 27, 2019

rwinch added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Jun 27, 2019

rwinch changed the title ~~issue/6506: AuthenticationConverter implementation~~ Add AuthenticationConverter Jun 27, 2019

sbespalov force-pushed the issue/6506 branch from 3c88cfc to 66a3f24 Compare July 5, 2019 02:58

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 5, 2019

rwinch requested changes Jul 10, 2019

View reviewed changes

sbespalov force-pushed the issue/6506 branch from 66a3f24 to 6aa46d4 Compare July 11, 2019 01:20

sbespalov force-pushed the issue/6506 branch from 6aa46d4 to 7cf3a85 Compare July 11, 2019 01:54

rwinch reviewed Jul 16, 2019

View reviewed changes

.../main/java/org/springframework/security/web/authentication/AuthenticationSuccessHandler.java Show resolved Hide resolved

rwinch requested changes Jul 16, 2019

View reviewed changes

sbespalov force-pushed the issue/6506 branch 2 times, most recently from dd5f83f to e32cc90 Compare July 17, 2019 02:21

issue/6506: AuthenticationConverter implementation

f5d224b

sbespalov force-pushed the issue/6506 branch from e32cc90 to f5d224b Compare July 17, 2019 02:29

rwinch added status: duplicate A duplicate of another issue and removed status: feedback-provided Feedback has been provided labels Jul 23, 2019

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

rwinch merged commit f1187bd into spring-projects:master Jul 23, 2019

rwinch changed the title ~~Add AuthenticationConverter~~ Add Generic AuthenticationFilter Jul 23, 2019

carlspring mentioned this pull request Oct 21, 2019

Upgrade Spring Boot + Spring Security strongbox/strongbox#1552

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Generic AuthenticationFilter #7025

Add Generic AuthenticationFilter #7025

sbespalov commented Jun 20, 2019

sbespalov commented Jun 26, 2019

rwinch left a comment

sbespalov commented Jul 5, 2019 •

edited

Loading

rwinch left a comment

sbespalov commented Jul 11, 2019

rwinch left a comment

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

rwinch Jul 16, 2019

sbespalov Jul 17, 2019

sbespalov commented Jul 17, 2019

rwinch commented Jul 23, 2019

carlspring commented Sep 6, 2019

jzheaux commented Sep 9, 2019

carlspring commented Sep 9, 2019

Add Generic AuthenticationFilter #7025

Add Generic AuthenticationFilter #7025

Conversation

sbespalov commented Jun 20, 2019

sbespalov commented Jun 26, 2019

rwinch left a comment

Choose a reason for hiding this comment

sbespalov commented Jul 5, 2019 • edited Loading

rwinch left a comment

Choose a reason for hiding this comment

sbespalov commented Jul 11, 2019

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbespalov commented Jul 17, 2019

rwinch commented Jul 23, 2019

carlspring commented Sep 6, 2019

jzheaux commented Sep 9, 2019

carlspring commented Sep 9, 2019

sbespalov commented Jul 5, 2019 •

edited

Loading