-
Notifications
You must be signed in to change notification settings - Fork 117
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(auth): Add aws-core and AWSCredentialsProvider #2316
Conversation
@@ -16,6 +16,7 @@ package com.amplifyframework.auth | |||
|
|||
import com.amplifyframework.auth.result.AuthSessionResult | |||
|
|||
@Deprecated("This class was released with public visibility, but is not intended to be consumed.") |
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.
Want to start a discussion on this deprecated messaging. I've added to a few files that shouldn't be consumed. For this file in particular, I don't think there is any risk a customer would have used. It was adding within the last month and named "Internal".
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.
Would the deprecation message generate errors during normal build, or only if a customer consumes the deprecated symbols directly in their app code?
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 think its only if they directly consume, but I will confirm. We have build warnings right now that will show when building Amplify directly that we can choose to suppress or ignore ( there are quite a few files I'll need to add a suppress warning on some of these internally).
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.
Could we add a new annotation specific to amplify library? something similar to this
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.
@sdhuka I decided against it since we shouldn't remove these implementations since they were once public. I don't want us to forget down the line that removal would be a breaking change because that comment block wasn't always there. Open to reconsidering options here though, especially for a class like this that has "Internal" in the name.
interface AWSCredentialsProvider<out T : AWSCredentials> { | ||
|
||
fun fetchAWSCredentials( | ||
onSuccess: Consumer<@UnsafeVariance T>, |
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.
Want to get thoughts on usage of @UnsafeVariance
here, rather than using in
. While in
is technically correct, it forces customers to use a non obvious Consumer<in AWSTemporaryCredentials
in onSuccess and Java users to type Consumer<super ? AWSTemporaryCredentials>
.
UnsafeVariance still provides compile time safety here and appears to be commonly used in Kotlin Std Lib, such as List implementation.
) | ||
} | ||
|
||
internal fun <T : AWSCredentials> convertToCredentialsProvider( |
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.
This internal conversion is so that we can take a customer provided credentials provider and pass to the Kotlin SDK.
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.
naming suggestion to go with Tim's comment: convertToSdkCredentialsProvider
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.
opened up convertToSdkCredentialsProvider to public to allow customers an easy way to convert to SDKs credentialsprovider to be used in the escape hatch. This way a single provider could be shared.
@@ -26,6 +26,7 @@ import kotlin.coroutines.suspendCoroutine | |||
/** | |||
* Wrapper to provide credentials from Auth synchronously and asynchronously | |||
*/ | |||
@Deprecated("This interface was released with public visibility, but is not intended to be consumed.") | |||
open class CognitoCredentialsProvider : AuthCredentialsProvider { |
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'm having second thoughts about this. If the customer uses the Kotlin SDK directly, this CredentialsProvider allows them to easily use a pre-built one that fetches credentials via Amplify.Auth
.
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 removed the deprecation. This gives customers the AWSCredentialsProvider that we intend for them to implement on their own, while keeping this as a prebuilt one that can be passed into Kotlin SDK.
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
* Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
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.
FWIW, I think our standard is to adopt the no-year version of the copyright message. See examples at https://github.com/aws-amplify/amplify-swift/blob/main/Amplify/Amplify.swift#L1-L6, enforced by https://github.com/aws-amplify/amplify-swift/blob/main/.swiftformat#L12-L13
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.
Great! I'll create a ticket for us to do in a future PR.
@@ -85,3 +86,12 @@ class AWSTemporaryCredentials( | |||
*/ | |||
val expiration: Instant | |||
) : AWSCredentials(accessKeyId, secretAccessKey) | |||
|
|||
internal fun AWSCredentials.toCredentials(): Credentials { |
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.
Credentials
as a return type makes sense b/c it's from the Kotlin SDK. For the method name, I might consider something a more specific, especially since it's internal: toSdkCredentials
or similar?
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.
Sure, this is pre-existing and moved files where I felt like it belongs, but I like the suggestion.
aws-core/src/main/java/com/amplifyframework/auth/AWSCredentialsProvider.kt
Outdated
Show resolved
Hide resolved
aws-core/src/main/java/com/amplifyframework/auth/CognitoCredentialsProvider.kt
Outdated
Show resolved
Hide resolved
…tialsProvider.kt Co-authored-by: Tim Schmelter <[email protected]>
Co-authored-by: Tim Schmelter <[email protected]>
Co-authored-by: Tim Schmelter <[email protected]>
…sProvider.kt Co-authored-by: Tim Schmelter <[email protected]>
@@ -99,6 +104,14 @@ subprojects { | |||
failOnPassedAfterRetry.set(true) | |||
} | |||
} | |||
|
|||
tasks.withType<KotlinCompile> { |
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.
🙌
aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/AWSS3StoragePlugin.java
Show resolved
Hide resolved
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2316 +/- ##
==========================================
- Coverage 41.04% 41.01% -0.03%
==========================================
Files 764 765 +1
Lines 24930 24936 +6
Branches 3538 3539 +1
==========================================
- Hits 10232 10227 -5
- Misses 13585 13595 +10
- Partials 1113 1114 +1 |
aws-geo-location/src/main/java/com/amplifyframework/geo/location/AWSLocationGeoPlugin.kt
Outdated
Show resolved
Hide resolved
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.
One code change, otherwise LGTM.
Co-authored-by: Divyesh Chitroda <[email protected]>
) | ||
} | ||
|
||
fun <T : AWSCredentials> convertToSdkCredentialsProvider( |
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 dont think this needs to be publicly exposed. If the goal is to pass AWSCredentialsProvider to other components and SDK CredentialsProvider is always accessible through escape hatch, I dont see a reason for this conversion API. Maybe start with non-public and if customers need it we can exposed it in the future.
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.
@div5yesh The reason I exposed is because I don't think we want customers having to use multiple credentials providers, or at least, having to create their own AWSCredentialsProvider and SDKs CredentialsProvider.
Our suggested implementation should be to create a single AWSCredentialsProvider, and when that is needed for the Kotlin SDK escape hatch, they can use this public conversion method.
* fix(core): remove unused dynamic nav dependency (#2132) * fix(datastore): remove typename from ModelMetadata (#2122) * fix(datastore): remove typename from ModelMetadata * Test a potential fix * Test a potential fix * fix: callbacks not invoked when attached using getTransfer api (#2111) * fix: listener not invoked when attached using getTransfer api * address PR comments * default state to unknown * add comment * add comment * override getRequest in storage operation & make SocketExcpetion retryable * add nullable annotation * address nullable request Co-authored-by: Tyler Roach <[email protected]> * Prevent attempting to read backed up EncryptedSharedPreferences that are no longer readable (#2113) * fix(auth): device metadata migration (#2114) * Number of attributes being too high is not retryable (#2112) * Number of attributes being too high is not retryable * Match iOS at not retrying bad request error -- covering multiple 400 errors * Fix lint * Fix lint * Update Kotlin SDK version Co-authored-by: Thomas Leing <[email protected]> * Change errors returned on some apis while federated (#2116) * release: Amplify Android 2.0.0 (#2115) * release: Amplify Android 2.0.0 * add core-kotlin changelog * add more info in changelog * revert unintended change * remove breaking change for analytics * fix(datastore): remove typename from ModelMetadata * fix(datastore): remove typename from ModelMetadata * remove unneeded file * small cleanup * remove print statements * fix comment * force tests * force tests * force tests Co-authored-by: Michael Law <[email protected]> Co-authored-by: Michael Schneider <[email protected]> Co-authored-by: Saijad Dhuka <[email protected]> Co-authored-by: Tyler Roach <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Sunil Timalsina <[email protected]> * chore: Remove deprecated maven plugin (#2137) * Remove deprecated maven project * Fix task name grep * chore: Remove Javadoc tasks (#2139) * Remove deprecated maven project * Fix task name grep * Remove Javadoc tasks * fix: Change order of updating state in local cache (#2141) * fix: fix integration test and added logger to integration test (#2143) * fix: Change order of updating state in local cache * change order for updating status and add logger to integ tests * change log level to debug * Fix for when move to idle state is called twice (#2152) * Update README.md (#2120) remove dev-preview APIs note. * Dengdan stress test (#2153) * Initial commit * Work in progress * finish codes * change build * update build * test excludeStressTest * Revert "Merge branch 'main' into dengdan-stress-test" This reverts commit b50840e, reversing changes made to 3bacf1b. * remove categories * remove external changes * remove external changes * remove more changes * Update copyright and refactor * Update StorageStressTest.kt * Update StorageStressTest.kt * Update StorageStressTest.kt * Update StorageStressTest.kt * linting * Update StorageStressTest.kt * Delete StorageStressTest.kt * Delete amplifyconfigurationupdated.json * Delete amplifyconfigurationupdated.json * Update DataStoreStressTest.kt * Fix(Auth): Sign up if successful should return DONE instead of Confirm sign up (#2130) * If sign up is successful in the first try return DONE * Sign up should send DONE if it is successful * revert jsongenerator cleandir fun * lint fix * Feat(Auth Test): Custom party testing for Custom Test without SRP (#2149) * Adding custom auth test cases * Updating test cases for Custom Auth to ensure they pass * lint format * Fix for phone number * Recreate all tests * Unignore storage and pinpoint tests (#2156) * unignore tests * extend timeout to 60s Co-authored-by: Saijad Dhuka <[email protected]> * feat(Geo): Add Kotlin Geo Facade (#2155) * Add Kotlin Geo Facade * Add return docs to the function comments * Add tests to verify options are passed * fix: Add missing apis in storage Kotlin & RxJava facade (#2160) * fix: Add pause, resume api to kotlin and rxJava facade * add unit tests * remove irrelevant code changes * add assertion * Update DeviceFarm build config (#2168) * fix: user metadata was persisted empty in the database (#2165) * fix: usermeta was persisted as empty in the database * fix compilation error * fix compilation error * avoid adding metadata if it is null * Add Geo Rx Bindings (#2159) * chore: Re-add storage tests (#2163) * Readd storage tests * rename file * reduce stress Co-authored-by: Saijad Dhuka <[email protected]> * Add a network status listener to restart DataStore after the network … (#2148) * Add a network status listener to restart DataStore after the network comes back online. * Add Reachability monitor * working pretty well * cleanup * update test * fix: fix integration test and added logger to integration test (#2143) * fix: Change order of updating state in local cache * change order for updating status and add logger to integ tests * change log level to debug * Fix for when move to idle state is called twice (#2152) * Update README.md (#2120) remove dev-preview APIs note. * Dengdan stress test (#2153) * Initial commit * Work in progress * finish codes * change build * update build * test excludeStressTest * Revert "Merge branch 'main' into dengdan-stress-test" This reverts commit b50840e, reversing changes made to 3bacf1b. * remove categories * remove external changes * remove external changes * remove more changes * Update copyright and refactor * Update StorageStressTest.kt * Update StorageStressTest.kt * Update StorageStressTest.kt * Update StorageStressTest.kt * linting * Update StorageStressTest.kt * Delete StorageStressTest.kt * Delete amplifyconfigurationupdated.json * Delete amplifyconfigurationupdated.json * Update DataStoreStressTest.kt * force build * force build * force build * fix typo * Add a network status listener to restart DataStore after the network comes back online. * Add Reachability monitor * working pretty well * cleanup * update test * force build * force build * force build * fix typo * reply to comments * Add testImplementation lin eto compile tests correctly in intellij * reply to comments * make ReachabilityMonitor expose the observable * Update datastore plugin to use the reachability monitor * cleanup * cleanup * cleanup * force tests Co-authored-by: Michael Schneider <[email protected]> Co-authored-by: Saijad Dhuka <[email protected]> Co-authored-by: gpanshu <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> Co-authored-by: dengdan154 <[email protected]> * chore: Upgrade Gradle, AGP, and KtLint (#2172) * Remove deprecated maven project * Fix task name grep * Upgrade Gradle to 7.5.1 * Upgrade AGP * Add VERSION_NAME back to BuildConfig This was removed for library projects in AGP 4.1. We may consider renaming this in the future to disambiguate the Amplify version and the application version. * Update KtLint and fix all new lint errors * Use JDK11 on codebuild * Use JDK11 in workflows * Upgrade compileSdkVersion to 31 * Try using custom commands to install Android SDK 31 * Update device farm buildspec with manual Android SDK install * Fix additional ktlint errors * Upgrade Desugar to JDK11-compatible version * Upgrade Robolectric * Set locale explicitly to match expectation * fix(geo): Increase Geo timeout so that it runs successfully on a Pixel 3a XL (#2177) * Increase Geo timeout so that it runs successfully on a Pixel 3a XL * Fix lint Co-authored-by: Thomas Leing <[email protected]> * Add a buildspec file for nightly tests (#2180) * Chore(Auth): Implementation of the custom auth with SRP parity testing use case (#2167) * Added the test case for custom auth with SRP * ktlint * release: Amplify Android 2.1.0 (manually created) (#2185) Co-authored-by: Thomas Leing <[email protected]> * chore: Add PR checker workflow (#2188) * fix(Auth): Fix for when loading credentials the success/error is fired twice (#2184) * chore: update changelog for Amplify Android 2.1.0 (#2193) * feat(Auth): Overriding sign in when the State machine is already in the signing in state (#2187) * chore: fix inconsistency with endpointWithAttributes test (#2196) * chore: update changelog for v2.1.0 (#2198) * chore: replace md5 with sha-256 for file data validation (#2199) * chore: Remove unused version and group properties (#2186) * chore: Add a label that will disable the PR title check (#2195) Co-authored-by: gpanshu <[email protected]> * chore: add release tag to PR title checker config (#2194) Co-authored-by: Matt Creaser <[email protected]> * fix(datastore): Fix lock contention issue when running DataStore.start() from the callback of DataStore.stop() (#2208) Co-authored-by: Michael Schneider <[email protected]> * chore: Add group and version back to all subprojects (#2213) * chore: Remove the release-kotlin_v block from prepare release script (#2231) * fix(core): Remove unused dependencies (#2207) Co-authored-by: Matt Creaser <[email protected]> * chore: Modify the bump_version branch name to be specific to the base branch (#2240) * fix(geo): Bump MapLibre SDK to 9.6.0 (#2254) * release: Amplify Android 2.1.1 (#2257) Co-authored-by: amplify-android-dev+ghops <[email protected]> * fix(analytics): Remove test dependencies from implementation configuration (#2253) * chore: Ignore clearStopsSyncAndDeletesDatabase test (#2262) * fix(auth): Fix Authorization header for HostedUI fetchToken when appSecret is used (#2264) * chore: Supply base_branch when calling `create_next_release_pr` lane (#2245) * chore: Update PR template to include security checklist item (#2251) * feat(auth): add required hash param to cognito api calls (#2266) Co-authored-by: Banji Jolaoso <[email protected]> Co-authored-by: AWS Mobile SDK Bot <[email protected]> Co-authored-by: Saijad Dhuka <[email protected]> Co-authored-by: Tyler Roach <[email protected]> * feat(datastore): Add recoverability improvements (#2201) Co-authored-by: Matt Creaser <[email protected]> * feat(auth): Added parity test for fetchDevices,rememberDevice,forgetDevice and fetchUserAttributes (#2174) Co-authored-by: Sunil Timalsina <[email protected]> Co-authored-by: Banji Jolaoso <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> Co-authored-by: Matt Creaser <[email protected]> Co-authored-by: Saijad Dhuka <[email protected]> Co-authored-by: Tyler Roach <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: gpanshu <[email protected]> Co-authored-by: AWS Mobile SDK Bot <[email protected]> * release: Amplify Android 2.2.0 (#2269) Co-authored-by: amplify-android-dev+ghops <[email protected]> * chore: Convert build.gradle files to Kotlin (#2183) * Convert build.gradle files to Kotlin * Fix missing test dependency * Update copyright year * chore: Set the signing information in project extras (#2277) * fix(auth): Moving credential provider to main (#2273) * chore(analytics): Simplify error code checking, uniformly across platforms (#2274) Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Matt Creaser <[email protected]> * feat(auth): added kover plugin for coverage (#2267) Co-authored-by: Banji Jolaoso <[email protected]> Co-authored-by: gpanshu <[email protected]> * release: Amplify Android 2.2.1 (#2285) Co-authored-by: amplify-android-dev+ghops <[email protected]> Co-authored-by: Matt Creaser <[email protected]> * chore: Remove circleci configuration file (#2279) Co-authored-by: gpanshu <[email protected]> * fix(auth): fix npe in initialize fetch auth session (#2284) Co-authored-by: Banji Jolaoso <[email protected]> * fix(auth): Fix confirm signin when incorrect MFA code is entered (#2286) * release: Amplify Android 2.2.2 (#2292) Co-authored-by: amplify-android-dev+ghops <[email protected]> * chore: Fix using amplify-android in a composite build (#2294) * chore: update device farm configuration for nightly build (#2281) * chore: remove buildspec files (#2308) * chore: remove amplify-data from codeowners (#2252) Co-authored-by: Saijad Dhuka <[email protected]> * fix(datastore): Fix aliasing of column names (#2312) * feat(storage): Add support for S3 acceleration mode (#2304) * feat(aws-datastore): Make the reachability component configurable (#2307) Signed-off-by: Manuel Iglesias <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> * release: Amplify Android 2.3.0 (#2323) Co-authored-by: amplify-android-dev+ghops <[email protected]> * feat(aws-api,aws-datastore): WebSocket improvements (#2283) Signed-off-by: Manuel Iglesias <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> Co-authored-by: Matt Creaser <[email protected]> * chore(auth): Use restricted API to surface the JSON used to configure the Auth category (#2311) Co-authored-by: Thomas Leing <[email protected]> * chore: Remove incorrect Developer Preview label (#2328) * fix(auth): Delete user invalid state fixes (#2326) Co-authored-by: Anshul Gupta <[email protected]> Co-authored-by: gpanshu <[email protected]> * Restore publishing sources jar (#2329) Co-authored-by: Thomas Leing <[email protected]> * release: Amplify Android 2.3.0 (#2330) Co-authored-by: amplify-android-dev+ghops <[email protected]> Co-authored-by: Thomas Leing <[email protected]> * feat(auth): Add aws-core and AWSCredentialsProvider (#2316) Co-authored-by: Tim Schmelter <[email protected]> Co-authored-by: Erica Eaton <[email protected]> Co-authored-by: Divyesh Chitroda <[email protected]> --------- Signed-off-by: Manuel Iglesias <[email protected]> Co-authored-by: Erica Eaton <[email protected]> Co-authored-by: Michael Schneider <[email protected]> Co-authored-by: Michael Law <[email protected]> Co-authored-by: Michael Schneider <[email protected]> Co-authored-by: Saijad Dhuka <[email protected]> Co-authored-by: Tyler Roach <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Thomas Leing <[email protected]> Co-authored-by: Sunil Timalsina <[email protected]> Co-authored-by: Matt Creaser <[email protected]> Co-authored-by: gpanshu <[email protected]> Co-authored-by: dengdan154 <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: amplify-android-dev+ghops <[email protected]> Co-authored-by: banji180 <[email protected]> Co-authored-by: Banji Jolaoso <[email protected]> Co-authored-by: AWS Mobile SDK Bot <[email protected]> Co-authored-by: Manuel Iglesias <[email protected]> Co-authored-by: Dane Pilcher <[email protected]> Co-authored-by: Anshul Gupta <[email protected]> Co-authored-by: Tim Schmelter <[email protected]>
Issue #, if available:
Description of changes:
There is currently a Kotlin SDK dependency on aws.credentials that should not exist in core. AWS specific files have been moved out of core into a new aws-core lib. While this is technically a breaking change, all aws feature libraries now include aws-core, so there shouldn't be any customer impact. If a customer pulls in any aws packages, aws-core will be pulled in.
How did you test these changes?
(Please add a line here how the changes were tested)
Documentation update required?
General Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.