From fc36f35111fb296d04a80ce0d85d8e865f1bf08e Mon Sep 17 00:00:00 2001 From: Diego Luces Date: Wed, 21 Sep 2016 10:05:37 -0600 Subject: [PATCH 1/4] Make ServiceRestProxy::groupQueryValues static --- src/Blob/BlobRestProxy.php | 2 +- src/Common/Internal/ServiceRestProxy.php | 2 +- .../unit/Common/Internal/ServiceRestProxyTest.php | 15 ++++++--------- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/Blob/BlobRestProxy.php b/src/Blob/BlobRestProxy.php index 40954b0ae..a1cef6050 100644 --- a/src/Blob/BlobRestProxy.php +++ b/src/Blob/BlobRestProxy.php @@ -1173,7 +1173,7 @@ public function listBlobs($container, $options = null) $includeSnapshots = $options->getIncludeSnapshots(); $includeUncommittedBlobs = $options->getIncludeUncommittedBlobs(); - $includeValue = $this->groupQueryValues( + $includeValue = static::groupQueryValues( array( $includeMetadata ? 'metadata' : null, $includeSnapshots ? 'snapshots' : null, diff --git a/src/Common/Internal/ServiceRestProxy.php b/src/Common/Internal/ServiceRestProxy.php index 5b946ec81..98dd1bdfc 100644 --- a/src/Common/Internal/ServiceRestProxy.php +++ b/src/Common/Internal/ServiceRestProxy.php @@ -503,7 +503,7 @@ public function addPostParameter( * * @return string */ - public function groupQueryValues($values) + public static function groupQueryValues($values) { Validate::isArray($values, 'values'); $joined = Resources::EMPTY_STRING; diff --git a/tests/unit/Common/Internal/ServiceRestProxyTest.php b/tests/unit/Common/Internal/ServiceRestProxyTest.php index 336d82df2..2859620da 100644 --- a/tests/unit/Common/Internal/ServiceRestProxyTest.php +++ b/tests/unit/Common/Internal/ServiceRestProxyTest.php @@ -147,16 +147,15 @@ public function testAddOptionalSourceAccessContitionHeader($restRestProxy) /** * @covers MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy::groupQueryValues - * @depends test__construct */ - public function testGroupQueryValues($restRestProxy) + public function testGroupQueryValues() { // Setup $values = array('A', 'B', 'C'); $expected = 'A,B,C'; // Test - $actual = $restRestProxy->groupQueryValues($values); + $actual = ServiceRestProxy::groupQueryValues($values); // Assert $this->assertEquals($expected, $actual); @@ -164,15 +163,14 @@ public function testGroupQueryValues($restRestProxy) /** * @covers MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy::groupQueryValues - * @depends test__construct */ - public function testGroupQueryValuesWithNulls($restRestProxy) + public function testGroupQueryValuesWithNulls() { // Setup $values = array(null, '', null); // Test - $actual = $restRestProxy->groupQueryValues($values); + $actual = ServiceRestProxy::groupQueryValues($values); // Assert $this->assertTrue(empty($actual)); @@ -180,16 +178,15 @@ public function testGroupQueryValuesWithNulls($restRestProxy) /** * @covers MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy::groupQueryValues - * @depends test__construct */ - public function testGroupQueryValuesWithMix($restRestProxy) + public function testGroupQueryValuesWithMix() { // Setup $values = array(null, 'B', 'C', ''); $expected = 'B,C'; // Test - $actual = $restRestProxy->groupQueryValues($values); + $actual = ServiceRestProxy::groupQueryValues($values); // Assert $this->assertEquals($expected, $actual); From 3eac8c24a1d55036a3f371ea7eaf693494d3c686 Mon Sep 17 00:00:00 2001 From: Diego Luces Date: Wed, 21 Sep 2016 10:54:33 -0600 Subject: [PATCH 2/4] Sort query param values in ServiceRestProxy::groupedQueryParams --- src/Common/Internal/ServiceRestProxy.php | 2 ++ .../Common/Internal/ServiceRestProxyTest.php | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/Common/Internal/ServiceRestProxy.php b/src/Common/Internal/ServiceRestProxy.php index 98dd1bdfc..9c5d02e41 100644 --- a/src/Common/Internal/ServiceRestProxy.php +++ b/src/Common/Internal/ServiceRestProxy.php @@ -508,6 +508,8 @@ public static function groupQueryValues($values) Validate::isArray($values, 'values'); $joined = Resources::EMPTY_STRING; + sort($values); + foreach ($values as $value) { if (!is_null($value) && !empty($value)) { $joined .= $value . Resources::SEPARATOR; diff --git a/tests/unit/Common/Internal/ServiceRestProxyTest.php b/tests/unit/Common/Internal/ServiceRestProxyTest.php index 2859620da..3e00e75dc 100644 --- a/tests/unit/Common/Internal/ServiceRestProxyTest.php +++ b/tests/unit/Common/Internal/ServiceRestProxyTest.php @@ -161,6 +161,22 @@ public function testGroupQueryValues() $this->assertEquals($expected, $actual); } + /** + * @covers MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy::groupQueryValues + */ + public function testGroupQueryValuesWithUnorderedValues() + { + // Setup + $values = array('B', 'C', 'A'); + $expected = 'A,B,C'; + + // Test + $actual = ServiceRestProxy::groupQueryValues($values); + + // Assert + $this->assertEquals($expected, $actual); + } + /** * @covers MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy::groupQueryValues */ From 51bd0121bba6a7a24eab5471718ddea18c3d13ea Mon Sep 17 00:00:00 2001 From: Diego Luces Date: Wed, 21 Sep 2016 11:30:39 -0600 Subject: [PATCH 3/4] Assume that StorageAuthScheme::computeCanonicalizedResource receives ordered query param values --- src/Common/Internal/Authentication/StorageAuthScheme.php | 9 +++------ .../Internal/Authentication/StorageAuthSchemeTest.php | 9 ++++++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/Common/Internal/Authentication/StorageAuthScheme.php b/src/Common/Internal/Authentication/StorageAuthScheme.php index 7dd4b1f2c..7cb3d3a94 100644 --- a/src/Common/Internal/Authentication/StorageAuthScheme.php +++ b/src/Common/Internal/Authentication/StorageAuthScheme.php @@ -181,12 +181,9 @@ protected function computeCanonicalizedResource($url, $queryParams) // 9. Group query parameters // 10. Append a new line character (\n) after each name-value pair. foreach ($queryParams as $key => $value) { - // Grouping query parameters - $values = explode(Resources::SEPARATOR, $value); - sort($values); - $separated = implode(Resources::SEPARATOR, $values); - - $canonicalizedResource .= "\n" . $key . ':' . $separated; + // $value must already be ordered lexicographically + // See: ServiceRestProxy::groupQueryValues + $canonicalizedResource .= "\n" . $key . ':' . $value; } return $canonicalizedResource; diff --git a/tests/unit/Common/Internal/Authentication/StorageAuthSchemeTest.php b/tests/unit/Common/Internal/Authentication/StorageAuthSchemeTest.php index 48c42467e..a0e8fe472 100644 --- a/tests/unit/Common/Internal/Authentication/StorageAuthSchemeTest.php +++ b/tests/unit/Common/Internal/Authentication/StorageAuthSchemeTest.php @@ -24,6 +24,7 @@ namespace MicrosoftAzure\Storage\Tests\Unit\Common\Internal\Authentication; use MicrosoftAzure\Storage\Common\Internal\Authentication\StorageAuthScheme; +use MicrosoftAzure\Storage\Common\Internal\ServiceRestProxy; use MicrosoftAzure\Storage\Tests\Unit\Utilities; use MicrosoftAzure\Storage\Tests\Mock\Common\Internal\Authentication\StorageAuthSchemeMock; use MicrosoftAzure\Storage\Tests\Framework\TestResources; @@ -94,7 +95,13 @@ public function testComputeCanonicalizedResourceMockMultipleValues() { $queryVariables = array(); $queryVariables['COMP'] = 'list'; - $queryVariables[Resources::QP_INCLUDE] = 'snapshots,metadata,uncommittedblobs'; + $queryVariables[Resources::QP_INCLUDE] = ServiceRestProxy::groupQueryValues( + array( + 'snapshots', + 'metadata', + 'uncommittedblobs' + ) + ); $expectedQueryPart = "comp:list\ninclude:metadata,snapshots,uncommittedblobs"; $accountName = TestResources::ACCOUNT_NAME; $url = TestResources::URI1; From d5385c84f661063320883955b0461abb4e8148e5 Mon Sep 17 00:00:00 2001 From: Diego Luces Date: Wed, 21 Sep 2016 11:35:44 -0600 Subject: [PATCH 4/4] Add entry for fix in ChangeLog.md --- ChangeLog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog.md b/ChangeLog.md index 4ae32fb2b..be3f21f08 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -23,6 +23,7 @@ MicrosoftAzure\Storage\Queue\Models\GetQueueMetadataResult.setMetadata MicrosoftAzure\Storage\Queue\Models\Queue.setMetadata ``` * Removed test code from composer package. +* `StorageAuthScheme::computeCanonicalizedResource` assumes that the query parameters are already grouped. That is, multi-value query parameters must be assembled using `ServiceRestProxy::groupQueryValues`. This fixes an issue with other single-value query parameters that might contain the separator character in the value. Blob * Added support for user to upload large files with minimum memory usage.