feat(datastore): StartSync with Auth #471

lawmicha · 2020-05-20T15:49:31Z

Issue #, if available:

Description of changes:
This adds support for auth enabled models.

dependency on Amplify.Auth's cognito auth plugin, ie. "awsCognitoAuthPlugin"
moved StorageEngine's startSync to tryStartSync that does much more: checks if auth is required, check if signed in, if not then wait for sign in. if signed in, start waiting for signed out
on sign-in, it will remoteSyncEngine.start()
on sign-out, it will clear(). Added new clearCompleted event for tests to listen on
pass the auth plugin down to the actual creation of the subscriptions, if auth exists, it will retrieve necessary identityClaim (initial support for this defaults to username at the moment, could be sub, or something else) to pass as ownerId to the subscription APIs

Misc updates

change AuthError.service check to AuthError.signedOut check, to match @royjit 's expected response for unauthenticated users when processing errors in ProcessMutationErrorFromCloudOperation

Acceptance tests

testUnauthenticatedSavesToLocalStoreIsReconciledWithCloudStoreAfterAuthentication
testOwnerCreatedDataCanBeReadByOtherUsersForReadableModel

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

drochetti · 2020-05-20T16:28:17Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/AWSDataStorePlugin.swift

@@ -82,7 +88,7 @@ final public class AWSDataStorePlugin: DataStoreCategoryPlugin {
        let filter = HubFilters.forEventName(HubPayload.EventName.Amplify.configured)
        var token: UnsubscribeToken?
        token = Amplify.Hub.listen(to: .dataStore, isIncluded: filter) { _ in
-            if self.hasValidAPIPlugin() {
+            if self.hasRequiredPlugins() {
                self.storageEngine.startSync()
            } else {
                self.log.info("Unable to find suitable plugin for syncEngine.  syncEngine will not be started")


I think we could be more informative here. Since the sync engine could not start for different reasons it would be cool if we provided a more detailed message of the actual reason it didn't start (and maybe use log.warn?)

i consolidated this logic and the logic from RemoteSyncEngine over to StorageEngine and added more logging. I have some warn and some error as well now. the initial check (in the new code) checks for APIPlugin and that is an info because this is to account for developers using local store first without API. all other logging are warn afterwards where there is API but something else is unexpected

drochetti · 2020-05-20T16:30:45Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift

+    }
+
+    private func checkIfAuthenticationRequired() {
+        log.debug(#function)


what will this print? We could have a more informative message here, so it's easier to search in the logs

i believe checkIfAuthenticationRequired(). added more to all

log.debug("\(#function) <message>")

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift

...toreCategoryPluginTests/Sync/MutationQueue/ProcessMutationErrorFromCloudOperationTests.swift

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift

wooj2 · 2020-05-20T16:47:36Z

Amplify/Categories/DataStore/Model/Schema/ModelSchema+Attributes.swift

+    var isAuthEnabled: Bool {
+        return !authRules.isEmpty
+    }


I'm curious as to what isAuthEnabled refers to. Is it simply to say that the models use an @auth directive? If so, then using "Enabled" might be misleading. Perhaps a better name would be isAuth?

Interesting feedback, maybe hasAuth?

i was thinking it means these "models are enabled with auth rules for finer grain control", which means that they require auth, or an authenticated signed request when syncing to the cloud. By having auth directive on the model, they can control how the service checks for access, whether that is checking the token or signed request.

from the two isAuth and hasAuth, i think isAuth makes me think of "the model is authenticated" which doesn't make a lot of sense, maybe "hasAuth" is "hasAuthentication" or "hasAuthenticationRules" which is closer to the check !authRules.isEmpty

hasAuthenticationRules lgtm, I in favor of verbosity to communicate clear intent

wooj2 · 2020-05-20T16:51:04Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/AWSDataStorePlugin.swift

+        if !requireAuthPlugin() {
+            return true
+        }
+
+        if requireAuthPlugin() && hasValidAuthPlugin() {
+            return true
+        }
+
+        return false


Feels like this could be combined:

let requireAuthPlugin = requireAuthPlugin() return !requireAuthPlugin || (requireAuthPlugin && hasValidAuthPlugin())

nice, thanks! i ended up adding a bunch of guard statements to expand the checks and add logging in between

wooj2 · 2020-05-20T17:00:01Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine+Resolver.swift

+            case (.clearingStateOutgoingMutations, .clearedStateOutgoingMutations):
+                return .checkIfAuthenticationRequired
+            case (.checkIfAuthenticationRequired, .requireAuthenticatedUser):
+                return .waitForAuthenticatedUser
+            case (.waitForAuthenticatedUser, .authenticatedUser(let api,
+                                                                let storageEngineAdapter,
+                                                                let userId)):
+                return .initializingSubscriptions(api, storageEngineAdapter, userId)
+            case (.checkIfAuthenticationRequired, .authenticatedUser(let api,
+                                                                     let storageEngineAdapter,
+                                                                     let userId)):
+                return .initializingSubscriptions(api, storageEngineAdapter, userId)
+            case (.checkIfAuthenticationRequired, .authenticationNotRequired(let api, let storageEngineAdapter)):


Seems like we've adopted a strategy where we handle listening to auth state within the sync engine. Did we consider another approach like listening to auth state in StorageEngine and determine whether or not we should start/stop the sync engine from there?

Been chatting with @elorzafe regarding the auth strategy and we agree that DataStore should listen to auth state changes in general

JS current requires the developers to listen to auth state changes themselves:
@elorzafe, please correct me if i'm wrong here:
If sign-in event: no public facing API to start the sync engine, but can call DataStore.clear() which re-iniitializes the syncEngine as a secondary behavior
if sign out event: call DataStore.clear() to clear local data

JS DataStore
JS customers have to make a call to DataStore APIs like DataStore.query(). this wll try to

start to start the sync engine (fail if not authenticated, fall back to IAM credentials for sigv4 signing, different architecture from iOS intercategory plugin communication - IOS cannot fall back to IAM, but utimately there is an eventual fail case).

assuming this is async, the data returned to the customer may not be up to date since sync engine's reconcilation path may not have caught up.

The most efficient time to start the sync engine is by listening to auth sign in event. if all customers which use auth enabled models require the user to be signed in before making API calls (subscriptions and sync query) then DataStore should hold this listening logic. it should be waiting for Auth events like sign-in and sign-out.

where to put auth state listener
The different places this auth state check can live:

At the very top level, DataStorePlugin will wait for DataStore configure event, check for API plugin before starting the sync engine. then check if there are required API and optionally Auth plugin. this can be expanded to add the auth state change listener here

As you mentioned, in the StorageEngine, this looks like a possible candidate and be a good place when we address the auth state listener for sign-out because the clear() API is there. (we may then refactor the auth state lisnter code for sign-in event to the same place)

where it is currently in syncEngine: this allows the sync engine to do some of the non-auth related house keeping work that can be done without the API calls (subscription and sync query). leaving the wait for auth state change in the most optimal spot. and SyncEngine already exists a state machine like schedulingRestart and errored which will allow us to easily add "expoential retry if user is not signed in, then give up and listen for auth state change"

refactored code to put it in StorageEngine

also added the signOut listener

removed auth listener code (removed sign in/sign out listener code) for less impactful change

… listener

wooj2 · 2020-05-22T17:36:01Z

...SDataStoreCategoryPlugin/Sync/SubscriptionSync/IncomingAsyncSubscriptionEventPublisher.swift

-                                                           subscriptionType: subscriptionType)
+        let request: GraphQLRequest<Payload>
+        if let auth = auth, modelType.schema.hasAuthenticationRules, let user = auth.getCurrentUser() {
+            // TODO: check model schema for identityClaim to figure out which is the ownerId field coming from


Seems like we should open an issue around this (which contains the impact for not doing this work), and then update the comment with a link to the issue.

good idea, adding issue #485, impact, and linked in comment

...CategoryPluginAuthIntegrationTests/AWSDataStoreCategoryPluginAuthIntegrationTests+Auth.swift

wooj2 · 2020-05-22T17:39:56Z

...CategoryPluginAuthIntegrationTests/AWSDataStoreCategoryPluginAuthIntegrationTests+Auth.swift

+                XCTFail("Failed to get auth session \(error)")
+            }
+        })
+        wait(for: [retrieveUserSubCompleted], timeout: TestCommonConstants.networkTimeout)
+        guard let result = resultOptional else {
+            fatalError("Could not get userSub for user")


We seem to use a mix of XCTFail and fatalError throughout this file. Is there any reason for using one over the other? If not, i think it makes sense to just use XCTFail

the reason for fatalError is because when using XCTFail, the guard statement still requires a return. although the return will not be used because the test will fail on XCTFail first, it's confusing to have to code a return statement that returns something

i've updated to XCTFail because in these cases, the return type is just String or Bool which makes it easy to return "" and false

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginAuthIntegrationTests/README.md

wooj2 · 2020-05-22T18:11:27Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginAuthIntegrationTests/README.md

+? Do you want to generate code for your newly created GraphQL API `No`
+```
+
+5. Copy `amplifyconfiguration.json` over as `AWSDataStoreCategoryPluginAuthIntegrationTests-amplifyconfiguration.json`


Are you saying to change the file name from amplifyconfiguration.json to AWSDataStoreCategoryPluginAuthIntegrationTests-amplifyconfiguration.json ?

yes, currently they're added to the HostApp's bundle. If we have multiple integration test targets, we need to prefix the configuration so each target reads their own configuration file

I see, we can word this to:
Copy amplifyconfiguration.json to a new file named AWSDataStoreCategoryPluginAuthIntegrationTests-amplifyconfiguration.json

wooj2 · 2020-05-22T18:39:09Z

...StoreCategoryPluginAuthIntegrationTests/AWSDataStoreCategoryPluginAuthIntegrationTests.swift

+        if isSignedIn() {
+            signOut()
+        }


is there any reason for this... because we are calling tearDown() which has signOut and reset?

this was to prevent previous test executions failures so tearDown isn't called, or tearDown() is called but failed for some reason. i think this is applicable for a single test run as well since one test method may fail while the next one continues

hmmm.. if teardown isnt' being called on a single test run, then should we just add signout to each one of the integration tests?

teardown should be called on a single test method run. i was referring to more of a scenario where test is stopped, say i manually terminate it by pressing Stop, then tearDown is never run

...StoreCategoryPluginAuthIntegrationTests/AWSDataStoreCategoryPluginAuthIntegrationTests.swift

lawmicha requested review from drochetti and wooj2 May 20, 2020 15:49

lawmicha force-pushed the feature/ds-auth-directive branch from 31f3888 to e541a79 Compare May 20, 2020 15:49

drochetti reviewed May 20, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift Outdated Show resolved Hide resolved

drochetti reviewed May 20, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift Outdated Show resolved Hide resolved

drochetti reviewed May 20, 2020

View reviewed changes

...toreCategoryPluginTests/Sync/MutationQueue/ProcessMutationErrorFromCloudOperationTests.swift Outdated Show resolved Hide resolved

drochetti reviewed May 20, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift Outdated Show resolved Hide resolved

lawmicha requested a review from palpatim May 20, 2020 16:39

wooj2 reviewed May 20, 2020

View reviewed changes

lawmicha force-pushed the feature/ds-auth-directive branch 2 times, most recently from f19d4e4 to 7d8450d Compare May 20, 2020 19:15

lawmicha changed the title ~~feat(datastore): RemoteSyncEngine handles authentication state~~ feat(datastore): Handle authentication state May 21, 2020

lawmicha force-pushed the feature/ds-auth-directive branch from d920c48 to d46e2c1 Compare May 21, 2020 15:55

lawmicha changed the title ~~feat(datastore): Handle authentication state~~ feat(datastore): TryStartSync handles Auth Directive Models with auth state listeners May 21, 2020

lawmicha requested review from drochetti and wooj2 May 21, 2020 17:43

lawmicha changed the title ~~feat(datastore): TryStartSync handles Auth Directive Models with auth state listeners~~ feat(datastore): Handles Auth enabled Models to control sync and clear May 21, 2020

lawmicha self-assigned this May 22, 2020

lawmicha added the datastore Issues related to the DataStore category label May 22, 2020

lawmicha added 8 commits May 22, 2020 07:03

feat(datastore): RemoteSyncEngine handles authentication state

78850fd

Add Integration test target

a749403

Add integration test set up

3a0e4e7

Move sync requirement to SyncEngine

c267d99

add check for listeners since each DataStore call will register a new…

9151ef3

… listener

update Auth plugin name

8546a82

Add clearCompleted event with integ test passing

796cd84

remove auth state listeners

4cd55d7

lawmicha force-pushed the feature/ds-auth-directive branch from 0302789 to 4cd55d7 Compare May 22, 2020 14:09

SyncQuery propagate error to error handler

24b246f

lawmicha changed the title ~~feat(datastore): Handles Auth enabled Models to control sync and clear~~ feat(datastore): StartSync with Auth May 22, 2020

Coode clean up, remove clearCompleted hub event, Amplify core changes

7b406e0

lawmicha force-pushed the feature/ds-auth-directive branch from dcf4814 to 7b406e0 Compare May 22, 2020 16:22

change hasAuth to hasAuthenticationRules

5db547f

wooj2 reviewed May 22, 2020

View reviewed changes

PR feedback

159f364

wooj2 approved these changes May 26, 2020

View reviewed changes

lawmicha added 2 commits May 26, 2020 11:13

readme update

c7a8787

fix unit test

c2342ba

lawmicha merged commit 7cab76f into master May 26, 2020

palpatim deleted the feature/ds-auth-directive branch May 27, 2020 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datastore): StartSync with Auth #471

feat(datastore): StartSync with Auth #471

lawmicha commented May 20, 2020 •

edited

Loading

drochetti May 20, 2020

lawmicha May 21, 2020

drochetti May 20, 2020

lawmicha May 21, 2020

wooj2 May 20, 2020

drochetti May 20, 2020

lawmicha May 21, 2020

drochetti May 21, 2020

wooj2 May 20, 2020

lawmicha May 21, 2020

wooj2 May 20, 2020

lawmicha May 20, 2020 •

edited

Loading

lawmicha May 21, 2020

lawmicha May 22, 2020 •

edited

Loading

wooj2 May 22, 2020 •

edited

Loading

lawmicha May 26, 2020

wooj2 May 22, 2020

lawmicha May 26, 2020 •

edited

Loading

lawmicha May 26, 2020

wooj2 May 22, 2020

lawmicha May 26, 2020

wooj2 May 26, 2020

wooj2 May 22, 2020

lawmicha May 26, 2020

wooj2 May 26, 2020

lawmicha May 26, 2020

feat(datastore): StartSync with Auth #471

feat(datastore): StartSync with Auth #471

Conversation

lawmicha commented May 20, 2020 • 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

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 May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha May 22, 2020 • edited Loading

Choose a reason for hiding this comment

wooj2 May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha May 26, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha commented May 20, 2020 •

edited

Loading

lawmicha May 20, 2020 •

edited

Loading

lawmicha May 22, 2020 •

edited

Loading

wooj2 May 22, 2020 •

edited

Loading

lawmicha May 26, 2020 •

edited

Loading