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.

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

Add RequestCache setter in OAuth2AuthorizationCodeGrantFilter #8120

Closed
mdindoffer opened this issue Mar 16, 2020 · 10 comments
Closed

Add RequestCache setter in OAuth2AuthorizationCodeGrantFilter #8120

mdindoffer opened this issue Mar 16, 2020 · 10 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Milestone

Comments

@mdindoffer
Copy link

Summary

Please provide a way to inject specific RequestCache to the OAuth2AuthorizationCodeGrantFilter.
The "sister" filter OAuth2AuthorizationRequestRedirectFilter already has a #setRequestCache method for injecting specific RequestCache, but the OAuth2AuthorizationCodeGrantFilter uses a hardcoded HttpSessionRequestCache.

Reasoning

Current setup does not allow for easy customization of redirect scenarios after successful OAuth2 Code grant authorization flow.

Two workarounds exist to my knowledge:

  1. Use the HttpSessionRequestCache with a specially crafted HttpServletRequestWrapper, such that the DefaultSavedRequest created from it in the HttpSessionRequestCache will return a desired redirect uri.
  2. Avoid saving the request via RequestCache's interface, and store it directly in the session via
request.getSession().setAttribute(customSavedRequest, "SPRING_SECURITY_SAVED_REQUEST")

The first one is prone to breakage and complicated to do so, because the DefaultSavedRequest#getRedirectUrl() builds a URL from multiple fields, that have to be specifically overriden in the request wrapper.
The second workaround is slightly less complicated, but even more prone to breakage, as the session attribute name is not a public constant and can change anytime.

The addition of injectable RequestCache would make the OAuth2AuthorizationCodeGrantFilter on par with OAuth2AuthorizationRequestRedirectFilter.

Version

5.2.2

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 16, 2020
@rwinch rwinch added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 16, 2020
@rwinch
Copy link
Member

rwinch commented Mar 16, 2020

Thanks for the feedback. Is this something you would be willing to contribute?

@parikshitdutta
Copy link
Contributor

Hi @rwinch, how is this for contribution? I can take a look.

@rwinch
Copy link
Member

rwinch commented Apr 7, 2020

Thanks for volunteering @parikshitdutta! The issue is yours 😄

@parikshitdutta
Copy link
Contributor

Hi @rwinch, the build process is failing at OAuth2ResourceServerSpecTests, is it okay to discuss it here or should I open an issue rather?

@rwinch
Copy link
Member

rwinch commented Apr 9, 2020

@parikshitdutta Here is fine. What is the failure you are getting?

@parikshitdutta
Copy link
Contributor

Hi @rwinch, there are 27 tests failing with "java.lang.IllegalStateException: Failed to load ApplicationContext", from OAuth2ResourceServerSpecTests.

Result page attached for your reference:
(https://github.com/spring-projects/spring-security/files/4456729/org.springframework.security.config.web.server.OAuth2ResourceServerSpecTests.zip)

@rwinch
Copy link
Member

rwinch commented Apr 9, 2020

How are you running the tests? It looks like you might be running them from the commandline. What command do you run and from what folder? Do any of the other tests work?

@parikshitdutta
Copy link
Contributor

parikshitdutta commented Apr 9, 2020

I am running plain build from command line, i.e "gradlew build", also have tried other variations such as clean, refresh-dependencies etc, same result, only those 27 tests failing, all the other tests (about 1800 tests) are passed.

I am trying to build from spring-security root folder.

@parikshitdutta
Copy link
Contributor

parikshitdutta commented Apr 9, 2020

FYI, I was following "README.adoc" to try the building process with "gradlew build", which is failing at my end.

Then I tried gradle tasks for respective module, such as "spring-security-config" etc, using intelliJ "run test" option, those are getting through!

While, the same with "gradlew spring-security-config:test" from console again failing for those 27 tests.

Finally, I tried "gradlew clean build integrationTest" from "spring-security" folder from console, that passed all the tests, and build was successful.

@rwinch please guide me with the right approach to test and ensure my done changes are good for production.

Is running "gradlew clean build integrationTest" from spring-security folder sufficient after I am done with my code changes?

@parikshitdutta
Copy link
Contributor

@rwinch Please take a look at PR #8392, or Please assign it to respective reviewer.

Thank you.

@jgrandja jgrandja self-assigned this Apr 17, 2020
@jgrandja jgrandja removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Apr 17, 2020
@jgrandja jgrandja added this to the 5.4.x milestone Apr 17, 2020
parikshitdutta added a commit to parikshitdutta/spring-security that referenced this issue May 15, 2020
@jgrandja jgrandja modified the milestones: 5.4.x, 5.4.0-M2 May 15, 2020
@jgrandja jgrandja assigned parikshitdutta and unassigned jgrandja May 15, 2020
@jgrandja jgrandja changed the title Make RequestCache injectable in OAuth2AuthorizationCodeGrantFilter Add RequestCache setter in OAuth2AuthorizationCodeGrantFilter May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants