Skip to content
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

feat: Bearer token auth #786

Merged
merged 23 commits into from
Jul 25, 2024
Merged

feat: Bearer token auth #786

merged 23 commits into from
Jul 25, 2024

Conversation

sichanyoo
Copy link
Contributor

@sichanyoo sichanyoo commented Jul 17, 2024

Companion PR: awslabs/aws-sdk-swift#1637

Issue #

awslabs/aws-sdk-swift#1083

Description of changes

Refer to comments on github diff

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor Author

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments to help reviewers

.testTarget(
name: "SmithyHTTPAuthTests",
dependencies: ["SmithyHTTPAuth", "SmithyHTTPAPI", "Smithy", "SmithyIdentity", "ClientRuntime"]
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New target needed to add tests for BearerTokenSigner addd to the SmithyHTTPAuth module.

@@ -0,0 +1,26 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new auth scheme for HTTP bearer auth. It scheme ID comes from the trait id for @httpBearerAuth Smithy trait. And it uses BearerTokenSigner to sign the request.

@@ -0,0 +1,32 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BearerTokenSigner signs the request by simply appending Auhotrization: Bearer <token-string> header to the HTTP request.

@@ -0,0 +1,20 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BearerTokenIdentity that acts as credentials for HTTP bearer auth. Its token: String field literally just gets chucked onto Authorization header of the HTTP request during signing.

@@ -0,0 +1,12 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new identity resolver protocol that all concrete implementations for identity resolvers that resolve token identity must conform to.

@@ -126,6 +126,7 @@ class MiddlewareExecutionGenerator(
writer.write(" .withAuthSchemeResolver(value: config.authSchemeResolver)")
writer.write(" .withUnsignedPayloadTrait(value: ${op.hasTrait(UnsignedPayloadTrait::class.java)})")
writer.write(" .withSocketTimeout(value: config.httpClientConfiguration.socketTimeout)")
writer.write(" .withIdentityResolver(value: config.bearerTokenIdentityResolver, schemeID: \$S)", "smithy.api#httpBearerAuth")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need bearerTokenIdentityResolver added to middleware context.

@@ -0,0 +1,16 @@
package software.amazon.smithy.swift.codegen.swiftmodules
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BearerTokenAuthScheme, but in codegen side now.

@@ -6,6 +6,9 @@ import software.amazon.smithy.swift.codegen.SwiftDependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the new runtime types but in codegen side now.

@@ -0,0 +1,39 @@
package software.amazon.smithy.swift.codegen.utils
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a refactor for how we codegen the list of auth schemes supported by the service. Before, there was no auth scheme in smithy-swift, and so in smithy-swift codegen, the value of authSchemes field was always an empty array. The getModeledAuthSchemesSupportedBySDK was in aws-sdk-swift and only concerned itself with sigv4 & sigv4a. But now that BearerTokenAuthScheme is added to smithy-swift, generating list of auth schemes was pulled into smithy-swift.

Comment on lines 35 to 38
open fun addAdditionalSchemes(writer: SwiftWriter, authSchemeList: Array<String>): Array<String> {
// Override to add any additional auth schemes supported
return authSchemeList
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called within the getModeledAuthSchemesSupportedBySDK method above. THIS is the extension point for any custom codegen built on top of smithy-swift, e.g., aws-sdk-swift.

aws-sdk-swift detects & adds AWS specific auth schemes using this extension point.

writer: SwiftWriter,
): String {
val effectiveAuthSchemes = ServiceIndex(ctx.model).getEffectiveAuthSchemes(ctx.service)
var authSchemeList = arrayOf<String>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lists are usually preferred in Kotlin over arrays since they are a lot more powerful. It would probably be more idiomatic to use mutableListOf<String>() here and update related use cases accordingly

Copy link
Contributor Author

@sichanyoo sichanyoo Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested!

Comment on lines 30 to 32
authSchemeList = addAdditionalSchemes(writer, authSchemeList)

return "[${authSchemeList.joinToString(", ")}]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider changing to return addAdditionalSchemes(writer, authSchemeList).joinToString(prefix = "[", postfix = "]") or return authSchemeList.joinToString(prefix = "[", postfix = "]") to use idiomatic kotlin-provided functions -- prefix / postfix makes it more clear what's being added

the ", " in joinToString() should be unnecessary as comma separation is the default behavior (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/join-to-string.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested!

return "[${authSchemeList.joinToString(", ")}]"
}

open fun addAdditionalSchemes(writer: SwiftWriter, authSchemeList: Array<String>): Array<String> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change function signature to use Lists instead of Arrays

open fun addAdditionalSchemes(writer: SwiftWriter, authSchemeList: MutableList<String>): List<String>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested!

@sichanyoo sichanyoo requested a review from dayaffe July 25, 2024 17:38
@sichanyoo sichanyoo merged commit f5f1fd6 into main Jul 25, 2024
27 checks passed
@sichanyoo sichanyoo deleted the feat/bearer-token-auth branch July 25, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants