Add AllowAnyUser annotation and wildcards in access annotations (#3213)

* Add AllowAnyUser annotation and wildcards in access annotations

* Move to explicit boolean for allowAny*

* Add missing test cases, cleanup AllowAnyUser annotation

* Update API

* Accept breaking API change

---------

Co-authored-by: Alex Szlavik <[email protected]>

Author: adrw

.palantir/revapi.yml

@@ -6192,6 +6192,48 @@ acceptedBreaks:
       old: "parameter void misk.redis.RealPipelinedRedis::<init>(===misk.redis.JedisPipeline===)"
       new: "parameter void misk.redis.RealPipelinedRedis::<init>(===redis.clients.jedis.AbstractPipeline===)"
       justification: "RealPipelinedRedis is internal"
+  "2024.03.26.112919-01b20b7":
+    com.squareup.misk:misk-redis:
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.lang.Double> misk.redis.DeferredRedis::zscore(java.lang.String,\
+        \ java.lang.String)"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.lang.Long> misk.redis.DeferredRedis::zadd(java.lang.String,\
+        \ double, java.lang.String, misk.redis.Redis.ZAddOptions[])"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.lang.Long> misk.redis.DeferredRedis::zadd(java.lang.String,\
+        \ java.util.Map<java.lang.String, java.lang.Double>, misk.redis.Redis.ZAddOptions[])"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.lang.Long> misk.redis.DeferredRedis::zcard(java.lang.String)"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.lang.Long> misk.redis.DeferredRedis::zremRangeByRank(java.lang.String,\
+        \ misk.redis.Redis.ZRangeRankMarker, misk.redis.Redis.ZRangeRankMarker)"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.util.List<kotlin.Pair<okio.ByteString,\
+        \ java.lang.Double>>> misk.redis.DeferredRedis::zrangeWithScores(java.lang.String,\
+        \ misk.redis.Redis.ZRangeType, misk.redis.Redis.ZRangeMarker, misk.redis.Redis.ZRangeMarker,\
+        \ boolean, misk.redis.Redis.ZRangeLimit)"
+      justification: "Additive change only"
+    - code: "java.method.addedToInterface"
+      new: "method java.util.function.Supplier<java.util.List<okio.ByteString>> misk.redis.DeferredRedis::zrange(java.lang.String,\
+        \ misk.redis.Redis.ZRangeType, misk.redis.Redis.ZRangeMarker, misk.redis.Redis.ZRangeMarker,\
+        \ boolean, misk.redis.Redis.ZRangeLimit)"
+      justification: "Additive change only"
+  "2024.04.05.204820-9331701":
+    com.squareup.misk:misk:
+    - code: "java.method.numberOfParametersChanged"
+      old: "method misk.security.authz.AccessAnnotationEntry misk.security.authz.AccessAnnotationEntry::copy(kotlin.reflect.KClass<?\
+        \ extends java.lang.annotation.Annotation>, java.util.List<java.lang.String>,\
+        \ java.util.List<java.lang.String>)"
+      new: "method misk.security.authz.AccessAnnotationEntry misk.security.authz.AccessAnnotationEntry::copy(kotlin.reflect.KClass<?\
+        \ extends java.lang.annotation.Annotation>, java.util.List<java.lang.String>,\
+        \ java.util.List<java.lang.String>, boolean, boolean)"
+      justification: "New parameters are optional"
   misk-0.18.0:
     com.squareup.misk:misk-gcp:
     - code: "java.method.numberOfParametersChanged"

misk-actions/api/misk-actions.api

@@ -135,6 +135,8 @@ public abstract interface annotation class misk/security/authz/AllowAnyService :
 }
 
 public abstract interface annotation class misk/security/authz/Authenticated : java/lang/annotation/Annotation {
+	public abstract fun allowAnyService ()Z
+	public abstract fun allowAnyUser ()Z
 	public abstract fun capabilities ()[Ljava/lang/String;
 	public abstract fun services ()[Ljava/lang/String;
 }

misk-actions/src/main/kotlin/misk/security/authz/Authenticated.kt

@@ -11,7 +11,13 @@ annotation class Authenticated(
   val services: Array<String> = [],
 
   /** Calling users must have at least one of these capabilities to be authenticated */
-  val capabilities: Array<String> = []
+  val capabilities: Array<String> = [],
+
+  /** Allow any service to be authenticated. */
+  val allowAnyService: Boolean = false,
+
+  /** Allow any user to be authenticated. */
+  val allowAnyUser: Boolean = false
 )
 
 /**
@@ -24,6 +30,7 @@ annotation class Unauthenticated
 /**
  * Annotation indicating that any authenticated service is allowed to access this endpoint.
  */
+@Deprecated("Use Authenticated(allowAnyService = true) instead.")
 @Retention(AnnotationRetention.RUNTIME)
 @Target(AnnotationTarget.FUNCTION)
 annotation class AllowAnyService

misk/api/misk.api

@@ -948,13 +948,19 @@ public final class misk/security/authz/AccessAnnotationEntry {
 	public fun <init> (Lkotlin/reflect/KClass;)V
 	public fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;)V
 	public fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;)V
-	public synthetic fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;Z)V
+	public fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ZZ)V
+	public synthetic fun <init> (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ZZILkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun component1 ()Lkotlin/reflect/KClass;
 	public final fun component2 ()Ljava/util/List;
 	public final fun component3 ()Ljava/util/List;
-	public final fun copy (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;)Lmisk/security/authz/AccessAnnotationEntry;
-	public static synthetic fun copy$default (Lmisk/security/authz/AccessAnnotationEntry;Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ILjava/lang/Object;)Lmisk/security/authz/AccessAnnotationEntry;
+	public final fun component4 ()Z
+	public final fun component5 ()Z
+	public final fun copy (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ZZ)Lmisk/security/authz/AccessAnnotationEntry;
+	public static synthetic fun copy$default (Lmisk/security/authz/AccessAnnotationEntry;Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ZZILjava/lang/Object;)Lmisk/security/authz/AccessAnnotationEntry;
 	public fun equals (Ljava/lang/Object;)Z
+	public final fun getAllowAnyService ()Z
+	public final fun getAllowAnyUser ()Z
 	public final fun getAnnotation ()Lkotlin/reflect/KClass;
 	public final fun getCapabilities ()Ljava/util/List;
 	public final fun getServices ()Ljava/util/List;
@@ -969,7 +975,7 @@ public final class misk/security/authz/AccessControlModule : misk/scope/ActionSc
 
 public final class misk/security/authz/AccessInterceptor : misk/ApplicationInterceptor {
 	public static final field Companion Lmisk/security/authz/AccessInterceptor$Companion;
-	public synthetic fun <init> (Ljava/util/Set;Ljava/util/Set;Lmisk/scope/ActionScoped;ZLjava/util/Set;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public synthetic fun <init> (Ljava/util/Set;Ljava/util/Set;Lmisk/scope/ActionScoped;ZLjava/util/Set;ZLkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun getAllowedCapabilities ()Ljava/util/Set;
 	public final fun getAllowedServices ()Ljava/util/Set;
 	public fun intercept (Lmisk/Chain;)Ljava/lang/Object;

misk/src/main/kotlin/misk/security/authz/AccessAnnotationEntry.kt

@@ -42,7 +42,9 @@ import kotlin.reflect.KClass
 data class AccessAnnotationEntry @JvmOverloads constructor(
   val annotation: KClass<out Annotation>,
   val services: List<String> = listOf(),
-  val capabilities: List<String> = listOf()
+  val capabilities: List<String> = listOf(),
+  val allowAnyService: Boolean = false,
+  val allowAnyUser: Boolean = false,
 )
 
 inline fun <reified T : Annotation> AccessAnnotationEntry(

misk/src/main/kotlin/misk/security/authz/AccessInterceptor.kt

@@ -16,7 +16,8 @@ class AccessInterceptor private constructor(
   val allowedCapabilities: Set<String>,
   private val caller: ActionScoped<MiskCaller?>,
   private val allowAnyService: Boolean,
-  private val excludeFromAllowAnyService: Set<String>
+  private val excludeFromAllowAnyService: Set<String>,
+  private val allowAnyUser: Boolean,
 ) : ApplicationInterceptor {
   override fun intercept(chain: Chain): Any {
     val caller = caller.get() ?: throw UnauthenticatedException()
@@ -31,12 +32,16 @@ class AccessInterceptor private constructor(
   /** Check whether the caller is allowed to access this endpoint */
   private fun isAuthorized(caller: MiskCaller): Boolean {
     // Allow if we don't have any requirements on service or capability
-    if (allowedServices.isEmpty() && allowedCapabilities.isEmpty() && !allowAnyService) return true
+    if (allowedServices.isEmpty() && allowedCapabilities.isEmpty() && !allowAnyService && !allowAnyUser) return true
 
     if (allowAnyService && caller.service != null && !excludeFromAllowAnyService.contains(caller.service)) {
       return true
     }
 
+    if (allowAnyUser && caller.user != null) {
+      return true
+    }
+
     // Allow if the caller has provided an allowed capability
     return caller.hasCapability(allowedCapabilities) || caller.isService(allowedServices)
   }
@@ -70,10 +75,11 @@ class AccessInterceptor private constructor(
         return null
       }
 
-      val allowAnyService = action.hasAnnotation<AllowAnyService>()
+      val allowAnyService = action.hasAnnotation<AllowAnyService>() || actionEntries.any { it.allowAnyService }
+      val allowAnyUser = actionEntries.any { it.allowAnyUser }
 
       // No access annotations. Fail with a useful message.
-      check(allowAnyService || actionEntries.isNotEmpty()) {
+      check(allowAnyService || allowAnyUser || actionEntries.isNotEmpty()) {
         val requiredAnnotations = mutableListOf<KClass<out Annotation>>()
         requiredAnnotations += Authenticated::class
         requiredAnnotations += Unauthenticated::class
@@ -114,7 +120,7 @@ class AccessInterceptor private constructor(
       val allowedServices = actionEntries.flatMap { it.services }.toSet()
       val allowedCapabilities = actionEntries.flatMap { it.capabilities }.toSet()
 
-      if (!allowAnyService && allowedServices.isEmpty() && allowedCapabilities.isEmpty()) {
+      if (!allowAnyService && allowedServices.isEmpty() && !allowAnyUser && allowedCapabilities.isEmpty()) {
         logger.warn { "${action.name}::${action.function.name}() has an empty set of allowed services and capabilities. This method of allowing all services and users is deprecated."}
       }
 
@@ -123,12 +129,17 @@ class AccessInterceptor private constructor(
         allowedCapabilities,
         caller,
         allowAnyService,
-        excludeFromAllowAnyService.toSet()
+        excludeFromAllowAnyService.toSet(),
+        allowAnyUser,
       )
     }
 
     private fun Authenticated.toAccessAnnotationEntry() = AccessAnnotationEntry(
-      Authenticated::class, services.toList(), capabilities.toList()
+      annotation = Authenticated::class,
+      services = services.toList(),
+      capabilities = capabilities.toList(),
+      allowAnyService = allowAnyService,
+      allowAnyUser = allowAnyUser
     )
 
     private inline fun <reified T : Annotation> Action.hasAnnotation() =

misk/src/test/kotlin/misk/web/actions/AuthenticationTest.kt

@@ -249,6 +249,28 @@ class AuthenticationTest {
       .isEqualTo("unauthorized")
   }
 
+  @Test fun testAllowAnyUser() {
+    val caller = MiskCaller(user = "sandy")
+    assertThat(
+      executeRequest(
+        path = "/allow_any_user_access",
+        user = caller.user
+      )
+    )
+      .isEqualTo("$caller authorized as any user")
+  }
+
+  @Test fun testAllowAnyUserDenyServiceOnly() {
+    val caller = MiskCaller(service = "test")
+    assertThat(
+      executeRequest(
+        path = "/allow_any_user_access",
+        user = caller.user
+      )
+    )
+      .isEqualTo("unauthenticated")
+  }
+
   private class MixesUnauthenticatedWithOtherAnnotations @Inject constructor() : WebAction {
     @Get("/oops")
     @Unauthenticated

misk/src/test/kotlin/misk/web/actions/TestWebActionModule.kt

@@ -49,6 +49,7 @@ class TestWebActionModule : KAbstractModule() {
     install(WebActionModule.create<EmptyAuthenticatedAccessAction>())
     install(WebActionModule.create<AllowAnyServiceAccessAction>())
     install(WebActionModule.create<AllowAnyServicePlusAuthenticatedAccessAction>())
+    install(WebActionModule.create<AllowAnyUserAccessAction>())
 
     multibind<AccessAnnotationEntry>().toInstance(
       AccessAnnotationEntry<CustomServiceAccess>(services = listOf("payments"))
@@ -187,3 +188,13 @@ class AllowAnyServicePlusAuthenticatedAccessAction @Inject constructor() : WebAc
   @Authenticated(services = ["web-proxy"], capabilities = ["admin"])
   fun get() = "${scopedCaller.get()} authorized as any service".toResponseBody()
 }
+
+class AllowAnyUserAccessAction @Inject constructor() : WebAction {
+  @Inject
+  lateinit var scopedCaller: ActionScoped<MiskCaller?>
+
+  @Get("/allow_any_user_access")
+  @ResponseContentType(MediaTypes.TEXT_PLAIN_UTF8)
+  @Authenticated(allowAnyUser = true)
+  fun get() = "${scopedCaller.get()} authorized as any user".toResponseBody()
+}