From 074ef94d5b80c009c7fda614d16cc511bfe01cbf Mon Sep 17 00:00:00 2001 From: root Date: Tue, 10 Dec 2024 18:23:59 +0000 Subject: [PATCH 1/3] chore(dependencies): Autobump fiatVersion --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 8d8e92489..8339eb949 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ -fiatVersion=1.52.0 +fiatVersion=1.53.0 includeProviders=azure,gcs,oracle,redis,s3,swift,sql korkVersion=7.247.0 org.gradle.parallel=true From 3774aa611d1cd2aeb54733c478c1414c96640633 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Fri, 6 Dec 2024 23:32:10 +0530 Subject: [PATCH 2/3] refactor(retrofit2): refactor the code to align with the retrofit2 upgrade of fiat-api --- .../ApplicationPermissionsService.java | 3 ++- .../front50/ServiceAccountsService.java | 11 +++++++--- .../ApplicationPermissionsServiceSpec.groovy | 6 +++++- .../front50/ServiceAccountsServiceSpec.groovy | 20 ++++++++++++++++--- front50-web/front50-web.gradle | 1 + .../v2/ApplicationsController.java | 3 ++- 6 files changed, 35 insertions(+), 9 deletions(-) diff --git a/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java b/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java index cc396ab8e..4f717fc25 100644 --- a/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java +++ b/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java @@ -25,6 +25,7 @@ import com.netflix.spinnaker.front50.model.application.ApplicationDAO; import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO; import com.netflix.spinnaker.kork.exceptions.SystemException; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.kork.web.exceptions.NotFoundException; import java.util.AbstractMap.SimpleEntry; @@ -182,7 +183,7 @@ private void syncUsers(Permission newPermission, Permission oldPermission) { if (fiatConfigurationProperties.getRoleSync().isEnabled()) { try { - fiatService.get().sync(new ArrayList<>(roles)); + Retrofit2SyncCall.execute(fiatService.get().sync(new ArrayList<>(roles))); } catch (SpinnakerServerException e) { log.warn("Error syncing users", e); } diff --git a/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java b/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java index 4fdb57cfe..97dfc38b6 100644 --- a/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java +++ b/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java @@ -23,6 +23,7 @@ import com.netflix.spinnaker.front50.config.annotations.ConditionalOnAnyProviderExceptRedisIsEnabled; import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccount; import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.kork.web.exceptions.NotFoundException; import java.util.Collection; @@ -86,7 +87,8 @@ public void deleteServiceAccounts(Collection serviceAccountsToDe sa -> { try { serviceAccountDAO.delete(sa.getId()); - fiatService.ifPresent(service -> service.logoutUser(sa.getId())); + fiatService.ifPresent( + service -> Retrofit2SyncCall.execute(service.logoutUser(sa.getId()))); } catch (Exception e) { log.warn("Could not delete service account user {}", sa.getId(), e); } @@ -129,7 +131,7 @@ private void syncUsers(Collection serviceAccounts) { .flatMap(Collection::stream) .distinct() .collect(Collectors.toList()); - fiatService.get().sync(rolesToSync); + Retrofit2SyncCall.execute(fiatService.get().sync(rolesToSync)); log.debug("Synced users with roles: {}", rolesToSync); // Invalidate the current user's permissions in the local cache Authentication auth = SecurityContextHolder.getContext().getAuthentication(); @@ -149,7 +151,10 @@ private void syncServiceAccount(ServiceAccount serviceAccount) { return; } try { - fiatService.get().syncServiceAccount(serviceAccount.getId(), serviceAccount.getMemberOf()); + Retrofit2SyncCall.execute( + fiatService + .get() + .syncServiceAccount(serviceAccount.getId(), serviceAccount.getMemberOf())); log.debug( "Synced service account {} with roles: {}", serviceAccount.getId(), diff --git a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy index e35bfea1f..7958a399c 100644 --- a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy +++ b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy @@ -23,6 +23,8 @@ import com.netflix.spinnaker.front50.config.FiatConfigurationProperties import com.netflix.spinnaker.front50.model.application.Application import com.netflix.spinnaker.front50.model.application.ApplicationDAO import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO +import retrofit2.Call +import retrofit2.Response import spock.lang.Specification import spock.lang.Unroll @@ -33,6 +35,7 @@ class ApplicationPermissionsServiceSpec extends Specification { def "test application creation will sync roles in fiat"(permission, expectedSyncedRoles) { given: def fiatService = Mock(FiatService) + def syncCall = Mock(Call) ApplicationPermissionsService subject = createSubject( fiatService, Mock(ApplicationPermissionDAO) { @@ -44,7 +47,8 @@ class ApplicationPermissionsServiceSpec extends Specification { subject.createApplicationPermission(permission) then: - 1 * fiatService.sync(expectedSyncedRoles) + 1 * fiatService.sync(expectedSyncedRoles) >> syncCall + 1 * syncCall.execute() >> Response.success(null) where: permission | expectedSyncedRoles diff --git a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy index 043b9ebe0..8f499ebbf 100644 --- a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy +++ b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy @@ -26,12 +26,17 @@ import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO import org.springframework.security.core.Authentication import org.springframework.security.core.context.SecurityContext import org.springframework.security.core.context.SecurityContextHolder +import retrofit2.Call +import retrofit2.Response import spock.lang.Specification import spock.lang.Subject class ServiceAccountsServiceSpec extends Specification { ServiceAccountDAO serviceAccountDAO = Mock(ServiceAccountDAO) FiatService fiatService = Mock(FiatService) + Call syncCall = Mock(Call) + Call syncSaCall = Mock(Call) + Call logoutCall = Mock(Call) FiatClientConfigurationProperties fiatClientConfigurationProperties = Mock(FiatClientConfigurationProperties) { isEnabled() >> true } @@ -67,13 +72,15 @@ class ServiceAccountsServiceSpec extends Specification { } SecurityContextHolder.setContext(securityContext) fiatConfigurationProperties.isDisableRoleSyncWhenSavingServiceAccounts() >> false + when: serviceAccountDAO.create(serviceAccount.id, serviceAccount) >> serviceAccount service.createServiceAccount(serviceAccount) then: 1 * fiatPermissionsEvaluator.invalidatePermission(_) - 1 * fiatService.sync(["test-role"]) + 1 * fiatService.sync(["test-role"]) >> syncCall + 1 * syncCall.execute() >> Response.success(null) } def "deleting multiple service account should call sync once"() { @@ -92,13 +99,15 @@ class ServiceAccountsServiceSpec extends Specification { ] )] fiatConfigurationProperties.isDisableRoleSyncWhenSavingServiceAccounts() >> false + when: service.deleteServiceAccounts(serviceAccounts) then: 1 * serviceAccountDAO.delete("test-svc-acct-1") 1 * serviceAccountDAO.delete("test-svc-acct-2") - 1 * fiatService.sync(['test-role-1', 'test-role-2']) + 1 * fiatService.sync(['test-role-1', 'test-role-2']) >> syncCall + 1 * syncCall.execute() >> Response.success(null) } def "unknown managed service accounts should not throw exception"() { @@ -118,6 +127,10 @@ class ServiceAccountsServiceSpec extends Specification { 1 * serviceAccountDAO.findById(test1ServiceAccount.id) >> test1ServiceAccount 1 * serviceAccountDAO.findById(test2ServiceAccount.id) >> { throw new NotFoundException(test2ServiceAccount.id) } 1 * serviceAccountDAO.delete(test1ServiceAccount.id) + 1 * fiatService.logoutUser(_) >> logoutCall + 1 * logoutCall.execute() >> Response.success(null) + 1 * fiatService.sync(_) >> syncCall + 1 * syncCall.execute() >> Response.success(1L) 0 * serviceAccountDAO.delete(test2ServiceAccount.id) } @@ -144,7 +157,8 @@ class ServiceAccountsServiceSpec extends Specification { then: 1 * fiatPermissionsEvaluator.invalidatePermission(_) - 1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) + 1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) >> syncSaCall + 1 * syncSaCall.execute() >> Response.success(1L) 0 * fiatService.sync(["test-role"]) } } diff --git a/front50-web/front50-web.gradle b/front50-web/front50-web.gradle index 95ac9b175..88917cd11 100644 --- a/front50-web/front50-web.gradle +++ b/front50-web/front50-web.gradle @@ -32,6 +32,7 @@ dependencies { implementation "io.spinnaker.fiat:fiat-core:$fiatVersion" implementation "io.spinnaker.kork:kork-artifacts" implementation "io.spinnaker.kork:kork-config" + implementation "io.spinnaker.kork:kork-retrofit" implementation "io.spinnaker.kork:kork-web" implementation "io.spinnaker.kork:kork-exceptions" implementation "com.squareup.retrofit:converter-jackson" diff --git a/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java b/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java index 7c8879b17..19fd5305f 100644 --- a/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java +++ b/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java @@ -12,6 +12,7 @@ import com.netflix.spinnaker.front50.model.application.ApplicationDAO; import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO; import com.netflix.spinnaker.front50.model.application.ApplicationService; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import com.netflix.spinnaker.kork.web.exceptions.NotFoundException; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; @@ -125,7 +126,7 @@ public Application create(@RequestBody final Application app) { && fiatConfigurationProperties.getRoleSync().isEnabled() && fiatService.isPresent()) { try { - fiatService.get().sync(); + Retrofit2SyncCall.execute(fiatService.get().sync()); } catch (Exception e) { log.warn("failed to trigger fiat permission sync", e); } From 5ac36d8bb166e76f8f3ea5c45457de72e61beb93 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Mon, 9 Dec 2024 18:07:05 +0530 Subject: [PATCH 3/3] refactor(retrofit2): use retrofit-mock library instead of mocking Call. --- front50-core/front50-core.gradle | 1 + .../ApplicationPermissionsServiceSpec.groovy | 7 ++----- .../front50/ServiceAccountsServiceSpec.groovy | 21 ++++++------------- 3 files changed, 9 insertions(+), 20 deletions(-) diff --git a/front50-core/front50-core.gradle b/front50-core/front50-core.gradle index 00919eb28..a196e2ee4 100644 --- a/front50-core/front50-core.gradle +++ b/front50-core/front50-core.gradle @@ -47,5 +47,6 @@ dependencies { // in front50-test to front50-common, but front50-test seems like a better place. testImplementation project(":front50-test") testImplementation "io.spinnaker.kork:kork-sql-test" + testImplementation "com.squareup.retrofit2:retrofit-mock" } diff --git a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy index 7958a399c..9961bb48b 100644 --- a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy +++ b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy @@ -23,8 +23,7 @@ import com.netflix.spinnaker.front50.config.FiatConfigurationProperties import com.netflix.spinnaker.front50.model.application.Application import com.netflix.spinnaker.front50.model.application.ApplicationDAO import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO -import retrofit2.Call -import retrofit2.Response +import retrofit2.mock.Calls import spock.lang.Specification import spock.lang.Unroll @@ -35,7 +34,6 @@ class ApplicationPermissionsServiceSpec extends Specification { def "test application creation will sync roles in fiat"(permission, expectedSyncedRoles) { given: def fiatService = Mock(FiatService) - def syncCall = Mock(Call) ApplicationPermissionsService subject = createSubject( fiatService, Mock(ApplicationPermissionDAO) { @@ -47,8 +45,7 @@ class ApplicationPermissionsServiceSpec extends Specification { subject.createApplicationPermission(permission) then: - 1 * fiatService.sync(expectedSyncedRoles) >> syncCall - 1 * syncCall.execute() >> Response.success(null) + 1 * fiatService.sync(expectedSyncedRoles) >> Calls.response(null) where: permission | expectedSyncedRoles diff --git a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy index 8f499ebbf..2c4b83596 100644 --- a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy +++ b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy @@ -26,17 +26,13 @@ import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO import org.springframework.security.core.Authentication import org.springframework.security.core.context.SecurityContext import org.springframework.security.core.context.SecurityContextHolder -import retrofit2.Call -import retrofit2.Response +import retrofit2.mock.Calls import spock.lang.Specification import spock.lang.Subject class ServiceAccountsServiceSpec extends Specification { ServiceAccountDAO serviceAccountDAO = Mock(ServiceAccountDAO) FiatService fiatService = Mock(FiatService) - Call syncCall = Mock(Call) - Call syncSaCall = Mock(Call) - Call logoutCall = Mock(Call) FiatClientConfigurationProperties fiatClientConfigurationProperties = Mock(FiatClientConfigurationProperties) { isEnabled() >> true } @@ -79,8 +75,7 @@ class ServiceAccountsServiceSpec extends Specification { then: 1 * fiatPermissionsEvaluator.invalidatePermission(_) - 1 * fiatService.sync(["test-role"]) >> syncCall - 1 * syncCall.execute() >> Response.success(null) + 1 * fiatService.sync(["test-role"]) >> Calls.response(null) } def "deleting multiple service account should call sync once"() { @@ -106,8 +101,7 @@ class ServiceAccountsServiceSpec extends Specification { then: 1 * serviceAccountDAO.delete("test-svc-acct-1") 1 * serviceAccountDAO.delete("test-svc-acct-2") - 1 * fiatService.sync(['test-role-1', 'test-role-2']) >> syncCall - 1 * syncCall.execute() >> Response.success(null) + 1 * fiatService.sync(['test-role-1', 'test-role-2']) >> Calls.response(null) } def "unknown managed service accounts should not throw exception"() { @@ -127,10 +121,8 @@ class ServiceAccountsServiceSpec extends Specification { 1 * serviceAccountDAO.findById(test1ServiceAccount.id) >> test1ServiceAccount 1 * serviceAccountDAO.findById(test2ServiceAccount.id) >> { throw new NotFoundException(test2ServiceAccount.id) } 1 * serviceAccountDAO.delete(test1ServiceAccount.id) - 1 * fiatService.logoutUser(_) >> logoutCall - 1 * logoutCall.execute() >> Response.success(null) - 1 * fiatService.sync(_) >> syncCall - 1 * syncCall.execute() >> Response.success(1L) + 1 * fiatService.logoutUser(_) >> Calls.response(null) + 1 * fiatService.sync(_) >> Calls.response(1L) 0 * serviceAccountDAO.delete(test2ServiceAccount.id) } @@ -157,8 +149,7 @@ class ServiceAccountsServiceSpec extends Specification { then: 1 * fiatPermissionsEvaluator.invalidatePermission(_) - 1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) >> syncSaCall - 1 * syncSaCall.execute() >> Response.success(1L) + 1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) >> Calls.response(1L) 0 * fiatService.sync(["test-role"]) } }