vorburger commented on a change in pull request #1258:
URL: https://github.com/apache/fineract/pull/1258#discussion_r475442010
##########
File path: fineract-provider/gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.6-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.6-all.zip
Review comment:
@maektwain this seems "suspicious".. what is this for, why is it needed,
what does it change? FYI Gradle upgrades for this project are now automated,
see e.g. #1243, so we need a good reason (and I would suggest a separate PR and
JIRA issue, instead of doing that as part of this).
##########
File path: fineract-provider/dependencies.gradle
##########
@@ -31,14 +31,14 @@ dependencies {
implementation(
//'ch.vorburger.mariaDB4j:mariaDB4j:2.4.0',
+ 'org.springframework.boot:spring-boot-starter-security',
'org.springframework.boot:spring-boot-starter-web',
'org.springframework.boot:spring-boot-starter-security',
'org.springframework.boot:spring-boot-starter-cache',
'org.springframework:spring-jms',
'org.springframework:spring-context-support',
'org.springframework.security.oauth:spring-security-oauth2',
-
Review comment:
nit pick: don't remove this blank line, separating springframework and
jersey dependencies.
##########
File path: fineract-provider/build.gradle
##########
@@ -548,6 +550,7 @@ if (project.hasProperty('security') &&
project.getProperty('security') == 'oauth
into 'src/main/resources/'
include '*.properties'
}
+ exclude
Review comment:
this looks curious... is it needed? What does it exclude?
##########
File path: fineract-provider/dependencies.gradle
##########
@@ -31,14 +31,14 @@ dependencies {
implementation(
//'ch.vorburger.mariaDB4j:mariaDB4j:2.4.0',
+ 'org.springframework.boot:spring-boot-starter-security',
Review comment:
please remove, as we already have this one, just 2 below in line 36
##########
File path: fineract-provider/build.gradle
##########
@@ -85,14 +86,15 @@ apply plugin: "org.hidetake.swagger.generator"
dependencyManagement {
imports {
mavenBom 'org.springframework:spring-framework-bom:5.2.6.RELEASE'
+ mavenBom
'org.springframework.security:spring-security-bom:5.3.2.RELEASE'
}
dependencies {
// We use fixed versions, instead of inheriting them from the Spring
BOM, to be able to be on more recent ones.
// We do not use :+ to get the latest available version available on
Maven Central, as that could suddenly break things.
// We use the Renovate Bot to automatically propose Pull Requests
(PRs) when upgrades for all of these versions are available.
- dependency
'org.springframework.security.oauth:spring-security-oauth2:2.5.0.RELEASE'
+ dependency
'org.springframework.security.oauth:spring-security-oauth2:2.3.3.RELEASE'
Review comment:
So basically we're DOWNGRADING `spring-security-oauth2` from 2.5.0 to
2.3.3 here, which is an even older version than the 2.3.6 that was originally
used in #863 here, before the last upgrade, and is not the FINERACT-1012 Spring
Security OAuth 2.x to Spring Security 5.2.x upgrade? I'm confused now.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/api/UserDetailsApiResource.java
##########
@@ -58,9 +59,8 @@
@Component
@Profile("oauth")
@Scope("singleton")
-
-@Tag(name = "Fetch authenticated user details", description = "")
-@SuppressWarnings("deprecation") // TODO FINERACT-1012
+@Tag(name = "User Details", description = "Ability to fetch Authenticated User
Details")
+@EnableResourceServer
Review comment:
I'm not super familiar with Spring Security, but it strikes me as
curious that the `UserDetailsApiResource`, which must work but with and without
OAuth, needs a `pringframework.security.oauth2` specific annotation. Is this
required? What does this do?
##########
File path: fineract-provider/build.gradle
##########
@@ -85,14 +86,15 @@ apply plugin: "org.hidetake.swagger.generator"
dependencyManagement {
imports {
mavenBom 'org.springframework:spring-framework-bom:5.2.6.RELEASE'
+ mavenBom
'org.springframework.security:spring-security-bom:5.3.2.RELEASE'
Review comment:
Question for other reviewers (I honestly don't know): Does the
`spring-framework-bom` not already include `spring-security-bom` ? Also, if we
are fixing the version of `spring-security-oauth2` below anyway, then what do
we need to use this BOM for at all?
##########
File path: fineract-provider/build.gradle
##########
@@ -57,6 +57,7 @@ buildscript {
classpath "com.diffplug.spotless:spotless-plugin-gradle:5.1.1"
classpath "io.swagger.core.v3:swagger-gradle-plugin:2.1.4"
classpath
"gradle.plugin.org.hidetake:gradle-swagger-generator-plugin:2.18.2"
+ classpath
"org.springframework.security.oauth:spring-security-oauth2:2.3.3.RELEASE"
Review comment:
please remove, this is wrong and not required, as
`spring-security-oauth2` is obviously a dependency at runtime (below) and not a
`buildscript` dependency for Gradle itself (which is what this section is for).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
