diff --git a/gleap/src/main/java/io/gleap/GleapPreferencesHelper.java b/gleap/src/main/java/io/gleap/GleapPreferencesHelper.java index 5f795d8..249d661 100644 --- a/gleap/src/main/java/io/gleap/GleapPreferencesHelper.java +++ b/gleap/src/main/java/io/gleap/GleapPreferencesHelper.java @@ -93,18 +93,29 @@ public byte[] decode(String text) { }; private final SharedPreferences prefs; - // null: no usable key, values are stored as plain text. - private final ValueCipher cipher; + // Loaded on first use: the key lookup is a keystore call, which must not run when the SDK + // is initialized on the main thread. Null once resolved: no usable key, values are plain text. + private ValueCipher cipher; + private boolean cipherResolved; private static GleapPreferencesHelper instance; private GleapPreferencesHelper(Context context) { - this(context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE), keystoreCipher()); + this.prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE); } GleapPreferencesHelper(SharedPreferences prefs, ValueCipher cipher) { this.prefs = prefs; this.cipher = cipher; + this.cipherResolved = true; + } + + private synchronized ValueCipher cipher() { + if (!cipherResolved) { + cipher = keystoreCipher(); + cipherResolved = true; + } + return cipher; } static synchronized GleapPreferencesHelper getInstance(Context context) { @@ -120,6 +131,7 @@ public void putString(String key, String value) { prefs.edit().remove(key).apply(); return; } + ValueCipher cipher = cipher(); if (cipher != null) { try { prefs.edit().putString(key, cipher.encrypt(value)).apply(); @@ -137,6 +149,7 @@ public String getString(String key, String defaultValue) { if (stored == null) { return defaultValue; } + ValueCipher cipher = cipher(); if (cipher != null) { try { return cipher.decrypt(stored); diff --git a/gleap/src/main/java/io/gleap/GleapSessionController.java b/gleap/src/main/java/io/gleap/GleapSessionController.java index 9ac8822..2e207e2 100644 --- a/gleap/src/main/java/io/gleap/GleapSessionController.java +++ b/gleap/src/main/java/io/gleap/GleapSessionController.java @@ -11,6 +11,11 @@ public class GleapSessionController { private static volatile GleapSessionController instance; private GleapSessionProperties gleapSessionProperties; private volatile GleapSession gleapSession; + // The stored session and user are read on first use (see ensureStateLoaded). + private volatile boolean stateLoaded = false; + // Only guards the load. Not `this`: the load is slow, and the many methods that lock the + // controller (e.g. setPendingIdentificationAction) must not wait for it. + private final Object stateLoadLock = new Object(); private GleapSessionProperties pendingIdentificationAction; private GleapSessionProperties pendingUpdateAction; private volatile boolean isSessionLoaded = false; @@ -32,22 +37,45 @@ private GleapSessionController(Application application) { GleapSessionController(KeyValueStore store) { this.store = store; + } - // Load existing session. - String id = store.getString("session_id", ""); - String hash = store.getString("session_hash", ""); - if (!id.equals("") && !hash.equals("")) { - gleapSession = new GleapSession(id, hash); + /** + * Loads the stored session and user. Reading them decrypts every value with the keystore key + * (a binder call each), so the constructor, which runs on the app's main thread, leaves it + * to the first caller and {@link #initialize} warms it up on the SDK's own thread. Every method + * that writes the session or the user calls this first, so a load never overwrites a write. + * The load takes only {@code stateLoadLock}, never the controller's monitor, so it can be + * awaited while holding it. + */ + private void ensureStateLoaded() { + if (stateLoaded) { + return; + } + synchronized (stateLoadLock) { + if (stateLoaded) { + return; + } + String id = store.getString("session_id", ""); + String hash = store.getString("session_hash", ""); + if (!id.equals("") && !hash.equals("")) { + gleapSession = new GleapSession(id, hash); + } + this.gleapSessionProperties = getStoredGleapUser(); + stateLoaded = true; } - - // Load existing session properties. - this.gleapSessionProperties = getStoredGleapUser(); } public static GleapSessionController initialize(Application application) { synchronized (GleapSessionController.class) { if (instance == null) { instance = new GleapSessionController(application); + final GleapSessionController controller = instance; + GleapExecutor.SERIAL.execute(new Runnable() { + @Override + public void run() { + controller.ensureStateLoaded(); + } + }); } return instance; } @@ -136,6 +164,7 @@ boolean hasIdentifyHash() { * project requires a file session and the current session has no valid one. */ boolean needsFileAccessIdentify(GleapSessionProperties identify) { + ensureStateLoaded(); GleapSession session = gleapSession; return identify != null && identify.getHash() != null && !identify.getHash().isEmpty() && session != null && session.isAuthenticatedFilesRequired() @@ -189,6 +218,7 @@ boolean isCurrentGeneration(int generation) { } public void clearUserSession() { + ensureStateLoaded(); synchronized (this) { identityGeneration.incrementAndGet(); // Revoke the file session before the session that holds it is dropped. @@ -244,6 +274,7 @@ void keepIdentifyPending(GleapSessionProperties identify, int generation, boolea } public void mergeUserSession(String id, String hash) { + ensureStateLoaded(); if (gleapSession == null) { gleapSession = new GleapSession(id, hash); } else { @@ -255,10 +286,12 @@ public void mergeUserSession(String id, String hash) { } public GleapSession getUserSession() { + ensureStateLoaded(); return gleapSession; } public void setGleapUserSession(GleapSessionProperties gleapUser) { + ensureStateLoaded(); this.gleapSessionProperties = gleapUser; if (gleapUser == null) { @@ -395,6 +428,7 @@ void processSessionActionResult(JSONObject result, boolean restartEventServices, // registerPushMessageGroup(hash) below would no-op when the previous // value happens to be cached here. GleapSession session; + ensureStateLoaded(); synchronized (this) { if (!isCurrentGeneration(generation)) { GleapLog.i("Dropped a session answer from before the logout"); @@ -477,6 +511,7 @@ private static boolean sameValue(String a, String b) { } public GleapSessionProperties getGleapUserSession() { + ensureStateLoaded(); return gleapSessionProperties; } diff --git a/gleap/src/test/java/io/gleap/GleapSessionControllerTest.java b/gleap/src/test/java/io/gleap/GleapSessionControllerTest.java index 6a446d2..628a209 100644 --- a/gleap/src/test/java/io/gleap/GleapSessionControllerTest.java +++ b/gleap/src/test/java/io/gleap/GleapSessionControllerTest.java @@ -15,6 +15,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import io.gleap.callbacks.GetActivityCallback; import io.gleap.callbacks.InitializationDoneCallback; @@ -190,4 +192,76 @@ public void theIdentifiedUserSurvivesARestart() throws Exception { assertEquals(3, restored.getSla(), 0); assertEquals("gold", restored.getCustomData().getString("tier")); } + + @Test + public void storedSessionIsNotReadWhenTheControllerIsCreated() { + final int[] reads = {0}; + InMemoryKeyValueStore counting = new InMemoryKeyValueStore() { + @Override + public String getString(String key, String defaultValue) { + reads[0]++; + return super.getString(key, defaultValue); + } + }; + counting.values.put("session_id", "stored-id"); + counting.values.put("session_hash", "stored-hash"); + + GleapSessionController created = new GleapSessionController(counting); + + assertEquals(0, reads[0]); + assertEquals("stored-id", created.getUserSession().getId()); + assertEquals("stored-hash", created.getUserSession().getHash()); + } + + @Test + public void queueingAnIdentifyDoesNotWaitForTheStoredStateToLoad() throws Exception { + final CountDownLatch loadStarted = new CountDownLatch(1); + final CountDownLatch releaseLoad = new CountDownLatch(1); + InMemoryKeyValueStore slow = new InMemoryKeyValueStore() { + @Override + public String getString(String key, String defaultValue) { + loadStarted.countDown(); + try { + releaseLoad.await(10, TimeUnit.SECONDS); + } catch (InterruptedException ignored) { + } + return super.getString(key, defaultValue); + } + }; + slow.values.put("session_id", "stored-id"); + slow.values.put("session_hash", "stored-hash"); + final GleapSessionController slowController = new GleapSessionController(slow); + + Thread loader = new Thread(new Runnable() { + @Override + public void run() { + slowController.getUserSession(); + } + }); + loader.start(); + try { + assertTrue(loadStarted.await(5, TimeUnit.SECONDS)); + + final CountDownLatch queued = new CountDownLatch(1); + final GleapSessionProperties identify = new GleapSessionProperties(); + identify.setUserId("user-1"); + Thread caller = new Thread(new Runnable() { + @Override + public void run() { + slowController.setPendingIdentificationAction(identify); + slowController.getPendingIdentificationAction(); + slowController.isIdentifyInFlight(); + queued.countDown(); + } + }); + caller.start(); + + assertTrue("queueing an identify waited for the load", queued.await(2, TimeUnit.SECONDS)); + assertEquals(identify, slowController.getPendingIdentificationAction()); + } finally { + releaseLoad.countDown(); + loader.join(5000); + } + assertEquals("stored-id", slowController.getUserSession().getId()); + } }