Skip to content

Clean up sign in state management #4491

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Clean up sign in state management #4491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6608c2
Simplify sign in state management
lfkellogg Dec 22, 2022
da17514
Reword comment
lfkellogg Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions ...appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,48 +24,58 @@

/** Class that handles storage for App Distribution SignIn persistence. */
class SignInStorage {

private static final String SIGNIN_PREFERENCES_NAME = "FirebaseAppDistributionSignInStorage";
private static final String SIGNIN_TAG = "firebase_app_distribution_signin";

private final Context applicationContext;
@Background private final Executor backgroundExecutor;
private SharedPreferences sharedPreferences;

private interface SharedPreferencesFunction<T> {
T apply(SharedPreferences sharedPreferences);
}

SignInStorage(Context applicationContext, @Background Executor backgroundExecutor) {
this.applicationContext = applicationContext;
this.backgroundExecutor = backgroundExecutor;
}

Task<Void> setSignInStatus(boolean testerSignedIn) {
return getSharedPreferences()
.onSuccessTask(
backgroundExecutor,
sharedPreferences -> {
sharedPreferences.edit().putBoolean(SIGNIN_TAG, testerSignedIn).apply();
return null;
});
return applyToSharedPreferences(
sharedPreferences -> {
sharedPreferences.edit().putBoolean(SIGNIN_TAG, testerSignedIn).apply();
return null;
});
}

Task<Boolean> getSignInStatus() {
return getSharedPreferences()
.onSuccessTask(
backgroundExecutor,
sharedPreferences -> Tasks.forResult(sharedPreferences.getBoolean(SIGNIN_TAG, false)));
return applyToSharedPreferences(
sharedPreferences -> sharedPreferences.getBoolean(SIGNIN_TAG, false));
}

boolean getSignInStatusBlocking() {
return getSharedPreferencesBlocking().getBoolean(SIGNIN_TAG, false);
}

private Task<SharedPreferences> getSharedPreferences() {
TaskCompletionSource<SharedPreferences> taskCompletionSource = new TaskCompletionSource<>();
backgroundExecutor.execute(
() -> taskCompletionSource.setResult(getSharedPreferencesBlocking()));
return taskCompletionSource.getTask();
}

private SharedPreferences getSharedPreferencesBlocking() {
// This may construct a new SharedPreferences object, which requires storage I/O
return applicationContext.getSharedPreferences(SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE);
}

private <T> Task<T> applyToSharedPreferences(SharedPreferencesFunction<T> func) {
// Check nullness of sharedPreferences directly even though multiple threads could be calling
// this function at once. This isn't a problem because: 1) once it is set it will never be reset
// back to null, and 2) even if it is initialized twice on different threads the second call
// will get the exact same instance.
if (sharedPreferences != null) {
return Tasks.forResult(func.apply(sharedPreferences));
}
TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
backgroundExecutor.execute(
() -> {
sharedPreferences = getSharedPreferencesBlocking();
taskCompletionSource.setResult(func.apply(sharedPreferences));
});
return taskCompletionSource.getTask();
}
}
20 changes: 14 additions & 6 deletions ...tribution/src/main/java/com/google/firebase/appdistribution/impl/TesterSignInManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package com.google.firebase.appdistribution.impl;

import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UNKNOWN;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;

Expand All @@ -34,7 +36,6 @@
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApp;
import com.google.firebase.annotations.concurrent.Blocking;
import com.google.firebase.annotations.concurrent.Lightweight;
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
import com.google.firebase.inject.Provider;
Expand Down Expand Up @@ -84,7 +85,7 @@ class TesterSignInManager {
@NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
@NonNull final SignInStorage signInStorage,
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
@Lightweight Executor blockingExecutor) {
@Blocking Executor blockingExecutor) {
this.firebaseApp = firebaseApp;
this.firebaseInstallationsApiProvider = firebaseInstallationsApiProvider;
this.signInStorage = signInStorage;
Expand All @@ -101,8 +102,15 @@ void onActivityCreated(Activity activity) {
// result of the signIn Task in the onActivityCreated callback
if (activity instanceof SignInResultActivity) {
LogWrapper.v(TAG, "Sign in completed");
this.setSuccessfulSignInResult();
this.signInStorage.setSignInStatus(true);
this.signInStorage
.setSignInStatus(true)
.addOnSuccessListener(blockingExecutor, unused -> this.setSuccessfulSignInResult())
.addOnFailureListener(
blockingExecutor,
e ->
this.setSignInTaskCompletionError(
new FirebaseAppDistributionException(
"Error storing tester sign in state", UNKNOWN, e)));
}
}

Expand Down Expand Up @@ -160,14 +168,14 @@ private Task<Void> doSignInTester() {
.getId()
.addOnFailureListener(
blockingExecutor,
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE))
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE))
.addOnSuccessListener(
blockingExecutor,
fid ->
getForegroundActivityAndOpenSignInFlow(fid)
.addOnFailureListener(
blockingExecutor,
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN)));
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, UNKNOWN)));

return signInTaskCompletionSource.getTask();
}
Expand Down
45 changes: 36 additions & 9 deletions ...ution/src/test/java/com/google/firebase/appdistribution/impl/TesterSignInManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@

import static com.google.common.truth.Truth.assertThat;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UNKNOWN;
import static com.google.firebase.appdistribution.impl.TestUtils.assertTaskFailure;
import static com.google.firebase.appdistribution.impl.TestUtils.awaitAsyncOperations;
import static com.google.firebase.appdistribution.impl.TestUtils.awaitTask;
import static com.google.firebase.appdistribution.impl.TestUtils.awaitTaskFailure;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
Expand All @@ -41,6 +44,8 @@
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApp;
import com.google.firebase.FirebaseOptions;
import com.google.firebase.annotations.concurrent.Background;
import com.google.firebase.annotations.concurrent.Blocking;
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
import com.google.firebase.appdistribution.impl.FirebaseAppDistributionServiceImplTest.TestActivity;
Expand Down Expand Up @@ -83,12 +88,13 @@ public class TesterSignInManagerTest {
private TestActivity activity;
private ShadowActivity shadowActivity;
private ShadowPackageManager shadowPackageManager;
private ExecutorService blockingExecutor;
@Blocking private ExecutorService blockingExecutor = TestOnlyExecutors.blocking();
@Background private ExecutorService backgroundExecutor = TestOnlyExecutors.background();
private SignInStorage signInStorage;

@Mock private Provider<FirebaseInstallationsApi> mockFirebaseInstallationsProvider;
@Mock private FirebaseInstallationsApi mockFirebaseInstallations;
@Mock private InstallationTokenResult mockInstallationTokenResult;
@Mock private SignInStorage mockSignInStorage;
@Mock private FirebaseAppDistributionLifecycleNotifier mockLifecycleNotifier;
@Mock private SignInResultActivity mockSignInResultActivity;

Expand All @@ -114,7 +120,8 @@ public void setUp() {

when(mockInstallationTokenResult.getToken()).thenReturn(TEST_AUTH_TOKEN);

when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
signInStorage =
spy(new SignInStorage(ApplicationProvider.getApplicationContext(), backgroundExecutor));

shadowPackageManager =
shadowOf(ApplicationProvider.getApplicationContext().getPackageManager());
Expand All @@ -137,21 +144,19 @@ public void setUp() {
shadowActivity = shadowOf(activity);
TestUtils.mockForegroundActivity(mockLifecycleNotifier, activity);

blockingExecutor = TestOnlyExecutors.blocking();

testerSignInManager =
new TesterSignInManager(
firebaseApp,
mockFirebaseInstallationsProvider,
mockSignInStorage,
signInStorage,
mockLifecycleNotifier,
blockingExecutor);
}

@Test
public void signInTester_alreadySignedIn_doesNothing()
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));
TestUtils.awaitTask(signInStorage.setSignInStatus(true));

Task signInTask = testerSignInManager.signInTester();
awaitTask(signInTask);
Expand Down Expand Up @@ -181,7 +186,7 @@ public void signInTester_whenUnexpectedFailureInTask_failsWithUnknownError() {

Task signInTask = testerSignInManager.signInTester();

awaitTaskFailure(signInTask, Status.UNKNOWN, "Unknown", unexpectedException);
awaitTaskFailure(signInTask, UNKNOWN, "Unknown", unexpectedException);
}

@Test
Expand All @@ -193,6 +198,7 @@ public void signInTester_whenChromeAvailable_opensCustomTab() throws Interrupted
shadowPackageManager.addResolveInfoForIntent(customTabIntent, resolveInfo);

testerSignInManager.signInTester();
awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

verify(mockFirebaseInstallations, times(1)).getId();
Expand All @@ -207,6 +213,7 @@ public void signInTester_whenChromeNotAvailable_opensBrowserIntent() throws Inte
shadowPackageManager.addResolveInfoForIntent(browserIntent, resolveInfo);

testerSignInManager.signInTester();
awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

verify(mockFirebaseInstallations, times(1)).getId();
Expand All @@ -219,6 +226,7 @@ public void signInTester_whenSignInCalledMultipleTimes_secondCallHasNoEffect()
testerSignInManager.signInTester();
testerSignInManager.signInTester();

awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

verify(mockFirebaseInstallationsProvider, times(1)).get();
Expand All @@ -228,18 +236,37 @@ public void signInTester_whenSignInCalledMultipleTimes_secondCallHasNoEffect()
public void signInTester_whenReturnFromSignIn_taskSucceeds()
throws InterruptedException, FirebaseAppDistributionException, ExecutionException {
Task signInTask = testerSignInManager.signInTester();
awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

// Simulate re-entering app after successful sign in, via SignInResultActivity
testerSignInManager.onActivityCreated(mockSignInResultActivity);
awaitTask(signInTask);

assertTrue(signInTask.isSuccessful());
verify(mockSignInStorage).setSignInStatus(true);
assertThat(signInStorage.getSignInStatusBlocking()).isTrue();
}

@Test
public void signInTester_whenStorageFailsToRecordSignInStatus_taskFails()
throws InterruptedException {
Exception expectedException = new RuntimeException("Error");
when(signInStorage.setSignInStatus(anyBoolean()))
.thenReturn(Tasks.forException(expectedException));
Task signInTask = testerSignInManager.signInTester();
awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

// Simulate re-entering app after successful sign in, via SignInResultActivity
testerSignInManager.onActivityCreated(mockSignInResultActivity);

awaitTaskFailure(signInTask, UNKNOWN, "Error storing tester sign in state", expectedException);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: long line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:googleJavaFormat says this is fine too. Looks like 100 chars is the max.

}

@Test
public void signInTester_whenAppReenteredDuringSignIn_taskFails() throws InterruptedException {
Task signInTask = testerSignInManager.signInTester();
awaitAsyncOperations(backgroundExecutor);
awaitAsyncOperations(blockingExecutor);

// Simulate re-entering app before completing sign in
Expand Down