Skip to content

Commit 88149c1

Browse files
SergeyRyabininSergeyRyabinin
committed
Code review and self-code review for bearer auth
1 parent 7ad1276 commit 88149c1

File tree

18 files changed

+79
-57
lines changed

18 files changed

+79
-57
lines changed

‎aws-cpp-sdk-core-tests/aws/client/AWSClientTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,7 @@ TEST_F(AWSConfigTestSuite, TestClientConfigurationSetsRegionToProfile)
718718
{
719719
// create a config file with profile named Dijkstra
720720
Aws::OFStream configFileNew(m_configFileName.c_str(), Aws::OFStream::out | Aws::OFStream::trunc);
721-
configFileNew << "[Dijkstra]" << std::endl;
721+
configFileNew << "[profile Dijkstra]" << std::endl; // profile keyword is mandatory per specification
722722
configFileNew << "region = " << Aws::Region::US_WEST_2 << std::endl;
723723

724724
configFileNew.flush();

‎aws-cpp-sdk-core-tests/aws/config/AWSConfigFileProfileConfigLoaderTest.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ TEST(AWSConfigFileProfileConfigLoaderTest, TestConfigWithSSOParsing)
221221
ASSERT_TRUE(configFile.good());
222222
static const Aws::String SSO_AWS_PROFILE = "AwsSdkBearerIntegrationTest-profile"; // arbitrary
223223
Aws::String profileFileName = configFile.GetFileName().find_last_of(R"(/\)") == std::string::npos ?
224-
configFile.GetFileName() : configFile.GetFileName().substr(configFile.GetFileName().find_last_of(R"(/\)"));
224+
configFile.GetFileName() : configFile.GetFileName().substr(configFile.GetFileName().find_last_of(R"(/\)") + 1);
225225
static const Aws::String SSO_SESSION_NAME = profileFileName + "-sso-session"; // arbitrary
226226
ASSERT_TRUE(WriteConfigFileWithSSO(configFile, SSO_AWS_PROFILE, SSO_SESSION_NAME));
227227

@@ -264,7 +264,7 @@ TEST(AWSConfigFileProfileConfigLoaderTest, TestProfileDumping)
264264
ASSERT_TRUE(configFile.good());
265265
static const Aws::String SSO_AWS_PROFILE = "AwsSdkBearerIntegrationTest-profile"; // arbitrary
266266
Aws::String profileFileName = configFile.GetFileName().find_last_of(R"(/\)") == std::string::npos ?
267-
configFile.GetFileName() : configFile.GetFileName().substr(configFile.GetFileName().find_last_of(R"(/\)"));
267+
configFile.GetFileName() : configFile.GetFileName().substr(configFile.GetFileName().find_last_of(R"(/\)") + 1);
268268
static const Aws::String SSO_SESSION_NAME = profileFileName + "-sso-session"; // arbitrary
269269
ASSERT_TRUE(WriteConfigFileWithSSO(configFile, SSO_AWS_PROFILE, SSO_SESSION_NAME));
270270

‎aws-cpp-sdk-core/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ file(GLOB AWS_HEADERS "include/aws/core/*.h")
4545
file(GLOB AWS_AUTH_HEADERS "include/aws/core/auth/*.h")
4646
file(GLOB AWS_AUTH_SIGNER_HEADERS "include/aws/core/auth/signer/*.h")
4747
file(GLOB AWS_AUTH_SIGNER_PROVIDER_HEADERS "include/aws/core/auth/signer-provider/*.h")
48-
file(GLOB AWS_AUTH_BEARER_TOKEN_PROVIDER_HEADERS "include/aws/core/auth/bearer-token-provider*.h")
48+
file(GLOB AWS_AUTH_BEARER_TOKEN_PROVIDER_HEADERS "include/aws/core/auth/bearer-token-provider/*.h")
4949
file(GLOB AWS_CLIENT_HEADERS "include/aws/core/client/*.h")
5050
file(GLOB AWS_INTERNAL_HEADERS "include/aws/core/internal/*.h")
5151
file(GLOB NET_HEADERS "include/aws/core/net/*.h")

‎aws-cpp-sdk-core/include/aws/core/auth/AWSAuthSignerProvider.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,5 @@
88

99
#include <aws/core/auth/signer-provider/AWSAuthSignerProviderBase.h>
1010
#include <aws/core/auth/signer-provider/DefaultAuthSignerProvider.h>
11-
#include <aws/core/auth/signer-provider/BearerTokenAuthSignerProvider.h>
1211

1312
// This is a header that represents old legacy all-in-one header to maintain backward compatibility

‎aws-cpp-sdk-core/include/aws/core/auth/AWSBearerToken.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ namespace Aws
4848
}
4949

5050
/**
51-
* If token has not been initialized or been initialized to emtpy value.
51+
* If token has not been initialized or been initialized to empty value.
5252
* Expiration date does not affect the result of this function.
5353
*/
5454
inline bool IsEmpty() const { return m_token.empty(); }

‎aws-cpp-sdk-core/include/aws/core/auth/AWSCredentials.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ namespace Aws
6868
}
6969

7070
/**
71-
* If credentials haven't been initialized or been initialized to emtpy values.
71+
* If credentials haven't been initialized or been initialized to empty values.
7272
* Expiration date does not affect the result of this function.
7373
*/
7474
inline bool IsEmpty() const { return m_accessKeyId.empty() && m_secretKey.empty(); }

‎aws-cpp-sdk-core/include/aws/core/auth/bearer-token-provider/DefaultBearerTokenProviderChain.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ namespace Aws
1414
{
1515
namespace Auth
1616
{
17-
class AWSBearerTokenProvider;
1817
/**
18+
* Default built-in AWSBearerTokenProviderChainBase implementation that includes Aws::Auth::SSOBearerTokenProvider in the chain.
1919
*/
2020
class AWS_CORE_API DefaultBearerTokenProviderChain : public AWSBearerTokenProviderChainBase
2121
{
@@ -24,6 +24,7 @@ namespace Aws
2424
virtual ~DefaultBearerTokenProviderChain() = default;
2525

2626
/**
27+
* Return bearer token, implementation of a base class interface
2728
*/
2829
virtual AWSBearerToken GetAWSBearerToken() override;
2930

‎aws-cpp-sdk-core/include/aws/core/auth/signer-provider/BearerTokenAuthSignerProvider.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@
33
* SPDX-License-Identifier: Apache-2.0.
44
*/
55

6-
76
#pragma once
87

98
#include <aws/core/auth/signer-provider/AWSAuthSignerProviderBase.h>
10-
119
#include <aws/core/utils/memory/stl/AWSSet.h>
1210
#include <aws/core/auth/signer/AWSAuthBearerSigner.h>
1311

‎aws-cpp-sdk-core/include/aws/core/auth/signer/AWSAuthBearerSigner.h

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ namespace Aws
3636

3737
public:
3838
/**
39-
*
39+
* An implementation of a signer interface that uses bearer token auth signature.
4040
*/
4141
AWSAuthBearerSigner(const std::shared_ptr<Aws::Auth::AWSBearerTokenProviderBase> bearerTokenProvider)
4242
: m_bearerTokenProvider(bearerTokenProvider)
@@ -45,46 +45,53 @@ namespace Aws
4545
virtual ~AWSAuthBearerSigner() {};
4646

4747
/**
48-
*
48+
* Return the signer's name
4949
*/
5050
const char* GetName() const override
5151
{
5252
return Aws::Auth::BEARER_SIGNER;
5353
}
5454

5555
/**
56-
*
56+
* Sign request with a bearer auth token
57+
* @return true if success, false if fail to sign
5758
*/
5859
bool SignRequest(Aws::Http::HttpRequest& ) const override;
5960

6061
/**
61-
*
62+
* Dummy function to satisfy the interface requirements of a base Signer interface
63+
* additional arguments are not used.
64+
* @return true if success, false if fail to sign
6265
*/
6366
bool SignRequest(Aws::Http::HttpRequest& ioRequest, const char* /*region*/, const char* /*serviceName*/, bool /*signBody*/) const override
6467
{
6568
return SignRequest(ioRequest);
6669
}
6770

68-
6971
/**
70-
*
71-
*/
72+
* Dummy function to satisfy the interface requirements of a base Signer interface
73+
* @return true if success, false if fail to sign
74+
*/
7275
bool PresignRequest(Aws::Http::HttpRequest& ioRequest, long long /*expirationInSeconds = 0*/) const override
7376
{
7477
return SignRequest(ioRequest);
7578
}
7679

7780
/**
78-
*
79-
*/
81+
* Dummy function to satisfy the interface requirements of a base Signer interface
82+
* additional arguments are not used.
83+
* @return true if success, false if fail to sign
84+
*/
8085
bool PresignRequest(Aws::Http::HttpRequest& ioRequest, const char* /*region*/, long long expirationInSeconds = 0) const override
8186
{
8287
return PresignRequest(ioRequest, expirationInSeconds);
8388
}
8489

8590
/**
86-
*
87-
*/
91+
* Dummy function to satisfy the interface requirements of a base Signer interface
92+
* additional arguments are not used.
93+
* @return true if success, false if fail to sign
94+
*/
8895
bool PresignRequest(Aws::Http::HttpRequest& ioRequest, const char* /*region*/, const char* /*serviceName*/, long long expirationInSeconds = 0) const override
8996
{
9097
return PresignRequest(ioRequest, expirationInSeconds);

‎aws-cpp-sdk-core/source/auth/bearer-token-provider/DefaultBearerTokenProviderChain.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,20 @@
66
#include <aws/core/auth/bearer-token-provider/DefaultBearerTokenProviderChain.h>
77
#include <aws/core/auth/AWSBearerToken.h>
88
#include <aws/core/auth/bearer-token-provider/SSOBearerTokenProvider.h>
9+
#include <aws/core/utils/logging/LogMacros.h>
10+
911

1012
static const char SSO_DEFAULT_BEARER_TOKEN_PROVIDER_CHAIN_LOG_TAG[] = "SSOBearerTokenProvider";
1113

1214
Aws::Auth::AWSBearerToken Aws::Auth::DefaultBearerTokenProviderChain::GetAWSBearerToken()
1315
{
1416
for (auto&& bearerTokenProvider : m_providerChain)
1517
{
18+
if(!bearerTokenProvider) {
19+
AWS_LOGSTREAM_FATAL(SSO_DEFAULT_BEARER_TOKEN_PROVIDER_CHAIN_LOG_TAG,
20+
"Unexpected nullptr in DefaultBearerTokenProviderChain::m_providerChain");
21+
break;
22+
}
1623
AWSBearerToken bearerToken = bearerTokenProvider->GetAWSBearerToken();
1724
if(!bearerToken.IsExpiredOrEmpty())
1825
{
@@ -25,4 +32,4 @@ Aws::Auth::AWSBearerToken Aws::Auth::DefaultBearerTokenProviderChain::GetAWSBear
2532
Aws::Auth::DefaultBearerTokenProviderChain::DefaultBearerTokenProviderChain()
2633
{
2734
AddProvider(Aws::MakeShared<Aws::Auth::SSOBearerTokenProvider>(SSO_DEFAULT_BEARER_TOKEN_PROVIDER_CHAIN_LOG_TAG));
28-
}
35+
}

‎aws-cpp-sdk-core/source/auth/bearer-token-provider/SSOBearerTokenProvider.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ AWSBearerToken SSOBearerTokenProvider::GetAWSBearerToken()
6464
/* If a loaded token has expired and has insufficient metadata to perform a refresh the SSO token
6565
provider must raise an exception that the token has expired and cannot be refreshed.
6666
Error logging and returning an empty object instead because of disabled exceptions and poor legacy API design. */
67-
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider is unable to provide a token!");
67+
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider is unable to provide a token");
6868
return Aws::Auth::AWSBearerToken("", Aws::Utils::DateTime(0.0));
6969
}
7070
return m_token;
@@ -109,6 +109,10 @@ void SSOBearerTokenProvider::RefreshFromSso()
109109
ssoCreateTokenRequest.grantType = SSO_GRANT_TYPE;
110110
ssoCreateTokenRequest.refreshToken = cachedSsoToken.refreshToken;
111111

112+
if(!m_client) {
113+
AWS_LOGSTREAM_FATAL(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "Unexpected nullptr in SSOBearerTokenProvider::m_client");
114+
return;
115+
}
112116
Aws::Internal::SSOCredentialsClient::SSOCreateTokenResult result = m_client->CreateToken(ssoCreateTokenRequest);
113117
if(!result.accessToken.empty())
114118
{
@@ -136,7 +140,7 @@ SSOBearerTokenProvider::CachedSsoToken SSOBearerTokenProvider::LoadAccessTokenFi
136140

137141
const Aws::Config::Profile& profile = Aws::Config::GetCachedConfigProfile(m_profileToUse);
138142
if(!profile.IsSsoSessionSet()) {
139-
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider set to use a profile " << m_profileToUse << " without a sso_session. Unable to load cached token");
143+
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider set to use a profile " << m_profileToUse << " without a sso_session. Unable to load cached token.");
140144
return retValue;
141145
}
142146

@@ -183,7 +187,8 @@ bool SSOBearerTokenProvider::WriteAccessTokenFile(const CachedSsoToken& token) c
183187
{
184188
const Aws::Config::Profile& profile = Aws::Config::GetCachedConfigProfile(m_profileToUse);
185189
if(!profile.IsSsoSessionSet()) {
186-
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider set to use a profile " << m_profileToUse << " without a sso_session. Unable to write cached token");
190+
AWS_LOGSTREAM_ERROR(SSO_BEARER_TOKEN_PROVIDER_LOG_TAG, "SSOBearerTokenProvider set to use a profile "
191+
<< m_profileToUse << " without a sso_session. Unable to write a cached token.");
187192
return false;
188193
}
189194

‎aws-cpp-sdk-core/source/auth/signer-provider/BearerTokenAuthSignerProvider.cpp

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,35 +11,30 @@
1111
#include <aws/core/auth/AWSCredentialsProvider.h>
1212
#include <aws/core/utils/memory/stl/AWSAllocator.h>
1313

14-
const char BTASP_ALLOC_TAG[] = "BearerTokenAuthSignerProvider";
14+
const char BEARER_TOKEN_AUTH_SIGNER_PROVIDER_ALLOC_TAG[] = "BearerTokenAuthSignerProvider";
1515

1616
using namespace Aws::Auth;
1717

1818
BearerTokenAuthSignerProvider::BearerTokenAuthSignerProvider(const std::shared_ptr<Aws::Auth::AWSBearerTokenProviderBase> bearerTokenProvider)
1919
{
20-
m_signers.emplace_back(Aws::MakeShared<Aws::Client::AWSAuthBearerSigner>(BTASP_ALLOC_TAG, bearerTokenProvider));
21-
m_signers.emplace_back(Aws::MakeShared<Aws::Client::AWSNullSigner>(BTASP_ALLOC_TAG));
20+
m_signers.emplace_back(Aws::MakeShared<Aws::Client::AWSAuthBearerSigner>(BEARER_TOKEN_AUTH_SIGNER_PROVIDER_ALLOC_TAG, bearerTokenProvider));
21+
m_signers.emplace_back(Aws::MakeShared<Aws::Client::AWSNullSigner>(BEARER_TOKEN_AUTH_SIGNER_PROVIDER_ALLOC_TAG));
2222
}
2323

24-
//BearerTokenAuthSignerProvider::BearerTokenAuthSignerProvider(const std::shared_ptr<Aws::Client::AWSAuthSigner>& signer)
25-
//{
26-
// m_signers.emplace_back(Aws::MakeShared<Aws::Client::AWSNullSigner>(CLASS_TAG));
27-
// if(signer)
28-
// {
29-
// m_signers.emplace_back(signer);
30-
// }
31-
//}
32-
3324
std::shared_ptr<Aws::Client::AWSAuthSigner> BearerTokenAuthSignerProvider::GetSigner(const Aws::String& signerName) const
3425
{
3526
for(const auto& signer : m_signers)
3627
{
28+
if(!signer) {
29+
AWS_LOGSTREAM_FATAL(BEARER_TOKEN_AUTH_SIGNER_PROVIDER_ALLOC_TAG, "Unexpected nullptr in BearerTokenAuthSignerProvider::m_signers");
30+
break;
31+
}
3732
if(signer->GetName() == signerName)
3833
{
3934
return signer;
4035
}
4136
}
42-
AWS_LOGSTREAM_ERROR(BTASP_ALLOC_TAG, "Request's signer: '" << signerName << "' is not found in the signer's map.");
37+
AWS_LOGSTREAM_ERROR(BEARER_TOKEN_AUTH_SIGNER_PROVIDER_ALLOC_TAG, "Request's signer: '" << signerName << "' is not found in the signer's map.");
4338
assert(false);
4439
return nullptr;
4540
}

‎aws-cpp-sdk-core/source/auth/signer/AWSAuthBearerSigner.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ namespace Aws
3333
}
3434
if(!m_bearerTokenProvider)
3535
{
36-
AWS_LOGSTREAM_ERROR(LOGGING_TAG, "Unexpected nullptr AWSAuthBearerSigner::m_bearerTokenProvider");
36+
AWS_LOGSTREAM_FATAL(LOGGING_TAG, "Unexpected nullptr AWSAuthBearerSigner::m_bearerTokenProvider");
3737
return false;
3838
}
3939
const Aws::Auth::AWSBearerToken& token = m_bearerTokenProvider->GetAWSBearerToken();

‎aws-cpp-sdk-core/source/config/AWSConfigFileProfileConfigLoader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ namespace Aws
347347
} while(0); // end of goto in a form of "do { break; } while(0);"
348348

349349
ioSectionName.erase();
350-
ioState = FAILURE;
350+
ioState = UNKNOWN_SECTION_FOUND;
351351
return;
352352
}
353353

‎aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -616,12 +616,13 @@ namespace Aws
616616
// An internal SSO CreateToken implementation to lightweight core package and not introduce a dependency on sso-oidc
617617
SSOCredentialsClient::SSOCreateTokenResult SSOCredentialsClient::CreateToken(const SSOCreateTokenRequest& request)
618618
{
619-
Aws::StringStream ssUri;
620-
ssUri << m_endpoint << SSO_GET_ROLE_RESOURCE;
621-
622619
std::shared_ptr<HttpRequest> httpRequest(CreateHttpRequest(m_endpoint, HttpMethod::HTTP_GET,
623620
Aws::Utils::Stream::DefaultResponseStreamFactoryMethod));
624-
621+
SSOCreateTokenResult result;
622+
if(!httpRequest) {
623+
AWS_LOGSTREAM_FATAL(SSO_RESOURCE_CLIENT_LOG_TAG, "Failed to CreateHttpRequest: nullptr returned");
624+
return result;
625+
}
625626
httpRequest->SetUserAgent(ComputeUserAgentString());
626627

627628
Json::JsonValue requestDoc;
@@ -639,6 +640,10 @@ namespace Aws
639640
}
640641

641642
std::shared_ptr<Aws::IOStream> body = Aws::MakeShared<Aws::StringStream>("SSO_BEARER_TOKEN_CREATE_TOKEN");
643+
if(!body) {
644+
AWS_LOGSTREAM_FATAL(SSO_RESOURCE_CLIENT_LOG_TAG, "Failed to allocate body"); // exceptions disabled
645+
return result;
646+
}
642647
*body << requestDoc.View().WriteReadable();;
643648

644649
httpRequest->AddContentBody(body);
@@ -653,7 +658,7 @@ namespace Aws
653658
Aws::String rawReply = GetResourceWithAWSWebServiceResult(httpRequest).GetPayload();
654659
Json::JsonValue refreshTokenDoc(rawReply);
655660
Utils::Json::JsonView jsonValue = refreshTokenDoc.View();
656-
SSOCreateTokenResult result;
661+
657662
if(jsonValue.ValueExists("accessToken")) {
658663
result.accessToken = jsonValue.GetString("accessToken");
659664
}
@@ -664,10 +669,10 @@ namespace Aws
664669
result.expiresIn = jsonValue.GetInteger("expiresIn");
665670
}
666671
if(jsonValue.ValueExists("idToken")) {
667-
result.idToken = jsonValue.GetInteger("idToken");
672+
result.idToken = jsonValue.GetString("idToken");
668673
}
669674
if(jsonValue.ValueExists("refreshToken")) {
670-
result.refreshToken = jsonValue.GetInteger("refreshToken");
675+
result.refreshToken = jsonValue.GetString("refreshToken");
671676
}
672677

673678
return result;

0 commit comments

Comments
 (0)