Skip to content

getRemoteUser() returns name of object implementing AuthenticatedPrincipal #9102

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
Closed
wants to merge 1 commit into from
Closed

getRemoteUser() returns name of object implementing AuthenticatedPrincipal #9102

wants to merge 1 commit into from

Conversation

sjrd218
Copy link
Contributor

@sjrd218 sjrd218 commented Oct 7, 2020

Returns the name of the authenticated principle instead of falling through to the toString() method which may render a string representation of the entire object rather than a username.

This behavior is helpful in OAuth2 and Saml2 configurations.

Fixes: #3357

@pivotal-issuemaster
Copy link

@sjrd218 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

@sjrd218 Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 7, 2020
@sjrd218
Returns the name of the authenticated principle instead of falling th…
75d0324 
…rough to the toString() method which may render a string representation of the entire object rather than a username.
@jzheaux jzheaux self-assigned this Nov 12, 2020
@jzheaux jzheaux changed the title getRemoteUser() returns name of object implementing AuthenticatedPrinciple getRemoteUser() returns name of object implementing AuthenticatedPrincipal Nov 13, 2020
jzheaux
jzheaux requested changes Nov 13, 2020
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux 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, @sjrd218! I've left some feedback inline.

Also would you please format your commit to follow the contribution guidelines? Thanks!

...va/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapper.java
@@ -106,6 +107,9 @@ public String getRemoteUser() {
if (auth.getPrincipal() instanceof UserDetails) {
return ((UserDetails) auth.getPrincipal()).getUsername();
}
if (auth.getPrincipal() instanceof AuthenticatedPrincipal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that this is too narrow of a change to resolve #3357.

I wonder if this should instead do

if (auth instanceof AbstractAuthenticationToken) {
    return auth.getName();
}

since AbstractAuthenticationToken already has this same logic for AuthenticatedPrincipal.

...g/springframework/security/web/servletapi/SecurityContextHolderAwareRequestWrapperTests.java
@@ -130,4 +135,18 @@ public void testRolePrefixNotAppliedIfRoleStartsWith() {
assertThat(wrapper.isUserInRole("ROLE_FOOBAR")).isTrue();
}

@Test
public void testGetRemoteUserStringWithAuthenticatedPrinciple() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you please change Principle to Principal?

@sjrd218
Copy link
Contributor Author

sjrd218 commented Nov 17, 2020

@jzheaux Having trouble running gradle tasks for this project now.

Constantly seeing:

> Could not resolve all artifacts for configuration ':classpath'.
   > Could not download jackson-module-parameter-names-2.11.1.jar (com.fasterxml.jackson.module:jackson-module-parameter-names:2.11.1)
      > Could not get resource 'https://repo.spring.io/plugins-snapshot/com/fasterxml/jackson/module/jackson-module-parameter-names/2.11.1/jackson-module-parameter-names-2.11.1.jar'.
         > Could not GET 'https://repo.spring.io/plugins-snapshot/com/fasterxml/jackson/module/jackson-module-parameter-names/2.11.1/jackson-module-parameter-names-2.11.1.jar'. Received status code 401 from server: Unauthorized

@jzheaux
Copy link
Contributor

jzheaux commented Nov 17, 2020

Thanks, @sjrd218, will you please rebase and try again? There were some changes made yesterday to address that issue.

@sjrd218 sjrd218 closed this Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux requested changes

Assignees

@jzheaux jzheaux

Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SEC-3198: SecurityContextHolderAwareRequestWrapper#getRemoteUser ignores Authenticaion#getName
4 participants
@sjrd218 @pivotal-issuemaster @jzheaux @spring-projects-issues