From fc249c024dee8a3a10f9e0f79e4c93f8ecadbeff Mon Sep 17 00:00:00 2001 From: Grantland Chew Date: Wed, 26 Aug 2015 00:53:01 -0700 Subject: [PATCH] Fix signUp and linkWith with an anonymous user Fixes integration test failures caused by #52 We strip anonymity in `signUp` and `linkWith` before calling into `saveAsync` where there's a fork in logic for `isLazy()`. Since we now determine `isLazy()` based off `objectId != null && ParseAnonymousUtils.isLinked(this)`, `isLazy()` returns the incorrect value for these logic paths. The solution is to get the value of `isLazy()` before stripping anonymity and passing it through to `saveAsync()`. * Callers of resolveLazinessAsync never use it's return value, might as well make it Void * Make signUp more robust by reverting current user's password on failure if that's how it originally was * Make signUp more robust by reverting the current user's password on success --- Parse/src/main/java/com/parse/ParseUser.java | 100 ++++++++---------- .../test/java/com/parse/ParseUserTest.java | 58 +++++----- 2 files changed, 73 insertions(+), 85 deletions(-) diff --git a/Parse/src/main/java/com/parse/ParseUser.java b/Parse/src/main/java/com/parse/ParseUser.java index 9f9c07f7d..da65bf99e 100644 --- a/Parse/src/main/java/com/parse/ParseUser.java +++ b/Parse/src/main/java/com/parse/ParseUser.java @@ -475,26 +475,28 @@ private void restoreAnonymity(Map anonymousData) { @Override /* package */ Task saveAsync(String sessionToken, Task toAwait) { - synchronized (mutex) { - Task task; - if (isLazy()) { - task = resolveLazinessAsync(toAwait).makeVoid(); - } else { - task = super.saveAsync(sessionToken, toAwait); - } + return saveAsync(sessionToken, isLazy(), toAwait); + } - return task.onSuccessTask(new Continuation>() { - @Override - public Task then(Task task) throws Exception { - // If the user is the currently logged in user, we persist all data to disk - if (isCurrentUser()) { - cleanUpAuthData(); - return saveCurrentUserAsync(ParseUser.this); - } - return Task.forResult(null); - } - }); + /* package for tests */ Task saveAsync(String sessionToken, boolean isLazy, Task toAwait) { + Task task; + if (isLazy) { + task = resolveLazinessAsync(toAwait); + } else { + task = super.saveAsync(sessionToken, toAwait); } + + return task.onSuccessTask(new Continuation>() { + @Override + public Task then(Task task) throws Exception { + // If the user is the currently logged in user, we persist all data to disk + if (isCurrentUser()) { + cleanUpAuthData(); + return saveCurrentUserAsync(ParseUser.this); + } + return Task.forResult(null); + } + }); } @Override @@ -622,6 +624,7 @@ public Task then(Task task) throws Exception { checkForChangesToMutableContainers(); user.checkForChangesToMutableContainers(); + boolean isLazy = user.isLazy(); final String oldUsername = user.getUsername(); final String oldPassword = user.getPassword(); final Map anonymousData = user.getAuthData(ParseAnonymousUtils.AUTH_TYPE); @@ -631,21 +634,26 @@ public Task then(Task task) throws Exception { user.setPassword(getPassword()); revert(); - return user.saveAsync(sessionToken, toAwait).continueWithTask(new Continuation>() { + return user.saveAsync(sessionToken, isLazy, toAwait).continueWithTask(new Continuation>() { @Override public Task then(Task task) throws Exception { if (task.isCancelled() || task.isFaulted()) { // Error synchronized (user.mutex) { if (oldUsername != null) { user.setUsername(oldUsername); + } else { + user.revert(KEY_USERNAME); } if (oldPassword != null) { user.setPassword(oldPassword); + } else { + user.revert(KEY_PASSWORD); } user.restoreAnonymity(anonymousData); } return task; } else { // Success + user.revert(KEY_PASSWORD); revert(KEY_PASSWORD); } @@ -1165,7 +1173,7 @@ public Task then(Task task) throws Exception { user.stripAnonymity(); user.putAuthData(authType, authData); - return user.resolveLazinessAsync(task).makeVoid(); + return user.resolveLazinessAsync(task); } } }).continueWithTask(new Continuation>() { @@ -1224,20 +1232,14 @@ private Task linkWithAsync( final Map authData, final Task toAwait, final String sessionToken) { - final Map oldAnonymousData = getAuthData(ParseAnonymousUtils.AUTH_TYPE); synchronized (mutex) { + final boolean isLazy = isLazy(); + final Map oldAnonymousData = getAuthData(ParseAnonymousUtils.AUTH_TYPE); + stripAnonymity(); putAuthData(authType, authData); - // TODO(grantland): Should we really be getting the current user's sessionToken? - // What if we're not the current user? - // TODO(mengyan): Delete getCurrentSessionTokenAsync or delete the inject sessionToken - return ParseUser.getCurrentSessionTokenAsync().onSuccessTask(new Continuation>() { - @Override - public Task then(Task task) throws Exception { - return saveAsync(sessionToken, toAwait); - } - }).continueWithTask(new Continuation>() { + return saveAsync(sessionToken, isLazy, toAwait).continueWithTask(new Continuation>() { @Override public Task then(Task task) throws Exception { synchronized (mutex) { @@ -1285,32 +1287,22 @@ private void logOutWith(String authType) { * @return A {@code Task} that will resolve to the current user. If this is a SignUp it'll be this * {@code ParseUser} instance, otherwise it'll be a new {@code ParseUser} instance. */ - /* package for tests */ Task resolveLazinessAsync(Task toAwait) { + /* package for tests */ Task resolveLazinessAsync(Task toAwait) { synchronized (mutex) { - if (!isLazy()) { - return Task.forResult(null); - } if (getAuthData().size() == 0) { // TODO(grantland): Could we just check isDirty(KEY_AUTH_DATA)? // If there are no linked services, treat this as a SignUp. - return signUpAsync(toAwait).onSuccess(new Continuation() { - @Override - public ParseUser then(Task task) throws Exception { - synchronized (mutex) { - return ParseUser.this; - } - } - }); + return signUpAsync(toAwait); } final ParseOperationSet operations = startSave(); // Otherwise, treat this as a SignUpOrLogIn - return toAwait.onSuccessTask(new Continuation>() { + return toAwait.onSuccessTask(new Continuation>() { @Override - public Task then(Task task) throws Exception { - return getUserController().logInAsync(getState(), operations).onSuccessTask(new Continuation>() { + public Task then(Task task) throws Exception { + return getUserController().logInAsync(getState(), operations).onSuccessTask(new Continuation>() { @Override - public Task then(Task task) throws Exception { + public Task then(Task task) throws Exception { final ParseUser.State result = task.getResult(); Task resultTask; @@ -1319,29 +1311,25 @@ public Task then(Task task) throws Exception { if (Parse.isLocalDatastoreEnabled() && !result.isNew()) { resultTask = Task.forResult(result); } else { - resultTask = handleSaveResultAsync(result, operations).onSuccess(new Continuation() { + resultTask = handleSaveResultAsync(result, + operations).onSuccess(new Continuation() { @Override public ParseUser.State then(Task task) throws Exception { return result; } }); } - return resultTask.onSuccessTask(new Continuation>() { + return resultTask.onSuccessTask(new Continuation>() { @Override - public Task then(Task task) throws Exception { + public Task then(Task task) throws Exception { ParseUser.State result = task.getResult(); if (!result.isNew()) { // If the result is not a new user, treat this as a fresh logIn with complete // serverData, and switch the current user to the new user. final ParseUser newUser = ParseObject.from(result); - return saveCurrentUserAsync(newUser).onSuccess(new Continuation() { - @Override - public ParseUser then(Task task) throws Exception { - return newUser; - } - }); + return saveCurrentUserAsync(newUser); } - return Task.forResult(ParseUser.this); + return task.makeVoid(); } }); } diff --git a/Parse/src/test/java/com/parse/ParseUserTest.java b/Parse/src/test/java/com/parse/ParseUserTest.java index 2a7bf3acb..2b03db47d 100644 --- a/Parse/src/test/java/com/parse/ParseUserTest.java +++ b/Parse/src/test/java/com/parse/ParseUserTest.java @@ -15,6 +15,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Matchers; import org.robolectric.RobolectricGradleTestRunner; import org.robolectric.annotation.Config; @@ -217,10 +218,11 @@ public void testSignUpAsyncWithMergeInDiskAnonymousUser() throws Exception { ParseUser currentUser = mock(ParseUser.class); when(currentUser.getUsername()).thenReturn("oldUserName"); when(currentUser.getPassword()).thenReturn("oldPassword"); + when(currentUser.isLazy()).thenReturn(false); when(currentUser.isLinked(ParseAnonymousUtils.AUTH_TYPE)).thenReturn(true); when(currentUser.getSessionToken()).thenReturn("oldSessionToken"); when(currentUser.getAuthData()).thenReturn(new HashMap>()); - when(currentUser.saveAsync(anyString(), Matchers.>any())) + when(currentUser.saveAsync(anyString(), eq(false), Matchers.>any())) .thenReturn(Task.forResult(null)); ParseUser.State state = new ParseUser.State.Builder() .put("oldKey", "oldValue") @@ -248,7 +250,8 @@ public void testSignUpAsyncWithMergeInDiskAnonymousUser() throws Exception { verify(currentUser, times(1)).setUsername("userName"); verify(currentUser, times(1)).setPassword("password"); // Make sure we save currentUser - verify(currentUser, times(1)).saveAsync(eq("oldSessionToken"), Matchers.>any()); + verify(currentUser, times(1)) + .saveAsync(eq("oldSessionToken"), eq(false), Matchers.>any()); // Make sure we merge currentUser with user after save assertEquals("oldValue", user.get("oldKey")); // Make sure set currentUser @@ -262,14 +265,15 @@ public void testSignUpAsyncWithMergeInDiskAnonymousUserSaveFailure() throws Exce Map oldAnonymousAuthData = new HashMap<>(); oldAnonymousAuthData.put("oldKey", "oldToken"); currentUser.putAuthData(ParseAnonymousUtils.AUTH_TYPE, oldAnonymousAuthData); - ParseUser partialMockCurrentUser = spy(currentUser); + ParseUser partialMockCurrentUser = spy(currentUser); // Spy since we need mutex when(partialMockCurrentUser.getUsername()).thenReturn("oldUserName"); when(partialMockCurrentUser.getPassword()).thenReturn("oldPassword"); when(partialMockCurrentUser.getSessionToken()).thenReturn("oldSessionToken"); + when(partialMockCurrentUser.isLazy()).thenReturn(false); ParseException saveException = new ParseException(ParseException.OTHER_CAUSE, ""); doReturn(Task.forError(saveException)) .when(partialMockCurrentUser) - .saveAsync(anyString(), Matchers.>any()); + .saveAsync(anyString(), eq(false), Matchers.>any()); ParseCurrentUserController currentUserController = mock(ParseCurrentUserController.class); when(currentUserController.getAsync(anyBoolean())) .thenReturn(Task.forResult(partialMockCurrentUser)); @@ -294,7 +298,7 @@ public void testSignUpAsyncWithMergeInDiskAnonymousUserSaveFailure() throws Exce verify(partialMockCurrentUser, times(1)).copyChangesFrom(eq(user)); // Make sure we save currentUser verify(partialMockCurrentUser, times(1)) - .saveAsync(eq("oldSessionToken"), Matchers.>any()); + .saveAsync(eq("oldSessionToken"), eq(false), Matchers.>any()); // Make sure we restore old username and password after save fails verify(partialMockCurrentUser, times(1)).setUsername("oldUserName"); verify(partialMockCurrentUser, times(1)).setPassword("oldPassword"); @@ -574,7 +578,7 @@ public void testLinkWithAsyncWithSaveAsyncSuccess() throws Exception { ParseUser partialMockUser = spy(user); doReturn(Task.forResult(null)) .when(partialMockUser) - .saveAsync(anyString(), Matchers.>any()); + .saveAsync(anyString(), eq(false), Matchers.>any()); String authType = "facebook"; Map authData = new HashMap<>(); authData.put("token", "test"); @@ -586,7 +590,8 @@ public void testLinkWithAsyncWithSaveAsyncSuccess() throws Exception { // Make sure new authData is added assertSame(authData, partialMockUser.getAuthData().get("facebook")); // Make sure we save the user - verify(partialMockUser, times(1)).saveAsync(eq("sessionTokenAgain"), Matchers.>any()); + verify(partialMockUser, times(1)) + .saveAsync(eq("sessionTokenAgain"), eq(false), Matchers.>any()); // Make sure synchronizeAuthData() is called verify(provider, times(1)).restoreAuthentication(authData); } @@ -609,7 +614,7 @@ public void testLinkWithAsyncWithSaveAsyncFailure() throws Exception { Exception saveException = new Exception(); doReturn(Task.forError(saveException)) .when(partialMockUser) - .saveAsync(anyString(), Matchers.>any()); + .saveAsync(anyString(), eq(false), Matchers.>any()); String facebookAuthType = "facebook"; Map facebookAuthData = new HashMap<>(); facebookAuthData.put("facebookToken", "facebookTest"); @@ -621,7 +626,8 @@ public void testLinkWithAsyncWithSaveAsyncFailure() throws Exception { // Make sure new authData is added assertSame(facebookAuthData, partialMockUser.getAuthData().get("facebook")); // Make sure we save the user - verify(partialMockUser, times(1)).saveAsync(eq("sessionTokenAgain"), Matchers.>any()); + verify(partialMockUser, times(1)) + .saveAsync(eq("sessionTokenAgain"), eq(false), Matchers.>any()); // Make sure old authData is restored assertSame(anonymousAuthData, partialMockUser.getAuthData().get(ParseAnonymousUtils.AUTH_TYPE)); // Verify exception @@ -632,13 +638,6 @@ public void testLinkWithAsyncWithSaveAsyncFailure() throws Exception { //region testResolveLazinessAsync - @Test - public void testResolveLazinessAsyncWithNotLazyUser() throws Exception { - ParseUser user = new ParseUser(); - - ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); - } - @Test public void testResolveLazinessAsyncWithAuthDataAndNotNewUser() throws Exception { ParseUser user = new ParseUser(); @@ -660,14 +659,16 @@ public void testResolveLazinessAsyncWithAuthDataAndNotNewUser() throws Exception .thenReturn(Task.forResult(null)); ParseCorePlugins.getInstance().registerCurrentUserController(currentUserController); - ParseUser userAfterResolveLaziness = - ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); + ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); + ArgumentCaptor userAfterResolveLazinessCaptor = + ArgumentCaptor.forClass(ParseUser.class); // Make sure we logIn the lazy user verify(userController, times(1)).logInAsync( any(ParseUser.State.class), any(ParseOperationSet.class)); // Make sure we save currentUser - verify(currentUserController, times(1)).setAsync(any(ParseUser.class)); + verify(currentUserController, times(1)).setAsync(userAfterResolveLazinessCaptor.capture()); + ParseUser userAfterResolveLaziness = userAfterResolveLazinessCaptor.getValue(); // Make sure user's data is correct assertEquals("newSessionToken", userAfterResolveLaziness.getSessionToken()); assertEquals("newValue", userAfterResolveLaziness.get("newKey")); @@ -697,8 +698,7 @@ public void testResolveLazinessAsyncWithAuthDataAndNewUser() throws Exception { ParseCurrentUserController currentUserController = mock(ParseCurrentUserController.class); ParseCorePlugins.getInstance().registerCurrentUserController(currentUserController); - ParseUser userAfterResolveLaziness = - ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); + ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); // Make sure we logIn the lazy user verify(userController, times(1)).logInAsync( @@ -706,12 +706,10 @@ public void testResolveLazinessAsyncWithAuthDataAndNewUser() throws Exception { // Make sure we do not save currentUser verify(currentUserController, never()).setAsync(any(ParseUser.class)); // Make sure userAfterResolveLaziness's data is correct - assertEquals("newSessionToken", userAfterResolveLaziness.getSessionToken()); - assertEquals("newValue", userAfterResolveLaziness.get("newKey")); + assertEquals("newSessionToken", user.getSessionToken()); + assertEquals("newValue", user.get("newKey")); // Make sure userAfterResolveLaziness is not lazy - assertFalse(userAfterResolveLaziness.isLazy()); - // Make sure we do not create new user - assertSame(user, userAfterResolveLaziness); + assertFalse(user.isLazy()); } @Test @@ -739,8 +737,9 @@ public void testResolveLazinessAsyncWithAuthDataAndNotNewUserAndLDSEnabled() thr // Enable LDS Parse.enableLocalDatastore(null); - ParseUser userAfterResolveLaziness = - ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); + ParseTaskUtils.wait(user.resolveLazinessAsync(Task.forResult(null))); + ArgumentCaptor userAfterResolveLazinessCaptor = + ArgumentCaptor.forClass(ParseUser.class); // Make sure we logIn the lazy user verify(userController, times(1)).logInAsync( @@ -749,7 +748,8 @@ public void testResolveLazinessAsyncWithAuthDataAndNotNewUserAndLDSEnabled() thr // field should be cleaned assertEquals("password", user.getPassword()); // Make sure we do not save currentUser - verify(currentUserController, times(1)).setAsync(any(ParseUser.class)); + verify(currentUserController, times(1)).setAsync(userAfterResolveLazinessCaptor.capture()); + ParseUser userAfterResolveLaziness = userAfterResolveLazinessCaptor.getValue(); // Make sure userAfterResolveLaziness's data is correct assertEquals("newSessionToken", userAfterResolveLaziness.getSessionToken()); assertEquals("newValue", userAfterResolveLaziness.get("newKey"));