Skip to content

Fix exception from empty basic auth header token #8109

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 16, 2020
Merged

Fix exception from empty basic auth header token #8109

merged 1 commit into from
Mar 16, 2020

Conversation

zeeshanadnan
Copy link
Contributor

fixes gh-7976

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 12, 2020
@eleftherias eleftherias self-assigned this Mar 12, 2020
eleftherias
eleftherias suggested changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @zeeshanadnan! I have left one comment inline.

In addition please add a test to BasicAuthenticationFilterTests.
This can be very similar to the existing test invalidBase64IsIgnored.

@zeeshanadnan
Copy link
Contributor Author

@eleftherias updated the PR with the requested changes. Also in case of empty basic auth token filter chain should not proceed. Changed it too. Any further code improvements and comments is highly appreciated. Thanks.

eleftherias
eleftherias suggested changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zeeshanadnan.
I have left one more comment.

...a/org/springframework/security/web/authentication/www/BasicAuthenticationConverterTests.java Outdated
@@ -111,4 +112,12 @@ public void convertWhenEmptyPassword() {
assertThat(authentication.getName()).isEqualTo("rod");
assertThat(authentication.getCredentials()).isEqualTo("");
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the expected exception to the @Test annotation.
For an example see

spring-security/web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationConverterTests.java

Lines 94 to 100 in 7411f25

@Test(expected = BadCredentialsException.class)
public void testWhenInvalidBase64ThenError() {
MockHttpServletRequest request = new MockHttpServletRequest();
request.addHeader("Authorization", "Basic NOT_VALID_BASE64");
converter.convert(request);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the convention of using AssertJ in new tests. So used assertThatThrownBy.

eleftherias
eleftherias suggested changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @zeeshanadnan.
I have one more suggestion and then we should be ready to merge this into master.

...java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java Outdated
@@ -424,4 +424,20 @@ public void doFilterWhenTokenAndFilterCharsetDoNotMatchThenUnauthorized() throws
assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull();
}

@Test
public void requestWhenEmptyBasicAuthorizationHeaderTokenThenIgnored() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the test name, since the header is not ignored, but rather returns unauthorized.

Suggested change
public void requestWhenEmptyBasicAuthorizationHeaderTokenThenIgnored() throws Exception {
public void requestWhenEmptyBasicAuthorizationHeaderTokenThenUnauthorized() throws Exception {

@eleftherias eleftherias merged commit 935c547 into spring-projects:master Mar 16, 2020
@eleftherias
Copy link
Contributor

Thank for the PR @zeeshanadnan! This is now merged into master.

@eleftherias eleftherias added in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 16, 2020
@eleftherias eleftherias added this to the 5.4.0.M1 milestone Mar 16, 2020
@zeeshanadnan zeeshanadnan deleted the gh-7976 branch March 17, 2020 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eleftherias eleftherias Awaiting requested review from eleftherias

Assignees

@eleftherias eleftherias

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.4.0-M1
Development

Successfully merging this pull request may close these issues.

Basic auth header without user results in exception
3 participants
@zeeshanadnan @eleftherias @spring-projects-issues