[Storage] SAS version cleanup. (#22255)

* deprecate BlobSasServiceVersion. Make version not settable. * More of that. * deprecate old constatnt... * fix test. * rework constants again. * ... * let's run some live tests. * fix live tests. * fix datalake tests. * PR feedback. * use separate constant for deprecated part. * remind us about this problem in files and queues. * fix ci.
Azure · Jun 15, 2021 · 8f341f2 · 8f341f2
1 parent f3fac45
commit 8f341f2
Show file tree

Hide file tree

Showing 26 changed files with 631 additions and 282 deletions.
diff --git a/sdk/storage/azure-storage-blob/pom.xml b/sdk/storage/azure-storage-blob/pom.xml
@@ -307,5 +307,44 @@
         </plugins>
       </build>
     </profile>
+    <profile>
+      <id>inject-sas-service-version</id>
+      <activation>
+        <property><name>env.AZURE_LIVE_TEST_SERVICE_VERSION</name></property>
+      </activation>
+      <build>
+        <plugins>
+          <plugin>
+            <groupId>org.codehaus.mojo</groupId>
+            <artifactId>build-helper-maven-plugin</artifactId>
+            <version>3.0.0</version> <!-- {x-version-update;org.codehaus.mojo:build-helper-maven-plugin;external_dependency} -->
+            <executions>
+              <execution>
+                <id>regex-property</id>
+                <goals>
+                  <goal>regex-property</goal>
+                </goals>
+                <configuration>
+                  <name>AZURE_STORAGE_SAS_SERVICE_VERSION</name>
+                  <value>${env.AZURE_LIVE_TEST_SERVICE_VERSION}</value>
+                  <regex>V(\d+)_(\d+)_(\d+)</regex>
+                  <replacement>$1-$2-$3</replacement>
+                </configuration>
+              </execution>
+            </executions>
+          </plugin>
+          <plugin>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-surefire-plugin</artifactId>
+            <version>3.0.0-M3</version> <!-- {x-version-update;org.apache.maven.plugins:maven-surefire-plugin;external_dependency} -->
+            <configuration>
+              <systemPropertyVariables>
+                <AZURE_STORAGE_SAS_SERVICE_VERSION>${AZURE_STORAGE_SAS_SERVICE_VERSION}</AZURE_STORAGE_SAS_SERVICE_VERSION>
+              </systemPropertyVariables>
+            </configuration>
+          </plugin>
+        </plugins>
+      </build>
+    </profile>
   </profiles>
 </project>
diff --git a/...torage-blob/src/main/java/com/azure/storage/blob/implementation/util/BlobSasImplUtil.java b/...torage-blob/src/main/java/com/azure/storage/blob/implementation/util/BlobSasImplUtil.java
@@ -3,14 +3,15 @@
 
 package com.azure.storage.blob.implementation.util;
 
+import com.azure.core.util.Configuration;
 import com.azure.core.util.Context;
 import com.azure.core.util.CoreUtils;
 import com.azure.core.util.logging.ClientLogger;
+import com.azure.storage.blob.BlobServiceVersion;
 import com.azure.storage.blob.models.UserDelegationKey;
 import com.azure.storage.blob.sas.BlobContainerSasPermission;
 import com.azure.storage.blob.sas.BlobSasPermission;
 import com.azure.storage.blob.sas.BlobServiceSasSignatureValues;
-import com.azure.storage.blob.sas.BlobSasServiceVersion;
 import com.azure.storage.common.StorageSharedKeyCredential;
 import com.azure.storage.common.implementation.Constants;
 import com.azure.storage.common.implementation.StorageImplUtils;
@@ -49,9 +50,10 @@ public class BlobSasImplUtil {
      */
     private static final String SAS_CONTAINER_CONSTANT = "c";
 
-    private final ClientLogger logger = new ClientLogger(BlobSasImplUtil.class);
+    private static final ClientLogger LOGGER = new ClientLogger(BlobSasImplUtil.class);
 
-    private String version;
+    private static final String VERSION = Configuration.getGlobalConfiguration()
+        .get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.getLatest().getVersion());
 
     private SasProtocol protocol;
 
@@ -112,10 +114,9 @@ public BlobSasImplUtil(BlobServiceSasSignatureValues sasValues, String container
         String snapshotId, String versionId) {
         Objects.requireNonNull(sasValues);
         if (snapshotId != null && versionId != null) {
-            throw logger.logExceptionAsError(
+            throw LOGGER.logExceptionAsError(
                 new IllegalArgumentException("'snapshot' and 'versionId' cannot be used at the same time."));
         }
-        this.version = null; /* Setting this to null forces the latest service version - see ensureState. */
         this.protocol = sasValues.getProtocol();
         this.startTime = sasValues.getStartTime();
         this.expiryTime = sasValues.getExpiryTime();
@@ -150,7 +151,7 @@ public String generateSas(StorageSharedKeyCredential storageSharedKeyCredentials
         // Signature is generated on the un-url-encoded values.
         final String canonicalName = getCanonicalName(storageSharedKeyCredentials.getAccountName());
         final String stringToSign = stringToSign(canonicalName);
-        StorageImplUtils.logStringToSign(logger, stringToSign, context);
+        StorageImplUtils.logStringToSign(LOGGER, stringToSign, context);
         final String signature = storageSharedKeyCredentials.computeHmac256(stringToSign);
 
         return encode(null /* userDelegationKey */, signature);
@@ -173,7 +174,7 @@ public String generateUserDelegationSas(UserDelegationKey delegationKey, String
         // Signature is generated on the un-url-encoded values.
         final String canonicalName = getCanonicalName(accountName);
         final String stringToSign = stringToSign(delegationKey, canonicalName);
-        StorageImplUtils.logStringToSign(logger, stringToSign, context);
+        StorageImplUtils.logStringToSign(LOGGER, stringToSign, context);
         String signature = StorageImplUtils.computeHMac256(delegationKey.getValue(), stringToSign);
 
         return encode(delegationKey, signature);
@@ -192,7 +193,7 @@ private String encode(UserDelegationKey userDelegationKey, String signature) {
          */
         StringBuilder sb = new StringBuilder();
 
-        tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_SERVICE_VERSION, this.version);
+        tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_SERVICE_VERSION, VERSION);
         tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_PROTOCOL, this.protocol);
         tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_START_TIME, formatQueryParameterDate(this.startTime));
         tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_EXPIRY_TIME, formatQueryParameterDate(this.expiryTime));
@@ -246,13 +247,9 @@ private String encode(UserDelegationKey userDelegationKey, String signature) {
      * https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Blobs/src/Sas/BlobSasBuilder.cs
      */
     private void ensureState() {
-        if (version == null) {
-            version = BlobSasServiceVersion.getLatest().getVersion();
-        }
-
         if (identifier == null) {
             if (expiryTime == null || permissions == null) {
-                throw logger.logExceptionAsError(new IllegalStateException("If identifier is not set, expiry time "
+                throw LOGGER.logExceptionAsError(new IllegalStateException("If identifier is not set, expiry time "
                     + "and permissions must be set"));
             }
         }
@@ -279,7 +276,7 @@ private void ensureState() {
                     break;
                 default:
                     // We won't reparse the permissions if we don't know the type.
-                    logger.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
+                    LOGGER.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
                     break;
             }
         }
@@ -306,7 +303,7 @@ private String stringToSign(String canonicalName) {
             this.identifier == null ? "" : this.identifier,
             this.sasIpRange == null ? "" : this.sasIpRange.toString(),
             this.protocol == null ? "" : this.protocol.toString(),
-            version,
+            VERSION,
             resource,
             versionSegment == null ? "" : versionSegment,
             this.cacheControl == null ? "" : this.cacheControl,
@@ -319,30 +316,55 @@ private String stringToSign(String canonicalName) {
 
     private String stringToSign(final UserDelegationKey key, String canonicalName) {
         String versionSegment = this.snapshotId == null ? this.versionId : this.snapshotId;
-        return String.join("\n",
-            this.permissions == null ? "" : this.permissions,
-            this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
-            this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
-            canonicalName,
-            key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
-            key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
-            key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
-            key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
-            key.getSignedService() == null ? "" : key.getSignedService(),
-            key.getSignedVersion() == null ? "" : key.getSignedVersion(),
-            this.authorizedAadObjectId == null ? "" : this.authorizedAadObjectId,
-            "", /* suoid - empty since this applies to HNS only accounts. */
-            this.correlationId == null ? "" : this.correlationId,
-            this.sasIpRange == null ? "" : this.sasIpRange.toString(),
-            this.protocol == null ? "" : this.protocol.toString(),
-            version,
-            resource,
-            versionSegment == null ? "" : versionSegment,
-            this.cacheControl == null ? "" : this.cacheControl,
-            this.contentDisposition == null ? "" : this.contentDisposition,
-            this.contentEncoding == null ? "" : this.contentEncoding,
-            this.contentLanguage == null ? "" : this.contentLanguage,
-            this.contentType == null ? "" : this.contentType
-        );
+        if (VERSION.compareTo(BlobServiceVersion.V2019_12_12.getVersion()) <= 0) {
+            return String.join("\n",
+                this.permissions == null ? "" : this.permissions,
+                this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
+                this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
+                canonicalName,
+                key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
+                key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
+                key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
+                key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
+                key.getSignedService() == null ? "" : key.getSignedService(),
+                key.getSignedVersion() == null ? "" : key.getSignedVersion(),
+                this.sasIpRange == null ? "" : this.sasIpRange.toString(),
+                this.protocol == null ? "" : this.protocol.toString(),
+                VERSION,
+                resource,
+                versionSegment == null ? "" : versionSegment,
+                this.cacheControl == null ? "" : this.cacheControl,
+                this.contentDisposition == null ? "" : this.contentDisposition,
+                this.contentEncoding == null ? "" : this.contentEncoding,
+                this.contentLanguage == null ? "" : this.contentLanguage,
+                this.contentType == null ? "" : this.contentType
+            );
+        } else {
+            return String.join("\n",
+                this.permissions == null ? "" : this.permissions,
+                this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
+                this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
+                canonicalName,
+                key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
+                key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
+                key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
+                key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
+                key.getSignedService() == null ? "" : key.getSignedService(),
+                key.getSignedVersion() == null ? "" : key.getSignedVersion(),
+                this.authorizedAadObjectId == null ? "" : this.authorizedAadObjectId,
+                "", /* suoid - empty since this applies to HNS only accounts. */
+                this.correlationId == null ? "" : this.correlationId,
+                this.sasIpRange == null ? "" : this.sasIpRange.toString(),
+                this.protocol == null ? "" : this.protocol.toString(),
+                VERSION,
+                resource,
+                versionSegment == null ? "" : versionSegment,
+                this.cacheControl == null ? "" : this.cacheControl,
+                this.contentDisposition == null ? "" : this.contentDisposition,
+                this.contentEncoding == null ? "" : this.contentEncoding,
+                this.contentLanguage == null ? "" : this.contentLanguage,
+                this.contentType == null ? "" : this.contentType
+            );
+        }
     }
 }
diff --git a/...ge/azure-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobSasServiceVersion.java b/...ge/azure-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobSasServiceVersion.java
@@ -7,7 +7,9 @@
 
 /**
  * The versions of Azure Storage Blob Sas supported by this client library.
+ * @deprecated The version is set to the latest version of sas.
  */
+@Deprecated
 public enum BlobSasServiceVersion implements ServiceVersion {
     V2019_02_02("2019-02-02"),
     V2019_07_07("2019-07-07"),

diff --git a/...-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobServiceSasSignatureValues.java b/...-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobServiceSasSignatureValues.java
@@ -3,8 +3,10 @@
 
 package com.azure.storage.blob.sas;
 
+import com.azure.core.util.Configuration;
 import com.azure.core.util.CoreUtils;
 import com.azure.core.util.logging.ClientLogger;
+import com.azure.storage.blob.BlobServiceVersion;
 import com.azure.storage.blob.models.UserDelegationKey;
 import com.azure.storage.common.Utility;
 import com.azure.storage.common.implementation.Constants;
@@ -43,9 +45,16 @@ public final class BlobServiceSasSignatureValues {
      */
     private static final String SAS_CONTAINER_CONSTANT = "c";
 
-    private final ClientLogger logger = new ClientLogger(BlobServiceSasSignatureValues.class);
+    private static final ClientLogger LOGGER = new ClientLogger(BlobServiceSasSignatureValues.class);
 
-    private final String version = BlobSasServiceVersion.getLatest().getVersion();
+    private static final String VERSION = Configuration.getGlobalConfiguration()
+        .get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.getLatest().getVersion());
+
+    /**
+     * Pin down to highest version that worked with string to sign defined here.
+     */
+    private static final String VERSION_DEPRECATED_STRING_TO_SIGN = Configuration.getGlobalConfiguration()
+        .get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.V2019_12_12.getVersion());
 
     private SasProtocol protocol;
 
@@ -171,7 +180,7 @@ public BlobServiceSasSignatureValues(String version, SasProtocol sasProtocol, Of
      * targeted by the library.
      */
     public String getVersion() {
-        return version;
+        return VERSION;
     }
 
     /**
@@ -543,8 +552,6 @@ public BlobServiceSasSignatureValues setCorrelationId(String correlationId) {
      *
      * <p><strong>Notes on SAS generation</strong></p>
      * <ul>
-     * <li>If {@link #setVersion(String) version} is not set,
-     * the {@link BlobSasServiceVersion#getLatest() latest service version} is used.</li>
      * <li>If {@link #setIdentifier(String) identifier} is set, {@link #setExpiryTime(OffsetDateTime) expiryTime} and
      * permissions should not be set. These values are inherited from the stored access policy.</li>
      * <li>Otherwise, {@link #setExpiryTime(OffsetDateTime) expiryTime} and {@link #getPermissions() permissions} must
@@ -582,7 +589,7 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(
         final String canonicalName = getCanonicalName(storageSharedKeyCredentials.getAccountName());
         final String signature = storageSharedKeyCredentials.computeHmac256(stringToSign(canonicalName));
 
-        return new BlobServiceSasQueryParameters(this.version, this.protocol, this.startTime, this.expiryTime,
+        return new BlobServiceSasQueryParameters(VERSION, this.protocol, this.startTime, this.expiryTime,
             this.sasIpRange, this.identifier, this.resource, this.permissions, signature, this.cacheControl,
             this.contentDisposition, this.contentEncoding, this.contentLanguage, this.contentType, null /* delegate */);
     }
@@ -592,8 +599,6 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(
      *
      * <p><strong>Notes on SAS generation</strong></p>
      * <ul>
-     * <li>If {@link #setVersion(String) version} is not set,
-     * the {@link BlobSasServiceVersion#getLatest() latest service version} is used.</li>
      * <li>If {@link #setIdentifier(String) identifier} is set, {@link #setExpiryTime(OffsetDateTime) expiryTime} and
      * permissions should not be set. These values are inherited from the stored access policy.</li>
      * <li>Otherwise, {@link #setExpiryTime(OffsetDateTime) expiryTime} and {@link #getPermissions() permissions} must
@@ -636,7 +641,7 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(UserDelegationKe
         String signature = StorageImplUtils.computeHMac256(
             delegationKey.getValue(), stringToSign(delegationKey, canonicalName));
 
-        return new BlobServiceSasQueryParameters(this.version, this.protocol, this.startTime, this.expiryTime,
+        return new BlobServiceSasQueryParameters(VERSION, this.protocol, this.startTime, this.expiryTime,
             this.sasIpRange, null /* identifier */, this.resource, this.permissions, signature,
             this.cacheControl, this.contentDisposition, this.contentEncoding, this.contentLanguage, this.contentType,
             delegationKey);
@@ -676,7 +681,7 @@ private void ensureState() {
                     break;
                 default:
                     // We won't reparse the permissions if we don't know the type.
-                    logger.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
+                    LOGGER.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
                     break;
             }
         }
@@ -702,8 +707,7 @@ private String stringToSign(String canonicalName) {
             this.identifier == null ? "" : this.identifier,
             this.sasIpRange == null ? "" : this.sasIpRange.toString(),
             this.protocol == null ? "" : this.protocol.toString(),
-            version, /* Pin down to version so old string to sign works. This is reflected in the declaration of
-            version */
+            VERSION_DEPRECATED_STRING_TO_SIGN, /* Pin down to version so old string to sign works. */
             resource,
             this.snapshotId == null ? "" : this.snapshotId,
             this.cacheControl == null ? "" : this.cacheControl,
@@ -728,8 +732,7 @@ private String stringToSign(final UserDelegationKey key, String canonicalName) {
             key.getSignedVersion() == null ? "" : key.getSignedVersion(),
             this.sasIpRange == null ? "" : this.sasIpRange.toString(),
             this.protocol == null ? "" : this.protocol.toString(),
-            version, /* Pin down to version so old string to sign works. This is reflected in the declaration of
-            version */
+            VERSION_DEPRECATED_STRING_TO_SIGN, /* Pin down to version so old string to sign works. */
             resource,
             this.snapshotId == null ? "" : this.snapshotId,
             this.cacheControl == null ? "" : this.cacheControl,

diff --git a/...e/azure-storage-blob/src/test/java/com/azure/storage/blob/BlobServiceSasModelsTest.groovy b/...e/azure-storage-blob/src/test/java/com/azure/storage/blob/BlobServiceSasModelsTest.groovy
@@ -217,18 +217,6 @@ class BlobServiceSasModelsTest extends Specification {
         ex.getMessage().contains("accountName")
     }
 
-    def "ensure state version"() {
-        when:
-        BlobSasImplUtil implUtil = new BlobSasImplUtil(new BlobServiceSasSignatureValues("id"), "container")
-        implUtil.version = null
-        implUtil.ensureState()
-
-        then:
-        implUtil.version // Version is set
-        implUtil.resource == "c" // Default resource is container
-        !implUtil.permissions // Identifier was used so permissions is null
-    }
-
     def "ensure state illegal argument"() {
         when:
         BlobSasImplUtil implUtil = new BlobSasImplUtil(new BlobServiceSasSignatureValues(), null)