fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] vorburger commented on a change in pull request #1258: FINERACT-1012
Date Mon, 24 Aug 2020 09:10:09 GMT

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:
users@infra.apache.org



Mime
View raw message