diff options
7 files changed, 269 insertions, 2 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index 1395f44..4b2c573 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.26 +94.27 diff --git a/app/src/androidTest/java/org/terst/listen/playback/StreamedServerAudioTest.kt b/app/src/androidTest/java/org/terst/listen/playback/StreamedServerAudioTest.kt new file mode 100644 index 0000000..6255a42 --- /dev/null +++ b/app/src/androidTest/java/org/terst/listen/playback/StreamedServerAudioTest.kt @@ -0,0 +1,135 @@ +package org.terst.listen.playback + +import android.content.ComponentName +import androidx.media3.common.MediaItem +import androidx.media3.common.Player +import androidx.media3.session.MediaBrowser +import androidx.media3.session.SessionToken +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.guava.await +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assume.assumeFalse +import org.junit.Test +import org.junit.runner.RunWith +import org.terst.listen.data.Graph +import java.net.HttpURLConnection +import java.net.URL + +/** + * The server's audio plays when it is streamed, not only when it is downloaded. + * + * The token used to ride in the audio URL's query string, which wrote a copy of the secret into + * Apache's access log on every fetch. The downloader moved to an `Authorization` header; the + * player had not, and the player is how the car plays anything not already on disk. Withdrawing + * the query parameter without this would have broken streaming in the car silently, as a 401 + * surfacing as "playback failed". + * + * Skipped unless a real token is supplied, because the thing being tested is a live server's + * answer and a fake one proves nothing about Apache, the vhost or the token: + * + * ``` + * ... -Pandroid.testInstrumentationRunnerArguments.listenToken=<token> + * ``` + * + * The token is never checked in. It arrives as an instrumentation argument for exactly that + * reason -- a test file carrying it would put the secret back in the repository, which is the + * same mistake in a different file. + */ +@RunWith(AndroidJUnit4::class) +class StreamedServerAudioTest { + + private val args = InstrumentationRegistry.getArguments() + private val token: String = args.getString("listenToken").orEmpty() + private val baseUrl: String = args.getString("listenServer") ?: "https://listen.terst.org" + private val episodeId: String = args.getString("listenEpisode").orEmpty() + + /** + * Asks the server for the audio exactly as the player now does: header, no query. + * + * Driven through the app's own [ServerAudioAuth] rather than a hand-written header, so what + * is proved is the decision the player actually makes -- including that it decides to send + * anything at all for this URL. + */ + @Test + fun theServerServesItsAudioToAHeaderWithNoTokenInTheUrl() = runBlocking { + assumeFalse("no listenToken supplied", token.isEmpty()) + assumeFalse("no listenEpisode supplied", episodeId.isEmpty()) + + val context = InstrumentationRegistry.getInstrumentation().targetContext + Graph.get(context).preferences.setServer(baseUrl, token) + val auth = Graph.get(context).serverAudioAuth.also { it.refresh() } + + val url = "$baseUrl/audio/$episodeId" + val header = auth.headerFor(url) + assertEquals("the player must send the token to the server's own audio", "Bearer $token", header) + + assertEquals("server should serve the audio to a header alone", 200, statusOf(url, header)) + assertEquals("and must still refuse an unauthenticated fetch", 401, statusOf(url, null)) + } + + /** + * The player itself streams it, which is the half the check above cannot reach. + * + * That one proves the app decides to send a token and the server accepts one. This proves + * ExoPlayer actually puts it on the wire: the header is attached by a `ResolvingDataSource` + * wrapped around the HTTP source, and if that wiring were wrong the decision would be made + * correctly and thrown away. A 401 surfaces here as a source error rather than READY. + */ + @Test + fun thePlayerReachesReadyOnTheServersAudio() = runBlocking { + assumeFalse("no listenToken supplied", token.isEmpty()) + assumeFalse("no listenEpisode supplied", episodeId.isEmpty()) + + val context = InstrumentationRegistry.getInstrumentation().targetContext + Graph.get(context).preferences.setServer(baseUrl, token) + + val token2 = SessionToken(context, ComponentName(context, PlaybackService::class.java)) + val browser = withContext(Dispatchers.Main) { + MediaBrowser.Builder(context, token2).buildAsync().await() + } + try { + withContext(Dispatchers.Main) { + browser.setMediaItem( + MediaItem.Builder().setUri("$baseUrl/audio/$episodeId").build() + ) + browser.prepare() + + val giveUpAt = System.currentTimeMillis() + 30_000 + while (browser.playbackState != Player.STATE_READY && + browser.playerError == null && + System.currentTimeMillis() < giveUpAt + ) { + delay(200) + } + assertNull("streaming the server's audio errored", browser.playerError) + assertEquals( + "player should be ready on the server's stream", + Player.STATE_READY, + browser.playbackState, + ) + } + } finally { + withContext(Dispatchers.Main) { + browser.stop() + browser.release() + } + } + } + + private fun statusOf(url: String, header: String?): Int { + val connection = URL(url).openConnection() as HttpURLConnection + return try { + connection.requestMethod = "HEAD" + header?.let { connection.setRequestProperty("Authorization", it) } + connection.responseCode + } finally { + connection.disconnect() + } + } +} diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index f08c484..0817178 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -10,6 +10,7 @@ import org.terst.listen.download.WaitingDownloads import org.terst.listen.server.LabelPusher import org.terst.listen.server.ServerSkips import org.terst.listen.server.SkipRefresher +import org.terst.listen.playback.ServerAudioAuth import org.terst.listen.server.SkipServer import org.terst.listen.server.SkipServerConfig import org.terst.listen.server.StoredRefusals @@ -227,6 +228,15 @@ class AppGraph(private val context: Context) : UiGraph { * WorkManager's ten-minute ceiling allows, and the attempt burned the battery for weeks * while confirming nothing. */ + /** + * The token the player sends, shared so the service and the session callback hold one cache. + * + * The callback refreshes it; the player reads it from ExoPlayer's loading thread, which has + * no coroutine to suspend in. Two instances would mean the one doing the reading was never + * the one that had been refreshed. + */ + val serverAudioAuth: ServerAudioAuth by lazy { ServerAudioAuth { skipServer.configured() } } + val skipServer: SkipServer by lazy { SkipServer( config = { diff --git a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt index d8e1856..758fb91 100644 --- a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt +++ b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt @@ -199,6 +199,9 @@ class ListenSessionCallback( controller: MediaSession.ControllerInfo, mediaItems: MutableList<MediaItem>, ): ListenableFuture<MutableList<MediaItem>> = scope.future { + // Before any stream URL is handed to the player: the resolver reads this cache from + // ExoPlayer's loading thread, where there is no coroutine to read DataStore in. + graph.serverAudioAuth.refresh() mediaItems.mapNotNull { item -> val episodeId = BrowseIds.episodeId(item.mediaId) ?: return@mapNotNull item val episode = graph.episodes.getById(episodeId) ?: return@mapNotNull null diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt index a2ca31c..d8ff882 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -8,7 +8,11 @@ import androidx.media3.common.C import androidx.media3.common.MediaItem import androidx.media3.common.PlaybackParameters import androidx.media3.common.Player +import androidx.media3.datasource.DefaultDataSource +import androidx.media3.datasource.DefaultHttpDataSource +import androidx.media3.datasource.ResolvingDataSource import androidx.media3.exoplayer.ExoPlayer +import androidx.media3.exoplayer.source.DefaultMediaSourceFactory import androidx.media3.session.MediaSession import androidx.media3.session.MediaLibraryService import kotlinx.coroutines.CoroutineScope @@ -54,7 +58,21 @@ class PlaybackService : MediaLibraryService() { override fun onCreate() { super.onCreate() + val graph = Graph.get(this) + // The server's audio needs the token in a header. It used to ride in the URL's query, + // which wrote a copy of the secret into Apache's access log on every fetch; the + // downloader moved to a header and streaming had to follow, because streaming is how the + // car plays anything not already downloaded. Resolved per URL rather than set as a + // default request property: the same player fetches publisher audio, and that gets none. + val audioSource = ResolvingDataSource.Factory( + DefaultDataSource.Factory(this, DefaultHttpDataSource.Factory()), + ) { spec -> + graph.serverAudioAuth.headerFor(spec.uri.toString()) + ?.let { spec.withAdditionalHeaders(mapOf("Authorization" to it)) } + ?: spec + } val exo = ExoPlayer.Builder(this) + .setMediaSourceFactory(DefaultMediaSourceFactory(audioSource)) .setAudioAttributes( AudioAttributes.Builder() .setContentType(C.AUDIO_CONTENT_TYPE_SPEECH) @@ -75,7 +93,6 @@ class PlaybackService : MediaLibraryService() { // Read before the session is built: the session's callback needs the reporter, because // a "this is an ad" press is answered from the player's position. - val graph = Graph.get(this) reporter = AdReporter(AdReportGesture(), graph.adReportStore) // A library session rather than a plain one: Android Auto only treats an app as diff --git a/app/src/main/java/org/terst/listen/playback/ServerAudioAuth.kt b/app/src/main/java/org/terst/listen/playback/ServerAudioAuth.kt new file mode 100644 index 0000000..ec8b4b5 --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/ServerAudioAuth.kt @@ -0,0 +1,43 @@ +package org.terst.listen.playback + +import org.terst.listen.server.ServerBearer +import org.terst.listen.server.SkipServerConfig + +/** + * The token the *player* sends, as opposed to the one the downloader sends. + * + * Both ask [ServerBearer] the same question and must answer it the same way, but they ask it from + * different places. The downloader is in a coroutine and can read the configuration when it needs + * it; ExoPlayer resolves a URL on its own loading thread with no coroutine in sight, so the + * configuration has to be sitting there already. Hence a cache and an explicit [refresh], rather + * than blocking the loading thread on a DataStore read before every stream starts. + * + * Why the player needs a token at all: the server's audio URL used to carry it in the query + * string, which put a copy of the secret in Apache's access log on every fetch. The downloader + * moved to a header; streaming did not, and streaming is how the car plays anything it has not + * already downloaded. Until both send it, the query parameter cannot be withdrawn. + * + * Nothing is cached per URL. The decision depends on the URL and must be made again every time: + * the same player fetches publisher audio too, and the point of [ServerBearer] is that those get + * nothing. + */ +class ServerAudioAuth(private val config: suspend () -> SkipServerConfig?) { + + @Volatile + private var cached: SkipServerConfig? = null + + /** Reads the current configuration, so [headerFor] can answer without suspending. */ + suspend fun refresh() { + cached = config() + } + + /** + * The `Authorization` value for [url], or null when it is not the server's to send to. + * + * Null before the first [refresh], which is the safe direction: a stream that should have + * carried the token fails visibly with a 401, where one that should not have carried it + * would hand Peter's credentials to a podcast host. + */ + fun headerFor(url: String): String? = + ServerBearer.forUrl(cached, url)?.let { "Bearer $it" } +} diff --git a/app/src/test/java/org/terst/listen/playback/ServerAudioAuthTest.kt b/app/src/test/java/org/terst/listen/playback/ServerAudioAuthTest.kt new file mode 100644 index 0000000..fa5c5bc --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/ServerAudioAuthTest.kt @@ -0,0 +1,59 @@ +package org.terst.listen.playback + +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.terst.listen.server.SkipServerConfig + +class ServerAudioAuthTest { + + private val server = SkipServerConfig(baseUrl = "https://listen.example.org", token = "s3cret") + + private fun auth(config: SkipServerConfig? = server) = ServerAudioAuth { config } + + @Test + fun sendsNothingBeforeTheConfigurationHasBeenRead() { + assertNull(auth().headerFor("https://listen.example.org/audio/abc")) + } + + @Test + fun sendsTheTokenToTheServersOwnAudio() = runBlocking { + val auth = auth().also { it.refresh() } + + assertEquals("Bearer s3cret", auth.headerFor("https://listen.example.org/audio/abc")) + } + + /** + * The failure this exists to prevent: the same player fetches whatever a publisher nominated, + * and a token sent there is Peter's server credentials handed to a podcast host. + */ + @Test + fun sendsNothingToAPublisher() = runBlocking { + val auth = auth().also { it.refresh() } + + assertNull(auth.headerFor("https://cdn.megaphone.fm/episode.mp3")) + assertNull(auth.headerFor("https://listen.example.org.evil.test/audio/abc")) + assertNull(auth.headerFor("http://listen.example.org/audio/abc")) + } + + @Test + fun sendsNothingWhenNoServerIsConfigured() = runBlocking { + val auth = auth(config = null).also { it.refresh() } + + assertNull(auth.headerFor("https://listen.example.org/audio/abc")) + } + + @Test + fun picksUpAServerConfiguredAfterItStarted() = runBlocking { + var current: SkipServerConfig? = null + val auth = ServerAudioAuth { current } + auth.refresh() + assertNull(auth.headerFor("https://listen.example.org/audio/abc")) + + current = server + auth.refresh() + + assertEquals("Bearer s3cret", auth.headerFor("https://listen.example.org/audio/abc")) + } +} |