Authorization code issue #281
Conversation
…nd use JDBCApprovalStore as the default to have support for H2
…nd use JDBCApprovalStore as the default to have support for H2
| @@ -4,4 +4,4 @@ aRMT;res_ManagementPortal,res_gateway;secret;MEASUREMENT.CREATE,SUBJECT.UPDATE,S | |||
| THINC-IT;res_ManagementPortal,res_gateway;secret;MEASUREMENT.CREATE,SUBJECT.UPDATE,SUBJECT.READ,PROJECT.READ,SOURCETYPE.READ,SOURCE.READ,SOURCETYPE.READ,SOURCEDATA.READ,USER.READ,ROLE.READ;refresh_token,authorization_code;;;43200;7948800;{"dynamic_registration": true}; | |||
| radar_restapi;res_ManagementPortal;secret;SUBJECT.READ,PROJECT.READ,SOURCE.READ,SOURCETYPE.READ;client_credentials;;;43200;259200;{}; | |||
| radar_redcap_integrator;res_ManagementPortal;secret;PROJECT.READ,SUBJECT.CREATE,SUBJECT.READ,SUBJECT.UPDATE;client_credentials;;;43200;259200;{}; | |||
| radar_dashboard;res_ManagementPortal,res_RestApi;secret;SUBJECT.READ,PROJECT.READ,SOURCE.READ,SOURCETYPE.READ;client_credentials;;;43200;259200;{}; | |||
| radar_dashboard;res_ManagementPortal,res_RestApi;secret;SUBJECT.READ,PROJECT.READ,SOURCE.READ,SOURCETYPE.READ;authorization_code;;;43200;259200;{}; | |||
There was a problem hiding this comment.
Refresh token here would also be useful
| import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment; | ||
| import org.springframework.boot.orm.jpa.hibernate.SpringPhysicalNamingStrategy; | ||
|
|
||
| public class RadarPhysicalNamingStrategy extends SpringPhysicalNamingStrategy { |
There was a problem hiding this comment.
Perhaps PostgresPhysicalNamingStrategy?
There was a problem hiding this comment.
Or CaseSensitivePhysicalNamingStrategy
| @@ -174,7 +181,11 @@ protected AuthorizationCodeServices authorizationCodeServices() { | |||
|
|
|||
| @Bean | |||
| public ApprovalStore approvalStore() { | |||
| return new JdbcApprovalStore(dataSource); | |||
| if (jpaProperties.getDatabase().equals(POSTGRESQL)) { | |||
There was a problem hiding this comment.
If the new ApprovalStore also works for H2, perhaps rename it to SanitizedJdbcApprovalStore And remove this if/else branch?
There was a problem hiding this comment.
It doesn't. That's why the if else was added later on. H2 doesn't like quotes.
There was a problem hiding this comment.
That's unfortunate... Would using hibernate solve that? Or would that be too large of a refactor?
There was a problem hiding this comment.
My guess would be not large. Currently, we do not have entities maintained by hibernate in for oauth related tables or in other words, we only have the database schema given by spring added. The rest are handled by spring-security-oauth. I assume having a local entity shouldn't effect the rest. Maybe a separate PR would be better.
There was a problem hiding this comment.
Okay, a separate PR seems fine.
| import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment; | ||
| import org.springframework.boot.orm.jpa.hibernate.SpringPhysicalNamingStrategy; | ||
|
|
||
| public class RadarPhysicalNamingStrategy extends SpringPhysicalNamingStrategy { |
There was a problem hiding this comment.
Or CaseSensitivePhysicalNamingStrategy
Authorization-code flow didn't work with Postgres as database so far. It happened due to hard-coded queries that have case sensitive values. These have to be escaped with quotes for postgres.
As a fix a
PostgresApprovalStoreis added with same implementation, but escaped queries.We have to keep the JdbcApprovalStore as well to support database other than postgres (e.g. H2)
PhysicalNamingStrategyandPostgresApprovalStoreTODO
Re-factor
PostgresApprovalStoreto simplify the implementation.Fixes #279