Add JDBC implementation of OAuth2AuthorizedClientService #7855

jgrandja · 2020-01-22T14:32:02Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

jgrandja · 2020-01-22T14:58:54Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+				authorizedClientData.clientRegistrationId,
+				authorizedClientData.principalName,
+				authorizedClientData.accessTokenType,
+				authorizedClientData.accessTokenValue,		// TODO Encrypt


@rwinch Do you have a preference/recommendation on the approach for encrypting/securing the access token and refresh token?

I think we should externalize the mapping (reading/writing) of the data into one of Spring's strategies. Then user's can inject their own strategies.

Which specific strategy were you thinking? Converter or you had another one in mind?

rwinch

I cannot help to think that we might want to think about discriminating based on other attributes...i.e. the scopes associated with the token. Perhaps this is something we do later, but we should if nothing else make sure there is a ticket for it

rwinch · 2020-01-22T23:35:13Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+	 * @param clientRegistrationRepository the repository of client registrations
+	 */
+	public JdbcOAuth2AuthorizedClientService(
+			DataSource dataSource, ClientRegistrationRepository clientRegistrationRepository) {


I think it would be good to allow injecting JdbcOperations instead.

rwinch · 2020-01-22T23:42:11Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+import org.springframework.security.oauth2.core.OAuth2RefreshToken;
+import org.springframework.stereotype.Repository;
+import org.springframework.transaction.annotation.EnableTransactionManagement;
+import org.springframework.transaction.annotation.Transactional;


We don't use @Transactional in Spring Security because that means users must have declarative transactions enabled and we have no way to enforce it (nor should we need to). JdbcTemplate will handle the transactions for you.

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

rwinch · 2020-01-22T23:47:21Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+				authorizedClientData.clientRegistrationId,
+				authorizedClientData.principalName,
+				authorizedClientData.accessTokenType,
+				authorizedClientData.accessTokenValue,		// TODO Encrypt


I think we should externalize the mapping (reading/writing) of the data into one of Spring's strategies. Then user's can inject their own strategies.

rwinch · 2020-01-22T23:49:00Z

oauth2/oauth2-client/src/test/resources/oauth2-client-schema.sql

@@ -0,0 +1,13 @@
+CREATE TABLE oauth2_authorized_client (


Can we publish this on the classpath similar to how we do with the users.ddl? We could also update the Javadoc of the service to mention the classpath resource.

jgrandja · 2020-01-23T17:31:20Z

@rwinch

I cannot help to think that we might want to think about discriminating based on other attributes...i.e. the scopes associated with the token

The only "finder" operation we have in OAuth2AuthorizedClientService is:

loadAuthorizedClient(String clientRegistrationId, String principalName)

so this wouldn't work for searching based on scopes. We would either need to add another operation or likely create another API that provides various "finder" operations. Is there a specific use case that you are trying to address that has been reported by the community?

It might make sense to log a ticket with the details you had in mind so we can address this separately?

rwinch

I provided feedback inline.

rwinch · 2020-01-27T13:57:49Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+ * @see JdbcOperations
+ * @see RowMapper
+ */
+@Repository


Similar to @Transactional we don't use @Repository on implementations within Spring Security because it requires specific Spring configuration that we cannot guarantee.

rwinch · 2020-01-27T14:01:44Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+	/**
+	 * The data (entity) representation of an {@link OAuth2AuthorizedClient}.
+	 */
+	public static class OAuth2AuthorizedClientData {


What is the need for this type vs allowing the user to inject a RowMapper that produces an OAuth2AuthorizedClient?

@rwinch The OAuth2AuthorizedClientData is an entity representation that IMO is simpler to deal with compared to returning an OAuth2AuthorizedClient. The logic in OAuth2AuthorizedClientDataConverter is a bit involved and would require a bit too much work for the user to implement. Instead they can simply reuse OAuth2AuthorizedClientDataConverter and implement a RowMapper<OAuth2AuthorizedClientData> that performs some data translation/conversion (if necessary), eg. decoding/decrypting the access/refresh token, access token scope storage format

Take a look at the test JdbcOAuth2AuthorizedClientServiceTests.tableDefinitionWhenCustomThenAbleToOverride. It demonstrates what the user would need to implement if they preferred to change the table definition from the standard definition it expects. The RowMapper<OAuth2AuthorizedClientData> needs to be implemented but both Converter's are reused.

I don't like having this additional type. If OAuth2AuthorizedClient is difficult to use, we should just make that easier vs creating another type.

I removed OAuth2AuthorizedClientData

rwinch · 2020-02-03T21:35:14Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+		List<OAuth2AuthorizedClientData> result = this.jdbcOperations.query(
+				LOAD_AUTHORIZED_CLIENT_SQL, pss, this.authorizedClientDataRowMapper);
+
+		return !result.isEmpty() ? (T) this.authorizedClientDataConverter.convert(result.get(0)) : null;


Since we are only looking for 0 or 1 results, it seems like queryForObject might make more sense here.

I used queryForObject() originally but it throws an IncorrectResultSizeDataAccessException if not found, which is not ideal. With List query(), I can avoid the try catch and exception being thrown.

rwinch · 2020-02-03T21:37:22Z

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java

+	/**
+	 * The data (entity) representation of an {@link OAuth2AuthorizedClient}.
+	 */
+	public static class OAuth2AuthorizedClientData {


I don't like having this additional type. If OAuth2AuthorizedClient is difficult to use, we should just make that easier vs creating another type.

rwinch · 2020-02-12T15:39:58Z

...lient/src/main/resources/org/springframework/security/oauth2/client/oauth2-client-schema.sql

@@ -0,0 +1,13 @@
+CREATE TABLE oauth2_authorized_client (


I would not be shy about making values larger rather than shorter. Storage space is typically not an issue these days. I'd consider making the token value and the refresh token value's both blobs since we should not ever need to search by them.

I made both token columns blob and increased the size for a couple of other columns too. This is now merged.

jgrandja · 2020-02-13T17:42:49Z

Merged via de8b558

Add JDBC implementation of OAuth2AuthorizedClientService

a313917

Fixes spring-projectsgh-7655

jgrandja commented Jan 22, 2020

View reviewed changes

.../main/java/org/springframework/security/oauth2/client/JdbcOAuth2AuthorizedClientService.java Show resolved Hide resolved

jgrandja commented Jan 22, 2020

View reviewed changes

jgrandja requested a review from rwinch January 22, 2020 14:59

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement labels Jan 22, 2020

jgrandja added this to the 5.3.0.RC1 milestone Jan 22, 2020

rwinch requested changes Jan 22, 2020

View reviewed changes

jgrandja self-assigned this Jan 23, 2020

jgrandja added 2 commits January 24, 2020 15:30

Allow customizing data mapping and table definition

ad23e88

Polish

3c8c593

jgrandja requested a review from rwinch January 24, 2020 21:34

rwinch requested changes Jan 27, 2020

View reviewed changes

Remove @repository

9105294

rwinch requested changes Feb 3, 2020

View reviewed changes

eleftherias modified the milestones: 5.3.0.RC1, 5.3.0 Feb 5, 2020

Remove OAuth2AuthorizedClientData

298df51

jgrandja requested a review from rwinch February 6, 2020 12:22

rwinch reviewed Feb 12, 2020

View reviewed changes

jgrandja closed this Feb 13, 2020

jgrandja deleted the gh-7655-jdbc-authz-client-service branch February 13, 2020 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JDBC implementation of OAuth2AuthorizedClientService #7855

Add JDBC implementation of OAuth2AuthorizedClientService #7855

jgrandja commented Jan 22, 2020

jgrandja Jan 22, 2020

rwinch Jan 22, 2020

jgrandja Jan 23, 2020

rwinch left a comment

rwinch Jan 22, 2020

rwinch Jan 22, 2020

rwinch Jan 22, 2020

rwinch Jan 22, 2020

jgrandja commented Jan 23, 2020 •

edited

Loading

rwinch left a comment

rwinch Jan 27, 2020

rwinch Jan 27, 2020

jgrandja Jan 29, 2020 •

edited

Loading

jgrandja Jan 29, 2020

rwinch Feb 3, 2020

jgrandja Feb 6, 2020

rwinch Feb 3, 2020

jgrandja Feb 6, 2020

rwinch Feb 3, 2020

rwinch Feb 12, 2020

jgrandja Feb 13, 2020

jgrandja commented Feb 13, 2020

Add JDBC implementation of OAuth2AuthorizedClientService #7855

Add JDBC implementation of OAuth2AuthorizedClientService #7855

Conversation

jgrandja commented Jan 22, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Jan 23, 2020 • edited Loading

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

jgrandja Jan 29, 2020 • 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

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 commented Feb 13, 2020

jgrandja commented Jan 23, 2020 •

edited

Loading

jgrandja Jan 29, 2020 •

edited

Loading