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: support for lambda authorizer #1334

Merged
merged 24 commits into from
Jul 30, 2021
Merged

feat: support for lambda authorizer #1334

merged 24 commits into from
Jul 30, 2021

Conversation

diegocstn
Copy link
Contributor

@diegocstn diegocstn commented Jul 21, 2021

Description of changes:
This PR adds support for a new authorization type supported by AppSync : AWS_LAMBDA.
Customers will be able to use an AWS Lambda function to determine whether to authorize a request based on a custom authorization token.

Similarly to OIDC, customers will be able to provide their own token to Amplify by providing an auth factory, an instance of a type conforming to APIAuthProviderFactory, and by overriding the functionAuthProvider() method to return an AmplifyFunctionAuthProvider as follow:

class MyFunctionAuthProvider: AmplifyFunctionAuthProvider {
  func getLatestAuthToken() -> Result<AuthToken, Error> {...}
}

class MyAPIAuthProviderFactory: APIAuthProviderFactory {
  open func functionAuthProvider() -> AmplifyFunctionAuthProvider? {
        return MyFunctionAuthProvider()
  }
}

let apiPlugin = AWSAPIPlugin(apiAuthProviderFactory: MyAPIAuthProviderFactory())
try Amplify.add(plugin: apiPlugin)

Support for Lambda in DataStore (multi-auth included) is achieved with new AuthRuleStrategy and AuthRuleProvider values custom and function.
We've decided to use custom as rule strategy for different reasons:

  • future implementations of additional auth modes as flexible as a Lambda function (i.e. http)
  • using a Lambda to decide whether a request is authenticated or not provides a high (if not the highest) degree of flexibility

Android PR: aws-amplify/amplify-android#1412
AppSync feature request: aws/aws-appsync-community#2

Check points:

  • Added new tests to cover change, if needed
  • All unit tests pass
  • All integration tests pass
  • Documentation update for the change if required
  • PR title conforms to conventional commit style

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

@diegocstn diegocstn changed the title Feat/lambda authorizer feat: support for lambda authorizer Jul 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #1334 (c7c15c7) into main (79c3cd9) will increase coverage by 0.07%.
The diff coverage is 56.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1334      +/-   ##
==========================================
+ Coverage   60.55%   60.62%   +0.07%     
==========================================
  Files         660      662       +2     
  Lines       19877    19951      +74     
==========================================
+ Hits        12036    12095      +59     
- Misses       7841     7856      +15     
Flag Coverage Δ
API_plugin_unit_test 65.83% <44.57%> (+0.18%) ⬆️
AWSPluginsCore 76.28% <80.95%> (-0.20%) ⬇️
Amplify 48.60% <0.00%> (+0.02%) ⬆️
Analytics_plugin_unit_test 73.41% <ø> (ø)
Auth_plugin_unit_test 82.31% <ø> (ø)
DataStore_plugin_unit_test 74.57% <100.00%> (+0.15%) ⬆️
Predictions_plugin_unit_test 33.51% <ø> (ø)
Storage_plugin_unit_test 57.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ries/API/AuthProvider/APIAuthProviderFactory.swift 0.00% <0.00%> (ø)
...ies/DataStore/Model/Internal/Schema/AuthRule.swift 0.00% <ø> (ø)
...ries/DataStore/Subscribe/MutationEvent+Model.swift 0.00% <ø> (ø)
.../RequestInterceptor/AuthTokenProviderWrapper.swift 0.00% <0.00%> (ø)
...scriptionInterceptor/OIDCAuthProviderWrapper.swift 0.00% <0.00%> (ø)
.../Configuration/AWSAuthorizationConfiguration.swift 0.00% <0.00%> (ø)
...uth/Configuration/AWSLambdaAuthConfiguration.swift 0.00% <0.00%> (ø)
...gin/Configuration/AWSAPIEndpointInterceptors.swift 60.86% <16.66%> (-10.19%) ⬇️
...tionFactory/AWSSubscriptionConnectionFactory.swift 63.46% <18.18%> (-6.11%) ⬇️
...CategoryPlugin/Operation/AWSGraphQLOperation.swift 61.62% <50.00%> (-0.28%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 318fcfd...c7c15c7. Read the comment docs.

@@ -10,12 +10,12 @@ import AWSPluginsCore
import Foundation
import AWSCore

struct UserPoolURLRequestInterceptor: URLRequestInterceptor {
struct AuthTokenURLRequestInterceptor: URLRequestInterceptor {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to better reflect its usage (used for CognitoUserPools, OIDC and Lambda)

Comment on lines +22 to +24
open func functionAuthProvider() -> AmplifyFunctionAuthProvider? {
return nil
}
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 choose the name function to avoid leaking service implementation detail into the library public API

@diegocstn diegocstn marked this pull request as ready for review July 22, 2021 15:51
case amazonCognitoUserPools = "AMAZON_COGNITO_USER_POOLS"

/// Control access by calling a lambda function,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lambda Auth dev guide hasn't been published yet, I'll add a SeeAlso section as soon as the guide is available

@diegocstn diegocstn requested a review from a team July 26, 2021 17:34

public protocol AmplifyAuthTokenProvider {
typealias AuthToken = String
func getLatestAuthToken() -> Result<AuthToken, Error>
Copy link
Member

Choose a reason for hiding this comment

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

Let's verify that the code path that invokes the token provider wraps any returned error in an Amplify APIError. If so, then I'm OK leaving this returning Error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • AuthTokenURLRequestInterceptor (mutations, queries) wraps the returned error in a APIError.operationError
  • OIDCAuthInterceptor (subscriptions) in case of failure retrieving the token it just skips the auth step and doesn't return/wrap the error

@@ -60,14 +60,21 @@ struct AWSAPIEndpointInterceptors {
"")
}
let provider = BasicUserPoolTokenProvider(authService: authService)
let interceptor = UserPoolURLRequestInterceptor(userPoolTokenProvider: provider)
let interceptor = AuthTokenURLRequestInterceptor(authTokenProvider: provider)
addInterceptor(interceptor)
case .openIDConnect:
guard let oidcAuthProvider = apiAuthProviderFactory.oidcAuthProvider() else {
return
Copy link
Member

Choose a reason for hiding this comment

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

Change to throws per discussion. Add note of changed behavior to CHANGELOG

addInterceptor(interceptor)
case .function:
guard let functionAuthProvider = apiAuthProviderFactory.functionAuthProvider() else {
return
Copy link
Member

Choose a reason for hiding this comment

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

Change to throws per discussion. Add note of changed behavior to CHANGELOG

Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

Comments inline, plus:

  • Add integ tests

Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

Checkpoint review so I'll have something to baseline against

@diegocstn
Copy link
Contributor Author

Comments inline, plus:

  • Add integ tests

Added API integration tests

@diegocstn diegocstn requested a review from palpatim July 27, 2021 17:44

/// General purpose authenticatication subscriptions interceptor for providers whose only
/// requirement is to provide an authentication token via the "Authorization" header
class AuthenticationTokenAuthInterceptor: AuthInterceptor {
Copy link
Contributor Author

@diegocstn diegocstn Jul 27, 2021

Choose a reason for hiding this comment

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

This is largely based on OIDCAuthInterceptor.
All the AuthInterceptors (this one included) share a common logic, we should evaluate if it's feasible to refactor in a common interceptor with customization points that others provider-specific interceptors can use to inject custom logic.

@diegocstn diegocstn requested a review from palpatim July 28, 2021 15:38
Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

Approved w/deprecation note

@diegocstn diegocstn merged commit ff756bb into main Jul 30, 2021
@diegocstn diegocstn deleted the feat/lambda-authorizer branch July 30, 2021 01:37
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.

3 participants