HttpSessionOAuth2AuthorizationRequestRepository storing one OAuth2AuthorizationRequest #9649

candrews · 2021-04-16T19:50:01Z

Uses the same class name so users get this implementation by default,
whether they're specifying an AuthorizationRequestRepository or relying on
Spring Security defaults.

Note that this implementation intentionally does not handle multiple
OAuth2AuthorizationRequest's per session, regressing gh-5110

Closes gh-5145

FYI @jgrandja @rwinch

jgrandja

Thanks @candrews.

The solution is quite simple. Instead of storing multiple OAuth2AuthorizationRequest in HttpSession, we store a single one only.

This would break existing applications if the backing store is external from the container. So to restore the current behaviour we need a way for applications to revert to this. So let's add this to HttpSessionOAuth2AuthorizationRequestRepository:

@Deprecated
public final void setAllowMultipleAuthorizationRequests(boolean allowMultipleAuthorizationRequests) {
...
}

NOTE: The default will store a single OAuth2AuthorizationRequest.

Makes sense?

jgrandja · 2021-04-19T16:58:07Z

...framework/security/oauth2/client/web/OldHttpSessionOAuth2AuthorizationRequestRepository.java

+ * @see AuthorizationRequestRepository
+ * @see OAuth2AuthorizationRequest
+ */
+public final class OldHttpSessionOAuth2AuthorizationRequestRepository


Please remove this as all changes will be contained in HttpSessionOAuth2AuthorizationRequestRepository

candrews · 2021-04-22T19:19:49Z

That sounds good to me.

I've implemented that approach and updated this PR.

candrews · 2021-04-23T21:01:27Z

@jgrandja what do you think?

🤞 this finally does it!

jgrandja

Thanks for the updates @candrews. Please see review comment.

Also, please update copyright year. Thanks.

jgrandja · 2021-04-27T15:59:46Z

...ingframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java

+			authorizationRequests = this.getAuthorizationRequests(request);
+		}
+		else {
+			authorizationRequests = new HashMap<>();


The default behaviour should store a single OAuth2AuthorizationRequest in session NOT a single entry in Map. Map is used only when allowMultipleAuthorizationRequests == true

I thought of that too. But that would make the serialization data incompatible; if a user switched between allowMultipleAuthorizationRequests then they would get lots of warnings logged. Maybe that's a small annoyance, but it still seems to be an issue that would best be avoided. That approach also requires more changes which seems contrary to your preference oo keep the fix as minimal as possible.

@jgrandja ping
I'd really like to get this work completed... can you please provide me the guidance necessary for that to happen?

Thank you again!

Apologies for the delay @candrews. I have a lot of things I'm juggling so I'm just rotating through my priorities. I'll get to this tomorrow. Thanks for your patience.

I also have a bunch of priorities, all conflicting and all urgent - I guess that's just how it goes :) Thank you for your continued attention to this issue, I greatly appreciate the opportunity to address it with you.

Thank you for thinking about passivity. However, I'd like to consider that we are also deprecating and thus removing setAllowMultipleAuthorizationRequests at some point. At that time, if there are values persisted as a Map, user applications will break when they update. This is not a huge deal since we will remove the deprecated method on a major release, but it is not ideal.

Instead, I'd prefer to read the session attribute and do an instanceof check. If it is a Map the code when the look up the proper value in the Map. Otherwise it would use the OAuth2AuthorizationRequest found in the session.

When saving, we would always save according to the allowMultipleAuthorizationRequests property.

This has the advantage that if users are not leveraging the deprecated setAllowMultipleAuthorizationRequests, then they can expect that an update to Spring Security 6 will be unlikely to cause a ClassCastException when they update to it. We also don't need any special code in Spring Security 6 to handle this scenario.

jgrandja

Thanks for the updates @candrews. There is one minor change left and I think we're good to go. Please squash commits on next update.

I will have @rwinch do one last review and then we'll be ready to merge.

jgrandja · 2021-04-30T12:08:18Z

...amework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepositoryTests.java

+	private HttpSessionOAuth2AuthorizationRequestRepository authorizationRequestRepository;
+
+	@Before
+	public void beforeEach() {


Rename to setup()

rwinch

Thanks for the pull request and your patience. I've provided feedback inline.

...ingframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java

rwinch · 2021-05-04T19:07:41Z

...ingframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java

+			authorizationRequests = this.getAuthorizationRequests(request);
+		}
+		else {
+			authorizationRequests = new HashMap<>();


Thank you for thinking about passivity. However, I'd like to consider that we are also deprecating and thus removing setAllowMultipleAuthorizationRequests at some point. At that time, if there are values persisted as a Map, user applications will break when they update. This is not a huge deal since we will remove the deprecated method on a major release, but it is not ideal.

Instead, I'd prefer to read the session attribute and do an instanceof check. If it is a Map the code when the look up the proper value in the Map. Otherwise it would use the OAuth2AuthorizationRequest found in the session.

When saving, we would always save according to the allowMultipleAuthorizationRequests property.

This has the advantage that if users are not leveraging the deprecated setAllowMultipleAuthorizationRequests, then they can expect that an update to Spring Security 6 will be unlikely to cause a ClassCastException when they update to it. We also don't need any special code in Spring Security 6 to handle this scenario.

candrews · 2021-05-04T21:15:45Z

Thanks for that additional information. I think I've now updated this PR with that change. The approach I've taken creates a new Map when allowMultipleAuthorizationRequests is not set (which could be a garbage concern) - if that's not acceptable, let me know and I'll try a different approach. I've already tried a few other ways, but they seem to create more complex code with more duplication.

rwinch

Please see the feedback inline

...ingframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java

...amework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepositoryTests.java

… default Add setAllowMultipleAuthorizationRequests allowing applications to revert to the previous functionality should they need to do so. Closes spring-projectsgh-5145 Intentionally regresses spring-projectsgh-5110

rwinch · 2021-05-12T20:08:10Z

Thanks for the PR @candrews This is merged into main via 35f5ebd I added a few more tests via 35f5ebd

…questRepository WebFlux version of spring-projectsgh-9649

…questRepository Related to gh-9649 Closes gh-9857

…questRepository Related to spring-projectsgh-9649 Closes spring-projectsgh-9857 Closes spring-projectsgh-9912

…questRepository Related to gh-9649 Closes gh-9857

…questRepository Related to spring-projectsgh-9649 Closes spring-projectsgh-9857

candrews mentioned this pull request Apr 16, 2021

Expire OAuth2AuthorizationRequest when saving to the session #9513

Closed

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 16, 2021

jzheaux assigned jgrandja Apr 16, 2021

jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 16, 2021

jgrandja requested changes Apr 19, 2021

View reviewed changes

jgrandja added this to the 5.5.0 milestone Apr 19, 2021

candrews force-pushed the limit-OAuth2AuthorizationRequest branch from 7e2ea57 to 5187280 Compare April 22, 2021 19:19

candrews force-pushed the limit-OAuth2AuthorizationRequest branch from 5187280 to e1fef10 Compare April 23, 2021 21:01

jgrandja requested changes Apr 27, 2021

View reviewed changes

jgrandja requested changes Apr 30, 2021

View reviewed changes

candrews force-pushed the limit-OAuth2AuthorizationRequest branch from 82b8ef9 to 9f21c76 Compare May 2, 2021 21:03

rwinch self-requested a review May 4, 2021 18:57

rwinch requested changes May 4, 2021

View reviewed changes

candrews force-pushed the limit-OAuth2AuthorizationRequest branch from 9f21c76 to b618cf9 Compare May 4, 2021 21:16

rwinch requested changes May 7, 2021

View reviewed changes

rwinch assigned rwinch and unassigned jgrandja May 7, 2021

candrews force-pushed the limit-OAuth2AuthorizationRequest branch from b618cf9 to ba5157a Compare May 10, 2021 16:18

rwinch mentioned this pull request May 12, 2021

OAuth2AuthorizationRequest should expire #5145

Closed

rwinch approved these changes May 12, 2021

View reviewed changes

rwinch closed this May 12, 2021

sjohnr added a commit to sjohnr/spring-security that referenced this pull request Jun 2, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

225765f

…questRepository WebFlux version of spring-projectsgh-9649

sjohnr mentioned this pull request Jun 2, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRequestRepository #9857

Merged

sjohnr added a commit that referenced this pull request Jun 15, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

aed993f

…questRepository Related to gh-9649 Closes gh-9857

sjohnr added a commit that referenced this pull request Jun 15, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

700bda6

…questRepository Related to gh-9649 Closes gh-9857

sjohnr added a commit to sjohnr/spring-security that referenced this pull request Jun 15, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

a108868

…questRepository Related to spring-projectsgh-9649 Closes spring-projectsgh-9857 Closes spring-projectsgh-9912

sjohnr added a commit that referenced this pull request Jun 15, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

ee9c8e2

…questRepository Related to gh-9649 Closes gh-9857

sjohnr added a commit that referenced this pull request Jun 15, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

67a18f5

…questRepository Related to gh-9649 Closes gh-9857

jgrandja mentioned this pull request Jun 16, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRequestRepository #9912

Closed

akohli96 pushed a commit to akohli96/spring-security that referenced this pull request Aug 25, 2021

Store one request by default in WebSessionOAuth2ServerAuthorizationRe…

27ee9d5

…questRepository Related to spring-projectsgh-9649 Closes spring-projectsgh-9857

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpSessionOAuth2AuthorizationRequestRepository storing one OAuth2AuthorizationRequest #9649

HttpSessionOAuth2AuthorizationRequestRepository storing one OAuth2AuthorizationRequest #9649

candrews commented Apr 16, 2021

jgrandja left a comment

jgrandja Apr 19, 2021

candrews commented Apr 22, 2021

candrews commented Apr 23, 2021

jgrandja left a comment

jgrandja Apr 27, 2021

candrews Apr 27, 2021 •

edited

Loading

candrews Apr 28, 2021

jgrandja Apr 29, 2021

candrews May 2, 2021

rwinch May 4, 2021

jgrandja left a comment

jgrandja Apr 30, 2021

candrews May 2, 2021

rwinch left a comment

rwinch May 4, 2021

candrews commented May 4, 2021

rwinch left a comment •

edited

Loading

rwinch commented May 12, 2021

HttpSessionOAuth2AuthorizationRequestRepository storing one OAuth2AuthorizationRequest #9649

HttpSessionOAuth2AuthorizationRequestRepository storing one OAuth2AuthorizationRequest #9649

Conversation

candrews commented Apr 16, 2021

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candrews commented Apr 22, 2021

candrews commented Apr 23, 2021

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candrews Apr 27, 2021 • edited Loading

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

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candrews commented May 4, 2021

rwinch left a comment • edited Loading

Choose a reason for hiding this comment

rwinch commented May 12, 2021

candrews Apr 27, 2021 •

edited

Loading

rwinch left a comment •

edited

Loading