review comments addressed

Author: pmathew92

auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt

@@ -325,11 +325,10 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
 
     /**
      * Retrieves API credentials from storage and automatically renews them using the refresh token if the access
-     * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid.
+     * token is expired. Otherwise, the retrieved API credentials will be returned as they are still valid.
      *
      * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials.
-     * New or renewed API credentials will be automatically stored in storage.
-     * This is a Coroutine that is exposed only for Kotlin.
+     * New or renewed API credentials will be automatically persisted in storage.
      *
      * @param audience Identifier of the API that your application is requesting access to.
      * @param scope    the scope to request for the access token. If null is passed, the previous scope will be kept.
@@ -556,10 +555,10 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
 
     /**
      *  Retrieves API credentials from storage and automatically renews them using the refresh token if the access
-     *  token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid.
+     *  token is expired. Otherwise, the retrieved API credentials will be returned via the success callback as they are still valid.
      *
      * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials.
-     * New or renewed API credentials will be automatically stored in storage.
+     * New or renewed API credentials will be automatically persisted in storage.
      *
      * @param audience Identifier of the API that your application is requesting access to.
      * @param scope the scope to request for the access token. If null is passed, the previous scope will be kept.
@@ -691,6 +690,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
      */
     override fun clearApiCredentials(audience: String) {
         storage.remove(audience)
+        Log.d(TAG, "API Credentials for $audience were just removed from the storage")
     }
 
     /**

auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt

@@ -439,11 +439,10 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
 
     /**
      * Retrieves API credentials from storage and automatically renews them using the refresh token if the access
-     * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid.
+     * token is expired. Otherwise, the retrieved API credentials will be returned as they are still valid.
      *
      * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials.
-     * New or renewed API credentials will be automatically stored in storage.
-     * This is a Coroutine that is exposed only for Kotlin.
+     * New or renewed API credentials will be automatically persisted in storage.
      *
      * @param audience Identifier of the API that your application is requesting access to.
      * @param scope    the scope to request for the access token. If null is passed, the previous scope will be kept.
@@ -627,10 +626,10 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
 
     /**
      * Retrieves API credentials from storage and automatically renews them using the refresh token if the access
-     * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid.
+     * token is expired. Otherwise, the retrieved API credentials will be returned via the success callback as they are still valid.
      *
      * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials.
-     * New or renewed API credentials will be automatically stored in storage.
+     * New or renewed API credentials will be automatically persisted in storage.
      *
      * @param audience Identifier of the API that your application is requesting access to.
      * @param scope    the scope to request for the access token. If null is passed, the previous scope will be kept.
@@ -873,7 +872,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
     internal fun continueGetApiCredentials(
         audience: String,
         scope: String?,
-        minTtl: Int = 0,
+        minTtl: Int,
         parameters: Map<String, String>,
         headers: Map<String, String>,
         callback: Callback<APICredentials, CredentialsManagerException>

auth0/src/main/java/com/auth0/android/result/APICredentials.kt

@@ -36,7 +36,7 @@ public data class APICredentials(
     @field:SerializedName("expires_at")
     val expiresAt: Date,
     /**
-     * Getter for the access token's granted scope. Only available if the requested scope differs from the granted one.
+     * Getter for the access token's granted scope.
      *
      * @return the granted scope.
      */
@@ -53,6 +53,6 @@ public data class APICredentials(
  * Converts a Credentials instance to an APICredentials instance.
  */
 internal fun Credentials.toAPICredentials(): APICredentials {
-    val newScope = scope ?: "openid"
+    val newScope = scope ?: ""
     return APICredentials(accessToken, type, expiresAt, newScope)
 }

auth0/src/test/java/com/auth0/android/authentication/AuthenticationAPIClientTest.kt

@@ -2561,7 +2561,7 @@ public class AuthenticationAPIClientTest {
         val auth0 = auth0
         val client = AuthenticationAPIClient(auth0)
         mockAPI.willReturnSuccessfulLogin()
-        val credentials = client.renewAuth(refreshToken = "refreshToken", scope = "read:data")
+        val credentials = client.renewAuth(refreshToken = "refreshToken", scope = "openid read:data")
             .execute()
         val request = mockAPI.takeRequest()
         assertThat(
@@ -2574,7 +2574,7 @@ public class AuthenticationAPIClientTest {
         assertThat(body, Matchers.hasEntry("client_id", CLIENT_ID))
         assertThat(body, Matchers.hasEntry("refresh_token", "refreshToken"))
         assertThat(body, Matchers.hasEntry("grant_type", "refresh_token"))
-        assertThat(body, Matchers.hasEntry("scope", "read:data openid"))
+        assertThat(body, Matchers.hasEntry("scope", "openid read:data"))
         assertThat(body, Matchers.not(Matchers.hasKey("audience")))
         assertThat(credentials, Matchers.`is`(Matchers.notNullValue()))
     }
@@ -2602,30 +2602,6 @@ public class AuthenticationAPIClientTest {
         assertThat(credentials, Matchers.`is`(Matchers.notNullValue()))
     }
 
-    @Test
-    public fun shouldRenewAuthWithOAuthAudienceAndScopeNotEnforcingOpendId() {
-        val auth0 = auth0
-        val client = AuthenticationAPIClient(auth0)
-        mockAPI.willReturnSuccessfulLogin()
-        val credentials =
-            client.renewAuth("refreshToken", "_audience", "openid read:data write:data")
-                .execute()
-        val request = mockAPI.takeRequest()
-        assertThat(
-            request.getHeader("Accept-Language"), Matchers.`is`(
-                defaultLocale
-            )
-        )
-        assertThat(request.path, Matchers.equalTo("/oauth/token"))
-        val body = bodyFromRequest<String>(request)
-        assertThat(body, Matchers.hasEntry("client_id", CLIENT_ID))
-        assertThat(body, Matchers.hasEntry("refresh_token", "refreshToken"))
-        assertThat(body, Matchers.hasEntry("grant_type", "refresh_token"))
-        assertThat(body, Matchers.hasEntry("audience", "_audience"))
-        assertThat(body, Matchers.hasEntry("scope", "openid read:data write:data"))
-        assertThat(credentials, Matchers.`is`(Matchers.notNullValue()))
-    }
-
     @Test
     public fun shouldFetchProfileSyncAfterLoginRequest() {
         mockAPI.willReturnSuccessfulLogin()

auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt

@@ -502,7 +502,7 @@ public class CredentialsManagerTest {
         Mockito.`when`(storage.retrieveString("audience")).thenReturn(gson.toJson(apiCredentials))
         Mockito.`when`(storage.retrieveString("com.auth0.refresh_token")).thenReturn("refreshToken")
         Mockito.`when`(
-            client.renewAuth("refreshToken", "audience", "newScope")
+            client.renewAuth("refreshToken", "audience", "scope")
         ).thenReturn(request)
         val newDate = Date(CredentialsMock.ONE_HOUR_AHEAD_MS + ONE_HOUR_SECONDS * 1000)
         val jwtMock = mock<Jwt>()
@@ -512,9 +512,9 @@ public class CredentialsManagerTest {
         // Trigger success
         val newRefresh: String? = null
         val renewedCredentials =
-            Credentials("newId", "newAccess", "newType", newRefresh, newDate, "newScope")
+            Credentials("newId", "newAccess", "newType", newRefresh, newDate, "scope")
         Mockito.`when`(request.execute()).thenReturn(renewedCredentials)
-        manager.getApiCredentials("audience", "newScope", callback = apiCredentialsCallback)
+        manager.getApiCredentials("audience", "scope", callback = apiCredentialsCallback)
         verify(apiCredentialsCallback).onSuccess(
             apiCredentialsCaptor.capture()
         )
@@ -530,7 +530,7 @@ public class CredentialsManagerTest {
         MatcherAssert.assertThat(newAPiCredentials.accessToken, Is.`is`("newAccess"))
         MatcherAssert.assertThat(newAPiCredentials.type, Is.`is`("newType"))
         MatcherAssert.assertThat(newAPiCredentials.expiresAt, Is.`is`(newDate))
-        MatcherAssert.assertThat(newAPiCredentials.scope, Is.`is`("newScope"))
+        MatcherAssert.assertThat(newAPiCredentials.scope, Is.`is`("scope"))
     }
 
     @Test
@@ -544,7 +544,7 @@ public class CredentialsManagerTest {
         Mockito.`when`(storage.retrieveString("audience")).thenReturn(gson.toJson(apiCredentials))
         Mockito.`when`(storage.retrieveString("com.auth0.refresh_token")).thenReturn("refreshToken")
         Mockito.`when`(
-            client.renewAuth("refreshToken", "audience", "newScope")
+            client.renewAuth("refreshToken", "audience", "scope")
         ).thenReturn(request)
         val newDate = Date(CredentialsMock.ONE_HOUR_AHEAD_MS + ONE_HOUR_SECONDS * 1000)
         val jwtMock = mock<Jwt>()
@@ -554,9 +554,9 @@ public class CredentialsManagerTest {
         // Trigger success
         val newRefresh: String? = null
         val renewedCredentials =
-            Credentials("newId", "newAccess", "newType", newRefresh, newDate, "newScope")
+            Credentials("newId", "newAccess", "newType", newRefresh, newDate, "scope")
         Mockito.`when`(request.execute()).thenReturn(renewedCredentials)
-        manager.getApiCredentials("audience", "newScope", minTtl = 10, callback = apiCredentialsCallback)
+        manager.getApiCredentials("audience", "scope", minTtl = 10, callback = apiCredentialsCallback)
         verify(apiCredentialsCallback).onSuccess(
             apiCredentialsCaptor.capture()
         )
@@ -572,7 +572,7 @@ public class CredentialsManagerTest {
         MatcherAssert.assertThat(newAPiCredentials.accessToken, Is.`is`("newAccess"))
         MatcherAssert.assertThat(newAPiCredentials.type, Is.`is`("newType"))
         MatcherAssert.assertThat(newAPiCredentials.expiresAt, Is.`is`(newDate))
-        MatcherAssert.assertThat(newAPiCredentials.scope, Is.`is`("newScope"))
+        MatcherAssert.assertThat(newAPiCredentials.scope, Is.`is`("scope"))
     }
 
     @Test
@@ -641,7 +641,7 @@ public class CredentialsManagerTest {
 
         // Verify the credentials are property stored
         verify(storage).store("com.auth0.id_token", renewedCredentials.idToken)
-        // RefreshToken should not be replaced
+        // RefreshToken should be replaced
         verify(storage).store("com.auth0.refresh_token", "newRefreshToken")
         verify(storage).store("audience", gson.toJson(renewedCredentials.toAPICredentials()))
         // Verify the returned credentials are the latest

auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt

@@ -24,6 +24,7 @@ import com.google.gson.Gson
 import com.nhaarman.mockitokotlin2.KArgumentCaptor
 import com.nhaarman.mockitokotlin2.any
 import com.nhaarman.mockitokotlin2.argumentCaptor
+import com.nhaarman.mockitokotlin2.doNothing
 import com.nhaarman.mockitokotlin2.eq
 import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.never
@@ -2176,18 +2177,20 @@ public class SecureCredentialsManagerTest {
         val renewedCredentials =
             Credentials("newId", "newAccess", "newType", newRefresh, newDate, "newScope")
         Mockito.`when`(request.execute()).thenReturn(renewedCredentials)
-        val expectedJson = gson.toJson(renewedCredentials)
+        val updatedCredentials = renewedCredentials.copy(refreshToken = "refreshToken")
+        val expectedJson = gson.toJson(updatedCredentials)
         Mockito.`when`(crypto.encrypt(any()))
             .thenReturn(expectedJson.toByteArray())
         manager.getApiCredentials("audience", "newScope", callback = apiCredentialsCallback)
         verify(apiCredentialsCallback).onSuccess(
             apiCredentialsCaptor.capture()
         )
 
-
         // Verify the credentials are property stored
         verify(storage).store(eq("com.auth0.credentials"), stringCaptor.capture())
         MatcherAssert.assertThat(stringCaptor.firstValue, Is.`is`(Matchers.notNullValue()))
+        val credentials = gson.fromJson(expectedJson,Credentials::class.java)
+        Assert.assertEquals("refreshToken",credentials.refreshToken)
         // Verify the returned credentials are the latest
         val newAPiCredentials = apiCredentialsCaptor.firstValue
         MatcherAssert.assertThat(newAPiCredentials, Is.`is`(Matchers.notNullValue()))
@@ -2233,7 +2236,6 @@ public class SecureCredentialsManagerTest {
             apiCredentialsCaptor.capture()
         )
 
-        // Verify the returned credentials are the latest
         val newAPiCredentials = apiCredentialsCaptor.firstValue
         MatcherAssert.assertThat(newAPiCredentials, Is.`is`(Matchers.notNullValue()))
         MatcherAssert.assertThat(newAPiCredentials.accessToken, Is.`is`("newAccess"))
@@ -2283,7 +2285,6 @@ public class SecureCredentialsManagerTest {
             apiCredentialsCaptor.capture()
         )
 
-        // Verify the returned credentials are the latest
         val newAPiCredentials = apiCredentialsCaptor.firstValue
         MatcherAssert.assertThat(newAPiCredentials, Is.`is`(Matchers.notNullValue()))
         MatcherAssert.assertThat(newAPiCredentials.accessToken, Is.`is`("newAccess"))
@@ -2328,7 +2329,6 @@ public class SecureCredentialsManagerTest {
             apiCredentialsCaptor.capture()
         )
 
-        // Verify the returned credentials are the latest
         val newAPiCredentials = apiCredentialsCaptor.firstValue
         MatcherAssert.assertThat(newAPiCredentials, Is.`is`(Matchers.notNullValue()))
         MatcherAssert.assertThat(newAPiCredentials.accessToken, Is.`is`("newAccess"))
@@ -2425,7 +2425,6 @@ public class SecureCredentialsManagerTest {
             exceptionCaptor.capture()
         )
 
-        // Verify the returned credentials are the latest
         val exception = exceptionCaptor.firstValue
         MatcherAssert.assertThat(exception, Is.`is`(Matchers.notNullValue()))
     }