Skip to content

Commit b07ef22

Browse files
rpdomerpdome
committed
wip
1 parent bbd2004 commit b07ef22

File tree

6 files changed

+200
-47
lines changed

6 files changed

+200
-47
lines changed

‎common/src/main/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClient.kt‎

Lines changed: 48 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
297297
telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?): BrokerData? {
298298
if (!shouldSkipCache) {
299299
getActiveBrokerFromInMemoryCache(telemetryCallback)?.let {
300-
return it
300+
return it.brokerData
301301
}
302302
}
303303

@@ -307,9 +307,9 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
307307
telemetryCallback?.onLockAcquired(System.nanoTime() - totalTimeSpentAcquiringLock)
308308

309309
if (!shouldSkipCache) {
310-
// Early return if another coroutine has already populated the in-memory cache.
310+
// Early return if another method call has already populated the in-memory cache.
311311
getActiveBrokerFromInMemoryCache(telemetryCallback)?.let {
312-
return@runBlocking it
312+
return@runBlocking it.brokerData
313313
}
314314

315315
getActiveBrokerFromStorageCache(telemetryCallback)?.let {
@@ -326,9 +326,16 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
326326
}
327327
}
328328

329+
/**
330+
* Loads active broker from in-memory cache.
331+
*
332+
* @param telemetryCallback Callback for telemetry events (for OneAuth).
333+
* @return [BrokerInMemoryCache] object - if null, there's no broker cached.
334+
* If the object isn't null, but it's brokerData is null, there's no valid broker installed.
335+
**/
329336
private fun getActiveBrokerFromInMemoryCache(
330337
telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?
331-
): BrokerData? {
338+
): BrokerInMemoryCache? {
332339
val methodTag = "$TAG:getValidBrokerFromInMemoryCache"
333340

334341
// There's no readWriteLock in coroutines library yet... volatile should be sufficient for our use case here.
@@ -339,21 +346,18 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
339346
methodTag,
340347
"[Cached] No broker app is installed."
341348
)
342-
return null
349+
return it
343350
}
344351

352+
// For the majority of the case, broker apps doesn't change - so we can skip other checks.
353+
// That said, this "isValidPackageInstalled" check is a security measure,
354+
// so i think we should still need to invoke it every time before we reach out to broker.
345355
val timeStartIsValidBroker = System.nanoTime()
346356
val isValidBroker = isValidBroker(it.brokerData)
347357
telemetryCallback?.onFinishCheckingIfValidBroker(System.nanoTime() - timeStartIsValidBroker)
348358
if (isValidBroker) {
349-
return it.brokerData
359+
return it
350360
}
351-
352-
Logger.info(
353-
methodTag,
354-
"Clearing cache as the installed app does not have a matching signature hash."
355-
)
356-
clearCachedData()
357361
}
358362

359363
// Nothing valid to return.
@@ -370,6 +374,29 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
370374
cache.setCachedActiveBroker(brokerData)
371375
}
372376

377+
/**
378+
* Loads active broker from AccountManager, then cache the result.
379+
* @param telemetryCallback Callback for telemetry events (for OneAuth).
380+
* @return The cached active broker if exists and valid; null otherwise.
381+
**/
382+
private fun getFromAccountManagerAndCacheResult(
383+
telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?): BrokerData? {
384+
val methodTag = "$TAG:getFromAccountManagerAndCacheResult"
385+
386+
telemetryCallback?.onUseAccountManager()
387+
val accountManagerResult = getActiveBrokerFromAccountManager()
388+
Logger.info(
389+
methodTag, "Tried getting active broker from account manager, " +
390+
"get ${accountManagerResult?.packageName}."
391+
)
392+
return accountManagerResult
393+
}
394+
395+
/**
396+
* Loads cached active broker from storage.
397+
* @param telemetryCallback Callback for telemetry events (for OneAuth).
398+
* @return The cached active broker if exists and valid; null otherwise.
399+
**/
373400
private fun getActiveBrokerFromStorageCache(
374401
telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?
375402
): BrokerData? {
@@ -379,11 +406,7 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
379406
telemetryCallback?.onReadFromCache(System.nanoTime() - timeStartReadingFromCache)
380407
if (cache.shouldUseAccountManager()) {
381408
telemetryCallback?.onUseAccountManager()
382-
val accountManagerBroker = getActiveBrokerFromAccountManager()
383-
384-
// Only cache in memory, but don't persist accountManager result to cache storage.
385-
cachedBroker = BrokerInMemoryCache(accountManagerBroker)
386-
return accountManagerBroker
409+
return getFromAccountManagerAndCacheResult(telemetryCallback)
387410
}
388411

389412
val timeStartIsPackageInstalled = System.nanoTime()
@@ -431,10 +454,15 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
431454
return null
432455
}
433456

457+
/**
458+
* Query the installed broker apps to get active broker, then cache it.
459+
*
460+
* @param telemetryCallback Callback for telemetry events (for OneAuth).
461+
* @return The cached active broker (if installed).
462+
**/
434463
private suspend fun getActiveBrokerFromBrokerApp(
435464
telemetryCallback: IBrokerDiscoveryClientTelemetryCallback?
436465
): BrokerData? {
437-
val methodTag = "$TAG:getActiveBrokerFromBrokerApp"
438466
val timeStartQueryFromBroker = System.nanoTime()
439467
val brokerData = queryFromBroker(
440468
brokerCandidates = brokerCandidates,
@@ -449,21 +477,13 @@ class BrokerDiscoveryClient(private val brokerCandidates: Set<BrokerData>,
449477
return brokerData
450478
}
451479

480+
// Fall back to AccountManager (for older brokers).
452481
cache.setShouldUseAccountManagerForTheNextMilliseconds(
453482
TimeUnit.MINUTES.toMillis(
454483
60
455484
)
456485
)
457486

458-
telemetryCallback?.onUseAccountManager()
459-
val accountManagerResult = getActiveBrokerFromAccountManager()
460-
Logger.info(
461-
methodTag, "Tried getting active broker from account manager, " +
462-
"get ${accountManagerResult?.packageName}."
463-
)
464-
465-
// Only cache in memory, but don't persist accountManager result to cache storage.
466-
cachedBroker = BrokerInMemoryCache(accountManagerResult)
467-
return accountManagerResult
487+
return getFromAccountManagerAndCacheResult(telemetryCallback)
468488
}
469489
}

‎common/src/main/java/com/microsoft/identity/common/internal/cache/ActiveBrokerCacheUpdater.kt‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,13 @@ package com.microsoft.identity.common.internal.cache
2424

2525
import android.content.Context
2626
import android.os.Bundle
27+
import com.microsoft.identity.common.internal.activebrokerdiscovery.BrokerDiscoveryClient
2728
import com.microsoft.identity.common.internal.broker.BrokerData
2829
import com.microsoft.identity.common.internal.broker.BrokerValidator
2930
import com.microsoft.identity.common.logging.Logger
31+
import kotlinx.coroutines.CoroutineScope
32+
import kotlinx.coroutines.Dispatchers
33+
import kotlinx.coroutines.async
3034
import java.util.concurrent.TimeUnit
3135

3236
/**
@@ -109,12 +113,26 @@ class ActiveBrokerCacheUpdater(
109113
*
110114
* @param bundle The result bundle. could be null (for backward compatibility).
111115
*/
112-
fun updateCachedActiveBrokerFromResultBundle(bundle: Bundle?) {
113-
val methodTag = "$TAG:updateCachedActiveBrokerFromResultBundle"
116+
fun updateCachedActiveBrokerFromResultBundleAsync(bundle: Bundle?) {
114117
if (bundle == null) {
115118
return
116119
}
117120

121+
CoroutineScope(Dispatchers.IO).async {
122+
updateCachedActiveBrokerFromResultBundle(bundle)
123+
}
124+
}
125+
126+
127+
/**
128+
* If the active broker is returned with the result bundle, update [IActiveBrokerCache] with it.
129+
* If the active broker indicates that the broker discovery is disabled, wipe [IActiveBrokerCache].
130+
*
131+
* @param bundle The result bundle. could be null (for backward compatibility).
132+
*/
133+
fun updateCachedActiveBrokerFromResultBundle(bundle: Bundle) {
134+
val methodTag = "$TAG:updateCachedActiveBrokerFromResultBundle"
135+
118136
val shouldWipeCachedActiveBroker = bundle.getBoolean(BROKER_DISCOVERY_DISABLED_KEY, false)
119137
if (shouldWipeCachedActiveBroker) {
120138
Logger.info(methodTag, "Got a response indicating that the broker discovery is disabled." +

‎common/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerOperationExecutor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ private <T> T performStrategy(@NonNull final IIpcStrategy strategy,
235235
final BrokerOperationBundle brokerOperationBundle = operation.getBundle();
236236
final Bundle resultBundle = strategy.communicateToBroker(brokerOperationBundle);
237237

238-
mCacheUpdaterManager.updateCachedActiveBrokerFromResultBundle(resultBundle);
238+
mCacheUpdaterManager.updateCachedActiveBrokerFromResultBundleAsync(resultBundle);
239239

240240
span.setStatus(StatusCode.OK);
241241
return operation.extractResultBundle(resultBundle);

‎common/src/test/java/com/microsoft/identity/common/internal/activebrokerdiscovery/BrokerDiscoveryClientTests.kt‎

Lines changed: 128 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ package com.microsoft.identity.common.internal.activebrokerdiscovery
2424

2525
import android.os.Bundle
2626
import com.microsoft.identity.common.exception.BrokerCommunicationException
27+
import com.microsoft.identity.common.internal.broker.BrokerData
2728
import com.microsoft.identity.common.internal.broker.BrokerData.Companion.prodCompanyPortal
2829
import com.microsoft.identity.common.internal.broker.BrokerData.Companion.prodLTW
2930
import com.microsoft.identity.common.internal.broker.BrokerData.Companion.prodMicrosoftAuthenticator
@@ -633,7 +634,6 @@ class BrokerDiscoveryClientTests {
633634
)
634635
}
635636

636-
637637
/**
638638
* Test if authapp does not support broker discovery, but cp does.
639639
* */
@@ -688,4 +688,131 @@ class BrokerDiscoveryClientTests {
688688
Assert.assertFalse(cache.shouldUseAccountManager())
689689
}
690690

691+
/**
692+
* Making 2 calls, the first one when the broker is not installed.
693+
* Then make a request after the broker is installed.
694+
* */
695+
@Test
696+
fun testBrokerAppRecentlyInstalled(){
697+
val cache = InMemoryActiveBrokerCache()
698+
var installedBrokerApp: BrokerData? = null
699+
700+
val client = BrokerDiscoveryClient(
701+
brokerCandidates = setOf(
702+
prodMicrosoftAuthenticator, prodCompanyPortal, prodLTW
703+
),
704+
getActiveBrokerFromAccountManager = {
705+
return@BrokerDiscoveryClient null
706+
},
707+
ipcStrategy = object : IIpcStrategy {
708+
override fun communicateToBroker(bundle: BrokerOperationBundle): Bundle {
709+
installedBrokerApp?.let {
710+
if (bundle.targetBrokerAppPackageName == it.packageName) {
711+
val returnBundle = Bundle()
712+
returnBundle.putString(
713+
BrokerDiscoveryClient.ACTIVE_BROKER_PACKAGE_NAME_BUNDLE_KEY,
714+
it.packageName
715+
)
716+
returnBundle.putString(
717+
BrokerDiscoveryClient.ACTIVE_BROKER_SIGNING_CERTIFICATE_THUMBPRINT_BUNDLE_KEY,
718+
it.signingCertificateThumbprint
719+
)
720+
return returnBundle
721+
}
722+
}
723+
724+
throw IllegalStateException()
725+
}
726+
override fun isSupportedByTargetedBroker(targetedBrokerPackageName: String): Boolean {
727+
return true
728+
}
729+
override fun getType(): IIpcStrategy.Type {
730+
return IIpcStrategy.Type.CONTENT_PROVIDER
731+
}
732+
},
733+
cache = cache,
734+
isPackageInstalled = {
735+
return@BrokerDiscoveryClient installedBrokerApp != null
736+
},
737+
isValidBroker = { true }
738+
)
739+
740+
// First call, no broker installed.
741+
Assert.assertNull(client.getActiveBroker())
742+
743+
// Then install Authenticator!
744+
installedBrokerApp = prodMicrosoftAuthenticator
745+
746+
// Second call, Authenticator is installed.
747+
Assert.assertEquals(prodMicrosoftAuthenticator, client.getActiveBroker())
748+
}
749+
750+
751+
/**
752+
* Once getActiveBroker() is invoked, the in-memory value should be used for the object's lifetime.
753+
* No IPC or storage read should occur after that.
754+
**/
755+
@Test
756+
fun testCacheVariableUsed(){
757+
val cache = object: InMemoryActiveBrokerCache() {
758+
// Throw an exception if we try to read the storage-cached value.
759+
override fun getCachedActiveBroker(): BrokerData? {
760+
if (activeBroker != null) {
761+
throw IllegalStateException()
762+
}
763+
return null
764+
}
765+
}
766+
767+
var shouldThrowErrorFromIpc = false
768+
769+
val client = BrokerDiscoveryClient(
770+
brokerCandidates = setOf(
771+
prodLTW
772+
),
773+
getActiveBrokerFromAccountManager = {
774+
throw IllegalStateException()
775+
},
776+
ipcStrategy = object : IIpcStrategy {
777+
override fun communicateToBroker(bundle: BrokerOperationBundle): Bundle {
778+
if (shouldThrowErrorFromIpc) {
779+
throw IllegalStateException()
780+
}
781+
782+
if (bundle.targetBrokerAppPackageName == prodLTW.packageName) {
783+
val returnBundle = Bundle()
784+
returnBundle.putString(
785+
BrokerDiscoveryClient.ACTIVE_BROKER_PACKAGE_NAME_BUNDLE_KEY,
786+
prodLTW.packageName
787+
)
788+
returnBundle.putString(
789+
BrokerDiscoveryClient.ACTIVE_BROKER_SIGNING_CERTIFICATE_THUMBPRINT_BUNDLE_KEY,
790+
prodLTW.signingCertificateThumbprint
791+
)
792+
return returnBundle
793+
}
794+
795+
throw IllegalStateException()
796+
}
797+
override fun isSupportedByTargetedBroker(targetedBrokerPackageName: String): Boolean {
798+
return true
799+
}
800+
override fun getType(): IIpcStrategy.Type {
801+
return IIpcStrategy.Type.CONTENT_PROVIDER
802+
}
803+
},
804+
cache = cache,
805+
isPackageInstalled = {
806+
return@BrokerDiscoveryClient true
807+
},
808+
isValidBroker = { true }
809+
)
810+
811+
// First call, get LTW.
812+
Assert.assertEquals(prodLTW, client.getActiveBroker())
813+
814+
// Second call, should get LTW from the in-memory variable, not from storage.
815+
shouldThrowErrorFromIpc = true
816+
Assert.assertEquals(prodLTW, client.getActiveBroker())
817+
}
691818
}

‎common/src/test/java/com/microsoft/identity/common/internal/activebrokerdiscovery/InMemoryActiveBrokerCache.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,10 @@ import com.microsoft.identity.common.internal.cache.IClientActiveBrokerCache
3030
/**
3131
* An [IActiveBrokerCache] which stores values in-memory.
3232
**/
33-
class InMemoryActiveBrokerCache: IClientActiveBrokerCache {
33+
open class InMemoryActiveBrokerCache: IClientActiveBrokerCache {
3434

35-
private var activeBroker: BrokerData? = null
36-
private var shouldUseAccountManagerUntil: Long? = null
35+
protected var activeBroker: BrokerData? = null
36+
protected var shouldUseAccountManagerUntil: Long? = null
3737

3838
@Synchronized
3939
override fun getCachedActiveBroker(): BrokerData? {

‎common/src/test/java/com/microsoft/identity/common/internal/cache/ActiveBrokerCacheUpdaterTest.kt‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,18 +57,6 @@ class ActiveBrokerCacheUpdaterTest {
5757
bundle.getString(ACTIVE_BROKER_SIGNING_CERTIFICATE_THUMBPRINT_KEY))
5858
}
5959

60-
@Test
61-
fun testTryUpdateWithNullBundle(){
62-
val cache = InMemoryActiveBrokerCache()
63-
val cacheUpdater = ActiveBrokerCacheUpdater (
64-
// Bypass the Validation check.
65-
{ _: BrokerData -> true },
66-
cache)
67-
68-
cacheUpdater.updateCachedActiveBrokerFromResultBundle(null)
69-
Assert.assertNull(cache.getCachedActiveBroker())
70-
}
71-
7260
@Test
7361
fun testTryUpdateWithIncompleteBundle(){
7462
val cache = InMemoryActiveBrokerCache()

0 commit comments

Comments
 (0)