fix(web): Better handling when ServiceAccountPermissionDAO does not… (

#583)
spinnaker · Aug 28, 2019 · a25e184 · a25e184
1 parent a7a36e9
commit a25e184
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 35 deletions.
diff --git a/...rc/main/groovy/com/netflix/spinnaker/front50/controllers/ServiceAccountsController.groovy b/...rc/main/groovy/com/netflix/spinnaker/front50/controllers/ServiceAccountsController.groovy
@@ -21,70 +21,71 @@ import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator
 import com.netflix.spinnaker.fiat.shared.FiatService
 import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccount
 import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO
+import com.netflix.spinnaker.kork.exceptions.SystemException
 import groovy.util.logging.Slf4j
-import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.beans.factory.annotation.Value
-import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression
 import org.springframework.security.core.Authentication
 import org.springframework.security.core.context.SecurityContextHolder
-import org.springframework.web.bind.annotation.PathVariable
-import org.springframework.web.bind.annotation.RequestBody
-import org.springframework.web.bind.annotation.RequestMapping
-import org.springframework.web.bind.annotation.RequestMethod
-import org.springframework.web.bind.annotation.RestController
+import org.springframework.web.bind.annotation.*
 import retrofit.RetrofitError
 
 @Slf4j
 @RestController
 @RequestMapping("/serviceAccounts")
-@ConditionalOnExpression('${spinnaker.gcs.enabled:false} || ${spinnaker.s3.enabled:false} || ${spinnaker.azs.enabled:false} || ${spinnaker.oracle.enabled:false}')
 public class ServiceAccountsController {
 
-  @Autowired
-  ServiceAccountDAO serviceAccountDAO;
-
-  @Autowired(required = false)
-  FiatService fiatService
-
-  @Autowired
+  Optional<ServiceAccountDAO> serviceAccountDAO
+  Optional<FiatService> fiatService
   FiatClientConfigurationProperties fiatClientConfigurationProperties
-
-  @Autowired
   FiatPermissionEvaluator fiatPermissionEvaluator
-
-  @Value('${fiat.role-sync.enabled:true}')
   Boolean roleSync
 
+  ServiceAccountsController(
+    Optional<ServiceAccountDAO> serviceAccountDAO,
+    Optional<FiatService> fiatService,
+    FiatClientConfigurationProperties fiatClientConfigurationProperties,
+    FiatPermissionEvaluator fiatPermissionEvaluator,
+    @Value('${fiat.role-sync.enabled:true}') Boolean roleSync
+  ) {
+    this.serviceAccountDAO = serviceAccountDAO
+    this.fiatService = fiatService
+    this.fiatClientConfigurationProperties = fiatClientConfigurationProperties
+    this.fiatPermissionEvaluator = fiatPermissionEvaluator
+    this.roleSync = roleSync
+  }
+
   @RequestMapping(method = RequestMethod.GET)
   Set<ServiceAccount> getAllServiceAccounts() {
-    serviceAccountDAO.all();
+    serviceAccountDAO().all()
   }
 
   @RequestMapping(method = RequestMethod.POST)
   ServiceAccount createServiceAccount(@RequestBody ServiceAccount serviceAccount) {
-    def acct = serviceAccountDAO.create(serviceAccount.id, serviceAccount)
+    def acct = serviceAccountDAO().create(serviceAccount.id, serviceAccount)
     syncUsers(acct)
     return acct
   }
 
   @RequestMapping(method = RequestMethod.DELETE, value = "/{serviceAccountId:.+}")
   void deleteServiceAccount(@PathVariable String serviceAccountId) {
-    def acct = serviceAccountDAO.findById(serviceAccountId)
-    serviceAccountDAO.delete(serviceAccountId)
+    def acct = serviceAccountDAO().findById(serviceAccountId)
+    serviceAccountDAO().delete(serviceAccountId)
     try {
-      fiatService.logoutUser(serviceAccountId)
+      if (fiatService.isPresent()) {
+        fiatService.get().logoutUser(serviceAccountId)
+      }
     } catch (RetrofitError re) {
       log.warn("Could not delete service account user $serviceAccountId", re)
     }
     syncUsers(acct)
   }
 
   private void syncUsers(ServiceAccount serviceAccount) {
-    if (!fiatClientConfigurationProperties.enabled || !fiatService || !serviceAccount || !roleSync) {
+    if (!fiatClientConfigurationProperties.enabled || !fiatService.isPresent() || !serviceAccount || !roleSync) {
       return
     }
     try {
-      fiatService.sync(serviceAccount.memberOf)
+      fiatService.get().sync(serviceAccount.memberOf)
       log.debug("Synced users with roles")
       // Invalidate the current user's permissions in the local cache
       Authentication auth = SecurityContextHolder.getContext().getAuthentication()
@@ -93,4 +94,13 @@ public class ServiceAccountsController {
       log.warn("Error syncing users", re)
     }
   }
+
+   private ServiceAccountDAO serviceAccountDAO() {
+    if (!serviceAccountDAO.isPresent()) {
+      throw new SystemException("Configured storage service does not support service account permissions")
+    }
+
+    return serviceAccountDAO.get()
+  }
+
 }
diff --git a/...est/groovy/com/netflix/spinnaker/front50/controllers/ServiceAccountsControllerSpec.groovy b/...est/groovy/com/netflix/spinnaker/front50/controllers/ServiceAccountsControllerSpec.groovy
@@ -25,18 +25,18 @@ import spock.lang.Specification
 import spock.lang.Subject
 
 class ServiceAccountsControllerSpec extends Specification {
-  def serviceAccountDAO = Mock(ServiceAccountDAO)
-  def fiatService = Mock(FiatService)
-  def fiatClientConfigurationProperties = Mock(FiatClientConfigurationProperties)
-  def fiatPermissionsEvaluator = Mock(FiatPermissionEvaluator)
+  ServiceAccountDAO serviceAccountDAO = Mock(ServiceAccountDAO)
+  FiatService fiatService = Mock(FiatService)
+  FiatClientConfigurationProperties fiatClientConfigurationProperties = Mock(FiatClientConfigurationProperties)
+  FiatPermissionEvaluator fiatPermissionsEvaluator = Mock(FiatPermissionEvaluator)
 
   @Subject
   def controller = new ServiceAccountsController(
-    serviceAccountDAO: serviceAccountDAO,
-    fiatService: fiatService,
-    fiatClientConfigurationProperties: fiatClientConfigurationProperties,
-    fiatPermissionEvaluator: fiatPermissionsEvaluator,
-    roleSync: true
+    Optional.of(serviceAccountDAO),
+    Optional.of(fiatService),
+    fiatClientConfigurationProperties,
+    fiatPermissionsEvaluator,
+    true
   )
 
   def "should invalidate local cache"() {