From 04f4d3a5cf14658d2be8a1123a334f1a480c2c87 Mon Sep 17 00:00:00 2001 From: George Arama <50641385+gearama@users.noreply.github.com> Date: Thu, 26 Aug 2021 10:45:36 -0700 Subject: [PATCH] Api review feedback (#2795) * first set of changes * another set * add new constructor * format files, remove pedantinc ; * factory * format * expect_death not behaving as expected --- .../CMakeLists.txt | 1 + .../secrets/keyvault_deleted_secret.hpp | 4 ++-- .../keyvault/secrets/keyvault_operations.hpp | 10 ++++----- .../keyvault/secrets/keyvault_secret.hpp | 12 ---------- .../secrets/keyvault_secret_properties.hpp | 8 ++++++- .../azure/keyvault/secrets/secret_client.hpp | 4 ++-- .../src/keyvault_operations.cpp | 9 ++++---- .../src/keyvault_secret_properties.cpp | 22 +++++++++++++++++++ .../src/secret_client.cpp | 9 ++++---- .../sample1-basic-operations.cpp | 4 +++- .../sample2-backup-restore.cpp | 9 +++++--- .../sample2-backup-restore.md | 2 +- .../sample3-delete-recover.cpp | 9 +++++++- .../sample4-get-secrets-deleted.cpp | 14 ++++++++++-- .../ut/secret_get_client_deserialize_test.cpp | 11 ++++++++++ .../ut/secret_get_client_deserialize_test.hpp | 4 ++-- 16 files changed, 92 insertions(+), 40 deletions(-) create mode 100644 sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_secret_properties.cpp diff --git a/sdk/keyvault/azure-security-keyvault-secrets/CMakeLists.txt b/sdk/keyvault/azure-security-keyvault-secrets/CMakeLists.txt index 83b1fddf7..a072bad15 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/CMakeLists.txt +++ b/sdk/keyvault/azure-security-keyvault-secrets/CMakeLists.txt @@ -51,6 +51,7 @@ set( src/secret_serializers.cpp src/keyvault_operations.cpp src/keyvault_secret_paged_response.cpp + src/keyvault_secret_properties.cpp ) add_library(azure-security-keyvault-secrets ${AZURE_SECURITY_KEYVAULT_SECRETS_HEADER} ${AZURE_SECURITY_KEYVAULT_SECRETS_SOURCE}) diff --git a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_deleted_secret.hpp b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_deleted_secret.hpp index 171cb3ba2..7265020a3 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_deleted_secret.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_deleted_secret.hpp @@ -26,12 +26,12 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { /** * @brief The time when the secret is scheduled to be purged, in UTC. */ - Azure::DateTime ScheduledPurgeDate; + Azure::Nullable ScheduledPurgeDate; /** * @brief The time when the secret was deleted, in UTC. */ - Azure::DateTime DeletedOn; + Azure::Nullable DeletedOn; /** * @brief Default constructor. diff --git a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_operations.hpp b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_operations.hpp index da720b1b1..270b641a4 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_operations.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_operations.hpp @@ -20,15 +20,15 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { /** * @brief Represents a long running operation to restore a deleted secret. */ - class RecoverDeletedSecretOperation final : public Azure::Core::Operation { + class RecoverDeletedSecretOperation final : public Azure::Core::Operation { private: friend class SecretClient; std::shared_ptr m_secretClient; - KeyVaultSecret m_value; + SecretProperties m_value; std::string m_continuationToken; - Azure::Response PollUntilDoneInternal( + Azure::Response PollUntilDoneInternal( std::chrono::milliseconds period, Azure::Core::Context& context) override; @@ -43,7 +43,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { */ RecoverDeletedSecretOperation( std::shared_ptr secretClient, - Azure::Response response); + Azure::Response response); RecoverDeletedSecretOperation( std::string resumeToken, @@ -65,7 +65,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { * * @return A Secret object. */ - KeyVaultSecret Value() const override { return m_value; } + SecretProperties Value() const override { return m_value; } /** * @brief Get an Url as string which can be used to get the status of the operation. diff --git a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret.hpp b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret.hpp index 28f8026ad..634c15f9d 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret.hpp @@ -42,18 +42,6 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { */ KeyVaultSecret() = default; - /** - * @brief The vault url of the secret. - * - */ - std::string VaultUrl() { return Properties.VaultUrl; } - - /** - * @brief The version of the secret. - * - */ - std::string Version() { return Properties.Version; } - /** * @brief Construct a new Secret object. * diff --git a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret_properties.hpp b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret_properties.hpp index 64aaae68a..d3f7d1d71 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret_properties.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_secret_properties.hpp @@ -10,7 +10,7 @@ #include #include - +#include #include namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { @@ -134,5 +134,11 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { throw std::invalid_argument("Name cannot be empty"); } }; + + /** + * @brief Construct a new secret Properties object. + * + */ + static SecretProperties CreateFromURL(std::string const& url); }; }}}} // namespace Azure::Security::KeyVault::Secrets diff --git a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp index aee85f0e9..09103c673 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp @@ -46,7 +46,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { #endif { - protected: + private: // Using a shared pipeline for a client to share it with LRO (like delete key) std::shared_ptr m_protocolClient; @@ -173,7 +173,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { * @return The Secret wrapped in the Response. */ Azure::Response RestoreSecretBackup( - std::vector const& backup, + BackupSecretResult const& backup, Azure::Core::Context const& context = Azure::Core::Context()) const; /** diff --git a/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_operations.cpp b/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_operations.cpp index d931c064f..df82aee25 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_operations.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_operations.cpp @@ -10,7 +10,7 @@ #include "azure/keyvault/secrets/secret_client.hpp" #include "private/secret_serializers.hpp" -Azure::Response RecoverDeletedSecretOperation::PollUntilDoneInternal( +Azure::Response RecoverDeletedSecretOperation::PollUntilDoneInternal( std::chrono::milliseconds period, Azure::Core::Context& context) { @@ -25,7 +25,7 @@ Azure::Response RecoverDeletedSecretOperation::PollUntilDoneInte std::this_thread::sleep_for(period); } - return Azure::Response( + return Azure::Response( m_value, std::make_unique(*m_rawResponse)); } @@ -60,7 +60,8 @@ std::unique_ptr RecoverDeletedSecretOperation::P if (m_status == Azure::Core::OperationStatus::Succeeded) { - m_value = _detail::SecretSerializer::Deserialize(m_value.Name, *rawResponse); + auto receivedSecret = _detail::SecretSerializer::Deserialize(m_value.Name, *rawResponse); + m_value = receivedSecret.Properties; } return rawResponse; @@ -68,7 +69,7 @@ std::unique_ptr RecoverDeletedSecretOperation::P RecoverDeletedSecretOperation::RecoverDeletedSecretOperation( std::shared_ptr secretClient, - Azure::Response response) + Azure::Response response) : m_secretClient(secretClient) { m_value = response.Value; diff --git a/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_secret_properties.cpp b/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_secret_properties.cpp new file mode 100644 index 000000000..037e45225 --- /dev/null +++ b/sdk/keyvault/azure-security-keyvault-secrets/src/keyvault_secret_properties.cpp @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT +/** + * @file + * @brief Declares SecretProperties. + * + */ + +#include "azure/keyvault/secrets/keyvault_secret.hpp" +#include "private/secret_serializers.hpp" + +using namespace Azure::Security::KeyVault::Secrets; + +SecretProperties SecretProperties::CreateFromURL(std::string const& url) +{ + // create a url object to validate the string is valid as url + Azure::Core::Url urlInstance(url); + SecretProperties result; + // parse the url into the result object + _detail::SecretSerializer::ParseIDUrl(result, urlInstance.GetAbsoluteUrl()); + return result; +} diff --git a/sdk/keyvault/azure-security-keyvault-secrets/src/secret_client.cpp b/sdk/keyvault/azure-security-keyvault-secrets/src/secret_client.cpp index 6573ac5ce..467e5c61e 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/src/secret_client.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/src/secret_client.cpp @@ -157,13 +157,13 @@ Azure::Response SecretClient::BackupSecret( } Azure::Response SecretClient::RestoreSecretBackup( - std::vector const& backup, + BackupSecretResult const& backup, Azure::Core::Context const& context) const { return m_protocolClient->SendRequest( context, Azure::Core::Http::HttpMethod::Post, - [&backup]() { return _detail::RestoreSecretSerializer::Serialize(backup); }, + [&backup]() { return _detail::RestoreSecretSerializer::Serialize(backup.Secret); }, [](Azure::Core::Http::RawResponse const& rawResponse) { return _detail::SecretSerializer::Deserialize(rawResponse); }, @@ -201,11 +201,12 @@ Azure::Security::KeyVault::Secrets::RecoverDeletedSecretOperation SecretClient:: { return Azure::Security::KeyVault::Secrets::RecoverDeletedSecretOperation( std::make_shared(*this), - m_protocolClient->SendRequest( + m_protocolClient->SendRequest( context, Azure::Core::Http::HttpMethod::Post, [&name](Azure::Core::Http::RawResponse const& rawResponse) { - return _detail::SecretSerializer::Deserialize(name, rawResponse); + auto parsedResponse = _detail::SecretSerializer::Deserialize(name, rawResponse); + return parsedResponse.Properties; }, {_detail::DeletedSecretPath, name, _detail::RecoverDeletedSecretPath})); } diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations/sample1-basic-operations.cpp b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations/sample1-basic-operations.cpp index d244b39d8..b62201dbf 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations/sample1-basic-operations.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample1-basic-operations/sample1-basic-operations.cpp @@ -62,7 +62,9 @@ int main() DeleteSecretOperation operation = secretClient.StartDeleteSecret(secret.Name); // You only need to wait for completion if you want to purge or recover the secret. - operation.PollUntilDone(2s); + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value + operation.PollUntilDone(20s); // purge the deleted secret secretClient.PurgeDeletedSecret(secret.Name); diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.cpp b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.cpp index a4bb43ef5..17e35c4f8 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.cpp @@ -74,6 +74,8 @@ int main() DeleteSecretOperation operation = secretClient.StartDeleteSecret(secret.Name); // You only need to wait for completion if you want to purge or recover the secret. + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value operation.PollUntilDone(2s); // purge the deleted secret secretClient.PurgeDeletedSecret(secret.Name); @@ -85,12 +87,13 @@ int main() std::cout << "\t-Read from file." << std::endl; std::ifstream inFile; inFile.open("backup.dat"); - std::vector inMemoryBackup(backUpSize); - inFile >> inMemoryBackup.data(); + BackupSecretResult backedUpSecret; + backedUpSecret.Secret = std::vector(backUpSize); + inFile >> backedUpSecret.Secret.data(); inFile.close(); std::cout << "\t-Restore Secret" << std::endl; - auto restoredSecret = secretClient.RestoreSecretBackup(inMemoryBackup).Value; + auto restoredSecret = secretClient.RestoreSecretBackup(backedUpSecret).Value; AssertSecretsEqual(secret, restoredSecret); diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.md b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.md index c190a2a15..89f4518cc 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.md +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample2-backup-restore/sample2-backup-restore.md @@ -84,7 +84,7 @@ Call RestoreSecretBackup to restore a secret from a backup obtained at the pre ```cpp Snippet:SecretSample2RestoreSecret std::cout << "\t-Restore Secret" << std::endl; -auto restoredSecret = secretClient.RestoreSecretBackup(inMemoryBackup).Value; +auto restoredSecret = secretClient.RestoreSecretBackup(backedUpSecret).Value; ``` ## Source diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample3-delete-recover/sample3-delete-recover.cpp b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample3-delete-recover/sample3-delete-recover.cpp index 269fea851..d790d9879 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample3-delete-recover/sample3-delete-recover.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample3-delete-recover/sample3-delete-recover.cpp @@ -57,6 +57,8 @@ int main() DeleteSecretOperation operation = secretClient.StartDeleteSecret(secret.Name); // You only need to wait for completion if you want to purge or recover the secret. + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value operation.PollUntilDone(2s); // call recover secret @@ -64,13 +66,18 @@ int main() = secretClient.StartRecoverDeletedSecret(secret.Name); // poll until done - KeyVaultSecret restoredSecret = recoverOperation.PollUntilDone(2s).Value; + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value + SecretProperties restoredSecretProperties = recoverOperation.PollUntilDone(2s).Value; + KeyVaultSecret restoredSecret = secretClient.GetSecret(restoredSecretProperties.Name).Value; AssertSecretsEqual(secret, restoredSecret); // cleanup // start deleting the secret DeleteSecretOperation cleanupOperation = secretClient.StartDeleteSecret(restoredSecret.Name); + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value cleanupOperation.PollUntilDone(2s); secretClient.PurgeDeletedSecret(restoredSecret.Name); } diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample4-get-secrets-deleted/sample4-get-secrets-deleted.cpp b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample4-get-secrets-deleted/sample4-get-secrets-deleted.cpp index 3ef95840b..893fd480b 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample4-get-secrets-deleted/sample4-get-secrets-deleted.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/samples/sample4-get-secrets-deleted/sample4-get-secrets-deleted.cpp @@ -47,12 +47,14 @@ int main() KeyVaultSecret secret1 = secretClient.SetSecret(secretName, secretValue).Value; KeyVaultSecret secret2 = secretClient.SetSecret(secretName2, secretValue).Value; - std::cout << "Secret1 Version : " << secret1.Version() << std::endl; + std::cout << "Secret1 Version : " << secret1.Properties.Version << std::endl; // get properties of secrets for (auto secrets = secretClient.GetPropertiesOfSecrets(); secrets.HasPage(); secrets.MoveToNextPage()) { // go through every secret of each page returned + // the number of results returned for in a page is not guaranteed + // it can be anywhere from 0 to 25 for (auto const& secret : secrets.Items) { std::cout << "Found Secret with name: " << secret.Name << std::endl; @@ -64,6 +66,8 @@ int main() secretsVersion.HasPage(); secretsVersion.MoveToNextPage()) { // go through each version of the secret + // the number of results returned for in a page is not guaranteed + // it can be anywhere from 0 to 25 for (auto const& secret : secretsVersion.Items) { std::cout << "Found Secret with name: " << secret.Name @@ -74,17 +78,23 @@ int main() // start deleting the secret DeleteSecretOperation operation = secretClient.StartDeleteSecret(secret1.Name); // You only need to wait for completion if you want to purge or recover the secret. + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value operation.PollUntilDone(2s); // start deleting the secret operation = secretClient.StartDeleteSecret(secret2.Name); // You only need to wait for completion if you want to purge or recover the secret. + // The duration of the delete operation might vary + // in case returns too fast increase the timeout value operation.PollUntilDone(2s); // get all the versions of a secret for (auto deletedSecrets = secretClient.GetDeletedSecrets(); deletedSecrets.HasPage(); deletedSecrets.MoveToNextPage()) - { // go through each version of the secret + { // the number of results returned for in a page is not guaranteed + // it can be anywhere from 0 to 25 + // go through each deleted secret for (auto const& deletedSecret : deletedSecrets.Items) { std::cout << "Found Secret with name: " << deletedSecret.Name << std::endl; diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.cpp b/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.cpp index 12c663ee0..e6c53e450 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.cpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.cpp @@ -94,3 +94,14 @@ TEST(DeletedSecretSerializer, GetDeletedClientDeserializeFull3) Helpers::RunFullExpect(secret, false); Helpers::RunDeletedExtras(secret); } + +TEST(SecretProperties, FactoryValid) +{ + std::string url( + "https://myvault.vault.azure.net/secrets/mysecretname/4387e9f3d6e14c459867679a90fd0f79"); + SecretProperties props = SecretProperties::CreateFromURL(url); + EXPECT_EQ(props.Name, "mysecretname"); + EXPECT_EQ(props.Version, "4387e9f3d6e14c459867679a90fd0f79"); + EXPECT_EQ(props.Id, url); + EXPECT_EQ(props.VaultUrl, "https://myvault.vault.azure.net"); +} diff --git a/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.hpp b/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.hpp index 94e139081..6222fa97e 100644 --- a/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.hpp +++ b/sdk/keyvault/azure-security-keyvault-secrets/test/ut/secret_get_client_deserialize_test.hpp @@ -151,8 +151,8 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets { { EXPECT_EQ( secret.RecoveryId, "https://myvault.vault.azure.net/deletedsecrets/GetDeletedSecretTest"); - EXPECT_EQ(secret.ScheduledPurgeDate.ToString(), "2017-08-02T22:53:53Z"); - EXPECT_EQ(secret.DeletedOn.ToString(), "2017-05-04T22:53:53Z"); + EXPECT_EQ(secret.ScheduledPurgeDate.Value().ToString(), "2017-08-02T22:53:53Z"); + EXPECT_EQ(secret.DeletedOn.Value().ToString(), "2017-05-04T22:53:53Z"); } }; }}}}} // namespace Azure::Security::KeyVault::Secrets::_test