From 06d66492ebb741e30731e4e85195f0b5ba0dc10b Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Date: Tue, 2 Feb 2021 17:09:33 -0800 Subject: [PATCH] Add TokenRequestOptions and ClientSecretCredentialOptions (#1527) * Add GetTokenOptions --- sdk/core/azure-core/CHANGELOG.md | 2 + .../azure-core/inc/azure/core/credentials.hpp | 12 ++-- .../azure-core/inc/azure/core/http/policy.hpp | 56 ++++++------------- .../bearer_token_authentication_policy.cpp | 5 +- sdk/identity/azure-identity/CHANGELOG.md | 3 + .../identity/client_secret_credential.hpp | 47 +++++++++++----- .../azure/identity/environment_credential.hpp | 5 +- .../src/client_secret_credential.cpp | 21 +++---- .../src/environment_credential.cpp | 9 ++- .../src/key_client.cpp | 11 +++- .../azure-storage-blobs/src/blob_client.cpp | 11 +++- .../src/blob_container_client.cpp | 11 +++- .../src/blob_service_client.cpp | 11 +++- .../src/datalake_directory_client.cpp | 11 +++- .../src/datalake_file_client.cpp | 11 +++- .../src/datalake_file_system_client.cpp | 11 +++- .../src/datalake_path_client.cpp | 11 +++- .../src/datalake_service_client.cpp | 11 +++- 18 files changed, 168 insertions(+), 91 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 8a4f2d378..023be79b8 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -9,6 +9,8 @@ ### Breaking Changes - Make `ToLower()` and `LocaleInvariantCaseInsensitiveEqual()` internal by moving them from `Azure::Core::Strings` to `Azure::Core::Internal::Strings`. +- `BearerTokenAuthenticationPolicy` constructor takes `TokenRequestOptions` struct instead of scopes vector. `TokenRequestOptions` struct has scopes vector as data member. +- `TokenCredential::GetToken()` takes `TokenRequestOptions` instead of scopes vector. ### Bug Fixes diff --git a/sdk/core/azure-core/inc/azure/core/credentials.hpp b/sdk/core/azure-core/inc/azure/core/credentials.hpp index 76d5a9e3b..0b88a7602 100644 --- a/sdk/core/azure-core/inc/azure/core/credentials.hpp +++ b/sdk/core/azure-core/inc/azure/core/credentials.hpp @@ -16,7 +16,6 @@ #include #include #include -#include namespace Azure { namespace Core { @@ -36,6 +35,10 @@ namespace Azure { namespace Core { DateTime ExpiresOn; }; + namespace Http { + struct TokenRequestOptions; + } // namespace Http + /** * @brief Token credential. */ @@ -45,10 +48,11 @@ namespace Azure { namespace Core { * @brief Get an authentication token. * * @param context #Context so that operation can be cancelled. - * @param scopes Authentication scopes. + * @param tokenRequestOptions Options to get the token. */ - virtual AccessToken GetToken(Context const& context, std::vector const& scopes) - const = 0; + virtual AccessToken GetToken( + Context const& context, + Http::TokenRequestOptions const& tokenRequestOptions) const = 0; /// Destructor. virtual ~TokenCredential() = default; diff --git a/sdk/core/azure-core/inc/azure/core/http/policy.hpp b/sdk/core/azure-core/inc/azure/core/http/policy.hpp index be9e7ca0f..6691d35b7 100644 --- a/sdk/core/azure-core/inc/azure/core/http/policy.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/policy.hpp @@ -306,13 +306,24 @@ namespace Azure { namespace Core { namespace Http { NextHttpPolicy nextHttpPolicy) const override; }; + /** + * @brief Defines options for getting token. + */ + struct TokenRequestOptions + { + /** + * @brief Authentication scopes. + */ + std::vector Scopes; + }; + /** * @brief Bearer Token authentication policy. */ class BearerTokenAuthenticationPolicy : public HttpPolicy { private: std::shared_ptr const m_credential; - std::vector m_scopes; + TokenRequestOptions m_tokenRequestOptions; mutable AccessToken m_accessToken; mutable std::mutex m_accessTokenMutex; @@ -322,54 +333,21 @@ namespace Azure { namespace Core { namespace Http { public: /** - * @brief Construct a Bearer Token authentication policy with single authentication scope. + * @brief Construct a Bearer Token authentication policy. * * @param credential A #TokenCredential to use with this policy. - * @param scope Authentication scope. + * @param tokenRequestOptions #TokenRequestOptions. */ explicit BearerTokenAuthenticationPolicy( std::shared_ptr credential, - std::string scope) - : m_credential(std::move(credential)) - { - m_scopes.emplace_back(std::move(scope)); - } - - /** - * @brief Construct a Bearer Token authentication policy with multiple authentication scopes. - * - * @param credential A #TokenCredential to use with this policy. - * @param scopes A vector of authentication scopes. - */ - explicit BearerTokenAuthenticationPolicy( - std::shared_ptr credential, - std::vector scopes) - : m_credential(std::move(credential)), m_scopes(std::move(scopes)) - { - } - - /** - * @brief Construct a Bearer Token authentication policy with multiple authentication scopes. - * - * @tparam A type of scopes sequence iterator. - * - * @param credential A #TokenCredential to use with this policy. - * @param scopesBegin An iterator pointing to begin of the sequence of scopes to use. - * @param scopesEnd An iterator pointing to an element after the last element in sequence of - * scopes to use. - */ - template - explicit BearerTokenAuthenticationPolicy( - std::shared_ptr credential, - ScopesIterator const& scopesBegin, - ScopesIterator const& scopesEnd) - : m_credential(std::move(credential)), m_scopes(scopesBegin, scopesEnd) + TokenRequestOptions tokenRequestOptions) + : m_credential(std::move(credential)), m_tokenRequestOptions(std::move(tokenRequestOptions)) { } std::unique_ptr Clone() const override { - return std::make_unique(m_credential, m_scopes); + return std::make_unique(m_credential, m_tokenRequestOptions); } std::unique_ptr Send( diff --git a/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp b/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp index ea4ef3615..c47ba0aa5 100644 --- a/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp +++ b/sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp @@ -16,9 +16,10 @@ std::unique_ptr BearerTokenAuthenticationPolicy::Send( { std::lock_guard lock(m_accessTokenMutex); - if (std::chrono::system_clock::now() > m_accessToken.ExpiresOn) + // Refresh the token in 2 or less minutes before the actual expiration. + if ((std::chrono::system_clock::now() - std::chrono::minutes(2)) > m_accessToken.ExpiresOn) { - m_accessToken = m_credential->GetToken(context, m_scopes); + m_accessToken = m_credential->GetToken(context, m_tokenRequestOptions); } request.AddHeader("authorization", "Bearer " + m_accessToken.Token); diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index 75c3d04b0..1e478352e 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -2,6 +2,9 @@ ## 1.0.0-beta.3 (Unreleased) +### Breaking Changes + +- `ClientSecretCredential ` constructor takes `ClientSecretCredentialOptions` struct instead of authority host string. `TokenCredentialOptions` struct has authority host string as data member. ## 1.0.0-beta.2 (2021-01-13) diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp index 451bf69fe..b4a9b6f2f 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp @@ -11,11 +11,38 @@ #include "azure/identity/dll_import_export.hpp" #include +#include #include #include namespace Azure { namespace Identity { + namespace Details { + AZ_IDENTITY_DLLEXPORT extern std::string const g_aadGlobalAuthority; + } + + /** + * @brief Defines options for token authentication. + */ + struct ClientSecretCredentialOptions + { + public: + /** + * @brief Authentication authority URL. + * @detail Default value is Azure AD global authority - + * "https://login.microsoftonline.com/". + * + * @note Example of a \p authority string: "https://login.microsoftonline.us/". See national + * clouds' Azure AD authentication endpoints: + * https://docs.microsoft.com/en-us/azure/active-directory/develop/authentication-national-cloud. + */ + std::string AuthorityHost = Details::g_aadGlobalAuthority; + + /** + * @brief #TransportPolicyOptions for authentication HTTP pipeline. + */ + Azure::Core::Http::TransportPolicyOptions TransportPolicyOptions; + }; /** * @brief This class is used by Azure SDK clients to authenticate with the Azure service using a @@ -23,12 +50,10 @@ namespace Azure { namespace Identity { */ class ClientSecretCredential : public Core::TokenCredential { private: - AZ_IDENTITY_DLLEXPORT static std::string const g_aadGlobalAuthority; - std::string m_tenantId; std::string m_clientId; std::string m_clientSecret; - std::string m_authority; + ClientSecretCredentialOptions m_options; public: /** @@ -37,25 +62,21 @@ namespace Azure { namespace Identity { * @param tenantId Tenant ID. * @param clientId Client ID. * @param clientSecret Client Secret. - * @param authority Authentication authority URL to set. If omitted, initializes credential with - * default authority (Azure AD global authority - "https://login.microsoftonline.com/"). - * - * @note Example of a \p authority string: "https://login.microsoftonline.us/". See national - * clouds' Azure AD authentication endpoints: - * https://docs.microsoft.com/en-us/azure/active-directory/develop/authentication-national-cloud. + * @param options #ClientSecretCredentialOptions. */ explicit ClientSecretCredential( std::string tenantId, std::string clientId, std::string clientSecret, - std::string authority = g_aadGlobalAuthority) + ClientSecretCredentialOptions options = ClientSecretCredentialOptions()) : m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)), - m_clientSecret(std::move(clientSecret)), m_authority(std::move(authority)) + m_clientSecret(std::move(clientSecret)), m_options(std::move(options)) { } - Core::AccessToken GetToken(Core::Context const& context, std::vector const& scopes) - const override; + Core::AccessToken GetToken( + Core::Context const& context, + Core::Http::TokenRequestOptions const& tokenRequestOptions) const override; }; }} // namespace Azure::Identity diff --git a/sdk/identity/azure-identity/inc/azure/identity/environment_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/environment_credential.hpp index 122a0f1b9..73d437f64 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/environment_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/environment_credential.hpp @@ -34,8 +34,9 @@ namespace Azure { namespace Identity { */ explicit EnvironmentCredential(); - Core::AccessToken GetToken(Core::Context const& context, std::vector const& scopes) - const override; + Core::AccessToken GetToken( + Core::Context const& context, + Core::Http::TokenRequestOptions const& tokenRequestOptions) const override; }; }} // namespace Azure::Identity diff --git a/sdk/identity/azure-identity/src/client_secret_credential.cpp b/sdk/identity/azure-identity/src/client_secret_credential.cpp index 263698b01..d54bd2fee 100644 --- a/sdk/identity/azure-identity/src/client_secret_credential.cpp +++ b/sdk/identity/azure-identity/src/client_secret_credential.cpp @@ -36,12 +36,12 @@ std::string UrlEncode(std::string const& s) } } // namespace -std::string const ClientSecretCredential::g_aadGlobalAuthority +std::string const Azure::Identity::Details::g_aadGlobalAuthority = "https://login.microsoftonline.com/"; Azure::Core::AccessToken ClientSecretCredential::GetToken( Azure::Core::Context const& context, - std::vector const& scopes) const + Azure::Core::Http::TokenRequestOptions const& tokenRequestOptions) const { using namespace Azure::Core; using namespace Azure::Core::Http; @@ -49,7 +49,7 @@ Azure::Core::AccessToken ClientSecretCredential::GetToken( static std::string const errorMsgPrefix("ClientSecretCredential::GetToken: "); try { - Url url(m_authority); + Url url(m_options.AuthorityHost); url.AppendPath(m_tenantId); url.AppendPath("oauth2/v2.0/token"); @@ -58,6 +58,7 @@ Azure::Core::AccessToken ClientSecretCredential::GetToken( body << "grant_type=client_credentials&client_id=" << UrlEncode(m_clientId) << "&client_secret=" << UrlEncode(m_clientSecret); + auto const& scopes = tokenRequestOptions.Scopes; if (!scopes.empty()) { auto scopesIter = scopes.begin(); @@ -81,12 +82,14 @@ Azure::Core::AccessToken ClientSecretCredential::GetToken( request.AddHeader("Content-Length", std::to_string(bodyString.size())); std::vector> policies; - policies.push_back(std::make_unique()); + policies.emplace_back(std::make_unique()); - RetryOptions retryOptions; - policies.push_back(std::make_unique(retryOptions)); + { + RetryOptions retryOptions; + policies.emplace_back(std::make_unique(retryOptions)); + } - policies.push_back(std::make_unique()); + policies.emplace_back(std::make_unique(m_options.TransportPolicyOptions)); HttpPipeline httpPipeline(policies); @@ -176,13 +179,11 @@ Azure::Core::AccessToken ClientSecretCredential::GetToken( } auto const tokenEnd = responseBodyPos; - expiresInSeconds -= 2 * 60; auto const responseBodyBegin = responseBody.begin(); return { std::string(responseBodyBegin + tokenBegin, responseBodyBegin + tokenEnd), - std::chrono::system_clock::now() - + std::chrono::seconds(expiresInSeconds < 0 ? 0 : expiresInSeconds), + std::chrono::system_clock::now() + std::chrono::seconds(expiresInSeconds), }; } catch (AuthenticationException const&) diff --git a/sdk/identity/azure-identity/src/environment_credential.cpp b/sdk/identity/azure-identity/src/environment_credential.cpp index 7fe84f0c9..ade3272dd 100644 --- a/sdk/identity/azure-identity/src/environment_credential.cpp +++ b/sdk/identity/azure-identity/src/environment_credential.cpp @@ -53,8 +53,11 @@ EnvironmentCredential::EnvironmentCredential() { if (authority != nullptr) { + ClientSecretCredentialOptions options; + options.AuthorityHost = authority; + m_credentialImpl.reset( - new ClientSecretCredential(tenantId, clientId, clientSecret, authority)); + new ClientSecretCredential(tenantId, clientId, clientSecret, options)); } else { @@ -78,7 +81,7 @@ EnvironmentCredential::EnvironmentCredential() Azure::Core::AccessToken EnvironmentCredential::GetToken( Azure::Core::Context const& context, - std::vector const& scopes) const + Azure::Core::Http::TokenRequestOptions const& tokenRequestOptions) const { using namespace Azure::Core; @@ -88,5 +91,5 @@ Azure::Core::AccessToken EnvironmentCredential::GetToken( "Environment variables are not fully configured."); } - return m_credentialImpl->GetToken(context, scopes); + return m_credentialImpl->GetToken(context, tokenRequestOptions); } diff --git a/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp b/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp index e054215ff..316f5c89d 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp +++ b/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp @@ -26,8 +26,15 @@ KeyClient::KeyClient( policies.emplace_back(std::make_unique("KeyVault", apiVersion)); policies.emplace_back(std::make_unique()); policies.emplace_back(std::make_unique(options.RetryOptions)); - policies.emplace_back(std::make_unique( - credential, "https://vault.azure.net/.default")); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{"https://vault.azure.net/.default"}}; + + policies.emplace_back( + std::make_unique(credential, tokenOptions)); + } + policies.emplace_back(std::make_unique()); policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); diff --git a/sdk/storage/azure-storage-blobs/src/blob_client.cpp b/sdk/storage/azure-storage-blobs/src/blob_client.cpp index dd9c53292..b9461b195 100644 --- a/sdk/storage/azure-storage-blobs/src/blob_client.cpp +++ b/sdk/storage/azure-storage-blobs/src/blob_client.cpp @@ -88,8 +88,15 @@ namespace Azure { namespace Storage { namespace Blobs { policies.emplace_back(p->Clone()); } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-blobs/src/blob_container_client.cpp b/sdk/storage/azure-storage-blobs/src/blob_container_client.cpp index d9695bcde..8b793c22d 100644 --- a/sdk/storage/azure-storage-blobs/src/blob_container_client.cpp +++ b/sdk/storage/azure-storage-blobs/src/blob_container_client.cpp @@ -84,8 +84,15 @@ namespace Azure { namespace Storage { namespace Blobs { policies.emplace_back(p->Clone()); } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-blobs/src/blob_service_client.cpp b/sdk/storage/azure-storage-blobs/src/blob_service_client.cpp index d26eee5f1..98ae121c2 100644 --- a/sdk/storage/azure-storage-blobs/src/blob_service_client.cpp +++ b/sdk/storage/azure-storage-blobs/src/blob_service_client.cpp @@ -79,8 +79,15 @@ namespace Azure { namespace Storage { namespace Blobs { policies.emplace_back(p->Clone()); } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_directory_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_directory_client.cpp index d69087051..094587398 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_directory_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_directory_client.cpp @@ -92,8 +92,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { policies.emplace_back(p->Clone()); } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Azure::Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_file_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_file_client.cpp index c0dc16387..df0b5bf59 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_file_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_file_client.cpp @@ -162,8 +162,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Azure::Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_file_system_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_file_system_client.cpp index 76186c43d..49b722d0f 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_file_system_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_file_system_client.cpp @@ -122,8 +122,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Azure::Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp index 21e541ca7..6177fe180 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_path_client.cpp @@ -151,8 +151,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Azure::Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies); diff --git a/sdk/storage/azure-storage-files-datalake/src/datalake_service_client.cpp b/sdk/storage/azure-storage-files-datalake/src/datalake_service_client.cpp index 932fe3437..43459acdc 100644 --- a/sdk/storage/azure-storage-files-datalake/src/datalake_service_client.cpp +++ b/sdk/storage/azure-storage-files-datalake/src/datalake_service_client.cpp @@ -154,8 +154,15 @@ namespace Azure { namespace Storage { namespace Files { namespace DataLake { policies.emplace_back(p->Clone()); } policies.emplace_back(std::make_unique()); - policies.emplace_back(std::make_unique( - credential, Azure::Storage::Details::StorageScope)); + + { + Azure::Core::Http::TokenRequestOptions const tokenOptions + = {{Storage::Details::StorageScope}}; + + policies.emplace_back(std::make_unique( + credential, tokenOptions)); + } + policies.emplace_back( std::make_unique(options.TransportPolicyOptions)); m_pipeline = std::make_shared(policies);