diff --git a/app/build.gradle b/app/build.gradle index f3d047808..8b8f7e9e7 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -150,6 +150,8 @@ dependencies { testImplementation "io.mockk:mockk:$mockk_version" testImplementation "io.mockk:mockk-android:$mockk_version" testImplementation "androidx.arch.core:core-testing:$android_arch_version" + // The Android stub of org.json returns defaults; the token authenticator parses error bodies with it. + testImplementation "org.json:json:20240303" } private def setupBranchConfigFields(buildType) { diff --git a/app/src/main/java/org/openedx/app/data/networking/HeadersInterceptor.kt b/app/src/main/java/org/openedx/app/data/networking/HeadersInterceptor.kt index baafe5a86..6feeb9e25 100644 --- a/app/src/main/java/org/openedx/app/data/networking/HeadersInterceptor.kt +++ b/app/src/main/java/org/openedx/app/data/networking/HeadersInterceptor.kt @@ -1,15 +1,18 @@ package org.openedx.app.data.networking +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.Interceptor import okhttp3.Response import org.openedx.core.config.Config import org.openedx.core.data.storage.CorePreferences +import org.openedx.core.instance.InstanceProvider import org.openedx.core.presentation.global.AppData class HeadersInterceptor( private val appData: AppData, private val config: Config, private val preferencesManager: CorePreferences, + private val instanceProvider: InstanceProvider, ) : Interceptor { override fun intercept(chain: Interceptor.Chain): Response = chain.run { @@ -18,7 +21,8 @@ class HeadersInterceptor( .newBuilder().apply { val token = preferencesManager.accessToken - if (token.isNotEmpty()) { + // The token belongs to the selected site; never send it to another host. + if (token.isNotEmpty() && targetsActiveSite(request().url.toString())) { addHeader("Authorization", "${config.getAccessTokenType()} $token") } @@ -29,4 +33,16 @@ class HeadersInterceptor( }.build() ) } + + private fun targetsActiveSite(url: String): Boolean { + val siteUrl = instanceProvider.currentInstance?.apiHostUrl ?: config.getApiHostURL() + val site = siteUrl.toHttpUrlOrNull() + val request = url.toHttpUrlOrNull() + return when { + // An unparseable configured host can't be compared; keep the previous behavior. + site == null -> true + request == null -> false + else -> request.isSameOrigin(site) + } + } } diff --git a/app/src/main/java/org/openedx/app/data/networking/InstanceHostInterceptor.kt b/app/src/main/java/org/openedx/app/data/networking/InstanceHostInterceptor.kt new file mode 100644 index 000000000..8d680ba14 --- /dev/null +++ b/app/src/main/java/org/openedx/app/data/networking/InstanceHostInterceptor.kt @@ -0,0 +1,57 @@ +package org.openedx.app.data.networking + +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import okhttp3.Interceptor +import okhttp3.Request +import okhttp3.Response +import org.openedx.core.instance.InstanceProvider + +/** + * Which instance a request was sent for, attached as an OkHttp tag when the request starts. + * Token refresh reads it, so a response that arrives after a switch is still handled for the + * site that sent it. + */ +data class RequestInstance(val instanceKey: String?) + +/** + * Re-targets requests built against the app's base URL at the selected instance: scheme, host + * and port change, path and query stay. Requests to any other host (CDNs, third-party SDKs) + * and every request made while no instance is selected pass through untouched. + */ +class InstanceHostInterceptor( + appBaseUrl: String, + private val instanceProvider: InstanceProvider, +) : Interceptor { + + private val appBase: HttpUrl? = appBaseUrl.toHttpUrlOrNull() + + override fun intercept(chain: Interceptor.Chain): Response { + val instance = instanceProvider.currentInstance + val request = chain.request() + val rewritten = rewriteHostIfNeeded(request, instance?.apiHostUrl) + .newBuilder() + .tag(RequestInstance::class.java, RequestInstance(instance?.key)) + .build() + return chain.proceed(rewritten) + } + + internal fun rewriteHostIfNeeded(request: Request, instanceHostUrl: String?): Request { + val target = instanceHostUrl?.toHttpUrlOrNull() + val base = appBase + val url = request.url + if (target == null || base == null || !url.isSameOrigin(base) || url.isSameOrigin(target)) { + return request + } + val newUrl = url.newBuilder() + .scheme(target.scheme) + .host(target.host) + .port(target.port) + .build() + return request.newBuilder().url(newUrl).build() + } +} + +internal fun HttpUrl.isSameOrigin(other: HttpUrl): Boolean { + return scheme == other.scheme && host == other.host && port == other.port +} diff --git a/app/src/main/java/org/openedx/app/data/networking/OauthRefreshTokenAuthenticator.kt b/app/src/main/java/org/openedx/app/data/networking/OauthRefreshTokenAuthenticator.kt index a60a3a988..4bec459ef 100644 --- a/app/src/main/java/org/openedx/app/data/networking/OauthRefreshTokenAuthenticator.kt +++ b/app/src/main/java/org/openedx/app/data/networking/OauthRefreshTokenAuthenticator.kt @@ -23,25 +23,29 @@ import org.openedx.core.ApiConstants.TOKEN_TYPE_JWT import org.openedx.core.BuildConfig import org.openedx.core.config.Config import org.openedx.core.data.storage.CorePreferences +import org.openedx.core.instance.InstanceProvider import org.openedx.core.system.notifier.app.AppNotifier import org.openedx.core.system.notifier.app.LogoutEvent import org.openedx.core.utils.TimeUtils import retrofit2.Retrofit import retrofit2.converter.gson.GsonConverterFactory import java.io.IOException +import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit class OauthRefreshTokenAuthenticator( private val config: Config, private val preferencesManager: CorePreferences, private val appNotifier: AppNotifier, + private val instanceProvider: InstanceProvider, + private val okHttpClient: OkHttpClient = defaultRefreshClient(), ) : Authenticator, Interceptor { - private val authApi: AuthApi + private val authApis = ConcurrentHashMap() private var lastTokenRefreshRequestTime = 0L override fun intercept(chain: Interceptor.Chain): Response { - if (isTokenExpired()) { + if (isTokenExpired() && isForActiveInstance(chain.request())) { val response = createUnauthorizedResponse(chain) val request = authenticate(chain.connection()?.route(), response) @@ -50,25 +54,33 @@ class OauthRefreshTokenAuthenticator( return chain.proceed(chain.request()) } - init { - val okHttpClient = OkHttpClient.Builder().apply { - writeTimeout(timeout = 60, TimeUnit.SECONDS) - readTimeout(timeout = 60, TimeUnit.SECONDS) - if (BuildConfig.DEBUG) { - addNetworkInterceptor(HttpLoggingInterceptor().setLevel(HttpLoggingInterceptor.Level.BODY)) - } - }.build() - authApi = Retrofit.Builder() - .baseUrl(config.getApiHostURL()) - .client(okHttpClient) - .addConverterFactory(GsonConverterFactory.create(Gson())) - .build() - .create(AuthApi::class.java) + /** Token endpoint of the selected instance, or of the app-level host when none is selected. */ + private fun authApi(): AuthApi { + val baseUrl = instanceProvider.currentInstance?.apiHostUrl ?: config.getApiHostURL() + return authApis.getOrPut(baseUrl) { + Retrofit.Builder() + .baseUrl(baseUrl) + .client(okHttpClient) + .addConverterFactory(GsonConverterFactory.create(Gson())) + .build() + .create(AuthApi::class.java) + } + } + + /** + * A 401 for a request sent before the learner switched sites says nothing about the site + * that is selected now: refreshing it, or logging out, would act on the wrong session. + */ + private fun isForActiveInstance(request: Request): Boolean { + val sentFor = request.tag(RequestInstance::class.java) ?: return true + return sentFor.instanceKey == instanceProvider.currentInstance?.key } @Suppress("ReturnCount") @Synchronized override fun authenticate(route: Route?, response: Response): Request? { + if (!isForActiveInstance(response.request)) return null + val accessToken = preferencesManager.accessToken val refreshToken = preferencesManager.refreshToken @@ -76,7 +88,7 @@ class OauthRefreshTokenAuthenticator( val errorCode = getErrorCode(response.peekBody(Long.MAX_VALUE).string()) ?: return null - return when (errorCode) { + val retry = when (errorCode) { TOKEN_EXPIRED_ERROR_MESSAGE, JWT_TOKEN_EXPIRED -> { handleTokenExpired(response, refreshToken, accessToken) } @@ -91,6 +103,9 @@ class OauthRefreshTokenAuthenticator( else -> null } + // Re-checked after the refresh: a switch during it must not retry this request with the + // new site's token. + return retry?.takeIf { isForActiveInstance(response.request) } } private fun handleDisabledUser(): Request? { @@ -159,13 +174,17 @@ class OauthRefreshTokenAuthenticator( private fun refreshAccessToken(refreshToken: String): AuthResponse? { var authResponse: AuthResponse? = null if (canRequestTokenRefresh()) { - val response = authApi.refreshAccessToken( + val instance = instanceProvider.currentInstance + val response = authApi().refreshAccessToken( ApiConstants.TOKEN_TYPE_REFRESH, - config.getOAuthClientId(), + instance?.oAuthClientId ?: config.getOAuthClientId(), refreshToken, config.getAccessTokenType() ).execute() authResponse = response.body()?.mapToDomain() + // The learner may have switched sites while the refresh was in flight; these tokens + // belong to the site that issued them, not to the one selected now. + if (instance?.key != instanceProvider.currentInstance?.key) return null if (response.isSuccessful && authResponse != null) { val newAccessToken = authResponse.accessToken ?: "" val newRefreshToken = authResponse.refreshToken ?: "" @@ -244,6 +263,14 @@ class OauthRefreshTokenAuthenticator( companion object { private const val HEADER_AUTHORIZATION = "Authorization" + private fun defaultRefreshClient(): OkHttpClient = OkHttpClient.Builder().apply { + writeTimeout(timeout = 60, TimeUnit.SECONDS) + readTimeout(timeout = 60, TimeUnit.SECONDS) + if (BuildConfig.DEBUG) { + addNetworkInterceptor(HttpLoggingInterceptor().setLevel(HttpLoggingInterceptor.Level.BODY)) + } + }.build() + private const val TOKEN_EXPIRED_ERROR_MESSAGE = "token_expired" private const val TOKEN_NONEXISTENT_ERROR_MESSAGE = "token_nonexistent" private const val TOKEN_INVALID_GRANT_ERROR_MESSAGE = "invalid_grant" diff --git a/app/src/main/java/org/openedx/app/di/NetworkingModule.kt b/app/src/main/java/org/openedx/app/di/NetworkingModule.kt index 6360e7fba..412f99a64 100644 --- a/app/src/main/java/org/openedx/app/di/NetworkingModule.kt +++ b/app/src/main/java/org/openedx/app/di/NetworkingModule.kt @@ -7,6 +7,7 @@ import org.openedx.app.data.api.NotificationsApi import org.openedx.app.data.networking.AppUpgradeInterceptor import org.openedx.app.data.networking.HandleErrorInterceptor import org.openedx.app.data.networking.HeadersInterceptor +import org.openedx.app.data.networking.InstanceHostInterceptor import org.openedx.app.data.networking.OauthRefreshTokenAuthenticator import org.openedx.auth.data.api.AuthApi import org.openedx.core.BuildConfig @@ -22,13 +23,15 @@ import java.util.concurrent.TimeUnit val networkingModule = module { - single { OauthRefreshTokenAuthenticator(get(), get(), get()) } + single { OauthRefreshTokenAuthenticator(get(), get(), get(), get()) } + single { InstanceHostInterceptor(get().getApiHostURL(), get()) } single { OkHttpClient.Builder().apply { writeTimeout(60, TimeUnit.SECONDS) readTimeout(60, TimeUnit.SECONDS) - addInterceptor(HeadersInterceptor(get(), get(), get())) + addInterceptor(get()) + addInterceptor(HeadersInterceptor(get(), get(), get(), get())) if (BuildConfig.DEBUG) { addNetworkInterceptor(HttpLoggingInterceptor().setLevel(HttpLoggingInterceptor.Level.BODY)) } diff --git a/app/src/test/java/org/openedx/app/data/networking/InstanceAwareNetworkingTest.kt b/app/src/test/java/org/openedx/app/data/networking/InstanceAwareNetworkingTest.kt new file mode 100644 index 000000000..67880402e --- /dev/null +++ b/app/src/test/java/org/openedx/app/data/networking/InstanceAwareNetworkingTest.kt @@ -0,0 +1,270 @@ +package org.openedx.app.data.networking + +import com.google.gson.JsonObject +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import okhttp3.Interceptor +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody +import okio.Buffer +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.openedx.core.config.Config +import org.openedx.core.data.storage.CorePreferences +import org.openedx.core.instance.Instance +import org.openedx.core.instance.InstanceProvider +import org.openedx.core.presentation.global.AppData +import org.openedx.core.system.notifier.app.AppNotifier +import org.openedx.core.system.notifier.app.LogoutEvent + +class InstanceAwareNetworkingTest { + + private class FakeInstanceProvider(instance: Instance?) : InstanceProvider { + private val flow = MutableStateFlow(instance) + override val currentInstance: Instance? get() = flow.value + override val currentInstanceFlow: StateFlow = flow + override val hasMultipleInstances: Boolean = true + fun select(instance: Instance?) { + flow.value = instance + } + } + + private fun instance(key: String, host: String) = requireNotNull( + Instance.fromJson( + JsonObject().apply { + addProperty("KEY", key) + addProperty("NAME", key) + addProperty("OAUTH_CLIENT_ID", "$key-client") + addProperty("API_HOST_URL", host) + } + ) + ) + + private val appBase = "https://app.example.com" + private val alpha = instance("alpha", "https://alpha.example.com") + private val beta = instance("beta", "http://beta.example.com:8080") + + /** Runs [interceptor] on [request] and returns the request it forwarded. */ + private fun forward(interceptor: Interceptor, request: Request): Request { + var forwarded: Request? = null + val client = OkHttpClient.Builder() + .addInterceptor(interceptor) + .addInterceptor { chain -> + forwarded = chain.request() + ok(chain.request(), "{}") + } + .build() + client.newCall(request).execute().close() + return requireNotNull(forwarded) + } + + private fun ok(request: Request, body: String, code: Int = 200) = Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("status $code") + .body(body.toResponseBody("application/json".toMediaType())) + .build() + + // region InstanceHostInterceptor + + @Test + fun `app base requests are re-targeted at the selected instance, path and query preserved`() { + val interceptor = InstanceHostInterceptor(appBase, FakeInstanceProvider(beta)) + + val forwarded = forward( + interceptor, + Request.Builder().url("https://app.example.com/api/courses/v1/?page=2&size=10").build() + ) + + assertEquals("http://beta.example.com:8080/api/courses/v1/?page=2&size=10", forwarded.url.toString()) + assertEquals(RequestInstance("beta"), forwarded.tag(RequestInstance::class.java)) + } + + @Test + fun `no selected instance passes requests through`() { + val interceptor = InstanceHostInterceptor(appBase, FakeInstanceProvider(null)) + + val forwarded = forward(interceptor, Request.Builder().url("https://app.example.com/api/x").build()) + + assertEquals("https://app.example.com/api/x", forwarded.url.toString()) + assertEquals(RequestInstance(null), forwarded.tag(RequestInstance::class.java)) + } + + @Test + fun `third party hosts are left untouched`() { + val interceptor = InstanceHostInterceptor(appBase, FakeInstanceProvider(alpha)) + + listOf( + "https://cdn.example.net/video.mp4", + "http://app.example.com/api/x", + "https://app.example.com:8443/api/x", + ).forEach { url -> + assertEquals(url, forward(interceptor, Request.Builder().url(url).build()).url.toString()) + } + } + + @Test + fun `requests already built for the instance host stay as they are`() { + val interceptor = InstanceHostInterceptor(appBase, FakeInstanceProvider(alpha)) + val request = Request.Builder().url("https://alpha.example.com/api/x").build() + + assertSame(request, interceptor.rewriteHostIfNeeded(request, alpha.apiHostUrl)) + } + + // endregion + + // region HeadersInterceptor + + private fun headersInterceptor(provider: InstanceProvider): HeadersInterceptor { + val config = mockk { + every { getAccessTokenType() } returns "JWT" + every { getApiHostURL() } returns appBase + } + val preferences = mockk { every { accessToken } returns "token-a" } + val appData = AppData(appName = "app", versionName = "1", applicationId = "org.openedx.app") + return HeadersInterceptor(appData, config, preferences, provider) + } + + @Test + fun `token is sent to the active site only`() { + val interceptor = headersInterceptor(FakeInstanceProvider(alpha)) + + val toSite = forward(interceptor, Request.Builder().url("https://alpha.example.com/api/x").build()) + val elsewhere = forward(interceptor, Request.Builder().url("https://beta.example.com/api/x").build()) + + assertEquals("JWT token-a", toSite.header("Authorization")) + assertNull(elsewhere.header("Authorization")) + } + + @Test + fun `without a selection the app level host gets the token`() { + val interceptor = headersInterceptor(FakeInstanceProvider(null)) + + val forwarded = forward(interceptor, Request.Builder().url("https://app.example.com/api/x").build()) + + assertEquals("JWT token-a", forwarded.header("Authorization")) + } + + // endregion + + // region OauthRefreshTokenAuthenticator + + private class Prefs : CorePreferences by mockk(relaxed = true) { + override var accessToken: String = "old-access" + override var refreshToken: String = "old-refresh" + override var accessTokenExpiresAt: Long = Long.MAX_VALUE + } + + private fun unauthorized(request: Request) = ok(request, """{"detail": "Token has expired."}""", code = 401) + + private fun taggedRequest(url: String, key: String?) = Request.Builder() + .url(url) + .header("Authorization", "JWT old-access") + .tag(RequestInstance::class.java, RequestInstance(key)) + .build() + + @Test + fun `refresh uses the selected instance host and OAuth client`() { + val tokenRequests = mutableListOf() + val refreshClient = OkHttpClient.Builder().addInterceptor { chain -> + tokenRequests += chain.request() + ok( + chain.request(), + """{"access_token": "new-access", "refresh_token": "new-refresh", "expires_in": 3600}""" + ) + }.build() + val prefs = Prefs() + val config = mockk(relaxed = true) { + every { getAccessTokenType() } returns "JWT" + every { getApiHostURL() } returns appBase + every { getOAuthClientId() } returns "app-client" + } + val authenticator = OauthRefreshTokenAuthenticator( + config, + prefs, + mockk(relaxed = true), + FakeInstanceProvider(beta), + refreshClient + ) + + val retry = authenticator.authenticate( + null, + unauthorized(taggedRequest("http://beta.example.com:8080/api/x", "beta")) + ) + + val tokenRequest = tokenRequests.single() + assertEquals("http://beta.example.com:8080/oauth2/access_token/", tokenRequest.url.toString()) + val form = Buffer().also { tokenRequest.body!!.writeTo(it) }.readUtf8() + assertTrue(form, form.contains("client_id=beta-client")) + assertEquals("JWT new-access", retry?.header("Authorization")) + assertEquals("new-access", prefs.accessToken) + } + + @Test + fun `a 401 for a request sent before a switch neither refreshes nor logs out`() { + val refreshClient = OkHttpClient.Builder() + .addInterceptor { throw AssertionError("no refresh expected") } + .build() + val notifier = mockk(relaxed = true) + val prefs = Prefs() + val authenticator = OauthRefreshTokenAuthenticator( + mockk(relaxed = true), + prefs, + notifier, + FakeInstanceProvider(beta), + refreshClient + ) + + val retry = authenticator.authenticate( + null, + unauthorized(taggedRequest("https://alpha.example.com/api/x", "alpha")) + ) + + assertNull(retry) + assertEquals("old-access", prefs.accessToken) + coVerify(exactly = 0) { notifier.send(any()) } + } + + @Test + fun `tokens refreshed while the learner switched sites are discarded`() { + val provider = FakeInstanceProvider(alpha) + val refreshClient = OkHttpClient.Builder().addInterceptor { chain -> + provider.select(beta) + ok( + chain.request(), + """{"access_token": "alpha-new", "refresh_token": "alpha-refresh", "expires_in": 3600}""" + ) + }.build() + val prefs = Prefs() + val authenticator = OauthRefreshTokenAuthenticator( + mockk(relaxed = true) { every { getAccessTokenType() } returns "JWT" }, + prefs, + mockk(relaxed = true), + provider, + refreshClient + ) + + val retry = authenticator.authenticate( + null, + unauthorized(taggedRequest("https://alpha.example.com/api/x", "alpha")) + ) + + assertNull(retry) + assertEquals("old-access", prefs.accessToken) + assertNotNull(provider.currentInstance) + } + + // endregion +}