Merge pull request #3221 from CruGlobal/expiredFacebookAccessTokens

We should still consider the user authenticated after the facebook token expires
CruGlobal · Nov 10, 2023 · affcf0e · affcf0e
2 parents c5aa0bb + 7d8d1fd
commit affcf0e
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 24 deletions.
diff --git a/...unt/src/main/kotlin/org/cru/godtools/account/provider/facebook/FacebookAccountProvider.kt b/...unt/src/main/kotlin/org/cru/godtools/account/provider/facebook/FacebookAccountProvider.kt
@@ -15,13 +15,12 @@ import dagger.hilt.android.qualifiers.ApplicationContext
 import javax.inject.Inject
 import javax.inject.Singleton
 import kotlinx.coroutines.ExperimentalCoroutinesApi
-import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.flow.flatMapLatest
 import kotlinx.coroutines.flow.flowOf
+import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.launch
 import org.ccci.gto.android.common.androidx.activity.result.contract.transformInput
 import org.ccci.gto.android.common.facebook.login.currentAccessTokenFlow
-import org.ccci.gto.android.common.facebook.login.isExpiredFlow
 import org.ccci.gto.android.common.facebook.login.refreshCurrentAccessToken
 import org.ccci.gto.android.common.kotlin.coroutines.getStringFlow
 import org.cru.godtools.account.AccountType
@@ -54,23 +53,11 @@ internal class FacebookAccountProvider @Inject constructor(
     @VisibleForTesting
     internal val prefs by lazy { context.getSharedPreferences(PREFS_FACEBOOK_ACCOUNT_PROVIDER, Context.MODE_PRIVATE) }
 
-    override val userId get() = accessTokenManager.currentAccessToken?.apiUserId
-    override val isAuthenticated
-        get() = accessTokenManager.currentAccessToken?.let { !it.isExpired && it.apiUserId != null } == true
+    override val isAuthenticated get() = userId != null
+    override val userId get() = accessTokenManager.currentAccessToken?.let { prefs.getString(it.PREF_USER_ID, null) }
+    override fun isAuthenticatedFlow() = userIdFlow().map { it != null }
     override fun userIdFlow() = accessTokenManager.currentAccessTokenFlow()
-        .flatMapLatest { it?.apiUserIdFlow() ?: flowOf(null) }
-    override fun isAuthenticatedFlow() = accessTokenManager.currentAccessTokenFlow()
-        .flatMapLatest {
-            when {
-                it != null -> combine(it.isExpiredFlow(), it.apiUserIdFlow()) { isExpired, userId ->
-                    !isExpired && userId != null
-                }
-                else -> flowOf(false)
-            }
-        }
-
-    private val AccessToken.apiUserId get() = prefs.getString(PREF_USER_ID, null)
-    private fun AccessToken.apiUserIdFlow() = prefs.getStringFlow(PREF_USER_ID, null)
+        .flatMapLatest { it?.let { prefs.getStringFlow(it.PREF_USER_ID, null) } ?: flowOf(null) }
 
     // region Login/Logout
     @Composable

diff --git a/...src/test/kotlin/org/cru/godtools/account/provider/facebook/FacebookAccountProviderTest.kt b/...src/test/kotlin/org/cru/godtools/account/provider/facebook/FacebookAccountProviderTest.kt
@@ -75,7 +75,7 @@ class FacebookAccountProviderTest {
     fun `Property isAuthenticated`() {
         assertFalse(provider.isAuthenticated)
 
-        val token = accessToken(expirationTime = Date(System.currentTimeMillis() + 100_000))
+        val token = accessToken()
         currentAccessTokenFlow.value = token
         assertFalse(provider.isAuthenticated)
 
@@ -89,14 +89,16 @@ class FacebookAccountProviderTest {
 
     @Test
     fun `Property isAuthenticated - token expired`() {
-        val token = accessToken(expirationTime = Date(System.currentTimeMillis() - 100_000))
+        val token = accessToken(expirationTime = Date(0))
+        currentAccessTokenFlow.value = token
         val user = UUID.randomUUID().toString()
         provider.prefs.edit { putString(token.PREF_USER_ID, user) }
-        currentAccessTokenFlow.value = token
-        assertFalse(provider.isAuthenticated)
 
-        currentAccessTokenFlow.value = accessToken(expirationTime = Date(System.currentTimeMillis() + 100_000))
-        assertTrue(provider.isAuthenticated)
+        assertTrue(token.isExpired)
+        assertTrue(
+            provider.isAuthenticated,
+            "We should still considered expired tokens as authenticated because refreshing the token might work"
+        )
     }
     // endregion Property: isAuthenticated