Merge branch 'housekeeping/refactoring-oauth-2' into 'develop'

Improving OAuth implementation

See merge request funkwhale/funkwhale-android!78

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) {
+    return (if (state != null) {
-      state.isAuthorized || refreshAccessToken(context)
+      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(
+        refreshAccessToken(state, context)
-          "OAuth",
-          "needsTokenRefresh()=${state.needsTokenRefresh}, refreshToken=${
-            state.refreshToken!!.subSequence(0, 5)
-          }..."
-        )
-        refreshAccessToken(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.refreshToken != null) {
-    return if (state != null && state.refreshToken != null) {
       val refreshRequest = state.createTokenRefreshRequest()
       val auth = ClientSecretPost(state.clientSecret)
       runBlocking {

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