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 4bec459ef..5d8d2c120 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,6 +23,7 @@ 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.Instance import org.openedx.core.instance.InstanceProvider import org.openedx.core.system.notifier.app.AppNotifier import org.openedx.core.system.notifier.app.LogoutEvent @@ -42,7 +43,9 @@ class OauthRefreshTokenAuthenticator( ) : Authenticator, Interceptor { private val authApis = ConcurrentHashMap() - private var lastTokenRefreshRequestTime = 0L + + /** Per instance key ("" for none): one site's refresh mustn't hold back another's. */ + private val lastTokenRefreshRequestTimes = ConcurrentHashMap() override fun intercept(chain: Interceptor.Chain): Response { if (isTokenExpired() && isForActiveInstance(chain.request())) { @@ -54,9 +57,9 @@ class OauthRefreshTokenAuthenticator( return chain.proceed(chain.request()) } - /** 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() + /** Token endpoint of [instance], or of the app-level host when there is none. */ + private fun authApi(instance: Instance?): AuthApi { + val baseUrl = instance?.apiHostUrl ?: config.getApiHostURL() return authApis.getOrPut(baseUrl) { Retrofit.Builder() .baseUrl(baseUrl) @@ -71,18 +74,23 @@ class OauthRefreshTokenAuthenticator( * 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 { + private fun isForActiveInstance(request: Request): Boolean = isFor(request, instanceProvider.currentInstance) + + private fun isFor(request: Request, instance: Instance?): Boolean { val sentFor = request.tag(RequestInstance::class.java) ?: return true - return sentFor.instanceKey == instanceProvider.currentInstance?.key + return sentFor.instanceKey == instance?.key } @Suppress("ReturnCount") @Synchronized override fun authenticate(route: Route?, response: Response): Request? { - if (!isForActiveInstance(response.request)) return null + // Resolved once: the tokens read, the endpoint called and the place the result is saved + // all belong to this site, even if the learner switches while the refresh is in flight. + val instance = instanceProvider.currentInstance + if (!isFor(response.request, instance)) return null - val accessToken = preferencesManager.accessToken - val refreshToken = preferencesManager.refreshToken + val accessToken = preferencesManager.accessToken(instance?.key) + val refreshToken = preferencesManager.refreshToken(instance?.key) if (refreshToken.isEmpty()) return null @@ -90,7 +98,7 @@ class OauthRefreshTokenAuthenticator( val retry = when (errorCode) { TOKEN_EXPIRED_ERROR_MESSAGE, JWT_TOKEN_EXPIRED -> { - handleTokenExpired(response, refreshToken, accessToken) + handleTokenExpired(response, refreshToken, accessToken, instance) } TOKEN_NONEXISTENT_ERROR_MESSAGE, TOKEN_INVALID_GRANT_ERROR_MESSAGE, JWT_INVALID_TOKEN -> { @@ -114,9 +122,14 @@ class OauthRefreshTokenAuthenticator( } // Helper function for handling token expiration logic - private fun handleTokenExpired(response: Response, refreshToken: String, accessToken: String): Request? { + private fun handleTokenExpired( + response: Response, + refreshToken: String, + accessToken: String, + instance: Instance?, + ): Request? { return try { - val newAuth = refreshAccessToken(refreshToken) + val newAuth = refreshAccessToken(refreshToken, instance) if (newAuth != null) { response.request.newBuilder() .header( @@ -125,7 +138,7 @@ class OauthRefreshTokenAuthenticator( ) .build() } else { - val actualToken = preferencesManager.accessToken + val actualToken = preferencesManager.accessToken(instance?.key) if (actualToken != accessToken) { response.request.newBuilder() .header( @@ -165,37 +178,35 @@ class OauthRefreshTokenAuthenticator( return time >= preferencesManager.accessTokenExpiresAt } - private fun canRequestTokenRefresh(): Boolean { - return TimeUtils.getCurrentTime() - lastTokenRefreshRequestTime > - REFRESH_TOKEN_INTERVAL_MINIMUM + private fun canRequestTokenRefresh(instance: Instance?): Boolean { + val last = lastTokenRefreshRequestTimes[instance?.key.orEmpty()] ?: 0L + return TimeUtils.getCurrentTime() - last > REFRESH_TOKEN_INTERVAL_MINIMUM } @Throws(IOException::class) - private fun refreshAccessToken(refreshToken: String): AuthResponse? { + private fun refreshAccessToken(refreshToken: String, instance: Instance?): AuthResponse? { var authResponse: AuthResponse? = null - if (canRequestTokenRefresh()) { - val instance = instanceProvider.currentInstance - val response = authApi().refreshAccessToken( + if (canRequestTokenRefresh(instance)) { + val response = authApi(instance).refreshAccessToken( ApiConstants.TOKEN_TYPE_REFRESH, 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 ?: "" val newExpireTime = authResponse.getTokenExpiryTime() if (newAccessToken.isNotEmpty() && newRefreshToken.isNotEmpty()) { - preferencesManager.accessToken = newAccessToken - preferencesManager.refreshToken = newRefreshToken - preferencesManager.accessTokenExpiresAt = newExpireTime - lastTokenRefreshRequestTime = TimeUtils.getCurrentTime() + // Saved for the site that issued them even if the learner switched away while + // the refresh was in flight: the server has already rotated the refresh token, + // so dropping these would sign that site out. + preferencesManager.saveTokens(instance?.key, newAccessToken, newRefreshToken, newExpireTime) + lastTokenRefreshRequestTimes[instance?.key.orEmpty()] = TimeUtils.getCurrentTime() } + if (instance?.key != instanceProvider.currentInstance?.key) return null } else if (response.code() == 400) { // another refresh already in progress Thread.sleep(REFRESH_TOKEN_THREAD_SLEEP) diff --git a/app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt b/app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt index 48b0d58a1..2650776b2 100644 --- a/app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt +++ b/app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt @@ -24,6 +24,8 @@ import org.openedx.core.domain.model.AppConfig import org.openedx.core.domain.model.CalendarType import org.openedx.core.domain.model.VideoQuality import org.openedx.core.domain.model.VideoSettings +import org.openedx.core.instance.StorageNamespace +import org.openedx.core.instance.StorageNamespaceProvider import org.openedx.core.system.CalendarManager import org.openedx.core.system.notifier.app.AppNotifier import org.openedx.core.system.notifier.app.LogoutEvent @@ -43,6 +45,7 @@ private val Context.dataStore: DataStore by preferencesDataStore( class PreferencesManager( private val context: Context, private val appNotifier: AppNotifier, + private val namespaces: StorageNamespaceProvider, ) : CorePreferences, ProfilePreferences, @@ -61,33 +64,46 @@ class PreferencesManager( @Volatile private var isKeyInvalidated = false + /** + * Keys tied to a learner on one site: they are namespaced per instance, so each site keeps + * its own session, profile and calendar state. Everything in [Keys] is device-wide. + */ + @Suppress("TooManyFunctions") + private object InstanceKeys { + fun accessToken(ns: StorageNamespace) = stringPreferencesKey(ns.key("access_token")) + fun refreshToken(ns: StorageNamespace) = stringPreferencesKey(ns.key("refresh_token")) + fun pushToken(ns: StorageNamespace) = stringPreferencesKey(ns.key("push_token")) + fun expiresIn(ns: StorageNamespace) = longPreferencesKey(ns.key("expires_in")) + fun user(ns: StorageNamespace) = stringPreferencesKey(ns.key("user")) + fun account(ns: StorageNamespace) = stringPreferencesKey(ns.key("account")) + fun appConfig(ns: StorageNamespace) = stringPreferencesKey(ns.key("app_config")) + fun isRelativeDatesEnabled(ns: StorageNamespace) = + booleanPreferencesKey(ns.key("is_relative_dates_enabled")) + fun calendarId(ns: StorageNamespace) = longPreferencesKey(ns.key("calendar_id")) + fun calendarUser(ns: StorageNamespace) = stringPreferencesKey(ns.key("calendar_user")) + fun calendarType(ns: StorageNamespace) = stringPreferencesKey(ns.key("calendar_type")) + fun isCalendarSyncEnabled(ns: StorageNamespace) = + booleanPreferencesKey(ns.key("is_calendar_sync_enabled")) + fun isHideInactiveCourses(ns: StorageNamespace) = + booleanPreferencesKey(ns.key("hide_inactive_courses")) + fun calendarSyncDialogShown(ns: StorageNamespace, courseName: String) = + booleanPreferencesKey(ns.key("calendar_sync_dialog_${courseName.replaceSpace("_")}")) + } + + private val ns: StorageNamespace + get() = namespaces.current + private object Keys { - val ACCESS_TOKEN = stringPreferencesKey("access_token") - val REFRESH_TOKEN = stringPreferencesKey("refresh_token") - val PUSH_TOKEN = stringPreferencesKey("push_token") - val EXPIRES_IN = longPreferencesKey("expires_in") - val USER = stringPreferencesKey("user") - val ACCOUNT = stringPreferencesKey("account") val VIDEO_SETTINGS_WIFI_DOWNLOAD_ONLY = booleanPreferencesKey("video_settings_wifi_download_only") val VIDEO_SETTINGS_STREAMING_QUALITY = stringPreferencesKey("video_settings_streaming_quality") val VIDEO_SETTINGS_DOWNLOAD_QUALITY = stringPreferencesKey("video_settings_download_quality") - val APP_CONFIG = stringPreferencesKey("app_config") val CAN_RESET_APP_DIRECTORY = booleanPreferencesKey("reset_app_directory") - val IS_RELATIVE_DATES_ENABLED = booleanPreferencesKey("is_relative_dates_enabled") - val CALENDAR_ID = longPreferencesKey("calendar_id") - val CALENDAR_USER = stringPreferencesKey("calendar_user") - val CALENDAR_TYPE = stringPreferencesKey("calendar_type") - val IS_CALENDAR_SYNC_ENABLED = booleanPreferencesKey("is_calendar_sync_enabled") - val IS_HIDE_INACTIVE_COURSES = booleanPreferencesKey("hide_inactive_courses") val LAST_WHATS_NEW_VERSION = stringPreferencesKey("last_whats_new_version") val LAST_REVIEW_VERSION = stringPreferencesKey("last_review_version") val WAS_POSITIVE_RATED = booleanPreferencesKey("app_was_positive_rated") - - fun calendarSyncDialogShown(courseName: String) = - booleanPreferencesKey("calendar_sync_dialog_${courseName.replaceSpace("_")}") } private fun getValue(key: Preferences.Key, defaultValue: T): T = @@ -121,47 +137,77 @@ class PreferencesManager( } override suspend fun clearCorePreferences() { + clearSession(namespaces.current) + } + + override fun hasSession(instanceKey: String?): Boolean { + val namespace = namespaces.namespaceFor(instanceKey) + return getValue(InstanceKeys.accessToken(namespace), "").isNotEmpty() && + getValue(InstanceKeys.user(namespace), "").isNotEmpty() + } + + override suspend fun clearSession(instanceKey: String?) { + clearSession(namespaces.namespaceFor(instanceKey)) + } + + private suspend fun clearSession(namespace: StorageNamespace) { dataStore.edit { prefs -> - prefs.remove(Keys.ACCESS_TOKEN) - prefs.remove(Keys.REFRESH_TOKEN) - prefs.remove(Keys.PUSH_TOKEN) - prefs.remove(Keys.EXPIRES_IN) - prefs.remove(Keys.USER) - prefs.remove(Keys.ACCOUNT) + prefs.remove(InstanceKeys.accessToken(namespace)) + prefs.remove(InstanceKeys.refreshToken(namespace)) + prefs.remove(InstanceKeys.pushToken(namespace)) + prefs.remove(InstanceKeys.expiresIn(namespace)) + prefs.remove(InstanceKeys.user(namespace)) + prefs.remove(InstanceKeys.account(namespace)) } } + override fun accessToken(instanceKey: String?): String { + return getEncryptedString(InstanceKeys.accessToken(namespaces.namespaceFor(instanceKey)), "") + } + + override fun refreshToken(instanceKey: String?): String { + return getEncryptedString(InstanceKeys.refreshToken(namespaces.namespaceFor(instanceKey)), "") + } + + override fun saveTokens(instanceKey: String?, accessToken: String, refreshToken: String, expiresAt: Long) { + val namespace = namespaces.namespaceFor(instanceKey) + setEncryptedString(InstanceKeys.accessToken(namespace), accessToken) + setEncryptedString(InstanceKeys.refreshToken(namespace), refreshToken) + setValue(InstanceKeys.expiresIn(namespace), expiresAt) + } + override suspend fun clearCalendarPreferences() { + val namespace = ns dataStore.edit { prefs -> - prefs.remove(Keys.CALENDAR_ID) - prefs.remove(Keys.CALENDAR_TYPE) - prefs.remove(Keys.IS_CALENDAR_SYNC_ENABLED) - prefs.remove(Keys.IS_HIDE_INACTIVE_COURSES) + prefs.remove(InstanceKeys.calendarId(namespace)) + prefs.remove(InstanceKeys.calendarType(namespace)) + prefs.remove(InstanceKeys.isCalendarSyncEnabled(namespace)) + prefs.remove(InstanceKeys.isHideInactiveCourses(namespace)) } } override var accessToken: String - get() = getEncryptedString(Keys.ACCESS_TOKEN, "") - set(value) = setEncryptedString(Keys.ACCESS_TOKEN, value) + get() = getEncryptedString(InstanceKeys.accessToken(ns), "") + set(value) = setEncryptedString(InstanceKeys.accessToken(ns), value) override var refreshToken: String - get() = getEncryptedString(Keys.REFRESH_TOKEN, "") - set(value) = setEncryptedString(Keys.REFRESH_TOKEN, value) + get() = getEncryptedString(InstanceKeys.refreshToken(ns), "") + set(value) = setEncryptedString(InstanceKeys.refreshToken(ns), value) override var pushToken: String - get() = getEncryptedString(Keys.PUSH_TOKEN, "") - set(value) = setEncryptedString(Keys.PUSH_TOKEN, value) + get() = getEncryptedString(InstanceKeys.pushToken(ns), "") + set(value) = setEncryptedString(InstanceKeys.pushToken(ns), value) override var accessTokenExpiresAt: Long - get() = getValue(Keys.EXPIRES_IN, 0L) - set(value) = setValue(Keys.EXPIRES_IN, value) + get() = getValue(InstanceKeys.expiresIn(ns), 0L) + set(value) = setValue(InstanceKeys.expiresIn(ns), value) override var user: User? get() { - val json = getEncryptedString(Keys.USER, "") + val json = getEncryptedString(InstanceKeys.user(ns), "") return if (json.isEmpty()) null else Gson().fromJson(json, User::class.java) } - set(value) = setEncryptedString(Keys.USER, Gson().toJson(value)) + set(value) = setEncryptedString(InstanceKeys.user(ns), Gson().toJson(value)) override var videoSettings: VideoSettings get() { @@ -188,25 +234,25 @@ class PreferencesManager( override var appConfig: AppConfig get() { - val json = getValue(Keys.APP_CONFIG, Gson().toJson(AppConfig())) + val json = getValue(InstanceKeys.appConfig(ns), Gson().toJson(AppConfig())) return Gson().fromJson(json, AppConfig::class.java) } - set(value) = setValue(Keys.APP_CONFIG, Gson().toJson(value)) + set(value) = setValue(InstanceKeys.appConfig(ns), Gson().toJson(value)) override var canResetAppDirectory: Boolean get() = getValue(Keys.CAN_RESET_APP_DIRECTORY, true) set(value) = setValue(Keys.CAN_RESET_APP_DIRECTORY, value) override var isRelativeDatesEnabled: Boolean - get() = getValue(Keys.IS_RELATIVE_DATES_ENABLED, true) - set(value) = setValue(Keys.IS_RELATIVE_DATES_ENABLED, value) + get() = getValue(InstanceKeys.isRelativeDatesEnabled(ns), true) + set(value) = setValue(InstanceKeys.isRelativeDatesEnabled(ns), value) override var profile: Account? get() { - val json = getEncryptedString(Keys.ACCOUNT, "") + val json = getEncryptedString(InstanceKeys.account(ns), "") return if (json.isEmpty()) null else Gson().fromJson(json, Account::class.java) } - set(value) = setEncryptedString(Keys.ACCOUNT, Gson().toJson(value)) + set(value) = setEncryptedString(InstanceKeys.account(ns), Gson().toJson(value)) override var lastWhatsNewVersion: String get() = getValue(Keys.LAST_WHATS_NEW_VERSION, "") @@ -229,33 +275,33 @@ class PreferencesManager( set(value) = setValue(Keys.WAS_POSITIVE_RATED, value) override var calendarId: Long - get() = getValue(Keys.CALENDAR_ID, CalendarManager.CALENDAR_DOES_NOT_EXIST) - set(value) = setValue(Keys.CALENDAR_ID, value) + get() = getValue(InstanceKeys.calendarId(ns), CalendarManager.CALENDAR_DOES_NOT_EXIST) + set(value) = setValue(InstanceKeys.calendarId(ns), value) override var calendarUser: String - get() = getValue(Keys.CALENDAR_USER, "") - set(value) = setValue(Keys.CALENDAR_USER, value) + get() = getValue(InstanceKeys.calendarUser(ns), "") + set(value) = setValue(InstanceKeys.calendarUser(ns), value) override var calendarType: CalendarType get() { - val stored = getValue(Keys.CALENDAR_TYPE, CalendarType.LOCAL.name) + val stored = getValue(InstanceKeys.calendarType(ns), CalendarType.LOCAL.name) return runCatching { CalendarType.valueOf(stored) }.getOrDefault(CalendarType.LOCAL) } - set(value) = setValue(Keys.CALENDAR_TYPE, value.name) + set(value) = setValue(InstanceKeys.calendarType(ns), value.name) override var isCalendarSyncEnabled: Boolean - get() = getValue(Keys.IS_CALENDAR_SYNC_ENABLED, true) - set(value) = setValue(Keys.IS_CALENDAR_SYNC_ENABLED, value) + get() = getValue(InstanceKeys.isCalendarSyncEnabled(ns), true) + set(value) = setValue(InstanceKeys.isCalendarSyncEnabled(ns), value) override var isHideInactiveCourses: Boolean - get() = getValue(Keys.IS_HIDE_INACTIVE_COURSES, true) - set(value) = setValue(Keys.IS_HIDE_INACTIVE_COURSES, value) + get() = getValue(InstanceKeys.isHideInactiveCourses(ns), true) + set(value) = setValue(InstanceKeys.isHideInactiveCourses(ns), value) override fun setCalendarSyncEventsDialogShown(courseName: String) { - setValue(Keys.calendarSyncDialogShown(courseName), true) + setValue(InstanceKeys.calendarSyncDialogShown(ns, courseName), true) } override fun isCalendarSyncEventsDialogShown(courseName: String): Boolean { - return getValue(Keys.calendarSyncDialogShown(courseName), false) + return getValue(InstanceKeys.calendarSyncDialogShown(ns, courseName), false) } } diff --git a/app/src/main/java/org/openedx/app/di/AppModule.kt b/app/src/main/java/org/openedx/app/di/AppModule.kt index a713834c1..8bd453326 100644 --- a/app/src/main/java/org/openedx/app/di/AppModule.kt +++ b/app/src/main/java/org/openedx/app/di/AppModule.kt @@ -40,10 +40,12 @@ import org.openedx.core.instance.InstanceCatalogCache import org.openedx.core.instance.InstanceConfigLoader import org.openedx.core.instance.InstanceProvider import org.openedx.core.instance.InstanceSelectionStorage +import org.openedx.core.instance.InstanceStorageNamespaces import org.openedx.core.instance.InstanceStore import org.openedx.core.instance.OkHttpInstanceApiService import org.openedx.core.instance.SharedPrefsInstanceCatalogCache import org.openedx.core.instance.SharedPrefsInstanceSelectionStorage +import org.openedx.core.instance.StorageNamespaceProvider import org.openedx.core.module.DownloadWorkerController import org.openedx.core.module.TranscriptManager import org.openedx.core.module.download.DownloadHelper @@ -96,7 +98,17 @@ import org.openedx.core.DatabaseManager as IDatabaseManager val appModule = module { single { Config(get()) } - single { SharedPrefsInstanceSelectionStorage(get()) } + single { SharedPrefsInstanceSelectionStorage(get()) } + single { get() } + single { + val config = get() + InstanceStorageNamespaces( + instanceStore = get(), + ownerStorage = get(), + appApiHostUrl = config.getApiHostURL(), + appOAuthClientId = config.getOAuthClientId(), + ) + } single { val bundledInstances by lazy { get().getInstancesConfig() } InstanceStore(get()) { bundledInstances } @@ -105,7 +117,7 @@ val appModule = module { single { SharedPrefsInstanceCatalogCache(get()) } single { OkHttpInstanceApiService(get().getInstancesCatalogUrl()) } single { InstanceConfigLoader(get(), get(), { get().getInstancesConfig() }) } - single { PreferencesManager(get(), get()) } + single { PreferencesManager(get(), get(), get()) } single { get() } single { get() } single { get() } 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 index 67880402e..3833b6f81 100644 --- a/app/src/test/java/org/openedx/app/data/networking/InstanceAwareNetworkingTest.kt +++ b/app/src/test/java/org/openedx/app/data/networking/InstanceAwareNetworkingTest.kt @@ -15,7 +15,6 @@ 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 @@ -165,6 +164,15 @@ class InstanceAwareNetworkingTest { override var accessToken: String = "old-access" override var refreshToken: String = "old-refresh" override var accessTokenExpiresAt: Long = Long.MAX_VALUE + val savedTokens = mutableMapOf() + + override fun accessToken(instanceKey: String?) = savedTokens[instanceKey] ?: accessToken + + override fun refreshToken(instanceKey: String?) = refreshToken + + override fun saveTokens(instanceKey: String?, accessToken: String, refreshToken: String, expiresAt: Long) { + savedTokens[instanceKey] = accessToken + } } private fun unauthorized(request: Request) = ok(request, """{"detail": "Token has expired."}""", code = 401) @@ -209,7 +217,30 @@ class InstanceAwareNetworkingTest { 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) + assertEquals(mapOf("beta" to "new-access"), prefs.savedTokens) + } + + @Test + fun `a refresh on one site does not hold back the other site's refresh`() { + val provider = FakeInstanceProvider(alpha) + val tokenHosts = mutableListOf() + val refreshClient = OkHttpClient.Builder().addInterceptor { chain -> + tokenHosts += chain.request().url.host + ok(chain.request(), """{"access_token": "new", "refresh_token": "rotated", "expires_in": 3600}""") + }.build() + val authenticator = OauthRefreshTokenAuthenticator( + mockk(relaxed = true) { every { getAccessTokenType() } returns "JWT" }, + Prefs(), + mockk(relaxed = true), + provider, + refreshClient + ) + + authenticator.authenticate(null, unauthorized(taggedRequest("https://alpha.example.com/api/x", "alpha"))) + provider.select(beta) + authenticator.authenticate(null, unauthorized(taggedRequest("http://beta.example.com:8080/api/x", "beta"))) + + assertEquals(listOf("alpha.example.com", "beta.example.com"), tokenHosts) } @Test @@ -238,7 +269,7 @@ class InstanceAwareNetworkingTest { } @Test - fun `tokens refreshed while the learner switched sites are discarded`() { + fun `tokens refreshed while the learner switched sites are kept for the site that issued them`() { val provider = FakeInstanceProvider(alpha) val refreshClient = OkHttpClient.Builder().addInterceptor { chain -> provider.select(beta) @@ -262,8 +293,8 @@ class InstanceAwareNetworkingTest { ) assertNull(retry) - assertEquals("old-access", prefs.accessToken) - assertNotNull(provider.currentInstance) + assertEquals(mapOf("alpha" to "alpha-new"), prefs.savedTokens) + assertEquals("beta", provider.currentInstance?.key) } // endregion diff --git a/core/src/main/java/org/openedx/core/data/storage/CorePreferences.kt b/core/src/main/java/org/openedx/core/data/storage/CorePreferences.kt index 9e42a5273..6cd94f8a8 100644 --- a/core/src/main/java/org/openedx/core/data/storage/CorePreferences.kt +++ b/core/src/main/java/org/openedx/core/data/storage/CorePreferences.kt @@ -15,5 +15,19 @@ interface CorePreferences { var canResetAppDirectory: Boolean var isRelativeDatesEnabled: Boolean + /** Clears the selected instance's session: tokens, user and profile. */ suspend fun clearCorePreferences() + + /** Whether [instanceKey]'s site has a stored session, without selecting it. */ + fun hasSession(instanceKey: String?): Boolean + + /** Clears one instance's session without touching the selected one. */ + suspend fun clearSession(instanceKey: String?) + + fun accessToken(instanceKey: String?): String + + fun refreshToken(instanceKey: String?): String + + /** Stores refreshed tokens for the site that issued them, whichever site is selected now. */ + fun saveTokens(instanceKey: String?, accessToken: String, refreshToken: String, expiresAt: Long) } diff --git a/core/src/main/java/org/openedx/core/instance/InstanceSelectionStorage.kt b/core/src/main/java/org/openedx/core/instance/InstanceSelectionStorage.kt index 91f2b77b3..74f99c72c 100644 --- a/core/src/main/java/org/openedx/core/instance/InstanceSelectionStorage.kt +++ b/core/src/main/java/org/openedx/core/instance/InstanceSelectionStorage.kt @@ -10,7 +10,9 @@ interface InstanceSelectionStorage { var selectedInstanceKey: String? } -class SharedPrefsInstanceSelectionStorage(context: Context) : InstanceSelectionStorage { +class SharedPrefsInstanceSelectionStorage(context: Context) : + InstanceSelectionStorage, + LegacyNamespaceOwnerStorage { private val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) @@ -22,8 +24,15 @@ class SharedPrefsInstanceSelectionStorage(context: Context) : InstanceSelectionS if (value == null) remove(KEY_SELECTED_INSTANCE) else putString(KEY_SELECTED_INSTANCE, value) } + override var legacyOwnerInstanceKey: String? + get() = prefs.getString(KEY_LEGACY_OWNER, null) + set(value) = prefs.edit(commit = true) { + if (value == null) remove(KEY_LEGACY_OWNER) else putString(KEY_LEGACY_OWNER, value) + } + private companion object { const val PREFS_NAME = "openedx_instance_store" const val KEY_SELECTED_INSTANCE = "selected_instance_key" + const val KEY_LEGACY_OWNER = "legacy_storage_owner_instance_key" } } diff --git a/core/src/main/java/org/openedx/core/instance/StorageNamespace.kt b/core/src/main/java/org/openedx/core/instance/StorageNamespace.kt new file mode 100644 index 000000000..f1d824e0b --- /dev/null +++ b/core/src/main/java/org/openedx/core/instance/StorageNamespace.kt @@ -0,0 +1,125 @@ +package org.openedx.core.instance + +import java.net.URI +import java.security.MessageDigest +import java.util.Locale + +/** + * Where one instance's local data lives: preference keys, the database file and the + * downloads folder. + * + * The legacy namespace ([instanceKey] `null`) is the storage a single-site build has always + * used, unprefixed. It belongs to "no instance selected" and to the one instance that is the + * same site as the app-level configuration, so upgrading keeps a signed-in learner's session, + * cached courses and downloads in place. + */ +data class StorageNamespace(val instanceKey: String?) { + + val isLegacy: Boolean get() = instanceKey == null + + /** e.g. `access_token` for the legacy namespace, `instance.acme.access_token` otherwise. */ + fun key(rawKey: String): String = if (instanceKey == null) rawKey else "$PREFIX$instanceKey.$rawKey" + + /** + * A file-system-safe, collision-resistant name segment, or `null` for the legacy namespace. + * The readable part is lossy; the hash keeps two keys that sanitize alike apart. + */ + val fileSegment: String? + get() = instanceKey?.let { key -> + val readable = key.replace(UNSAFE_FILE_CHARS, "_").take(MAX_READABLE_LENGTH) + "${readable}_${sha256(key).take(HASH_LENGTH)}" + } + + companion object { + val LEGACY = StorageNamespace(null) + + private const val PREFIX = "instance." + private const val MAX_READABLE_LENGTH = 32 + private const val HASH_LENGTH = 12 + private val UNSAFE_FILE_CHARS = "[^A-Za-z0-9_-]".toRegex() + + private fun sha256(value: String): String { + val digest = MessageDigest.getInstance("SHA-256").digest(value.toByteArray(Charsets.UTF_8)) + return digest.joinToString("") { "%02x".format(it) } + } + } +} + +/** Resolves instances to the namespace their local data lives under. */ +interface StorageNamespaceProvider { + /** The selected instance's namespace; legacy when nothing is selected. */ + val current: StorageNamespace + + fun namespaceFor(instance: Instance?): StorageNamespace + + /** Resolves by key through the catalog; a key no longer in the catalog keeps its own namespace. */ + fun namespaceFor(instanceKey: String?): StorageNamespace +} + +/** Persists which instance owns the legacy namespace once that has been decided. */ +interface LegacyNamespaceOwnerStorage { + var legacyOwnerInstanceKey: String? +} + +/** + * The instance whose API host and OAuth client are the app-level ones is the site a + * single-site build of this app was signing in to, so it inherits the legacy namespace. That + * decision is recorded the first time it is made and never revisited: a later catalog or + * configuration change can't hand existing data to a different site. + */ +class InstanceStorageNamespaces( + private val instanceStore: InstanceStore, + private val ownerStorage: LegacyNamespaceOwnerStorage, + private val appApiHostUrl: String, + private val appOAuthClientId: String, +) : StorageNamespaceProvider { + + private val lock = Any() + + override val current: StorageNamespace + get() = namespaceFor(instanceStore.currentInstance) + + override fun namespaceFor(instance: Instance?): StorageNamespace { + if (instance == null) return StorageNamespace.LEGACY + val owner = synchronized(lock) { + ownerStorage.legacyOwnerInstanceKey ?: instance.key + .takeIf { isAppLevelSite(instance) } + ?.also { ownerStorage.legacyOwnerInstanceKey = it } + } + return if (owner == instance.key) StorageNamespace.LEGACY else StorageNamespace(instance.key) + } + + override fun namespaceFor(instanceKey: String?): StorageNamespace { + val instance = instanceKey?.let { instanceStore.instance(it) } + val owner = synchronized(lock) { ownerStorage.legacyOwnerInstanceKey } + return when { + instanceKey == null -> StorageNamespace.LEGACY + instance != null -> namespaceFor(instance) + owner == instanceKey -> StorageNamespace.LEGACY + else -> StorageNamespace(instanceKey) + } + } + + private fun isAppLevelSite(instance: Instance): Boolean { + return instance.oAuthClientId == appOAuthClientId && + normalizedOrigin(instance.apiHostUrl) != null && + normalizedOrigin(instance.apiHostUrl) == normalizedOrigin(appApiHostUrl) + } + + private fun normalizedOrigin(url: String): String? = runCatching { + val uri = URI(url.trim()) + val scheme = uri.scheme?.lowercase(Locale.ROOT) ?: return null + val host = uri.host?.lowercase(Locale.ROOT) ?: return null + val port = when { + uri.port != -1 -> uri.port + scheme == "https" -> HTTPS_PORT + else -> HTTP_PORT + } + "$scheme://$host:$port${uri.path.orEmpty().trimEnd('/')}" + }.getOrNull() + + private companion object { + const val HTTPS_PORT = 443 + const val HTTP_PORT = 80 + } +} diff --git a/core/src/test/java/org/openedx/core/instance/StorageNamespaceTest.kt b/core/src/test/java/org/openedx/core/instance/StorageNamespaceTest.kt new file mode 100644 index 000000000..9c84b064e --- /dev/null +++ b/core/src/test/java/org/openedx/core/instance/StorageNamespaceTest.kt @@ -0,0 +1,134 @@ +package org.openedx.core.instance + +import com.google.gson.JsonObject +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class StorageNamespaceTest { + + private class InMemoryStorage( + override var selectedInstanceKey: String? = null, + override var legacyOwnerInstanceKey: String? = null, + ) : InstanceSelectionStorage, LegacyNamespaceOwnerStorage + + private fun instance(key: String, host: String, client: String = "$key-client") = requireNotNull( + Instance.fromJson( + JsonObject().apply { + addProperty("KEY", key) + addProperty("NAME", key) + addProperty("OAUTH_CLIENT_ID", client) + addProperty("API_HOST_URL", host) + } + ) + ) + + private val appSite = instance("app-site", "https://LMS.example.com/", client = "app-client") + private val other = instance("other", "https://other.example.com") + + private fun namespaces(storage: InMemoryStorage, vararg instances: Instance): InstanceStorageNamespaces { + val store = InstanceStore(storage) { InstancesConfig(instances.toList()) } + return InstanceStorageNamespaces(store, storage, "https://lms.example.com:443", "app-client") + } + + @Test + fun `legacy namespace keeps raw keys and has no file segment`() { + assertEquals("access_token", StorageNamespace.LEGACY.key("access_token")) + assertNull(StorageNamespace.LEGACY.fileSegment) + assertTrue(StorageNamespace.LEGACY.isLegacy) + } + + @Test + fun `instance namespace prefixes keys with the instance key`() { + assertEquals("instance.acme.access_token", StorageNamespace("acme").key("access_token")) + } + + @Test + fun `file segments are safe and keep keys that sanitize alike apart`() { + val slash = requireNotNull(StorageNamespace("a/b").fileSegment) + val colon = requireNotNull(StorageNamespace("a:b").fileSegment) + + assertTrue(slash, slash.matches("[A-Za-z0-9_-]+".toRegex())) + assertTrue(slash.startsWith("a_b_")) + assertNotEquals(slash, colon) + assertEquals(slash, StorageNamespace("a/b").fileSegment) + } + + @Test + fun `nothing selected uses the legacy namespace`() { + val namespaces = namespaces(InMemoryStorage(), appSite, other) + + assertEquals(StorageNamespace.LEGACY, namespaces.current) + assertEquals(StorageNamespace.LEGACY, namespaces.namespaceFor(null as String?)) + } + + @Test + fun `the instance matching the app level host and client inherits the legacy namespace`() { + val storage = InMemoryStorage() + val namespaces = namespaces(storage, appSite, other) + + assertEquals(StorageNamespace.LEGACY, namespaces.namespaceFor(appSite)) + assertEquals(StorageNamespace("other"), namespaces.namespaceFor(other)) + assertEquals("app-site", storage.legacyOwnerInstanceKey) + } + + @Test + fun `same host with a different OAuth client is a different site`() { + val sameHost = instance("same-host", "https://lms.example.com", client = "another-client") + val namespaces = namespaces(InMemoryStorage(), sameHost) + + assertEquals(StorageNamespace("same-host"), namespaces.namespaceFor(sameHost)) + } + + @Test + fun `different port or path is a different site`() { + val port = instance("port", "https://lms.example.com:8443", client = "app-client") + val path = instance("path", "https://lms.example.com/tenant", client = "app-client") + val namespaces = namespaces(InMemoryStorage(), port, path) + + assertEquals(StorageNamespace("port"), namespaces.namespaceFor(port)) + assertEquals(StorageNamespace("path"), namespaces.namespaceFor(path)) + } + + @Test + fun `the recorded owner never changes once decided`() { + val storage = InMemoryStorage(legacyOwnerInstanceKey = "other") + val namespaces = namespaces(storage, appSite, other) + + assertEquals(StorageNamespace.LEGACY, namespaces.namespaceFor(other)) + assertEquals(StorageNamespace("app-site"), namespaces.namespaceFor(appSite)) + assertEquals("other", storage.legacyOwnerInstanceKey) + } + + @Test + fun `catalog order does not decide ownership`() { + val first = namespaces(InMemoryStorage(), other, appSite) + val second = namespaces(InMemoryStorage(), appSite, other) + + assertEquals(first.namespaceFor(appSite), second.namespaceFor(appSite)) + assertEquals(first.namespaceFor(other), second.namespaceFor(other)) + } + + @Test + fun `a key no longer in the catalog keeps its own namespace`() { + val storage = InMemoryStorage(legacyOwnerInstanceKey = "app-site") + val namespaces = namespaces(storage, other) + + assertEquals(StorageNamespace("gone"), namespaces.namespaceFor("gone")) + assertEquals(StorageNamespace.LEGACY, namespaces.namespaceFor("app-site")) + } + + @Test + fun `current follows the selection`() { + val storage = InMemoryStorage() + val store = InstanceStore(storage) { InstancesConfig(listOf(appSite, other)) } + val namespaces = InstanceStorageNamespaces(store, storage, "https://lms.example.com", "app-client") + + store.select(other) + assertEquals(StorageNamespace("other"), namespaces.current) + store.select(appSite) + assertEquals(StorageNamespace.LEGACY, namespaces.current) + } +}