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.

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

Prevent JwtAuthenticationProvider from setting authentication details when jwtAuthenticationConverter returned an authentication instance with non null details #11823

Closed
wants to merge 1 commit into from
Closed

Prevent JwtAuthenticationProvider from setting authentication details when jwtAuthenticationConverter returned an authentication instance with non null details #11823

wants to merge 1 commit into from

Conversation

ch4mpy
Copy link
Contributor

@ch4mpy ch4mpy commented Sep 15, 2022

This could fix gh-11822

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 15, 2022
@sjohnr sjohnr self-assigned this Sep 16, 2022
@sjohnr sjohnr added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 16, 2022
@sjohnr sjohnr added this to the 5.8.x milestone Sep 16, 2022
sjohnr
sjohnr requested changes Sep 16, 2022
View reviewed changes
Copy link
Member

@sjohnr sjohnr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @ch4mpy! It would be helpful if you could add a test that ensures the provider is not overwriting details.

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Sep 16, 2022

@sjohnr done

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Sep 22, 2022

@sjohnr anything more needed on this PR?

@sjohnr
Copy link
Member

sjohnr commented Sep 22, 2022

@ch4mpy, thanks for asking! I don't think so, but I'm heads down on a few other things and will circle back to this a bit later.

sjohnr
sjohnr requested changes Sep 23, 2022
View reviewed changes
Copy link
Member

@sjohnr sjohnr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ch4mpy, when thinking more about this PR, we will need to make an adjustment. For the 5.8/6.0 releases, we are aiming to provide an upgrade to 5.8 that contains no breaking changes. Anything that breaks passivity would require a configuration option that allows users to opt-in to the change to test their compatibility in 5.8, so that an upgrade to 6.0 (where the change is the default) is seamless.

This change is technically a breaking change, as it would have been possible previously to return a non-null details which is overridden in the filter and will no longer be. Because the details can be used for things like authorization, we should not include this change in 5.8 without allowing it to be opt-in.

I think the simplest change is to move this PR to be based on 6.0 (main). Can you make this change? I've also added minor suggestions below.

...pringframework/security/oauth2/server/resource/authentication/JwtAuthenticationProvider.java Show resolved Hide resolved
...framework/security/oauth2/server/resource/authentication/JwtAuthenticationProviderTests.java Show resolved Hide resolved
@ch4mpy ch4mpy changed the base branch from 5.8.x to main September 26, 2022 18:38
@ch4mpy
Minimal fix to spring-projectsgh-11822
6132a40 
Prevent JwtAuthenticationProvider from setting authentication details
when jwtAuthenticationConverter returned an authentication instance
with non null details.
@ch4mpy
Copy link
Contributor Author

ch4mpy commented Sep 26, 2022 •
edited
Loading

@sjohnr rebased on main and updated copyright.

P.S.
Well, the scenario in which one sets detail in authentication converter and this details being overriden by the framework is the exact one a team I know went through and none expected to have details "lost". Reason for me opening the ticket as a "bug". Of course, whith my recent contribution on introspection and the discussions we had about this behavior, it didn't take me long to spot their problem and provide them with a work around.

I hadn't considered it an expected "feature", which is why I initialy based the PR on 5.8

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Nov 7, 2022

Any plan to merge this sometime?
This would allow me to use immutable Authentication implementations in servlets with JWT decoder (can currently do it in reactive apps only).

@sjohnr sjohnr modified the milestones: 5.8.x, 6.0.0-RC2, 6.1.0-M1 Nov 7, 2022
@sjohnr
Copy link
Member

sjohnr commented Nov 7, 2022

Hi @ch4mpy!

Any plan to merge this sometime?

Apologies, I wasn't able to circle back to this in time for RC1. I'm going to schedule this for 6.1 as I'd prefer to hold off on any code enhancements in the RC phase of 6.0. I'll merge this after the GA release.

@sjohnr
Copy link
Member

sjohnr commented Dec 13, 2022

This is merged via 7ad4ebd

@sjohnr sjohnr closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjohnr sjohnr sjohnr requested changes

Assignees

@sjohnr sjohnr

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: breaks-passivity A change that breaks passivity with the previous release type: enhancement A general enhancement
Projects
None yet
Milestone
6.1.0-M1
Development

Successfully merging this pull request may close these issues.

JwtAuthenticationProvider should use provided authentication details
3 participants
@ch4mpy @sjohnr @spring-projects-issues