-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Smithy Bearer Auth #3102
Smithy Bearer Auth #3102
Conversation
|
||
namespace smithy | ||
{ | ||
constexpr char BEARER_SIGNER[] = "Bearer"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was thinking about this the other day, do we really need this in its own constant or can we just pass the string in the parent ctor i.e.
AuthScheme("Bearer")
we avoid some namespace pollution that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had raised this before (during autha) but we agreed putting this in namespace smithy to comply with what Sergey originally proposed. We do pass it in the base class constructor. I can simply pass the raw string .
Whatever you finalize. I am ok with either
std::shared_ptr<AwsCredentialIdentityResolverT> identityResolver,
const Aws::String &serviceName, const Aws::String ®ion)
: AuthScheme(BEARER_SIGNER), m_identityResolver{identityResolver},
m_signer{Aws::MakeShared<smithy::BearerTokenSigner>(
"BearerTokenAuthScheme", serviceName, region)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raw string is better in my opinion thinking about it, one less thing in the namespace that is actually internal to us
}; | ||
|
||
AuthSchemeOption BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption = | ||
AuthSchemeOption("Bearer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did you get this string btw? in the smithy docs its smithy.api#HTTPBearerAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 587 in af8d1fb
operation.setSignerName("Aws::Auth::BEARER_SIGNER"); |
this is the signer we use.
const char BEARER_SIGNER[] = "Bearer"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we moving to a new signer name? Coz, that's not what legacy signer name is . If so, an easy change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think that is the name of the signer? look at what exists for the SigV4AuthSchemeOption
https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-core/include/smithy/identity/auth/built-in/SigV4AuthSchemeOption.h#L15
and SigV4aAuthSchemeOption
https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-core/include/smithy/identity/auth/built-in/SigV4aAuthSchemeOption.h#L15
this is the AuthScheme name, which as in the SRA reference is named as smithy.api#HTTPBearerAuth
. You can find the exact same in the scheme id for java for bearer auth
https://github.com/aws/aws-sdk-java-v2/blob/5253ae375004b0f51036657eda6fc539f9cd2035/core/http-auth/src/main/java/software/amazon/awssdk/http/auth/scheme/BearerAuthScheme.java#L36
namespace smithy | ||
{ | ||
|
||
static const char SSO_DEFAULT_BEARER_TOKEN_PROVIDER_CHAIN_LOG_TAG[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, maybe we should just inline these to avoid pollution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it in the class scope to make it better
|
||
char BearerTokenAuthSchemeOption::BEARER_SCHEME[] = "smithy.api#HTTPBearerAuth"; | ||
|
||
AuthSchemeOption BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change that but the main reason for having the variable is usage from other places, so that way we just refer to one constant instead of copies elsewhere. I think we should extend this to the other auths too if you like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can refer to it other places using BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption::schemeId
without needing the static const char
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.