From 4f9f605bbf7f97b17e36aacb75842113a29ee0e9 Mon Sep 17 00:00:00 2001
From: Ryan Harg <ryan.harg@mailbox.org>
Date: Thu, 26 Aug 2021 08:33:12 +0200
Subject: [PATCH] Improving OAuth implementation

---
 .../java/audio/funkwhale/ffa/utils/OAuth.kt   | 23 +++++++++---------
 .../audio/funkwhale/ffa/utils/OAuthTest.kt    | 24 ++++++++++++++++++-
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/app/src/main/java/audio/funkwhale/ffa/utils/OAuth.kt b/app/src/main/java/audio/funkwhale/ffa/utils/OAuth.kt
index b8678c4..c0c7c79 100644
--- a/app/src/main/java/audio/funkwhale/ffa/utils/OAuth.kt
+++ b/app/src/main/java/audio/funkwhale/ffa/utils/OAuth.kt
@@ -56,25 +56,21 @@ class OAuth(private val authorizationServiceFactory: AuthorizationServiceFactory
 
   fun isAuthorized(context: Context): Boolean {
     val state = tryState()
-    return if (state != null) {
-      state.isAuthorized || refreshAccessToken(context)
+    return (if (state != null) {
+      state.validAuthorization() || refreshAccessToken(state, context)
     } else {
       false
-    }.also {
+    }).also {
       it.logInfo("isAuthorized()")
     }
   }
 
+  private fun AuthState.validAuthorization() = this.isAuthorized && !this.needsTokenRefresh
+
   fun tryRefreshAccessToken(context: Context): Boolean {
     tryState()?.let { state ->
       return if (state.needsTokenRefresh && state.refreshToken != null) {
-        Log.i(
-          "OAuth",
-          "needsTokenRefresh()=${state.needsTokenRefresh}, refreshToken=${
-            state.refreshToken!!.subSequence(0, 5)
-          }..."
-        )
-        refreshAccessToken(context)
+        refreshAccessToken(state, context)
       } else {
         state.isAuthorized
       }.also { it.logInfo("tryRefreshAccessToken()") }
@@ -83,9 +79,12 @@ class OAuth(private val authorizationServiceFactory: AuthorizationServiceFactory
   }
 
   fun refreshAccessToken(context: Context): Boolean {
+    return tryState()?.let { refreshAccessToken(it, context) } ?: false
+  }
+
+  private fun refreshAccessToken(state: AuthState, context: Context): Boolean {
     Log.i("OAuth", "refreshAccessToken()")
-    val state = tryState()
-    return if (state != null && state.refreshToken != null) {
+    return if (state.refreshToken != null) {
       val refreshRequest = state.createTokenRefreshRequest()
       val auth = ClientSecretPost(state.clientSecret)
       runBlocking {
diff --git a/app/src/test/java/audio/funkwhale/ffa/utils/OAuthTest.kt b/app/src/test/java/audio/funkwhale/ffa/utils/OAuthTest.kt
index 5c91053..b5fb3ef 100644
--- a/app/src/test/java/audio/funkwhale/ffa/utils/OAuthTest.kt
+++ b/app/src/test/java/audio/funkwhale/ffa/utils/OAuthTest.kt
@@ -131,13 +131,35 @@ class OAuthTest {
   }
 
   @Test
-  fun `isAuthorized() should return true if existing state is authorized`() {
+  fun `isAuthorized() should return true if existing state is authorized and token needs no refresh`() {
     mockkStatic(PowerPreference::class)
     mockkStatic(AuthState::class)
 
     val authState = mockk<AuthState>()
     every { AuthState.jsonDeserialize(any<String>()) } returns authState
     every { authState.isAuthorized } returns true
+    every { authState.needsTokenRefresh } returns false
+
+    val mockPref = mockk<Preference>()
+    every { PowerPreference.getFileByName(any()) } returns mockPref
+    every { mockPref.getString(any()) } returns "{}"
+
+    expectThat(oAuth.isAuthorized(context)).isTrue()
+  }
+
+  @Test
+  fun `isAuthorized() should return true if existing state is authorized and token needs refresh`() {
+    mockkStatic(PowerPreference::class)
+    mockkStatic(AuthState::class)
+
+    val authState = mockk<AuthState>()
+    every { AuthState.jsonDeserialize(any<String>()) } returns authState
+    every { authState.isAuthorized } returns true
+    every { authState.needsTokenRefresh } returns true
+    every { authState.refreshToken } returns "refreshToken"
+    every { authState.createTokenRefreshRequest() } returns mockk()
+    every { authState.clientSecret } returns "clientSecret"
+    every { authServiceFactory.create(any()) } returns authService
 
     val mockPref = mockk<Preference>()
     every { PowerPreference.getFileByName(any()) } returns mockPref