WX-1264 Don't expire an unexpirable filesystem (#7216)

azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java

@@ -520,10 +520,15 @@ private void setExpiryFromSAS(AzureSasCredential token) {
         this.expiry = expiryString.map(es -> Instant.parse(es)).orElse(null);
     }
 
+    /**
+     * Return true if this filesystem has SAS credentials with an expiration data attached, and we're within
+     * `buffer` of the expiration. Return false if our credentials don't come with an expiration, or we
+     * aren't within `buffer` of our expiration.
+     */
     public boolean isExpired(Duration buffer) {
         return Optional.ofNullable(this.expiry)
             .map(e -> Instant.now().plus(buffer).isAfter(e))
-            .orElse(true);
+            .orElse(false);
 
     }
 }

filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala

@@ -13,7 +13,7 @@ import java.net.URI
 import java.nio.file._
 import java.nio.file.spi.FileSystemProvider
 import java.time.temporal.ChronoUnit
-import java.time.{Duration, Instant, OffsetDateTime}
+import java.time.{Duration, OffsetDateTime}
 import java.util.UUID
 import scala.jdk.CollectionConverters._
 import scala.util.{Failure, Success, Try}
@@ -32,10 +32,6 @@ case class AzureFileSystemAPI(private val provider: FileSystemProvider = new Azu
   * See BlobSasTokenGenerator for more information on how a SAS token is generated
   */
 object BlobFileSystemManager {
-  def parseTokenExpiry(token: AzureSasCredential): Option[Instant] = for {
-    expiryString <- token.getSignature.split("&").find(_.startsWith("se")).map(_.replaceFirst("se=","")).map(_.replace("%3A", ":"))
-    instant = Instant.parse(expiryString)
-  } yield instant
 
   def buildConfigMap(credential: AzureSasCredential, container: BlobContainerName): Map[String, Object] = {
     // Special handling is done here to provide a special key value pair if the placeholder token is provided
@@ -226,9 +222,12 @@ case class NativeBlobSasTokenGenerator(subscription: Option[SubscriptionId] = No
     *
     * @return an AzureSasCredential for accessing a blob container
     */
-  def generateBlobSasToken(endpoint: EndpointURL, container: BlobContainerName): Try[AzureSasCredential] = for {
-    bcc <- AzureUtils.buildContainerClientFromLocalEnvironment(container.toString, endpoint.toString, subscription.map(_.toString))
-    bsssv = new BlobServiceSasSignatureValues(OffsetDateTime.now.plusDays(1), bcsp)
-    asc = new AzureSasCredential(bcc.generateSas(bsssv))
-  } yield asc
+  def generateBlobSasToken(endpoint: EndpointURL, container: BlobContainerName): Try[AzureSasCredential] = {
+    val c = AzureUtils.buildContainerClientFromLocalEnvironment(container.toString, endpoint.toString, subscription.map(_.toString))
+
+    c.map { bcc =>
+        val bsssv = new BlobServiceSasSignatureValues(OffsetDateTime.now.plusDays(1), bcsp)
+        new AzureSasCredential(bcc.generateSas(bsssv))
+      }.orElse(Try(BlobFileSystemManager.PLACEHOLDER_TOKEN))
+  }
 }

filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala

@@ -1,25 +1,61 @@
 package cromwell.filesystems.blob
 
+import com.azure.core.credential.AzureSasCredential
 import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider}
 import org.scalatest.flatspec.AnyFlatSpec
 import org.scalatest.matchers.should.Matchers
 
-import java.time.Instant
+import java.time.{Duration, Instant}
+import java.time.temporal.ChronoUnit
 import scala.compat.java8.OptionConverters._
 import scala.jdk.CollectionConverters._
 
 class AzureFileSystemSpec extends AnyFlatSpec with Matchers {
-  val now = Instant.now()
-  val container = BlobContainerName("testConainer")
-  val exampleSas = BlobPathBuilderFactorySpec.buildExampleSasToken(now)
-  val exampleConfig = BlobFileSystemManager.buildConfigMap(exampleSas, container)
-  val exampleStorageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount")
-  val exampleCombinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(exampleStorageEndpoint, container)
 
-  it should "parse an expiration from a sas token" in {
+  val fiveMinutes: Duration = Duration.of(5, ChronoUnit.MINUTES)
+
+  private def makeFilesystemWithExpiration(expiration: Instant): AzureFileSystem =
+    makeFilesystemWithCreds(BlobPathBuilderFactorySpec.buildExampleSasToken(expiration))
+
+  private def makeFilesystemWithCreds(creds: AzureSasCredential): AzureFileSystem = {
+    val storageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount")
+    val container = BlobContainerName("testContainer")
+    val combinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(storageEndpoint, container)
+
     val provider = new AzureFileSystemProvider()
-    val filesystem : AzureFileSystem = provider.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem]
+    provider.newFileSystem(
+      combinedEndpoint,
+      BlobFileSystemManager.buildConfigMap(creds, container).asJava
+    ).asInstanceOf[AzureFileSystem]
+  }
+
+  it should "parse an expiration from a sas token" in {
+    val now = Instant.now()
+    val filesystem : AzureFileSystem = makeFilesystemWithExpiration(now)
     filesystem.getExpiry.asScala shouldBe Some(now)
-    filesystem.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true
+    filesystem.getFileStores.asScala.map(_.name()).exists(_ == "testContainer") shouldBe true
+  }
+
+  it should "not be expired when the token is fresh" in {
+    val anHourFromNow = Instant.now().plusSeconds(3600)
+    val filesystem : AzureFileSystem = makeFilesystemWithExpiration(anHourFromNow)
+    filesystem.isExpired(fiveMinutes) shouldBe false
+  }
+
+  it should "be expired when we're within the buffer" in {
+    val threeMinutesFromNow = Instant.now().plusSeconds(180)
+    val filesystem : AzureFileSystem = makeFilesystemWithExpiration(threeMinutesFromNow)
+    filesystem.isExpired(fiveMinutes) shouldBe true
+  }
+
+  it should "be expired when the token is stale" in {
+    val anHourAgo = Instant.now().minusSeconds(3600)
+    val filesystem : AzureFileSystem = makeFilesystemWithExpiration(anHourAgo)
+    filesystem.isExpired(fiveMinutes) shouldBe true
+  }
+
+  it should "not be expired with public credentials" in {
+    val fileSystem = makeFilesystemWithCreds(BlobFileSystemManager.PLACEHOLDER_TOKEN)
+    fileSystem.isExpired(fiveMinutes) shouldBe false
   }
 }

filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala

@@ -32,13 +32,6 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga
     testToken.getSignature should equal(sourceToken)
   }
 
-  it should "parse an expiration time from a sas token" in {
-    val expiryTime = generateTokenExpiration(20L)
-    val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(expiryTime)
-    val expiry = BlobFileSystemManager.parseTokenExpiry(sasToken)
-    expiry should contain(expiryTime)
-  }
-
   it should "test that a filesystem gets closed correctly" in {
     val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount")
     val container = BlobContainerName("test")