Reprovision Storage Integration test backend #256

lawmicha · 2019-12-12T16:47:59Z

Major changes

reprovisioned storage resource in test account
Adding readme for integration test set up

Minor changes

Moved AuthHelper to AmplifyTestCommon for API and Storage tests

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

palpatim

Need to move the password out of the file unless it is our intention to provision a new user pool/storage backend and tear it down after each integ test run.

...FunctionalTests/BlogPostCommentGraphQLWithAPIKey/BlogPostCommentGraphQLWithAPIKeyTests.swift

palpatim · 2019-12-13T18:03:50Z

...fyPlugins/Storage/AWSS3StoragePluginFunctionalTests/AWSS3StoragePluginAccessLevelTests.swift

@@ -19,8 +20,8 @@ class AWSS3StoragePluginAccessLevelTests: AWSS3StoragePluginTestBase {

    // This is a run once function to set up users then use console to verify and run rest of these tests.
    func testSetUpOnce() {
-        signUpUser(username: user1)
-        signUpUser(username: user2)
+        AuthHelper.signUpUser(username: user1, password: password)


We need to move the password out of the source file and into a setup run from the CLI. As it stands, if an attacker obtains our user pool ID (which remember is not considered sensitive), they could use the test storage bucket to store and distribute files.

depending on what the attacker obtains, couldn't they also just create their own user as well? I thought that if awsconfiguration.json and amplifyconfiguration.json is never exposed then we are good, and the username and password for this user is specific for the user pool and wouldn't provide any value to the attacker.

if we need to move this out, i can follow a similar pattern and have another file like credentials.json that gets pulled down like https://github.com/aws-amplify/aws-sdk-ios/blob/f3967e2e2a648e8a114ef5dd50bef5e075668693/AWSAuthSDK/AWSAuthSDKTestAppUITests/AWSDropInUIUserPoolTests.swift#L24

We treat things like pool IDs with different sensitivity than credentials like password. (e.g., we may log a user pool ID). A credentials.json pattern or similar would be one way to protect that, but let's talk about it IRL to make sure we don't end up with a catch-all for "everything that doesn't fit in a config file." :)

If this test is setting up something that allows guest access, we need to have some controls in place to ensure we don't have unauthorized access to our bucket. Again, let's discuss IRL.

discussed with tim and in the current state of things, we'll do a couple of things to make things more secure and less prone to abuse:

credentials.json to contain user/pass. this can be stored in the same pattern as the configuration files, ie. AWSS3StorageIntegrationTests-credentials.json.

Not as urgent for API plugin but will do the same

add *credentials.json to .gitignore

bucket policy updated to expire asap, looks like 1 day is the shortest possible.

The tests need to be prone to parallel execution. S3 keys defined in the tests should contain some unique identifier. there are many cases where the tests may be run in parallel. Multiple circleCI runs and developer using test account locally (though ideally developer should create their own resources using the readme instructions).

AmplifyPlugins/Storage/AWSS3StoragePluginFunctionalTests/AWSS3StoragePluginTestBase.swift

palpatim · 2019-12-13T18:08:58Z

AmplifyTestCommon/Helpers/AuthHelper.swift

@@ -13,8 +13,8 @@ class AuthHelper {
        let callbackInvoked = DispatchSemaphore(value: 0)

        AWSMobileClient.default().initialize { userState, error in
-            if let error = error {
-                fatalError("Error initializing AWSMobileClient. Error: \(error.localizedDescription)")
+            if let error = error as? AWSMobileClientError {


Why only fatal on AWSMobileClientErrors?

i didn't think AWSMobileClient returns anything other than AWSMobileClientErrors, so i was casting to it and extracting the message out for a friendly local debugging message

I follow your logic, but it relies a lot of knowledge of the code paths that lead up to this, which in AWSMobileClient is a little fraught right now. Consider a pattern that logs the message if it's an AWSMobielClientError and the localizedDescription otherwise.

adding check for AWSMobileClientError, else fatalError with localizedDescription

AmplifyTestCommon/Helpers/AuthHelper.swift

AmplifyPlugins/Storage/AWSS3StoragePluginFunctionalTests/README.md

AmplifyPlugins/Storage/AWSS3StoragePluginFunctionalTests/AWSS3StoragePluginTestBase.swift

- Moved AuthHelper to AmplifyTestCommon for API and Storage tests - Adding readme for integration test set up

- Storage tests use unique UUIDs as keys to allow parallel execution - updated README with credentials.json structure - Added for API's GraphQLWithUserPool tests as well

palpatim

Some small changes but otherwise LGTM

palpatim · 2019-12-17T18:35:58Z

...nTests/GraphQL/GraphQLWithUserPoolIntegrationTests/GraphQLWithUserPoolIntegrationTests.swift

+            let credentials = try TestConfigHelper.retrieveCredentials(
+                forResource: GraphQLWithUserPoolIntegrationTests.credentials)
+
+            if credentials["user1"] == nil || credentials["password"] == nil {


nit: prefer guard let when you're trying to get actual values:

guard let userName = credentials["user1"], let password = credentials["password"] else {...}

palpatim · 2019-12-17T18:38:04Z

...nTests/GraphQL/GraphQLWithUserPoolIntegrationTests/GraphQLWithUserPoolIntegrationTests.swift

@@ -42,18 +56,19 @@ class GraphQLWithUserPoolIntegrationTests: XCTestCase {
        Amplify.reset()
    }

-    let user1 = "[email protected]"
-    let password = "Abc123@@!!"
+
    // This is a run once function to set up users then use console to verify and run rest of these tests.
    func testSetUpOnce() {


Since running this by itself isn't sufficient to actually provision the test user, let's remove it altogether in favor of including CLI commands or console instructions in the test setup README.

updating readme with instructions on setting up a user and removing disabled method

palpatim · 2019-12-17T18:39:09Z

...nTests/GraphQL/GraphQLWithUserPoolIntegrationTests/GraphQLWithUserPoolIntegrationTests.swift


    override func setUp() {
        do {
+
+            let credentials = try TestConfigHelper.retrieveCredentials(


Put this into the static setUp since you only have to read the credentials file once

palpatim · 2019-12-17T18:41:21Z

AmplifyPlugins/Storage/AWSS3StoragePluginFunctionalTests/AWSS3StoragePluginTestBase.swift

    override func setUp() {
        do {
+
+            let credentials = try TestConfigHelper.retrieveCredentials(forResource: AWSS3StoragePluginTestBase.credentials)


make this a static set up method?

…provision

lawmicha requested review from palpatim and wooj2 December 12, 2019 16:49

lawmicha changed the title ~~- reprovisioned storage resource in test account~~ Reprovision Storage Integration tests Dec 12, 2019

lawmicha self-assigned this Dec 12, 2019

lawmicha added the storage Issues related to the Storage category label Dec 12, 2019

lawmicha changed the title ~~Reprovision Storage Integration tests~~ Reprovision Storage Integration test backend Dec 12, 2019

palpatim requested changes Dec 13, 2019

View reviewed changes

wooj2 reviewed Dec 13, 2019

View reviewed changes

lawmicha force-pushed the lawmicha/has-syncable-flag branch 2 times, most recently from fcc497a to f3b683c Compare December 16, 2019 17:19

- reprovisioned storage resource in test account

2ea82d4

- Moved AuthHelper to AmplifyTestCommon for API and Storage tests - Adding readme for integration test set up

lawmicha force-pushed the lawmicha/storage-reprovision branch from 7fe0c7d to 2ea82d4 Compare December 16, 2019 20:32

lawmicha changed the base branch from lawmicha/has-syncable-flag to master December 16, 2019 20:32

address PR comments

0915185

lawmicha mentioned this pull request Dec 16, 2019

APIPlugin Blog/Post/Comment Tests #263

Closed

lawmicha added 2 commits December 17, 2019 09:51

- Recreated Users and hiding them in the credentials.json file

d8c9930

- Storage tests use unique UUIDs as keys to allow parallel execution - updated README with credentials.json structure - Added for API's GraphQLWithUserPool tests as well

improve AuthHelper error check

40a5ea6

lawmicha requested a review from palpatim December 17, 2019 18:05

palpatim approved these changes Dec 17, 2019

View reviewed changes

lawmicha added 3 commits December 18, 2019 10:48

moving some setup to static set up

ca6e643

Merge remote-tracking branch 'origin/master' into lawmicha/storage-re…

034c1e8

…provision

finish testing after merge

7d2adb6

lawmicha merged commit 89fd3a6 into master Dec 18, 2019

palpatim deleted the lawmicha/storage-reprovision branch December 24, 2019 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reprovision Storage Integration test backend #256

Reprovision Storage Integration test backend #256

lawmicha commented Dec 12, 2019 •

edited

Loading

palpatim left a comment

palpatim Dec 13, 2019

lawmicha Dec 16, 2019

palpatim Dec 17, 2019 •

edited

Loading

lawmicha Dec 17, 2019

lawmicha Dec 17, 2019

palpatim Dec 13, 2019

lawmicha Dec 16, 2019

palpatim Dec 17, 2019

lawmicha Dec 17, 2019

palpatim left a comment

palpatim Dec 17, 2019

palpatim Dec 17, 2019

lawmicha Dec 18, 2019

palpatim Dec 17, 2019

palpatim Dec 17, 2019

Reprovision Storage Integration test backend #256

Reprovision Storage Integration test backend #256

Conversation

lawmicha commented Dec 12, 2019 • edited Loading

Major changes

Minor changes

palpatim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palpatim Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palpatim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha commented Dec 12, 2019 •

edited

Loading

palpatim Dec 17, 2019 •

edited

Loading