Allow in-memory authorized client services to be constructed with a map #5994

vpavic · 2018-10-18T17:07:01Z

This is the equivalent of #5918 only this time for InMemoryOAuth2AuthorizedClientService ~~and its reactive counterpart~~.

...n/java/org/springframework/security/oauth2/client/InMemoryOAuth2AuthorizedClientService.java

vpavic · 2018-11-12T17:32:00Z

I've updated this PR with a commit that that adds OAuth2AuthorizedClientId. I've put that commit as first, and updated the original commit to leverage the newly introduced identifier.

Please take a quick look before I get any further with this. The thing is that the only consumer of OAuth2AuthorizedClientService (and its reactive counterpart) is AuthenticatedPrincipalOAuth2AuthorizedClientRepository (and its reactive counterpart). I believe the APIs of those should also be updated to use OAuth2AuthorizedClientId in order to have a real benefit from the newly introduced identifier.

rwinch · 2018-11-14T21:40:21Z

@vpavic While I'm open to considering updating the APIs to use OAuth2AuthorizedClientId, I don't think we should make this a part of this PR as I don't think there is a need for it now. Instead, this should be limited to the Map based implementations. We can then consider updating the OAuth2AuthorizedClientService (and reactive equivalent) separately.

Thoughts?

vpavic · 2018-11-14T21:46:22Z

I agree it's better to handle that separately. I'm going to update this PR with that in mind, and open a new one to consider API changes once this (i.e. OAuth2AuthorizedClientId) is merged.

vpavic · 2018-11-14T22:56:21Z

I've updated the PR.

rwinch

Thanks for the updates. I have commented inline.

...rg/springframework/security/oauth2/client/InMemoryReactiveOAuth2AuthorizedClientService.java

rwinch · 2018-11-15T16:39:24Z

...n/java/org/springframework/security/oauth2/client/InMemoryOAuth2AuthorizedClientService.java

 	 * @param clientRegistrationRepository the repository of client registrations
 	 */
-	public InMemoryOAuth2AuthorizedClientService(ClientRegistrationRepository clientRegistrationRepository) {
+	public InMemoryOAuth2AuthorizedClientService(
+			Map<OAuth2AuthorizedClientId, OAuth2AuthorizedClient> authorizedClients,


Given Map is an optional parameter, I'd prefer it to be exposed via a setter. I understand that means it is mutable object, but this is the typical pattern Spring uses.

I agree with @rwinch that we should expose via setter instead - Map should be optional. Also, this object is mutable anyway given saveAuthorizedClient and removeAuthorizedClient.

@vpavic Apologies for back-tracking my previous comment but what are your thoughts on removing the setter and adding a 2nd constructor for both ClientRegistrationRepository and the Map?

jgrandja · 2018-11-19T10:59:30Z

...lient/src/main/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientId.java

+	 */
+	public static OAuth2AuthorizedClientId create(ClientRegistration clientRegistration,
+			String principalName) {
+		return new OAuth2AuthorizedClientId(clientRegistration.getRegistrationId(),


We should Assert.notNull on clientRegistration and Assert.hasText on principalName. Applying this update would than make Assert.notNull redundant in OAuth2AuthorizedClientId constructor.

I think the parameter ClientRegistration clientRegistration should be changed to String clientRegistrationId, which aligns with OAuth2AuthorizedClientService.loadAuthorizedClient(String clientRegistrationId, String principalName).

I'm also curious on why this factory method is needed? Alternatively, we can make the constructor public. What are your throughts?

jgrandja · 2018-11-19T11:02:52Z

...lient/src/main/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientId.java

+	public int hashCode() {
+		return Objects.hash(this.clientRegistrationId, this.principalName);
+	}
+


It might be useful to implement toString as well?

jgrandja · 2018-11-19T11:04:34Z

.../src/test/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientIdTests.java

+	}
+
+	@Test
+	public void equalsWhenDifferentRegistrationIdAndSamePrincipalThenShouldReturnTrue() {


This method name should be changed to equalsWhenDifferentRegistrationIdAndSamePrincipalThenShouldReturnFalse

jgrandja · 2018-11-19T11:05:48Z

.../src/test/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientIdTests.java

+	}
+
+	@Test
+	public void equalsWhenSameRegistrationIdAndDifferentPrincipalThenShouldReturnTrue() {


This method name should be changed to equalsWhenSameRegistrationIdAndDifferentPrincipalThenShouldReturnFalse

jgrandja · 2018-11-19T11:07:04Z

.../src/test/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientIdTests.java

+	}
+
+	@Test
+	public void hashCodeWhenDifferentRegistrationIdAndSamePrincipalThenShouldReturnTrue() {


This method name should be changed to hashCodeWhenDifferentRegistrationIdAndSamePrincipalThenNotEqual

jgrandja · 2018-11-19T11:08:36Z

.../src/test/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientIdTests.java

+	}
+
+	@Test
+	public void hashCodeWhenSameRegistrationIdAndDifferentPrincipalThenShouldReturnTrue() {


This method name should be changed to hashCodeWhenSameRegistrationIdAndDifferentPrincipalThenNotEqual

jgrandja · 2018-11-19T11:10:39Z

...lient/src/main/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientId.java

+
+	/**
+	 * Factory method for creating new {@link OAuth2AuthorizedClientId} using
+	 * {@link ClientRegistration} and {@link Authentication}.


{@link Authentication} should be -> the principal name

vpavic · 2019-06-24T20:09:52Z

Since RC1 is around the corner, heads-up that reactive bits of #5918 need to get reverted. Or maybe all of it, if this PR doesn't make the cut.

jgrandja · 2019-06-27T21:09:04Z

Thanks for the heads up @vpavic. I left a couple of comments. Also, would you be interested in submitting a PR that reverts the reactive bits of #5918?

jgrandja · 2019-06-28T08:34:40Z

@vpavic Assuming we merge this PR and revert the reactive bits in #5918, this will allow a Map to be set by the caller for both InMemoryClientRegistrationRepository and InMemoryOAuth2AuthorizedClientService. However, the reactive counterparts InMemoryReactiveClientRegistrationRepository and InMemoryReactiveOAuth2AuthorizedClientService will not allow a Map to be set by the caller since this could introduce a blocking operation as @rwinch mentioned.

What are your thoughts on this? This will ultimately introduce inconsistent implementations between the two which I'm not sure at this point is the right way to go. Given that you raised this issue to allow for a distributed Map to be set, you will not be able to achieve this for a Reactive implementation/application. Do you have any concerns with this?

vpavic · 2019-06-28T15:05:58Z

I'm afraid I won't have time to revisit this and the #5918 partial revert as I'm quite occupied by stuff for next Spring Session release so feel free to do what you feel is the best.

Generally I'm interested primarily in Servlet side of the things (and have included reactive bits for completeness sake) so that inconsistency is fine with me. I also don't think inconsistency should be the reason for not providing something that's supporting a perfectly valid (and IMO needed) use case for Servlet based implementation.

Also I'll express a concern that I've already express in other places, which I believe is important and is the motivation for this and other related PRs - all the store-like interfaces in new Spring Security OAuth 2.0 are only supported by in-memory implementations, and that's IMO something that you typically don't want to be running in production.

jgrandja · 2019-06-29T01:37:55Z

@vpavic No worries if you don't have the time...totally understand. I'll take care of the remaining work.

I'm interested primarily in Servlet side of the things

Ok I don't see any issues on merging the changes for Servlet.

...only supported by in-memory implementations, and that's IMO something that you typically don't want to be running in production

Totally agreed and yes the in-memory implementations are meant for development only. It's up to the user to define @Bean of either ClientRegistrationRepository and/or OAuth2AuthorizedClientService for a production setup. We very well may provide other implementations in a later release but we've had other priorities to focus on as of late.

Thanks again for the reminder on this ticket and #5918...it almost slipped through the cracks.

jgrandja · 2019-07-08T20:06:09Z

@vpavic I just merged this along with a polish commit 23d61d4. I also did a partial revert of #5918 in this commit e554547.
Let me know if you see any issues with the updates.
Thanks.

Fixes spring-projectsgh-5994

jgrandja requested changes Oct 18, 2018

View reviewed changes

...n/java/org/springframework/security/oauth2/client/InMemoryOAuth2AuthorizedClientService.java Outdated Show resolved Hide resolved

vpavic force-pushed the improve-authorized-client-service branch 2 times, most recently from 3b16c5e to 32557ff Compare November 12, 2018 17:28

vpavic force-pushed the improve-authorized-client-service branch from 32557ff to 68800e7 Compare November 12, 2018 17:48

vpavic force-pushed the improve-authorized-client-service branch from 68800e7 to 143a6d9 Compare November 14, 2018 22:54

rwinch requested changes Nov 15, 2018

View reviewed changes

jgrandja requested changes Nov 19, 2018

View reviewed changes

vpavic force-pushed the improve-authorized-client-service branch from 143a6d9 to 699347d Compare November 19, 2018 20:39

vpavic added 2 commits November 19, 2018 21:42

Add OAuth2AuthorizedClientId

db3162e

Allow supplying a custom map to InMemoryOAuth2AuthorizedClientService

09d07ae

vpavic force-pushed the improve-authorized-client-service branch from 699347d to 09d07ae Compare November 19, 2018 20:42

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019

jgrandja self-assigned this Jun 27, 2019

jgrandja 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 Jun 27, 2019

jgrandja closed this in 9432670 Jul 8, 2019

jgrandja added a commit that referenced this pull request Jul 8, 2019

Polish #5994

23d61d4

jgrandja added this to the 5.2.0.RC1 milestone Jul 8, 2019

vpavic deleted the improve-authorized-client-service branch July 8, 2019 20:15

kostya05983 pushed a commit to kostya05983/spring-security that referenced this pull request Aug 26, 2019

Allow InMemoryOAuth2AuthorizedClientService to be constructed with a Map

023632b

Fixes spring-projectsgh-5994

kostya05983 pushed a commit to kostya05983/spring-security that referenced this pull request Aug 26, 2019

Polish spring-projects#5994

bc9ea4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow in-memory authorized client services to be constructed with a map #5994

Allow in-memory authorized client services to be constructed with a map #5994

vpavic commented Oct 18, 2018 •

edited

Loading

vpavic commented Nov 12, 2018

rwinch commented Nov 14, 2018

vpavic commented Nov 14, 2018 •

edited

Loading

vpavic commented Nov 14, 2018

rwinch left a comment

rwinch Nov 15, 2018

jgrandja Nov 19, 2018

jgrandja Jun 27, 2019

jgrandja Nov 19, 2018

jgrandja Jun 27, 2019

jgrandja Nov 19, 2018

jgrandja Nov 19, 2018

jgrandja Nov 19, 2018

jgrandja Nov 19, 2018

jgrandja Nov 19, 2018

jgrandja Nov 19, 2018

vpavic commented Jun 24, 2019

jgrandja commented Jun 27, 2019

jgrandja commented Jun 28, 2019

vpavic commented Jun 28, 2019

jgrandja commented Jun 29, 2019

jgrandja commented Jul 8, 2019

Allow in-memory authorized client services to be constructed with a map #5994

Allow in-memory authorized client services to be constructed with a map #5994

Conversation

vpavic commented Oct 18, 2018 • edited Loading

vpavic commented Nov 12, 2018

rwinch commented Nov 14, 2018

vpavic commented Nov 14, 2018 • edited Loading

vpavic commented Nov 14, 2018

rwinch left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpavic commented Jun 24, 2019

jgrandja commented Jun 27, 2019

jgrandja commented Jun 28, 2019

vpavic commented Jun 28, 2019

jgrandja commented Jun 29, 2019

jgrandja commented Jul 8, 2019

vpavic commented Oct 18, 2018 •

edited

Loading

vpavic commented Nov 14, 2018 •

edited

Loading