gh-3572 Upgrade to Spring Security 5.2 #3573
Conversation
ghillert
changed the title
...ow/server/config/cloudfoundry/security/support/CloudFoundryDataflowAuthoritiesExtractor.java
Outdated
Show resolved
Hide resolved
| @@ -37,7 +37,7 @@ | |||
| /** | |||
| * Skip Ssl validation. | |||
| */ | |||
| private boolean skipSslValidation = true; | |||
| private boolean skipSslValidation; | |||
There was a problem hiding this comment.
I thought we wanted to enable SSL validation by default.
There was a problem hiding this comment.
We should never "skip ssl validation" by default. I am not sure how that got set to true in the first place.
...c/main/java/org/springframework/cloud/dataflow/server/config/features/TaskConfiguration.java
Outdated
Show resolved
Hide resolved
| <dependency> | ||
| <groupId>io.projectreactor</groupId> | ||
| <artifactId>reactor-core</artifactId> | ||
| <version>3.3.0.RELEASE</version> |
There was a problem hiding this comment.
Similar to the previous comment on version management.
There was a problem hiding this comment.
Actually this dep doesn't belong here. It needs to come from a transitive deps, which currently is only a cf java client.
| @@ -46,6 +46,12 @@ | |||
| <artifactId>spring-cloud-dataflow-rest-client</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.springframework.security.oauth</groupId> | |||
There was a problem hiding this comment.
Is there a way to avoid using this in tests?
There was a problem hiding this comment.
We need this for our test as we have an embedded test server. We may have to wait for Spring Security 5.3 to address this.
There was a problem hiding this comment.
And version dep needs to go to main pom.
| token-info-uri: http://127.0.0.1:${oauth2.port}/oauth/check_token | ||
| authorization: | ||
| check-token-access: isAuthenticated() | ||
| provider-role-mappings: |
There was a problem hiding this comment.
We need to add the documentation specifically on the changes in the properties configuration
| ROLE_DESTROY: dataflow.create | ||
| ROLE_MODIFY: dataflow.create | ||
| ROLE_SCHEDULE: dataflow.create | ||
| security: |
There was a problem hiding this comment.
This is another important property hierarchical change as well. We need to be explicit about this in the documentation.
sabbyanandan
changed the title
* Upgrade Spring Cloud Common Security dependency * Fix tests * Update documentation
| @@ -66,23 +66,21 @@ | |||
|
|
|||
| <!-- note - check version of reactor at deployer-cf/cf-java-client uses --> | |||
| <spring-cloud-deployer-cloudfoundry.version>2.1.0.BUILD-SNAPSHOT</spring-cloud-deployer-cloudfoundry.version> | |||
| <reactor.version>3.2.0.RELEASE</reactor.version> | |||
| <reactor.version>3.3.0.RELEASE</reactor.version> | |||
There was a problem hiding this comment.
I know that we're going to reactor 3.3.x when cf java client gets to it but why this change with this PR?
There was a problem hiding this comment.
There is a call in Spring Security WebClient that uses an API call of 3.3.x.
| @@ -181,12 +182,14 @@ public TaskExecutionService taskService(LauncherRepository launcherRepository, | |||
| TaskAppDeploymentRequestCreator taskAppDeploymentRequestCreator, | |||
| TaskExplorer taskExplorer, | |||
| DataflowTaskExecutionDao dataflowTaskExecutionDao, | |||
| DataflowTaskExecutionMetadataDao dataflowTaskExecutionMetadataDao) { | |||
| DataflowTaskExecutionMetadataDao dataflowTaskExecutionMetadataDao, | |||
| @Autowired(required = false) OAuth2TokenUtilsService oauth2TokenUtilsService) { | |||
There was a problem hiding this comment.
In spring5 we can now use Optional or even @Nullable. I think Optional is preferred way these days as this required thing was a spring4 thing.
There was a problem hiding this comment.
Changed to @Nullable
| @@ -363,7 +365,8 @@ public StreamDeploymentController updatableStreamDeploymentController( | |||
|
|
|||
| @Bean | |||
| public SkipperClient skipperClient(SkipperClientProperties properties, | |||
| RestTemplateBuilder restTemplateBuilder, ObjectMapper objectMapper) { | |||
| RestTemplateBuilder restTemplateBuilder, ObjectMapper objectMapper, | |||
| @Autowired(required = false) OAuth2TokenUtilsService oauth2TokenUtilsService) { | |||
There was a problem hiding this comment.
Same Optional comment here vs using @Autowired.
There was a problem hiding this comment.
Changed to @Nullable
| client: | ||
| registration: | ||
| uaa: | ||
| redirect-uri: '{baseUrl}/login/oauth2/code/{registrationId}' |
There was a problem hiding this comment.
I can't find where baseUrl and registrationId are coming from?
There was a problem hiding this comment.
Spring Security feature, see under https://docs.spring.io/spring-security/site/docs/5.2.0.RELEASE/reference/htmlsingle/#initiating-the-authorization-request Also, see the associated commit under: spring-projects/spring-security#6900
| --authorized_grant_types password,authorization_code,client_credentials,refresh_token \ | ||
| --authorities uaa.resource,dataflow.create,dataflow.deploy,dataflow.destroy,dataflow.manage,dataflow.modify,dataflow.schedule,dataflow.view,foo.view,foo.create\ | ||
| --redirect_uri http://localhost:9393/login \ | ||
| --autoapprove openid \ |
There was a problem hiding this comment.
We can remove the new line character here.
|
|
||
| [source,bash] | ||
| ---- | ||
| uaac target http://uaa:8080/uaa |
There was a problem hiding this comment.
Though it is implicit, we can say, uaa is the local host name in uaa:8080
|
This PR needs to be updated based on the recent package name changes on |
|
LGTM, merging. |
resolves #3572