Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ output
composeApp/src/desktopMain/resources/windows/WinSparkle.*

/composeApp/src/desktopMain/frameworks/
*.mmdb

certificates/*.pem
4 changes: 2 additions & 2 deletions composeApp/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,8 @@ android {
coreLibraryDesugaring(libs.android.desugar.jdk)
debugImplementation(compose.uiTooling)
"fullImplementation"(libs.bundles.full.android)
"fullImplementation"("org.ooni:oonimkall:3.27.0-android:@aar")
"fdroidImplementation"("org.ooni:oonimkall:3.27.0-android:@aar")
"fullImplementation"("org.ooni:oonimkall:3.28.0-alpha-android:@aar")
"fdroidImplementation"("org.ooni:oonimkall:3.28.0-alpha-android:@aar")
"xperimentalImplementation"(files("libs/android-oonimkall.aar"))
androidTestUtil(libs.android.orchestrator)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ class AndroidOonimkallBridge : OonimkallBridge {
it.softwareVersion = softwareVersion

it.assetsDir = assetsDir
// geoipDB may not exist in Android binding; set reflectively if available
geoIpDB?.let { path ->
it.geoipDB = path
}
it.stateDir = stateDir
it.tempDir = tempDir
it.tunnelDir = tunnelDir
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package org.ooni.probe.net

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import java.net.HttpURLConnection
import java.net.URL

actual suspend fun httpGetBytes(url: String): ByteArray =
withContext(Dispatchers.IO) {
val connection = (URL(url).openConnection() as HttpURLConnection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The openConnection() may also throw an IOException

connection.requestMethod = "GET"
connection.instanceFollowRedirects = true
connection.connectTimeout = 15000
connection.readTimeout = 30000
try {
val code = connection.responseCode
val stream = if (code in 200..299) connection.inputStream else connection.errorStream
val bytes = stream?.use { it.readBytes() } ?: ByteArray(0)
if (code !in 200..299) throw RuntimeException("HTTP $code while GET $url: ${String(bytes)}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuntimeException seems to be a bit excessive, it usually means it's time to crash. Maybe just Exception or IOException?

bytes
} finally {
connection.disconnect()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@
<string name="r2160p_ext" translatable="false">2160p (4k)</string>

<string name="twoParam" translatable="false">%1$s %2$s</string>
<string name="engine_mmdb_version" translatable="false">20250801</string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's easier if this is a regular constant, instead of a string resource? We're not taking advantage of the resource system for anything here.

</resources>
6 changes: 4 additions & 2 deletions composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class Engine(

private fun session(sessionConfig: OonimkallBridge.SessionConfig): OonimkallBridge.Session = bridge.newSession(sessionConfig)

private fun buildTaskSettings(
private suspend fun buildTaskSettings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suspend should not be needed.

netTest: NetTest,
taskOrigin: TaskOrigin,
preferences: EnginePreferences,
Expand All @@ -166,6 +166,7 @@ class Engine(
tunnelDir = "$baseFilePath/tunnel",
tempDir = cacheDir,
assetsDir = "$baseFilePath/assets",
geoIpDB = preferences.geoipDbVersion?.let { "$cacheDir/$it.mmdb" },
options = TaskSettings.Options(
noCollector = !preferences.uploadResults,
softwareName = buildSoftwareName(taskOrigin),
Expand Down Expand Up @@ -196,7 +197,7 @@ class Engine(
MAX_RUNTIME_DISABLED
}

private fun buildSessionConfig(
private suspend fun buildSessionConfig(
taskOrigin: TaskOrigin,
preferences: EnginePreferences,
) = OonimkallBridge.SessionConfig(
Expand All @@ -208,6 +209,7 @@ class Engine(
tunnelDir = "$baseFilePath/tunnel",
tempDir = cacheDir,
assetsDir = "$baseFilePath/assets",
geoIpDB = preferences.geoipDbVersion?.let { "$cacheDir/$it.mmdb" },
logger = oonimkallLogger,
verbose = false,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ interface OonimkallBridge {
val proxy: String?,
val probeServicesURL: String?,
val assetsDir: String,
val geoIpDB: String?,
val stateDir: String,
val tempDir: String,
val tunnelDir: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,15 @@ class TaskEventMapper(
asn = value?.probeAsn,
ip = value?.probeIp,
countryCode = value?.probeCc,
geoIpdb = value?.geoIpdb,
networkType = networkTypeFinder(),
)

"status.resolver_lookup" -> value?.geoIpdb?.let {
println(it)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to keep this, then we should use Logger.

null
}

"status.measurement_done" ->
TaskEvent.MeasurementDone(index = value?.idx ?: 0)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ data class EnginePreferences(
val taskLogLevel: TaskLogLevel,
val uploadResults: Boolean,
val proxy: String?,
val geoipDbVersion: String?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we also need to tell the engine what version of the Db?

val maxRuntime: Duration?,
)
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ sealed interface TaskEvent {
val ip: String?,
val asn: String?,
val countryCode: String?,
val geoIpdb: String?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to get the geoIpDb from the engine?

val networkType: NetworkType,
) : TaskEvent

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ data class TaskEventResult(
var idx: Int = 0,
@SerialName("report_id")
var reportId: String? = null,
@SerialName("geoip_db")
var geoIpdb: String? = null,
@SerialName("probe_ip")
var probeIp: String? = null,
@SerialName("probe_asn")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ data class TaskSettings(
@SerialName("temp_dir") val tempDir: String,
@SerialName("tunnel_dir") val tunnelDir: String,
@SerialName("assets_dir") val assetsDir: String,
@SerialName("geoip_db") val geoIpDB: String?,
@SerialName("options") val options: Options,
@SerialName("annotations") val annotations: Annotations,
// Optional
Expand Down
4 changes: 4 additions & 0 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/App.kt
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ fun App(
// ForegroundServiceDidNotStartInTimeException some users are getting
// dependencies.startSingleRunInner(RunSpecification.OnlyUploadMissingResults)
}
LaunchedEffect(Unit) {
// Check for GeoIP DB updates in the background
runCatching { dependencies.fetchGeoIpDbUpdates() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need runCatching if we're already try/catching inside the method. I think we just need to log the failures, but that should be going inside the method.

}
LaunchedEffect(Unit) {
dependencies.observeAndConfigureAutoUpdate()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ enum class SettingsKey(
DELETE_UPLOADED_JSONS("deleteUploadedJsons"),
IS_NOTIFICATION_DIALOG("isNotificationDialog"),
FIRST_RUN("first_run"),
MMDB_VERSION("mmdb_version"),
MMDB_LAST_CHECK("mmdb_last_check"),
CHOSEN_WEBSITES("chosen_websites"),
DESCRIPTOR_SECTIONS_COLLAPSED("descriptor_sections_collapsed"),

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,13 @@ class PreferenceRepository(
): PreferenceKey<*> {
val preferenceKey = getPreferenceKey(name = key.value, prefix = prefix, autoRun = autoRun)
return when (key) {
SettingsKey.MMDB_LAST_CHECK,
SettingsKey.MAX_RUNTIME,
SettingsKey.LEGACY_PROXY_PORT,
SettingsKey.DELETE_OLD_RESULTS_THRESHOLD,
-> PreferenceKey.IntKey(intPreferencesKey(preferenceKey))

SettingsKey.MMDB_VERSION,
SettingsKey.LEGACY_PROXY_HOSTNAME,
SettingsKey.LEGACY_PROXY_PROTOCOL,
SettingsKey.PROXY_SELECTED,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import org.ooni.engine.Engine
import org.ooni.engine.NetworkTypeFinder
import org.ooni.engine.OonimkallBridge
import org.ooni.engine.TaskEventMapper
import org.ooni.probe.net.httpGetBytes
import org.ooni.probe.Database
import org.ooni.probe.background.RunBackgroundTask
import org.ooni.probe.config.BatteryOptimization
Expand Down Expand Up @@ -50,7 +51,9 @@ import org.ooni.probe.domain.ClearStorage
import org.ooni.probe.domain.DeleteMeasurementsWithoutResult
import org.ooni.probe.domain.DeleteOldResults
import org.ooni.probe.domain.DeleteResults
import org.ooni.probe.domain.DownloadFile
import org.ooni.probe.domain.DownloadUrls
import org.ooni.probe.domain.FetchGeoIpDbUpdates
import org.ooni.probe.domain.FinishInProgressData
import org.ooni.probe.domain.GetAutoRunSettings
import org.ooni.probe.domain.GetAutoRunSpecification
Expand Down Expand Up @@ -116,6 +119,7 @@ import org.ooni.probe.ui.settings.proxy.ProxyViewModel
import org.ooni.probe.ui.settings.webcategories.WebCategoriesViewModel
import org.ooni.probe.ui.upload.UploadMeasurementsViewModel
import kotlin.coroutines.CoroutineContext
import kotlin.getValue

class Dependencies(
val platformInfo: PlatformInfo,
Expand Down Expand Up @@ -201,9 +205,25 @@ class Dependencies(
}

// Engine

private val taskEventMapper by lazy { TaskEventMapper(networkTypeFinder, json) }

private val downloader by lazy {
DownloadFile(
fileSystem = FileSystem.SYSTEM,
fetchBytes = { url -> httpGetBytes(url) },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as fetchBytes = ::httpGetBytes,.

)
}

val fetchGeoIpDbUpdates by lazy {
FetchGeoIpDbUpdates(
downloadFile = downloader::invoke,
cacheDir = cacheDir,
engineHttpDo = engine::httpDo,
json = json,
preferencesRepository = preferenceRepository,
)
}

@VisibleForTesting
val engine by lazy {
Engine(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package org.ooni.probe.domain

import okio.FileSystem
import okio.Path
import okio.Path.Companion.toPath
import okio.buffer
import okio.use

/**
* Downloads binary content to a target absolute path using the provided fetcher.
* - Creates parent directories if needed
* - Skips writing if the target already exists with the same size
*/
class DownloadFile(
private val fileSystem: FileSystem,
private val fetchBytes: suspend (url: String) -> ByteArray,
) {
suspend operator fun invoke(
url: String,
absoluteTargetPath: String,
): Path {
val target = absoluteTargetPath.toPath()
target.parent?.let { parent ->
if (fileSystem.metadataOrNull(parent) == null) fileSystem.createDirectories(parent)
}
val bytes = fetchBytes(url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no error handling here. fetchBytes should be returning a Result, and this should be returning one as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also be receiving a stream from the HTTP call, and be storing it straight away in a file, instead of keeping it all in memory. For a 15MB file it's not a huge deal, but it would be the optimal way.

val existing = fileSystem.metadataOrNull(target)
if (existing?.size == bytes.size.toLong()) return target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store if it's the same size? Couldn't it be a newer version, but with the same size?

fileSystem.sink(target).buffer().use { sink -> sink.write(bytes) }
return target
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package org.ooni.probe.domain

import kotlinx.coroutines.flow.first
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import kotlinx.serialization.json.Json
import okio.Path
import ooniprobe.composeapp.generated.resources.Res
import ooniprobe.composeapp.generated.resources.engine_mmdb_version
import org.jetbrains.compose.resources.getString
import org.ooni.engine.Engine
import org.ooni.engine.models.Failure
import org.ooni.engine.models.Result
import org.ooni.engine.models.Success
import org.ooni.engine.models.TaskOrigin
import org.ooni.probe.data.models.SettingsKey
import org.ooni.probe.data.repositories.PreferenceRepository
import kotlin.time.Clock

class FetchGeoIpDbUpdates(
private val downloadFile: suspend (url: String, absoluteTargetPath: String) -> Path,
private val cacheDir: String,
private val engineHttpDo: suspend (method: String, url: String, taskOrigin: TaskOrigin) -> Result<String?, Engine.MkException>,
private val preferencesRepository: PreferenceRepository,
private val json: Json,
) {
suspend operator fun invoke(): Result<Path?, Engine.MkException> =
try {
when (val versionRes = getLatestEngineVersion()) {
is Failure -> Failure(versionRes.reason)
is Success -> {
val (isLatest, _, latestVersion) = isGeoIpDbLatest(versionRes.value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're ignoring the currentVersion why return it?

if (isLatest) {
Success(null)
} else {
val versionName = latestVersion
val url = buildGeoIpDbUrl(versionName)
val target = "$cacheDir/$versionName.mmdb"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you should be using the Okio resolve to construct file paths, to make sure it's platform independent.


downloadFile(url, target).let {
preferencesRepository.setValueByKey(SettingsKey.MMDB_VERSION, versionName)
preferencesRepository.setValueByKey(SettingsKey.MMDB_LAST_CHECK, Clock.System.now().toEpochMilliseconds())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're storing this but never using it? Should we be checking this before even checking for new releases?

Success(it)
}
}
}
}
} catch (t: Throwable) {
Failure(t as? Engine.MkException ?: Engine.MkException(t))
}

/**
* Compare latest and current version integers and return pair of latest state and actual version number
* @return Triple<Boolean, String, String> where the first element is true if the DB is the latest,
* the second is the current version and the third is the latest version.
*/
private suspend fun isGeoIpDbLatest(latestVersion: String): Triple<Boolean, String, String> {
val currentGeoIpDbVersion: String =
(preferencesRepository.getValueByKey(SettingsKey.MMDB_VERSION).first() ?: getString(Res.string.engine_mmdb_version)) as String

return Triple(normalize(currentGeoIpDbVersion) >= normalize(latestVersion), currentGeoIpDbVersion, latestVersion)
}

private suspend fun getLatestEngineVersion(): Result<String, Engine.MkException> {
val url = "https://api.0.github.com/repos/aanorbel/oomplt-mmdb/releases/latest"

return engineHttpDo("GET", url, TaskOrigin.OoniRun).map { payload ->
val jsonStr = payload ?: throw Engine.MkException(Throwable("Empty body"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing from inside a Result.map does not turn the Result into a Result.Failure. Instead of map, just store the result val result = engineHttpDo("GET", url, TaskOrigin.OoniRun) and return a new Result from this method.

json.decodeFromString(GhRelease.serializer(), jsonStr).tag
}
}

private fun buildGeoIpDbUrl(version: String): String =
"https://github.com/aanorbel/oomplt-mmdb/releases/download/$version/$version-ip2country_as.mmdb"

private fun normalize(tag: String): Int = tag.removePrefix("v").trim().toInt()

@Serializable
data class GhRelease(
@SerialName("tag_name") val tag: String,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class GetEnginePreferences(
null
},
proxy = getProxyOption().first().value,
geoipDbVersion = getValueForKey(SettingsKey.MMDB_VERSION) as String?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should already be providing the DB path here, instead of having to construct it in multiple places.

)

private suspend fun getEnabledCategories(): List<String> {
Expand Down
7 changes: 7 additions & 0 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/net/Http.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package org.ooni.probe.net

/**
* Perform a simple HTTP GET and return the raw response body bytes.
* Implemented per-platform to ensure binary-safe downloads.
*/
expect suspend fun httpGetBytes(url: String): ByteArray
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't trust the engine httpDo to fetch a larger file? And you preferred not to use a library like ktor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this should return a Result<ByteArray, ...>.

Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ class DesktopOonimkallBridge : OonimkallBridge {
it.softwareVersion = softwareVersion

it.assetsDir = assetsDir
// geoipDB may or may not exist in this binding; set via reflection when available
geoIpDB?.let { path ->
// it.geoipDB = path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No desktop support from the start?

}
it.stateDir = stateDir
it.tempDir = tempDir
it.tunnelDir = tunnelDir
Expand Down
Loading
Loading