Updates for #6911 and #6918

fiat-api/fiat-api.gradle

@@ -16,6 +16,8 @@
 
 dependencies {
   api "io.spinnaker.kork:kork-api"
+  api "io.spinnaker.kork:kork-secrets"
+  api "io.spinnaker.kork:kork-security"
 
   implementation project(":fiat-core")
 
@@ -25,7 +27,6 @@ dependencies {
   implementation "org.springframework.security:spring-security-web"
   implementation "com.netflix.spectator:spectator-api"
   implementation "io.spinnaker.kork:kork-core"
-  implementation "io.spinnaker.kork:kork-security"
   implementation "io.spinnaker.kork:kork-telemetry"
   implementation "io.spinnaker.kork:kork-web"
   implementation "io.spinnaker.kork:kork-retrofit"
@@ -39,4 +40,5 @@ dependencies {
   implementation "com.github.ben-manes.caffeine:caffeine"
 
   testImplementation "org.slf4j:slf4j-api"
+  testImplementation "org.springframework.security:spring-security-test"
 }

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java

@@ -19,9 +19,7 @@
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import java.util.List;
 import javax.servlet.http.HttpServletRequest;
-import org.springframework.security.authentication.AnonymousAuthenticationToken;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.security.web.authentication.AuthenticationConverter;
 import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken;
 
@@ -38,11 +36,6 @@ public Authentication convert(HttpServletRequest request) {
         .map(
             user ->
                 (Authentication) new PreAuthenticatedAuthenticationToken(user, "N/A", List.of()))
-        .orElseGet(
-            () ->
-                new AnonymousAuthenticationToken(
-                    "anonymous",
-                    "anonymous",
-                    AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS")));
+        .orElse(FiatWebSecurityConfigurerAdapter.ANONYMOUS);
   }
 }

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java

@@ -41,10 +41,7 @@
 import org.springframework.context.annotation.Import;
 import org.springframework.core.annotation.Order;
 import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
-import org.springframework.security.config.annotation.web.builders.HttpSecurity;
 import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
-import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
-import org.springframework.security.web.authentication.AnonymousAuthenticationFilter;
 import org.springframework.security.web.authentication.AuthenticationConverter;
 import retrofit.Endpoints;
 import retrofit.RestAdapter;
@@ -112,41 +109,10 @@ AuthenticationConverter defaultAuthenticationConverter() {
     return new AuthenticatedRequestAuthenticationConverter();
   }
 
-  @Bean
-  FiatWebSecurityConfigurerAdapter fiatSecurityConfig(
-      FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) {
-    return new FiatWebSecurityConfigurerAdapter(fiatStatus, authenticationConverter);
-  }
-
   @Bean
   @Order(HIGHEST_PRECEDENCE)
   FiatAccessDeniedExceptionHandler fiatAccessDeniedExceptionHandler(
       ExceptionMessageDecorator exceptionMessageDecorator) {
     return new FiatAccessDeniedExceptionHandler(exceptionMessageDecorator);
   }
-
-  private static class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdapter {
-    private final FiatStatus fiatStatus;
-    private final AuthenticationConverter authenticationConverter;
-
-    private FiatWebSecurityConfigurerAdapter(
-        FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) {
-      super(true);
-      this.fiatStatus = fiatStatus;
-      this.authenticationConverter = authenticationConverter;
-    }
-
-    @Override
-    protected void configure(HttpSecurity http) throws Exception {
-      http.servletApi()
-          .and()
-          .exceptionHandling()
-          .and()
-          .anonymous()
-          .and()
-          .addFilterBefore(
-              new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
-              AnonymousAuthenticationFilter.class);
-    }
-  }
 }

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java

@@ -19,10 +19,8 @@
 import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities;
 import com.netflix.spinnaker.fiat.model.UserPermission;
 import com.netflix.spinnaker.kork.common.Header;
-import java.util.List;
 import javax.servlet.http.HttpServletRequest;
 import lombok.RequiredArgsConstructor;
-import org.springframework.security.authentication.AnonymousAuthenticationToken;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.web.authentication.AuthenticationConverter;
 import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken;
@@ -46,7 +44,6 @@ public Authentication convert(HttpServletRequest request) {
             user, "N/A", permission.toGrantedAuthorities());
       }
     }
-    return new AnonymousAuthenticationToken(
-        "anonymous", "anonymous", List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY));
+    return FiatWebSecurityConfigurerAdapter.ANONYMOUS;
   }
 }

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java

@@ -21,15 +21,15 @@
 import com.netflix.spectator.api.Id;
 import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.fiat.model.Authorization;
-import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities;
 import com.netflix.spinnaker.fiat.model.UserPermission;
 import com.netflix.spinnaker.fiat.model.resources.Account;
 import com.netflix.spinnaker.fiat.model.resources.Authorizable;
 import com.netflix.spinnaker.fiat.model.resources.ResourceType;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.kork.telemetry.caffeine.CaffeineStatsCounter;
-import com.netflix.spinnaker.security.AccessControlled;
+import com.netflix.spinnaker.security.AbstractPermissionEvaluator;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
+import com.netflix.spinnaker.security.SpinnakerUsers;
 import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Collections;
@@ -48,17 +48,14 @@
 import org.apache.commons.lang3.StringUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpStatus;
-import org.springframework.security.access.PermissionEvaluator;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.core.context.SecurityContextHolder;
-import org.springframework.security.core.userdetails.UserDetails;
 import org.springframework.stereotype.Component;
 import org.springframework.util.backoff.BackOffExecution;
 import org.springframework.util.backoff.ExponentialBackOff;
 
 @Component
 @Slf4j
-public class FiatPermissionEvaluator implements PermissionEvaluator {
+public class FiatPermissionEvaluator extends AbstractPermissionEvaluator {
   private static final ThreadLocal<AuthorizationFailure> authorizationFailure = new ThreadLocal<>();
 
   private final Registry registry;
@@ -146,37 +143,26 @@ private static RetryHandler buildRetryHandler(
     this.getPermissionCounterId = registry.createId("fiat.getPermission");
   }
 
+  @Override
+  protected boolean isDisabled() {
+    return !fiatStatus.isEnabled();
+  }
+
   @Override
   public boolean hasPermission(
       Authentication authentication, Object resource, Object authorization) {
     if (!fiatStatus.isGrantedAuthoritiesEnabled()) {
       return false;
     }
-    if (!fiatStatus.isEnabled()) {
-      return true;
-    }
-    if (authentication == null || resource == null) {
-      log.warn(
-          "Permission denied because at least one of the required arguments was null. authentication={}, resource={}",
-          authentication,
-          resource);
-      return false;
-    }
-    if (authentication.getAuthorities().contains(SpinnakerAuthorities.ADMIN_AUTHORITY)) {
-      return true;
-    }
-    if (resource instanceof AccessControlled) {
-      return ((AccessControlled) resource).isAuthorized(authentication, authorization);
-    }
-    return false;
+    return super.hasPermission(authentication, resource, authorization);
   }
 
   public boolean canCreate(String resourceType, Object resource) {
-    if (!fiatStatus.isEnabled()) {
+    if (isDisabled()) {
       return true;
     }
 
-    String username = getUsername(SecurityContextHolder.getContext().getAuthentication());
+    String username = SpinnakerUsers.getCurrentUserId();
 
     try {
       return AuthenticatedRequest.propagate(
@@ -208,7 +194,7 @@ public boolean canCreate(String resourceType, Object resource) {
    */
   @SuppressWarnings("unused")
   public boolean hasCachedPermission(String username) {
-    if (!fiatStatus.isEnabled()) {
+    if (isDisabled()) {
       return true;
     }
 
@@ -217,7 +203,7 @@ public boolean hasCachedPermission(String username) {
 
   public boolean hasPermission(
       String username, Serializable resourceName, String resourceType, Object authorization) {
-    if (!fiatStatus.isEnabled()) {
+    if (isDisabled()) {
       return true;
     }
     if (resourceName == null || resourceType == null || authorization == null) {
@@ -266,18 +252,6 @@ public boolean hasPermission(
     return hasPermission;
   }
 
-  @Override
-  public boolean hasPermission(
-      Authentication authentication,
-      Serializable resourceName,
-      String resourceType,
-      Object authorization) {
-    if (!fiatStatus.isEnabled()) {
-      return true;
-    }
-    return hasPermission(getUsername(authentication), resourceName, resourceType, authorization);
-  }
-
   /**
    * Invalidates the cached permissions for a user.
    *
@@ -370,34 +344,19 @@ public UserPermission.View getPermission(String username) {
   @SuppressWarnings("unused")
   @Deprecated
   public boolean storeWholePermission() {
-    if (!fiatStatus.isEnabled()) {
+    if (isDisabled()) {
       return true;
     }
 
-    val authentication = SecurityContextHolder.getContext().getAuthentication();
-    val permission = getPermission(getUsername(authentication));
+    var user = SpinnakerUsers.getCurrentUserId();
+    var permission = getPermission(user);
     return permission != null;
   }
 
   public static Optional<AuthorizationFailure> getAuthorizationFailure() {
     return Optional.ofNullable(authorizationFailure.get());
   }
 
-  private String getUsername(Authentication authentication) {
-    String username = "anonymous";
-    if (authentication != null
-        && authentication.isAuthenticated()
-        && authentication.getPrincipal() != null) {
-      Object principal = authentication.getPrincipal();
-      if (principal instanceof UserDetails) {
-        username = ((UserDetails) principal).getUsername();
-      } else if (StringUtils.isNotEmpty(principal.toString())) {
-        username = principal.toString();
-      }
-    }
-    return username;
-  }
-
   private boolean permissionContains(
       UserPermission.View permission,
       String resourceName,
@@ -500,10 +459,10 @@ public boolean isAdmin() {
   }
 
   public boolean isAdmin(Authentication authentication) {
-    if (!fiatStatus.isEnabled()) {
+    if (isDisabled()) {
       return true;
     }
-    UserPermission.View permission = getPermission(getUsername(authentication));
+    UserPermission.View permission = getPermission(SpinnakerUsers.getUserId(authentication));
     return permission != null && permission.isAdmin();
   }
 

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatWebSecurityConfigurerAdapter.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright 2024 Apple, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.fiat.shared;
+
+import static org.springframework.security.config.Customizer.withDefaults;
+
+import com.netflix.spinnaker.security.SpinnakerAuthorities;
+import com.netflix.spinnaker.security.SpinnakerUsers;
+import java.util.List;
+import org.springframework.security.authentication.AnonymousAuthenticationToken;
+import org.springframework.security.config.annotation.web.builders.HttpSecurity;
+import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
+import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.web.authentication.AnonymousAuthenticationFilter;
+import org.springframework.security.web.authentication.AuthenticationConverter;
+import org.springframework.stereotype.Component;
+
+@Component
+public class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdapter {
+  private static final String KEY = "spinnaker-anonymous";
+  private static final Object PRINCIPAL = SpinnakerUsers.ANONYMOUS;
+  private static final List<GrantedAuthority> AUTHORITIES =
+      List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY);
+  static final AnonymousAuthenticationToken ANONYMOUS =
+      new AnonymousAuthenticationToken(KEY, PRINCIPAL, AUTHORITIES);
+
+  private final FiatStatus fiatStatus;
+  private final AuthenticationConverter authenticationConverter;
+
+  public FiatWebSecurityConfigurerAdapter(
+      FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) {
+    super(true);
+    this.fiatStatus = fiatStatus;
+    this.authenticationConverter = authenticationConverter;
+  }
+
+  @Override
+  protected void configure(HttpSecurity http) throws Exception {
+    http.servletApi(withDefaults())
+        .exceptionHandling(withDefaults())
+        .anonymous(
+            anonymous ->
+                // https://github.com/spinnaker/spinnaker/issues/6918
+                // match the same anonymous userid as expected elsewhere
+                anonymous.principal(PRINCIPAL).key(KEY).authorities(AUTHORITIES))
+        .addFilterBefore(
+            new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
+            AnonymousAuthenticationFilter.class);
+  }
+}

fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy

@@ -323,4 +323,32 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification {
     'WRITE'            | false
     'EXECUTE'          | false
   }
+
+  def "should evaluate permissions for AuthorizationMapControlled objects"() {
+    given:
+    def resource = new PermissionsControlledResource()
+    with(resource.permissions) {
+      add(Authorization.READ, 'integration group')
+      add(Authorization.WRITE, 'test group')
+      add(Authorization.EXECUTE, 'test group')
+    }
+
+    when:
+    def hasPermission = evaluator.hasPermission(authentication, resource, authorization)
+
+    then:
+    hasPermission == expectedHasPermission
+
+    where:
+    authorization         | expectedHasPermission
+    'execute'             | true
+    "execute"             | true
+    'EXECUTE'             | true
+    "EXECUTE"             | true
+    Authorization.EXECUTE | true
+    'write'               | true
+    'WRITE'               | true
+    'read'                | false
+    Authorization.READ    | false
+  }
 }