Skip to content

Commit

Permalink
fix(fiat): only attempt to sync permissions when fiat is enabled (#830)
Browse files Browse the repository at this point in the history
* fix(fiat): only attempt to sync permissions when fiat is enabled

In [this PR](#619), we added logic to sync roles after an application is created. Let's only attempt to sync roles if the Fiat client is actually enabled. Elsewhere in Front50, such as in ApplicationPermissionsService.syncUsers, we only attempt to call Fiat to sync these roles if FiatClientConfigurationProperties is enabled, if FiatService is present, and if FiatConfigurationProperties.roleSync is enabled. Let's mirror this logic in the ApplicationsController.

* feat(config): add FiatStatus bean

* refactor(fiat): consume FiatStatus in ApplicationsController

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
maggieneterval and mergify[bot] committed May 21, 2020
1 parent 95fda78 commit 5f43e85
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 4 deletions.
Expand Up @@ -20,6 +20,8 @@ import com.netflix.hystrix.exception.HystrixRuntimeException
import com.netflix.spectator.api.Registry
import com.netflix.spinnaker.fiat.shared.EnableFiatAutoConfig
import com.netflix.spinnaker.fiat.shared.FiatAccessDeniedExceptionHandler
import com.netflix.spinnaker.fiat.shared.FiatClientConfigurationProperties
import com.netflix.spinnaker.fiat.shared.FiatStatus
import com.netflix.spinnaker.filters.AuthenticatedRequestFilter
import com.netflix.spinnaker.front50.model.application.ApplicationDAO
import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO
Expand All @@ -29,6 +31,7 @@ import com.netflix.spinnaker.front50.model.pipeline.PipelineStrategyDAO
import com.netflix.spinnaker.front50.model.pipeline.PipelineTemplateDAO
import com.netflix.spinnaker.front50.model.project.ProjectDAO
import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
import com.netflix.spinnaker.kork.web.context.AuthenticatedRequestContextProvider
import com.netflix.spinnaker.kork.web.context.RequestContextProvider
import com.netflix.spinnaker.kork.web.interceptors.MetricsInterceptor
Expand Down Expand Up @@ -144,6 +147,13 @@ public class Front50WebConfig extends WebMvcConfigurerAdapter {
return new AuthenticatedRequestContextProvider()
}

@Bean
FiatStatus fiatStatus(DynamicConfigService dynamicConfigService,
Registry registry,
FiatClientConfigurationProperties fiatClientConfigurationProperties) {
return new FiatStatus(registry, dynamicConfigService, fiatClientConfigurationProperties)
}

@ControllerAdvice
static class HystrixRuntimeExceptionHandler {
@ResponseStatus(HttpStatus.TOO_MANY_REQUESTS)
Expand Down
@@ -1,7 +1,9 @@
package com.netflix.spinnaker.front50.controllers.v2

import com.netflix.spinnaker.fiat.shared.FiatService
import com.netflix.spinnaker.fiat.shared.FiatStatus
import com.netflix.spinnaker.front50.ServiceAccountsService
import com.netflix.spinnaker.front50.config.FiatConfigurationProperties
import com.netflix.spinnaker.front50.controllers.exception.InvalidApplicationRequestException
import com.netflix.spinnaker.front50.events.ApplicationEventListener
import com.netflix.spinnaker.front50.exception.NotFoundException
Expand Down Expand Up @@ -67,6 +69,12 @@ public class ApplicationsController {
@Autowired
Optional<ServiceAccountsService> serviceAccountsService;

@Autowired
FiatConfigurationProperties fiatConfigurationProperties;

@Autowired
FiatStatus fiatStatus;

@PreAuthorize("#restricted ? @fiatPermissionEvaluator.storeWholePermission() : true")
@PostFilter("#restricted ? hasPermission(filterObject.name, 'APPLICATION', 'READ') : true")
@ApiOperation(value = "", notes = """Fetch all applications.
Expand Down Expand Up @@ -107,10 +115,12 @@ public class ApplicationsController {
@RequestMapping(method = RequestMethod.POST)
Application create(@RequestBody final Application app) {
Application createdApplication = getApplication().initialize(app).withName(app.getName()).save()
try {
fiatService.ifPresent { it.sync() }
} catch (Exception ignored) {
log.warn("failed to trigger fiat permission sync", ignored)
if (fiatStatus.isEnabled() && fiatConfigurationProperties.getRoleSync().isEnabled() && fiatService.isPresent()) {
try {
fiatService.get().sync()
} catch (Exception ignored) {
log.warn("failed to trigger fiat permission sync", ignored)
}
}
return createdApplication
}
Expand Down

0 comments on commit 5f43e85

Please sign in to comment.