From a5db5604d36f2c59c75424350c62178975da796d Mon Sep 17 00:00:00 2001 From: Nils Stinnesbeck Date: Sun, 18 Oct 2026 00:36:19 +0200 Subject: [PATCH] Code review fixes: app 2.9.0, server 2.2.0 Telemetry no longer loses queued batches. The server drops a bad value, or a sample without a plausible time, instead of rejecting the batch, and answers 200 with what it dropped. The app sends only values inside the server's ranges (speed as a magnitude, so reversing is fine) and no samples before the head unit's clock is set. Server: - Pairing hands out and accepts a token only after the device is saved; a failed save leaves the request pending. - A paired token serves only the installation id it was paired with (403 otherwise); the shared INGEST_TOKEN still serves any. - Wrong-password lockouts and open pairing requests are limited per client address (last X-Forwarded-For entry), so a stranger cannot lock the owner out; a car starting again replaces its own request. - ABRP forwarding skips samples stamped ahead of the server's clock, which used to stop it until a restart. App: - Settings written to plaintext during a keystore failure move back into encrypted storage; the stored installation id wins. - The installation id is derived from ANDROID_ID on first start, so a reinstall finds its history; a stored or imported id still wins. - The scanned config file is deleted after import (it holds the token). - GPS stops while live telemetry is off or the server unconfigured, and pauses while parked and idle, keeping the fix the car parked with. - Range estimates use the monotonic clock. - One HTTP helper (ServerHttp) for telemetry, sync and pairing. - Last Turkish strings translated; mise logs shows the NilsMG4 tags. Grafana reads InfluxDB with its own read-only token (new grafana-influx Secret, created by hand as described in k8s/README.md). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QLJfZQZeAKAwSqViTmLzki --- README.md | 9 +- SPECS.md | 16 +++ app/build.gradle | 4 +- .../com/evsuite/abrp/AbrpUploadService.java | 5 +- .../evsuite/abrp/ConsumptionCloudClient.java | 133 +++++------------- .../java/com/evsuite/abrp/MainActivity.java | 33 +++-- .../java/com/evsuite/abrp/PairingClient.java | 51 +------ .../java/com/evsuite/abrp/RangeEstimator.java | 4 +- .../java/com/evsuite/abrp/SecurePrefs.java | 53 ++++++- .../java/com/evsuite/abrp/ServerConfig.java | 33 ++++- .../java/com/evsuite/abrp/ServerHttp.java | 87 ++++++++++++ .../com/evsuite/abrp/TelemetryPayload.java | 56 +++++++- .../com/evsuite/abrp/TelemetryUploader.java | 86 ++++++----- .../com/evsuite/abrp/WifiAutoConnect.java | 4 +- app/src/main/res/values/strings.xml | 2 + .../com/evsuite/abrp/SecurePrefsTest.java | 40 ++++++ .../com/evsuite/abrp/ServerConfigTest.java | 17 +++ .../evsuite/abrp/TelemetryPayloadTest.java | 41 ++++++ .../evsuite/abrp/TelemetryUploaderTest.java | 19 +++ k8s/README.md | 30 +++- k8s/grafana.yaml | 6 +- k8s/secrets/secrets.example.yaml | 11 ++ k8s/server.yaml | 4 +- mise.toml | 2 +- server/README.md | 20 ++- server/abrp.go | 19 +-- server/api.go | 94 ++++++++++--- server/api_test.go | 25 +++- server/logic_test.go | 12 ++ server/model.go | 2 + server/pairing.go | 127 ++++++++++++----- server/pairing_http.go | 27 +++- server/pairing_test.go | 103 +++++++++++++- server/validate.go | 56 +++++--- server/validate_test.go | 54 ++++--- 35 files changed, 962 insertions(+), 323 deletions(-) create mode 100644 app/src/main/java/com/evsuite/abrp/ServerHttp.java create mode 100644 app/src/test/java/com/evsuite/abrp/SecurePrefsTest.java create mode 100644 app/src/test/java/com/evsuite/abrp/TelemetryUploaderTest.java diff --git a/README.md b/README.md index 6b1084f..18a0a39 100644 --- a/README.md +++ b/README.md @@ -131,13 +131,18 @@ out keep their current value: ``` server_url = https://car.k8s.stinnesbeck.com token = the server's INGEST_TOKEN (not needed after pairing) -installation_id = keep the old one when reinstalling, so history stays together +installation_id = only to carry over history from an install older than 2.9.0 interval_sec = 60 # 15 / 30 / 60 / 120 / 300 boost_low_soc = true low_soc_percent = 20 ``` -The file is read once and not kept; credentials live only in the app's encrypted store. +After a successful import the app deletes the file, since it holds the token; credentials live +only in the app's encrypted store. + +The installation ID is derived from the head unit, so a reinstall finds its history again on its +own. `installation_id` is only needed to carry history over from an install made before 2.9.0. +A paired token belongs to one installation ID: after changing the ID, pair again. ### Power diff --git a/SPECS.md b/SPECS.md index 5bb8cc2..433bd62 100644 --- a/SPECS.md +++ b/SPECS.md @@ -112,6 +112,22 @@ tyre pressures, GPS (lat, lon, elevation, heading) and the is_* flags. It does * INGEST_TOKEN) into git and pushed it. Commit amended and force-pushed; both values regenerated in the now gitignored `k8s/secrets.yaml` (since moved to `k8s/secrets/secrets.yaml`, outside `kubectl apply -f k8s/`). Filled-in secrets live only there from now on. +- 2026-10-18 — Code review fixes. App 2.9.0, server 2.2.0, Grafana token: + - The server drops a bad value (or a sample without a plausible time) instead of rejecting + the batch, and answers 200 with what it dropped; the app sends only values within the + server's ranges (speed as a magnitude) and no samples before its clock is set. Before, one + reversing sample cost the whole queued batch. + - Pairing: the token is handed out and accepted only after the device is saved; wrong-password + lockouts and open-request limits are per client address; a paired token only serves its + own installation id (403 otherwise). + - ABRP forwarding skips samples stamped ahead of the server's clock, which used to block it. + - App: settings written in plaintext during a keystore failure move back into encrypted + storage; the installation id is derived from ANDROID_ID, so a reinstall keeps its history; + the imported config file is deleted; GPS stops when live telemetry is off and pauses while + parked and idle; range estimates use the monotonic clock; one HTTP helper (`ServerHttp`) + for all server calls; the last Turkish strings translated. + - Grafana reads InfluxDB with its own read-only token (`grafana-influx` Secret, created by + hand: `k8s/README.md`). - 2026-10-18 — README rewritten for Nils' MG4. `OTA.md`, `mise.toml` and `docs/MG4_PLATFORM_INSTALL.md` use the new package ID and `NilsMG4-*.apk` names; `OTA.md` notes that updates are switched off. `CHANGELOG.md` keeps the old ID as upstream history. diff --git a/app/build.gradle b/app/build.gradle index 30a0a16..3abff06 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -34,8 +34,8 @@ android { applicationId "com.stinnesbeck.nilsmg4" minSdk 27 targetSdk 36 - versionCode 68 - versionName "2.8.2" + versionCode 69 + versionName "2.9.0" // Nothing about the server ships in the APK: its URL and ingest token are entered in // the app's Server tab (ServerConfig), and the database is only reachable by the server. diff --git a/app/src/main/java/com/evsuite/abrp/AbrpUploadService.java b/app/src/main/java/com/evsuite/abrp/AbrpUploadService.java index ec9e2ae..be5cb34 100644 --- a/app/src/main/java/com/evsuite/abrp/AbrpUploadService.java +++ b/app/src/main/java/com/evsuite/abrp/AbrpUploadService.java @@ -160,7 +160,8 @@ public class AbrpUploadService extends Service { long sampleMs = System.currentTimeMillis(); EnergySnapshot vehicle = energyReader.read(sampleMs); // Keeps the 5-minute range average going while the app is in the background. - RangeEstimator.shared().add(sampleMs, vehicle.getBatteryPowerKw(), vehicle.getSpeedKmh()); + RangeEstimator.shared().add(android.os.SystemClock.elapsedRealtime(), + vehicle.getBatteryPowerKw(), vehicle.getSpeedKmh()); consumptionTracker.sample(vehicle); chargeSessionTracker.sample(vehicle); consumptionCloudClient.syncIfDue(consumptionTracker, chargeSessionTracker, sampleMs); @@ -173,8 +174,10 @@ public class AbrpUploadService extends Service { UploadSettings settings = UploadSettings.from(prefs); if (!ServerConfig.get(this).isConfigured()) { + telemetryUploader.stop(); // no live telemetry, so no GPS either updateNotification(getString(R.string.server_not_configured)); } else if (!settings.abrpUploadEnabled) { + telemetryUploader.stop(); updateNotification(getString(R.string.notif_tracking_locally)); } else { telemetryUploader.tick(vehicle, chargeSessionTracker.snapshot(), settings); diff --git a/app/src/main/java/com/evsuite/abrp/ConsumptionCloudClient.java b/app/src/main/java/com/evsuite/abrp/ConsumptionCloudClient.java index 0ac0a0e..bf217d5 100644 --- a/app/src/main/java/com/evsuite/abrp/ConsumptionCloudClient.java +++ b/app/src/main/java/com/evsuite/abrp/ConsumptionCloudClient.java @@ -7,13 +7,6 @@ import android.util.Log; import org.json.JSONArray; import org.json.JSONObject; -import java.io.BufferedReader; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.OutputStream; -import java.net.HttpURLConnection; -import java.net.URL; -import java.nio.charset.StandardCharsets; import java.time.LocalDate; import java.util.ArrayList; import java.util.Collections; @@ -23,8 +16,9 @@ import java.util.List; * Best-effort sync to the car server. Never blocks local persistence. * * Server URL, token and installation id come from {@link ServerConfig}, read on every use so - * a change in the Server tab applies at once. The installation id is kept across updates; a - * reinstall gets it back through the config file, so the history is not orphaned under a new + * a change in the Server tab applies at once. The installation id is kept across updates, and + * a reinstall derives the same one from the head unit ({@link ServerConfig#installationIdFor}) + * or gets it back through the config file, so the history is not orphaned under a new * identity. The consumption UI reads back its own uploaded totals via * {@link #fetchHistoryIfDue}, so a reinstall picks up where the old one left off. */ @@ -33,7 +27,6 @@ final class ConsumptionCloudClient { private static final long CONSUMPTION_SYNC_INTERVAL_MS = 2 * 60_000L; private static final long CHARGE_RETRY_INTERVAL_MS = 5 * 60_000L; private static final long HISTORY_FETCH_INTERVAL_MS = 5 * 60_000L; - private static final int TIMEOUT_MS = 8_000; private static final String PREFS = "consumption_cloud"; private static final String CHUNK_CACHE = "chunk_cache"; // JSON array of pending chunks private static final int MAX_CHARGES_PER_SYNC = 10; @@ -244,36 +237,20 @@ final class ConsumptionCloudClient { } private boolean send(String path, String method, JSONObject body) throws Exception { - HttpURLConnection connection = null; + String url = trimSlash(config().url) + path; + Log.i(TAG, "Sending " + method + " request to " + url); try { - byte[] bytes = body.toString().getBytes(StandardCharsets.UTF_8); - String url = trimSlash(config().url) + path; - Log.i(TAG, "Sending " + method + " request to " + url + " (" + bytes.length + " bytes)"); - Log.i(TAG, "Body: " + body.toString()); - connection = (HttpURLConnection) new URL(url).openConnection(); - connection.setRequestMethod(method); - connection.setConnectTimeout(TIMEOUT_MS); - connection.setReadTimeout(TIMEOUT_MS); - connection.setDoOutput(true); - connection.setFixedLengthStreamingMode(bytes.length); - connection.setRequestProperty("Content-Type", "application/json"); - connection.setRequestProperty("Authorization", "Bearer " + config().token); - try (OutputStream out = connection.getOutputStream()) { out.write(bytes); } - int status = connection.getResponseCode(); - Log.i(TAG, "Response: HTTP " + status); - if (status < 200 || status >= 300) { - String errBody = readErrorBody(connection); - Log.w(TAG, "Sync rejected with HTTP " + status + (errBody.isEmpty() ? "" : ": " + errBody)); + ServerHttp.Response response = ServerHttp.request(method, url, config().token, body); + Log.i(TAG, "Response: HTTP " + response.status); + if (!response.ok()) { + String err = response.excerpt(); + Log.w(TAG, "Sync rejected with HTTP " + response.status + (err.isEmpty() ? "" : ": " + err)); return false; } - InputStream response = connection.getInputStream(); - if (response != null) try (InputStream ignored = response) { } return true; } catch (Exception e) { Log.w(TAG, "Network error: " + e.getMessage()); throw e; - } finally { - if (connection != null) connection.disconnect(); } } @@ -352,43 +329,32 @@ final class ConsumptionCloudClient { } private List fetchHistory() throws Exception { - HttpURLConnection connection = null; - try { - String url = trimSlash(config().url) + "/v1/consumption/history/" + config().installationId; - connection = (HttpURLConnection) new URL(url).openConnection(); - connection.setRequestMethod("GET"); - connection.setConnectTimeout(TIMEOUT_MS); - connection.setReadTimeout(TIMEOUT_MS); - connection.setRequestProperty("Authorization", "Bearer " + config().token); - int status = connection.getResponseCode(); - Log.i(TAG, "History GET response: HTTP " + status); - if (status < 200 || status >= 300) { - String err = readErrorBody(connection); - Log.w(TAG, "History fetch rejected with HTTP " + status + (err.isEmpty() ? "" : ": " + err)); - logEvent(false, status, "History GET HTTP " + status + (err.isEmpty() ? "" : " — " + err)); - return cachedHistory; - } - String body = readBody(connection); - JSONObject json = new JSONObject(body); - JSONArray records = json.optJSONArray("records"); - if (records == null) return Collections.emptyList(); - // Lifetime is derived by the backend (sum of all days), sent once at the response - // level. Each DayRecord carries it so the UI's LIFETIME period can read it. - ConsumptionTracker.Totals lifetime = json.has("lifetime") && !json.isNull("lifetime") - ? parseTotals(json.getJSONObject("lifetime")) : null; - List result = new ArrayList<>(records.length()); - for (int i = 0; i < records.length(); i++) { - JSONObject r = records.getJSONObject(i); - LocalDate date = LocalDate.parse(r.getString("date")); - ConsumptionTracker.Totals day = parseTotals(r.getJSONObject("day")); - Float soh = r.has("sohPercent") && !r.isNull("sohPercent") - ? (float) r.getDouble("sohPercent") : null; - result.add(new DayRecord(date, day, lifetime, soh)); - } - return result; - } finally { - if (connection != null) connection.disconnect(); + String url = trimSlash(config().url) + "/v1/consumption/history/" + config().installationId; + ServerHttp.Response response = ServerHttp.request("GET", url, config().token, null); + Log.i(TAG, "History GET response: HTTP " + response.status); + if (!response.ok()) { + String err = response.excerpt(); + Log.w(TAG, "History fetch rejected with HTTP " + response.status + (err.isEmpty() ? "" : ": " + err)); + logEvent(false, response.status, "History GET HTTP " + response.status + (err.isEmpty() ? "" : " — " + err)); + return cachedHistory; } + JSONObject json = new JSONObject(response.body); + JSONArray records = json.optJSONArray("records"); + if (records == null) return Collections.emptyList(); + // Lifetime is derived by the backend (sum of all days), sent once at the response + // level. Each DayRecord carries it so the UI's LIFETIME period can read it. + ConsumptionTracker.Totals lifetime = json.has("lifetime") && !json.isNull("lifetime") + ? parseTotals(json.getJSONObject("lifetime")) : null; + List result = new ArrayList<>(records.length()); + for (int i = 0; i < records.length(); i++) { + JSONObject r = records.getJSONObject(i); + LocalDate date = LocalDate.parse(r.getString("date")); + ConsumptionTracker.Totals day = parseTotals(r.getJSONObject("day")); + Float soh = r.has("sohPercent") && !r.isNull("sohPercent") + ? (float) r.getDouble("sohPercent") : null; + result.add(new DayRecord(date, day, lifetime, soh)); + } + return result; } private static ConsumptionTracker.Totals parseTotals(JSONObject o) { @@ -397,35 +363,6 @@ final class ConsumptionCloudClient { o.optDouble("hours", 0d), o.optDouble("socDrop", 0d)); } - private static String readBody(HttpURLConnection connection) throws Exception { - InputStream stream = connection.getInputStream(); - try (BufferedReader br = new BufferedReader( - new InputStreamReader(stream, StandardCharsets.UTF_8))) { - StringBuilder sb = new StringBuilder(); - String line; - while ((line = br.readLine()) != null) sb.append(line); - return sb.toString(); - } - } - - /** Reads the error response body (truncated) for logging — never throws. */ - private static String readErrorBody(HttpURLConnection connection) { - try { - InputStream stream = connection.getErrorStream(); - if (stream == null) return ""; - try (BufferedReader br = new BufferedReader( - new InputStreamReader(stream, StandardCharsets.UTF_8))) { - StringBuilder sb = new StringBuilder(); - String line; - while ((line = br.readLine()) != null) sb.append(line); - String body = sb.toString(); - return body.length() > 300 ? body.substring(0, 300) + "…" : body; - } - } catch (Exception e) { - return ""; - } - } - private static String trimSlash(String value) { int end = value.length(); while (end > 0 && value.charAt(end - 1) == '/') end--; diff --git a/app/src/main/java/com/evsuite/abrp/MainActivity.java b/app/src/main/java/com/evsuite/abrp/MainActivity.java index 6dc5165..1d27277 100644 --- a/app/src/main/java/com/evsuite/abrp/MainActivity.java +++ b/app/src/main/java/com/evsuite/abrp/MainActivity.java @@ -774,13 +774,28 @@ public class MainActivity extends AppCompatActivity { return dirs.length > 0 && dirs[0] != null ? dirs[0].getAbsolutePath() : ""; } + /** + * Imports a scanned file and then deletes it: it holds the ingest token in plaintext, maybe + * on a USB stick, and the settings live in encrypted storage from now on. + */ private void importConfig(java.io.File file) { String text = readText(file); if (text.isEmpty()) { setConnectionStatus(COLOR_ERROR, getString(R.string.import_err_read)); return; } - applyConfig(text); + if (!applyConfig(text)) return; + boolean deleted; + try { + deleted = file.delete(); + } catch (SecurityException e) { + deleted = false; + } + if (deleted) { + setConnectionStatus(COLOR_OK, getString(R.string.import_ok_deleted)); + } else { + setConnectionStatus(COLOR_ERROR, getString(R.string.import_ok_not_deleted, file.getAbsolutePath())); + } } private void importConfig(Uri uri) { @@ -800,11 +815,11 @@ public class MainActivity extends AppCompatActivity { * only the token does not wipe the URL. Server settings are validated and saved together; * cadence keys go to the plain prefs the same way the Upload page writes them. */ - private void applyConfig(String text) { + private boolean applyConfig(String text) { ConfigImport config = ConfigImport.parse(text); if (config.isEmpty()) { setConnectionStatus(COLOR_ERROR, getString(R.string.import_err_empty)); - return; + return false; } ServerConfig current = ServerConfig.get(this); if (config.serverUrl != null || config.token != null || config.installationId != null) { @@ -817,7 +832,7 @@ public class MainActivity extends AppCompatActivity { if (config.serverUrl != null) serverUrlInput.setText(config.serverUrl); if (config.token != null) serverTokenInput.setText(config.token); if (config.installationId != null) installationIdInput.setText(config.installationId); - return; + return false; } } SharedPreferences.Editor edit = prefs.edit(); @@ -828,6 +843,7 @@ public class MainActivity extends AppCompatActivity { bindCadenceControls(); setConnectionStatus(COLOR_OK, getString(R.string.import_ok)); refreshStatus(); + return true; } // ---------- Upload page ---------- @@ -993,15 +1009,16 @@ public class MainActivity extends AppCompatActivity { * The two range estimates. Both need the car to be moving on battery power; the average * also needs a minute or more of driving, which the service has usually collected already. */ - private void refreshRangeEstimates(EnergySnapshot s, long nowMs) { + private void refreshRangeEstimates(EnergySnapshot s) { RangeEstimator estimator = RangeEstimator.shared(); - estimator.add(nowMs, s.getBatteryPowerKw(), s.getSpeedKmh()); + long monotonicMs = android.os.SystemClock.elapsedRealtime(); + estimator.add(monotonicMs, s.getBatteryPowerKw(), s.getSpeedKmh()); Double remaining = RangeEstimator.remainingKwh(s.getSocPercent(), usableKwh()); showEstimate(vehRangePower, vehRangePowerNote, RangeEstimator.instant(remaining, s.getBatteryPowerKw(), s.getSpeedKmh()), R.string.range_not_driving); showEstimate(vehRangeAverage, vehRangeAverageNote, - estimator.average(remaining, nowMs), R.string.range_waiting); + estimator.average(remaining, monotonicMs), R.string.range_waiting); } private void showEstimate(TextView value, TextView note, RangeEstimator.Estimate e, int whyNot) { @@ -1041,7 +1058,7 @@ public class MainActivity extends AppCompatActivity { vehHvac.setText(fmt(s.getClimate().getDriverTargetCelsius(), "%.0f °C")); vehCharging.setText(chargingLabel(s)); - refreshRangeEstimates(s, System.currentTimeMillis()); + refreshRangeEstimates(s); vehTireFl.setText(fmt(s.getTirePressures().getFrontLeftKpa(), "%.0f kPa")); vehTireFr.setText(fmt(s.getTirePressures().getFrontRightKpa(), "%.0f kPa")); diff --git a/app/src/main/java/com/evsuite/abrp/PairingClient.java b/app/src/main/java/com/evsuite/abrp/PairingClient.java index e349dca..8990af1 100644 --- a/app/src/main/java/com/evsuite/abrp/PairingClient.java +++ b/app/src/main/java/com/evsuite/abrp/PairingClient.java @@ -2,12 +2,6 @@ package com.evsuite.abrp; import org.json.JSONObject; -import java.io.InputStream; -import java.io.OutputStream; -import java.net.HttpURLConnection; -import java.net.URL; -import java.nio.charset.StandardCharsets; - /** * The car's side of pairing with the server (an OAuth device-authorization flow): ask for a * pairing request, show its code as a QR code, then poll until a phone has approved it and @@ -15,8 +9,6 @@ import java.nio.charset.StandardCharsets; */ final class PairingClient { - private static final int TIMEOUT_MS = 8_000; - /** A pairing request as the server opened it. */ static final class Request { final String deviceCode; @@ -68,7 +60,7 @@ final class PairingClient { private PairingClient() { } static Request start(String serverUrl, String installationId) throws Exception { - Response r = post(serverUrl + "/v1/pair/start", + ServerHttp.Response r = post(serverUrl + "/v1/pair/start", new JSONObject().put("installationId", installationId)); // Only the server's own answer means "switched off". A 404 without it is a server (or // proxy) that has no pairing route at all, e.g. one older than 2.1.0. @@ -86,7 +78,7 @@ final class PairingClient { } static PollResult poll(String serverUrl, String deviceCode) throws Exception { - Response r = post(serverUrl + "/v1/pair/poll", new JSONObject().put("deviceCode", deviceCode)); + ServerHttp.Response r = post(serverUrl + "/v1/pair/poll", new JSONObject().put("deviceCode", deviceCode)); switch (r.status) { case 200: return new PollResult(Outcome.APPROVED, new JSONObject(r.body).getString("token")); case 202: return new PollResult(Outcome.PENDING, null); @@ -96,42 +88,7 @@ final class PairingClient { } } - private static final class Response { - final int status; - final String body; - - Response(int status, String body) { - this.status = status; - this.body = body; - } - } - - private static Response post(String url, JSONObject json) throws Exception { - byte[] body = json.toString().getBytes(StandardCharsets.UTF_8); - HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection(); - try { - connection.setRequestMethod("POST"); - connection.setConnectTimeout(TIMEOUT_MS); - connection.setReadTimeout(TIMEOUT_MS); - connection.setDoOutput(true); - connection.setFixedLengthStreamingMode(body.length); - connection.setRequestProperty("Content-Type", "application/json"); - try (OutputStream out = connection.getOutputStream()) { out.write(body); } - int status = connection.getResponseCode(); - InputStream in = status < 400 ? connection.getInputStream() : connection.getErrorStream(); - String text = ""; - if (in != null) { - try (InputStream stream = in) { - java.io.ByteArrayOutputStream buf = new java.io.ByteArrayOutputStream(); - byte[] chunk = new byte[2048]; - int n; - while ((n = stream.read(chunk)) != -1 && buf.size() < 16 * 1024) buf.write(chunk, 0, n); - text = buf.toString(StandardCharsets.UTF_8.name()); - } - } - return new Response(status, text); - } finally { - connection.disconnect(); - } + private static ServerHttp.Response post(String url, JSONObject json) throws Exception { + return ServerHttp.request("POST", url, null, json); } } diff --git a/app/src/main/java/com/evsuite/abrp/RangeEstimator.java b/app/src/main/java/com/evsuite/abrp/RangeEstimator.java index 120db9a..351bd1f 100644 --- a/app/src/main/java/com/evsuite/abrp/RangeEstimator.java +++ b/app/src/main/java/com/evsuite/abrp/RangeEstimator.java @@ -14,7 +14,9 @@ import java.util.Iterator; * regenerating, too little data — rather than a misleading number. * * Fed by both the service tick (15 s) and the open Vehicle page (2 s); the integration is - * time-weighted, so the two rates mix correctly. Free of Android types, unit-tested. + * time-weighted, so the two rates mix correctly. Times are on the monotonic clock + * (SystemClock.elapsedRealtime): the head unit sets its wall clock from GPS, and a jump back + * would make every new sample look out of order. Free of Android types, unit-tested. */ final class RangeEstimator { diff --git a/app/src/main/java/com/evsuite/abrp/SecurePrefs.java b/app/src/main/java/com/evsuite/abrp/SecurePrefs.java index 3d51282..d4ced58 100644 --- a/app/src/main/java/com/evsuite/abrp/SecurePrefs.java +++ b/app/src/main/java/com/evsuite/abrp/SecurePrefs.java @@ -7,10 +7,18 @@ import android.util.Log; import androidx.security.crypto.EncryptedSharedPreferences; import androidx.security.crypto.MasterKey; +import java.util.HashMap; +import java.util.Map; + /** * Encrypted storage for the server settings — above all the ingest token, which `adb backup` * would otherwise hand out with the regular SharedPreferences file. Backed by an * AndroidKeyStore master key. + * + * If the keystore fails, the settings go to a plaintext file for that run rather than the app + * crashing on the head unit. The next run that opens the encrypted store moves them back in + * and deletes the file ({@link #mergePlaintext}), so a token saved meanwhile is neither lost + * nor left in plaintext. */ final class SecurePrefs { @@ -33,16 +41,59 @@ final class SecurePrefs { MasterKey masterKey = new MasterKey.Builder(app) .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) .build(); - instance = EncryptedSharedPreferences.create( + SharedPreferences encrypted = EncryptedSharedPreferences.create( app, FILE_NAME, masterKey, EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM); + migrateFromPlaintext(app, encrypted); + instance = encrypted; } catch (Exception e) { Log.e(TAG, "Encrypted preferences unavailable, falling back to plaintext", e); instance = app.getSharedPreferences(PLAIN_NAME, Context.MODE_PRIVATE); } return instance; } + + /** Moves what a fallback run wrote into the encrypted store, then deletes the plaintext. */ + private static void migrateFromPlaintext(Context app, SharedPreferences encrypted) { + SharedPreferences plain = app.getSharedPreferences(PLAIN_NAME, Context.MODE_PRIVATE); + Map leftover = plain.getAll(); + if (leftover.isEmpty()) return; + Map moves = mergePlaintext(strings(leftover), strings(encrypted.getAll())); + SharedPreferences.Editor edit = encrypted.edit(); + for (Map.Entry e : moves.entrySet()) edit.putString(e.getKey(), e.getValue()); + if (edit.commit()) { + plain.edit().clear().commit(); + Log.i(TAG, "Moved " + moves.size() + " setting(s) from plaintext into encrypted storage"); + } + } + + /** + * What to write into the encrypted store from a fallback run's plaintext file. The plaintext + * is newer, so its URL and token win — that is a pairing done while the keystore was down. + * The installation id is the exception: the encrypted one is what the car's history is + * stored under, and a fallback run may only have made up a new one. + */ + static Map mergePlaintext(Map plain, Map encrypted) { + Map moves = new HashMap<>(); + for (Map.Entry e : plain.entrySet()) { + if (e.getValue() == null) continue; + String existing = encrypted.get(e.getKey()); + if (ServerConfig.KEY_INSTALLATION_ID.equals(e.getKey()) && existing != null && !existing.isEmpty()) { + continue; + } + moves.put(e.getKey(), e.getValue()); + } + return moves; + } + + private static Map strings(Map all) { + Map out = new HashMap<>(); + for (Map.Entry e : all.entrySet()) { + if (e.getValue() instanceof String) out.put(e.getKey(), (String) e.getValue()); + } + return out; + } } diff --git a/app/src/main/java/com/evsuite/abrp/ServerConfig.java b/app/src/main/java/com/evsuite/abrp/ServerConfig.java index 0f9fa2e..7a94d42 100644 --- a/app/src/main/java/com/evsuite/abrp/ServerConfig.java +++ b/app/src/main/java/com/evsuite/abrp/ServerConfig.java @@ -49,6 +49,9 @@ final class ServerConfig { return !url.isEmpty() && !token.isEmpty(); } + // ANDROID_ID on purpose: it is hashed into the installation id, so a reinstall keeps its + // history (installationIdFor). It is never sent or logged as it is. + @android.annotation.SuppressLint("HardwareIds") static ServerConfig get(Context context) { ServerConfig config = current; if (config != null) return config; @@ -57,8 +60,18 @@ final class ServerConfig { SharedPreferences prefs = SecurePrefs.get(context); String id = prefs.getString(KEY_INSTALLATION_ID, ""); if (!isValidInstallationId(id)) { - // First start: this installation gets its own identity, kept from now on. - id = UUID.randomUUID().toString(); + // First start, or the first after a reinstall: derived from the head unit, so + // the history stays under the same id (see installationIdFor). + String androidId = null; + try { + androidId = android.provider.Settings.Secure.getString( + context.getApplicationContext().getContentResolver(), + android.provider.Settings.Secure.ANDROID_ID); + } catch (RuntimeException ignored) { + // no id from the platform: a random one below + } + id = installationIdFor(androidId); + if (id == null) id = UUID.randomUUID().toString(); prefs.edit().putString(KEY_INSTALLATION_ID, id).apply(); } current = new ServerConfig(prefs.getString(KEY_URL, DEFAULT_URL), prefs.getString(KEY_TOKEN, ""), id); @@ -97,6 +110,22 @@ final class ServerConfig { return token != null && token.trim().length() >= MIN_TOKEN_LENGTH && !token.trim().contains(" "); } + /** + * The installation id for a head unit, or null without a usable ANDROID_ID. From Android 8 + * ANDROID_ID is fixed per device and signing key and survives an uninstall, so a + * reinstall of the same platform-signed build finds its history again without importing + * the id. Hashed into a name-based UUID: the server expects a UUID, and the raw + * ANDROID_ID never leaves the car. An id already stored (or imported) always wins over it. + */ + static String installationIdFor(String androidId) { + if (androidId == null) return null; + String trimmed = androidId.trim(); + // The emulator-era constant that many devices once shared; not an identity. + if (trimmed.isEmpty() || trimmed.equalsIgnoreCase("9774d56d682e549c")) return null; + byte[] name = ("nilsmg4:" + trimmed.toLowerCase(Locale.US)).getBytes(java.nio.charset.StandardCharsets.UTF_8); + return UUID.nameUUIDFromBytes(name).toString(); + } + static boolean isValidInstallationId(String id) { return id != null && UUID_PATTERN.matcher(id.trim()).matches(); } diff --git a/app/src/main/java/com/evsuite/abrp/ServerHttp.java b/app/src/main/java/com/evsuite/abrp/ServerHttp.java new file mode 100644 index 0000000..1e9b9de --- /dev/null +++ b/app/src/main/java/com/evsuite/abrp/ServerHttp.java @@ -0,0 +1,87 @@ +package com.evsuite.abrp; + +import org.json.JSONObject; + +import java.io.ByteArrayOutputStream; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.HttpURLConnection; +import java.net.URL; +import java.nio.charset.StandardCharsets; + +/** + * One JSON request to the car server, shared by telemetry, consumption sync and pairing. + * Blocking, with short timeouts — call it off the main thread. + * + * The token goes in the Authorization header, never the URL, and is never logged. The answer + * body is read for every status (error bodies name the problem) and capped, so a wrong server + * cannot fill memory. + */ +final class ServerHttp { + + static final int TIMEOUT_MS = 8_000; + private static final int MAX_BODY_BYTES = 64 * 1024; + + /** Status and body of an answer. */ + static final class Response { + final int status; + final String body; + + Response(int status, String body) { + this.status = status; + this.body = body; + } + + boolean ok() { return status >= 200 && status < 300; } + + /** The body for a log line: short, on one line. */ + String excerpt() { + String text = body.replace('\n', ' ').trim(); + return text.length() > 300 ? text.substring(0, 300) + "…" : text; + } + } + + private ServerHttp() { } + + /** + * Sends {@code json} (or nothing, when null) with {@code method} to {@code url}. + * {@code token} may be null for the unauthenticated pairing calls. + */ + static Response request(String method, String url, String token, JSONObject json) throws Exception { + HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection(); + try { + connection.setRequestMethod(method); + connection.setConnectTimeout(TIMEOUT_MS); + connection.setReadTimeout(TIMEOUT_MS); + if (token != null) connection.setRequestProperty("Authorization", "Bearer " + token); + if (json != null) { + byte[] body = json.toString().getBytes(StandardCharsets.UTF_8); + connection.setDoOutput(true); + connection.setFixedLengthStreamingMode(body.length); + connection.setRequestProperty("Content-Type", "application/json"); + try (OutputStream out = connection.getOutputStream()) { out.write(body); } + } + int status = connection.getResponseCode(); + return new Response(status, readBody(connection, status)); + } finally { + connection.disconnect(); + } + } + + private static String readBody(HttpURLConnection connection, int status) { + try { + InputStream in = status < 400 ? connection.getInputStream() : connection.getErrorStream(); + if (in == null) return ""; + try (InputStream stream = in) { + ByteArrayOutputStream buf = new ByteArrayOutputStream(); + byte[] chunk = new byte[4096]; + int n; + while ((n = stream.read(chunk)) != -1 && buf.size() < MAX_BODY_BYTES) buf.write(chunk, 0, n); + return buf.toString(StandardCharsets.UTF_8.name()); + } + } catch (Exception e) { + // The status is what matters; an unreadable body is just empty. + return ""; + } + } +} diff --git a/app/src/main/java/com/evsuite/abrp/TelemetryPayload.java b/app/src/main/java/com/evsuite/abrp/TelemetryPayload.java index 2d80635..031431f 100644 --- a/app/src/main/java/com/evsuite/abrp/TelemetryPayload.java +++ b/app/src/main/java/com/evsuite/abrp/TelemetryPayload.java @@ -64,7 +64,8 @@ final class TelemetryPayload { tlm.put("utc", utc); putIfPresent(tlm, "soc", soc); - putIfPresent(tlm, "speed", speedKmh == null ? null : Math.round(speedKmh)); + // Reversing reads negative; ABRP and the server take speed as a magnitude. + putIfPresent(tlm, "speed", speedKmh == null ? null : Math.round(Math.abs(speedKmh))); putIfPresent(tlm, "est_battery_range", rangeKm); if (powerKw != null) tlm.put("power", round2(powerKw)); putIfPresent(tlm, "is_charging", boolToInt(charging)); @@ -134,9 +135,62 @@ final class TelemetryPayload { // already exclude. Returning just the timestamp keeps the upload alive. return "{\"utc\":" + utc + "}"; } + dropOutOfRange(tlm); return tlm.toString(); } + /** + * The server's range for every numeric field (server/model.go, telemetryNumbers). A value + * outside it is left out here, as an unread one would be: the server would drop it anyway, + * and an older server rejects the whole batch for it. + */ + static final java.util.Map SERVER_LIMITS = new java.util.HashMap<>(); + static { + SERVER_LIMITS.put("soc", new double[]{0, 100}); + SERVER_LIMITS.put("speed", new double[]{0, 400}); + SERVER_LIMITS.put("power", new double[]{-1000, 1000}); + SERVER_LIMITS.put("est_battery_range", new double[]{0, 2000}); + SERVER_LIMITS.put("capacity", new double[]{0, 500}); + SERVER_LIMITS.put("soe", new double[]{0, 500}); + SERVER_LIMITS.put("kwh_charged", new double[]{0, 1000}); + SERVER_LIMITS.put("odometer", new double[]{0, 10_000_000}); + SERVER_LIMITS.put("lat", new double[]{-90, 90}); + SERVER_LIMITS.put("lon", new double[]{-180, 180}); + SERVER_LIMITS.put("elevation", new double[]{-1000, 10_000}); + SERVER_LIMITS.put("heading", new double[]{0, 360}); + SERVER_LIMITS.put("batt_temp", new double[]{-60, 100}); + SERVER_LIMITS.put("cabin_temp", new double[]{-60, 100}); + SERVER_LIMITS.put("ext_temp", new double[]{-60, 70}); + SERVER_LIMITS.put("hvac_setpoint", new double[]{0, 40}); + SERVER_LIMITS.put("voltage", new double[]{0, 1500}); + SERVER_LIMITS.put("current", new double[]{-2000, 2000}); + SERVER_LIMITS.put("tire_pressure_fl", new double[]{0, 1000}); + SERVER_LIMITS.put("tire_pressure_fr", new double[]{0, 1000}); + SERVER_LIMITS.put("tire_pressure_rl", new double[]{0, 1000}); + SERVER_LIMITS.put("tire_pressure_rr", new double[]{0, 1000}); + } + + private static void dropOutOfRange(JSONObject tlm) { + for (java.util.Map.Entry limit : SERVER_LIMITS.entrySet()) { + String key = limit.getKey(); + if (!tlm.has(key)) continue; + double value = tlm.optDouble(key, Double.NaN); + if (Double.isNaN(value) || value < limit.getValue()[0] || value > limit.getValue()[1]) tlm.remove(key); + } + // A position is only a position as a pair. + if (tlm.has("lat") != tlm.has("lon")) { + tlm.remove("lat"); + tlm.remove("lon"); + } + } + + /** Epoch seconds the server accepts: from 2001. Earlier means the head unit's clock is not set yet. */ + static final long MIN_UTC = 1_000_000_000L; + + static boolean isPlausibleUtc(long utcSeconds) { + return utcSeconds >= MIN_UTC; + } + /** * True for a temperature that a sensor could actually have measured. Excludes exactly * 0.0, which is what an unimplemented VHAL property returns — see cabin_temp above. diff --git a/app/src/main/java/com/evsuite/abrp/TelemetryUploader.java b/app/src/main/java/com/evsuite/abrp/TelemetryUploader.java index ce2b0a7..6616910 100644 --- a/app/src/main/java/com/evsuite/abrp/TelemetryUploader.java +++ b/app/src/main/java/com/evsuite/abrp/TelemetryUploader.java @@ -15,10 +15,6 @@ import com.evsuite.hardware.telemetry.TirePressureSnapshot; import org.json.JSONArray; import org.json.JSONObject; -import java.io.OutputStream; -import java.net.HttpURLConnection; -import java.net.URL; -import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Objects; @@ -28,7 +24,9 @@ import java.util.Objects; * Called on every service tick. While the car is in use every tick becomes a sample; the * queue is sent when {@link UploadCadence} says so — every tick on a state change, otherwise * at the configured interval. Parked and not charging, a sample is only taken when one is - * sent, every 15 minutes: nothing changes, and a sample every 15 s overnight is waste. + * sent, every 15 minutes: nothing changes, and a sample every 15 s overnight is waste. The GPS + * subscription follows the same rule: it is paused while parked and idle, and those samples + * carry the fix the car parked with. {@link #stop} ends it when live telemetry is off. * * Runs on the service's scheduler thread; the HTTP calls block it, with short timeouts. * Location updates arrive on the main looper and are only read here. @@ -36,7 +34,6 @@ import java.util.Objects; final class TelemetryUploader { private static final String TAG = "NilsMG4.Telemetry"; - private static final int TIMEOUT_MS = 8_000; /** Batches per tick: a backlog drains over a few ticks instead of stalling one. */ private static final int MAX_BATCHES_PER_TICK = 2; /** A fix older than this no longer says where the car is; position is omitted. */ @@ -48,6 +45,10 @@ final class TelemetryUploader { private final ChargingSignal chargingSignal = new ChargingSignal(); private volatile Location lastLocation; + /** The fix kept while parked and idle, with the GPS paused; null otherwise. */ + private volatile Location parkedLocation; + /** Whether the GPS should run; a request still queued on the main looper checks it. */ + private volatile boolean locationWanted; private volatile boolean locationActive; private volatile long lastLocationRequestMs; private LocationManager locationManager; @@ -64,7 +65,6 @@ final class TelemetryUploader { void tick(EnergySnapshot vehicle, ChargeSessionTracker.Snapshot session, UploadSettings settings) { long nowMs = System.currentTimeMillis(); - ensureLocationUpdates(settings); Integer soc = vehicle.getSocPercent() == null ? null : Math.round(vehicle.getSocPercent()); Float powerKw = vehicle.getBatteryPowerKw(); @@ -82,10 +82,24 @@ final class TelemetryUploader { lastCharging = charging; boolean idle = Boolean.TRUE.equals(parked) && Boolean.FALSE.equals(charging); + if (idle) { + // Parked and idle the car does not move: keep one fresh fix and stop the GPS until + // the car is in use again. Without a fix yet (just started, or in a garage) it + // keeps looking. + if (parkedLocation == null) parkedLocation = freshLocation(settings); + if (parkedLocation == null) ensureLocationUpdates(settings); + else if (locationWanted) stop(); + } else { + parkedLocation = null; + ensureLocationUpdates(settings); + } boolean uploadDue = UploadCadence.shouldUpload( nowMs, lastUploadAttemptMs, parked, charging, soc, stateChanged, settings); - if (!idle || uploadDue) { - queue.add(buildSample(nowMs, vehicle, session, settings, soc, charging, dcfc, parked)); + // Before the head unit has set its clock the sample's time is meaningless, and the + // server would drop it anyway. + boolean clockSet = TelemetryPayload.isPlausibleUtc(nowMs / 1000); + if (clockSet && (!idle || uploadDue)) { + queue.add(buildSample(nowMs, vehicle, session, settings, soc, charging, dcfc, parked, idle)); } if (!uploadDue) return; // Cadence counts attempts, not successes: retrying every tick while offline would @@ -99,7 +113,8 @@ final class TelemetryUploader { // ---------- Sample ---------- private String buildSample(long nowMs, EnergySnapshot vehicle, ChargeSessionTracker.Snapshot session, - UploadSettings settings, Integer soc, Boolean charging, Boolean dcfc, Boolean parked) { + UploadSettings settings, Integer soc, Boolean charging, Boolean dcfc, Boolean parked, + boolean idle) { TelemetryPayload tlm = new TelemetryPayload(nowMs / 1000); tlm.soc = soc; tlm.speedKmh = vehicle.getSpeedKmh(); @@ -124,7 +139,7 @@ final class TelemetryUploader { tlm.tirePressureFrKpa = tires.getFrontRightKpa(); tlm.tirePressureRlKpa = tires.getRearLeftKpa(); tlm.tirePressureRrKpa = tires.getRearRightKpa(); - Location loc = freshLocation(settings); + Location loc = idle ? parkedLocation : freshLocation(settings); if (loc != null) { tlm.lat = loc.getLatitude(); tlm.lon = loc.getLongitude(); @@ -146,14 +161,16 @@ final class TelemetryUploader { List batch = queue.peekBatch(); String summary = TelemetryPayload.summarize(batch.get(batch.size() - 1)); try { - int status = post(config, batch); - boolean ok = status >= 200 && status < 300; + ServerHttp.Response response = post(config, batch); + int status = response.status; + boolean ok = response.ok(); AbrpUploadService.log().record(new UploadLog.Entry(System.currentTimeMillis(), status, ok, - ok ? "Telemetry: " + batch.size() + " sample(s) sent" - : "Telemetry rejected (" + batch.size() + " queued)", summary)); + ok ? "Telemetry: " + batch.size() + " sample(s) sent" + droppedNote(response) + : "Telemetry rejected (" + batch.size() + " queued): " + response.excerpt(), summary)); if (!ok) { - // A 400 will never succeed on retry: drop the batch rather than block the - // queue behind it forever. Anything else (401, 5xx) is kept for later. + // The server drops bad values itself and answers 200, so a 400 is a request + // it cannot read at all and will never take: drop the batch rather than + // block the queue behind it forever. Anything else (401, 403, 5xx) is kept. if (status == 400) queue.remove(batch.size()); return; } @@ -167,28 +184,22 @@ final class TelemetryUploader { } } - private int post(ServerConfig config, List batch) throws Exception { + private ServerHttp.Response post(ServerConfig config, List batch) throws Exception { JSONArray samples = new JSONArray(); for (String sample : batch) samples.put(new JSONObject(sample)); - byte[] body = new JSONObject() + JSONObject body = new JSONObject() .put("installationId", config.installationId) - .put("samples", samples) - .toString().getBytes(StandardCharsets.UTF_8); - HttpURLConnection connection = null; + .put("samples", samples); + return ServerHttp.request("POST", config.url + "/v1/telemetry", config.token, body); + } + + /** ", 2 value(s) dropped by the server" when it left anything out, for the upload log. */ + static String droppedNote(ServerHttp.Response response) { try { - connection = (HttpURLConnection) new URL(config.url + "/v1/telemetry").openConnection(); - connection.setRequestMethod("POST"); - connection.setConnectTimeout(TIMEOUT_MS); - connection.setReadTimeout(TIMEOUT_MS); - connection.setDoOutput(true); - connection.setFixedLengthStreamingMode(body.length); - connection.setRequestProperty("Content-Type", "application/json"); - // The token goes in a header, never the URL, and is never logged. - connection.setRequestProperty("Authorization", "Bearer " + config.token); - try (OutputStream out = connection.getOutputStream()) { out.write(body); } - return connection.getResponseCode(); - } finally { - if (connection != null) connection.disconnect(); + int dropped = new JSONObject(response.body).optInt("droppedCount", 0); + return dropped > 0 ? ", " + dropped + " value(s) dropped by the server" : ""; + } catch (Exception e) { + return ""; // an older server answers 204 with no body } } @@ -207,6 +218,7 @@ final class TelemetryUploader { /** Subscribes to GPS once, and again after a failure or a provider change. */ private void ensureLocationUpdates(UploadSettings settings) { + locationWanted = true; if (locationActive) return; long now = SystemClock.elapsedRealtime(); if (lastLocationRequestMs != 0L && now - lastLocationRequestMs < LOCATION_RETRY_MS) return; @@ -215,6 +227,7 @@ final class TelemetryUploader { } private void requestLocationUpdates(UploadSettings settings) { + if (!locationWanted) return; // stopped while this request was queued try { if (locationManager == null) { locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE); @@ -237,11 +250,14 @@ final class TelemetryUploader { } } + /** Ends the GPS subscription; the next tick that needs a position starts it again. */ void stop() { + locationWanted = false; try { if (locationManager != null) locationManager.removeUpdates(locationListener); } catch (Exception ignored) { } locationActive = false; + lastLocationRequestMs = 0L; } private Location freshLocation(UploadSettings settings) { diff --git a/app/src/main/java/com/evsuite/abrp/WifiAutoConnect.java b/app/src/main/java/com/evsuite/abrp/WifiAutoConnect.java index 9228a75..ea2bfcc 100644 --- a/app/src/main/java/com/evsuite/abrp/WifiAutoConnect.java +++ b/app/src/main/java/com/evsuite/abrp/WifiAutoConnect.java @@ -154,10 +154,10 @@ final class WifiAutoConnect { static Result attempting(String ssid) { return new Result(false, ssid, "Connecting: " + ssid); } static final Result UNSUPPORTED = new Result(false, null, "Wi-Fi cannot be managed"); - static final Result WAITING = new Result(false, null, "Bekleniyor…"); + static final Result WAITING = new Result(false, null, "Waiting…"); static final Result ENABLING = new Result(false, null, "Turning Wi-Fi on…"); static final Result CANNOT_ENABLE = new Result(false, null, "Could not turn Wi-Fi on"); static final Result NO_SAVED_NETWORKS = new Result(false, null, "No saved networks"); - static final Result REFUSED = new Result(false, null, "Sistem izin vermedi"); + static final Result REFUSED = new Result(false, null, "The system refused"); } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 6d4d5ab..e2cf188 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -92,6 +92,8 @@ Config imported and saved — press Test to verify + Config imported and saved; the file was deleted + Config imported and saved, but the file could not be deleted. It holds your token: delete %1$s Could not read that file No settings found in that file No config file found. Copy it to %1$s diff --git a/app/src/test/java/com/evsuite/abrp/SecurePrefsTest.java b/app/src/test/java/com/evsuite/abrp/SecurePrefsTest.java new file mode 100644 index 0000000..cb60b31 --- /dev/null +++ b/app/src/test/java/com/evsuite/abrp/SecurePrefsTest.java @@ -0,0 +1,40 @@ +package com.evsuite.abrp; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +import java.util.HashMap; +import java.util.Map; + +import org.junit.Test; + +/** Moving a keystore-fallback run's settings back into encrypted storage — pure JVM. */ +public class SecurePrefsTest { + + private static Map map(String... kv) { + Map m = new HashMap<>(); + for (int i = 0; i < kv.length; i += 2) m.put(kv[i], kv[i + 1]); + return m; + } + + @Test + public void aTokenSavedDuringTheFallbackIsKept() { + Map moves = SecurePrefs.mergePlaintext( + map(ServerConfig.KEY_TOKEN, "new-token", ServerConfig.KEY_URL, "https://car.example.com"), + map(ServerConfig.KEY_TOKEN, "old-token")); + assertEquals("new-token", moves.get(ServerConfig.KEY_TOKEN)); + assertEquals("https://car.example.com", moves.get(ServerConfig.KEY_URL)); + } + + @Test + public void theStoredInstallationIdWins() { + Map moves = SecurePrefs.mergePlaintext( + map(ServerConfig.KEY_INSTALLATION_ID, "made-up-in-the-fallback"), + map(ServerConfig.KEY_INSTALLATION_ID, "the-one-history-is-under")); + assertFalse(moves.containsKey(ServerConfig.KEY_INSTALLATION_ID)); + + // With none stored yet, the fallback's id is the only one there is. + moves = SecurePrefs.mergePlaintext(map(ServerConfig.KEY_INSTALLATION_ID, "only-one"), map()); + assertEquals("only-one", moves.get(ServerConfig.KEY_INSTALLATION_ID)); + } +} diff --git a/app/src/test/java/com/evsuite/abrp/ServerConfigTest.java b/app/src/test/java/com/evsuite/abrp/ServerConfigTest.java index f5bf0c8..ea803d4 100644 --- a/app/src/test/java/com/evsuite/abrp/ServerConfigTest.java +++ b/app/src/test/java/com/evsuite/abrp/ServerConfigTest.java @@ -44,4 +44,21 @@ public class ServerConfigTest { assertFalse(ServerConfig.isValidInstallationId("not-a-uuid")); assertFalse(ServerConfig.isValidInstallationId("")); } + + @Test + public void theInstallationIdFollowsTheHeadUnit() { + String id = ServerConfig.installationIdFor("a1b2c3d4e5f60718"); + assertTrue(id, ServerConfig.isValidInstallationId(id)); + // The same head unit gets the same id after a reinstall; another gets another. + assertEquals(id, ServerConfig.installationIdFor(" A1B2C3D4E5F60718 ")); + assertFalse(id.equals(ServerConfig.installationIdFor("0000000000000001"))); + assertFalse(id.contains("a1b2c3d4e5f60718")); + } + + @Test + public void noUsableAndroidIdMeansNoDerivedId() { + assertNull(ServerConfig.installationIdFor(null)); + assertNull(ServerConfig.installationIdFor("")); + assertNull(ServerConfig.installationIdFor("9774d56d682e549c")); + } } diff --git a/app/src/test/java/com/evsuite/abrp/TelemetryPayloadTest.java b/app/src/test/java/com/evsuite/abrp/TelemetryPayloadTest.java index dc184a0..f91af99 100644 --- a/app/src/test/java/com/evsuite/abrp/TelemetryPayloadTest.java +++ b/app/src/test/java/com/evsuite/abrp/TelemetryPayloadTest.java @@ -311,4 +311,45 @@ public class TelemetryPayloadTest { assertTrue(summary, summary.startsWith("utc=1700000000")); assertTrue(summary, summary.contains("| omitted: soc,speed,")); } + + // ---------- The server's ranges (server/model.go) ---------- + + @Test + public void reversingSendsSpeedAsAMagnitude() throws Exception { + TelemetryPayload t = new TelemetryPayload(1_700_000_000L); + t.speedKmh = -4.2f; + assertEquals(4, new JSONObject(t.build()).getInt("speed")); + } + + @Test + public void valuesOutsideTheServersRangeAreLeftOut() throws Exception { + TelemetryPayload t = new TelemetryPayload(1_700_000_000L); + t.soc = 55; + t.extTempC = 72f; // plausible to the car's guard, above the server's 70 + t.hvacSetpointC = 50f; // a HI sentinel, above the server's 40 + t.rangeKm = 2500; + JSONObject json = new JSONObject(t.build()); + assertEquals(55, json.getInt("soc")); + assertFalse(json.has("ext_temp")); + assertFalse(json.has("hvac_setpoint")); + assertFalse(json.has("est_battery_range")); + } + + @Test + public void theAppsLimitsMatchEveryNumericServerField() { + String[] serverFields = {"soc", "speed", "power", "est_battery_range", "capacity", "soe", + "kwh_charged", "odometer", "lat", "lon", "elevation", "heading", "batt_temp", + "cabin_temp", "ext_temp", "hvac_setpoint", "voltage", "current", + "tire_pressure_fl", "tire_pressure_fr", "tire_pressure_rl", "tire_pressure_rr"}; + for (String field : serverFields) { + assertTrue(field, TelemetryPayload.SERVER_LIMITS.containsKey(field)); + } + } + + @Test + public void aClockBefore2001IsNotATime() { + assertFalse(TelemetryPayload.isPlausibleUtc(0L)); + assertFalse(TelemetryPayload.isPlausibleUtc(999_999_999L)); + assertTrue(TelemetryPayload.isPlausibleUtc(1_790_000_000L)); + } } diff --git a/app/src/test/java/com/evsuite/abrp/TelemetryUploaderTest.java b/app/src/test/java/com/evsuite/abrp/TelemetryUploaderTest.java new file mode 100644 index 0000000..517fe2f --- /dev/null +++ b/app/src/test/java/com/evsuite/abrp/TelemetryUploaderTest.java @@ -0,0 +1,19 @@ +package com.evsuite.abrp; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +/** Reading the server's answer to a telemetry batch — pure JVM. */ +public class TelemetryUploaderTest { + + @Test + public void theLogSaysWhatTheServerDropped() { + assertEquals(", 2 value(s) dropped by the server", TelemetryUploader.droppedNote( + new ServerHttp.Response(200, "{\"stored\":3,\"droppedCount\":2,\"dropped\":[]}"))); + assertEquals("", TelemetryUploader.droppedNote( + new ServerHttp.Response(200, "{\"stored\":3,\"droppedCount\":0,\"dropped\":[]}"))); + // An older server answers 204 without a body. + assertEquals("", TelemetryUploader.droppedNote(new ServerHttp.Response(204, ""))); + } +} diff --git a/k8s/README.md b/k8s/README.md index f2d2c0b..a2f694e 100644 --- a/k8s/README.md +++ b/k8s/README.md @@ -22,8 +22,8 @@ is applied automatically. Apply them yourself. covered by Traefik's default certificate. 2. **Create the Secrets.** `cp k8s/secrets/secrets.example.yaml k8s/secrets/secrets.yaml` (gitignored), replace every `CHANGE-ME` there (`openssl rand -hex 32`): `influxdb-auth`, `mg4-server` (ingest token, - approval password for pairing, your ABRP API key and user token) and `grafana-admin`. Keep - the template itself on placeholders only. + approval password for pairing, your ABRP API key and user token), `grafana-admin` and + `grafana-influx` (see below). Keep the template itself on placeholders only. 3. **Registry access.** The `mg4-server` package on git.stinnesbeck.com is public, so no pull secret is needed. If you make it private, add an `imagePullSecrets` entry to `server.yaml`. @@ -38,12 +38,34 @@ kubectl apply -k k8s/ Use **`-k`**, not `-f k8s/`: plain `-f` applies the files without kustomize, so the Grafana ConfigMaps (datasource and dashboards) are never created and Grafana does not start. +## Grafana's InfluxDB token + +Grafana reads InfluxDB with a token that can only read the `mg4` bucket, from the +`grafana-influx` Secret. InfluxDB has to create that token, so it is a step of its own: + +1. With InfluxDB running, create the token. The `influx` CLI in the pod is already signed in + as admin: + + ```sh + kubectl -n mg4 exec influxdb-0 -- sh -c \ + 'influx auth create --org mg4 --description grafana-read-only --json \ + --read-bucket "$(influx bucket find --name mg4 --hide-headers | cut -f1)"' + ``` + +2. Copy the `"token"` value from the output into `grafana-influx` → `token` in + `k8s/secrets/secrets.yaml`, apply that file, and restart Grafana: + `kubectl -n mg4 rollout restart deployment/grafana`. + +Until the Secret exists, Grafana does not start (the container waits for the missing Secret). +On a fresh install, apply everything once without Grafana working, create the token, then +apply the Secret. + ## Notes - InfluxDB's setup mode runs only on an empty volume. Changing `admin-token` in the Secret later does not change the token inside InfluxDB. -- The server and Grafana use the InfluxDB admin token. A token scoped to the `mg4` bucket (read - for Grafana, read/write for the server) can replace it later. +- The server uses the InfluxDB admin token; a token scoped to read/write on the `mg4` bucket can + replace it later. Grafana has its own read-only token (below). - The dashboards are read-only in Grafana's UI. Change the JSON in `grafana/dashboards/`; the ConfigMap name changes with its content, which rolls Grafana. - The server image is pinned by digest in `server.yaml`. To upgrade, push a new tag and put its diff --git a/k8s/grafana.yaml b/k8s/grafana.yaml index 135aa75..0cc1911 100644 --- a/k8s/grafana.yaml +++ b/k8s/grafana.yaml @@ -71,11 +71,13 @@ spec: # Read by datasources.yaml. - name: INFLUX_URL value: http://influxdb.mg4.svc:8086 + # Read-only on the mg4 bucket; the dashboards never write (README, "Grafana's + # InfluxDB token"). - name: INFLUX_TOKEN valueFrom: secretKeyRef: - name: influxdb-auth - key: admin-token + name: grafana-influx + key: token volumeMounts: - name: data mountPath: /var/lib/grafana diff --git a/k8s/secrets/secrets.example.yaml b/k8s/secrets/secrets.example.yaml index f97d66b..3064ca0 100644 --- a/k8s/secrets/secrets.example.yaml +++ b/k8s/secrets/secrets.example.yaml @@ -41,3 +41,14 @@ stringData: # Grafana's admin login (user "admin"). Only read on first start: change it in Grafana # afterwards, or reset it with grafana cli. admin-password: CHANGE-ME +--- +apiVersion: v1 +kind: Secret +metadata: + name: grafana-influx + namespace: mg4 +type: Opaque +stringData: + # A read-only InfluxDB token for the mg4 bucket, not the admin token. InfluxDB makes it; + # the command is in k8s/README.md ("Grafana's InfluxDB token"). + token: CHANGE-ME-read-only-token-from-influx-auth-create diff --git a/k8s/server.yaml b/k8s/server.yaml index d698e1e..5cb7dc3 100644 --- a/k8s/server.yaml +++ b/k8s/server.yaml @@ -29,8 +29,8 @@ spec: type: RuntimeDefault containers: - name: server - # 2.1.0, pinned by digest so a re-pushed tag cannot change what runs. - image: git.stinnesbeck.com/nils/mg4-server@sha256:e5f6e2d2ef9dc2650055a0823a3f86d8903e46918a76e990540482edf819766d + # 2.2.0, pinned by digest so a re-pushed tag cannot change what runs. + image: git.stinnesbeck.com/nils/mg4-server@sha256:09eb151ff65300b3990a74063cc44a14d24bef810461e4e0e63451cfbcfd8258 ports: - name: http containerPort: 8080 diff --git a/mise.toml b/mise.toml index 35eb1ab..dcd9206 100644 --- a/mise.toml +++ b/mise.toml @@ -121,7 +121,7 @@ run = './gradlew -Dorg.gradle.java.home=$JAVA_HOME installStableDebug' [tasks.logs] description = "Suit les logs de l'app sur le véhicule" -run = "adb logcat -s AbrpUploadService:V CarPropertyAdapter:V VhalProbe:V SecurePrefs:V OtaUpdater:V UpdateHook:V" +run = "adb logcat -s AbrpUploadService:V CarPropertyAdapter:V VhalProbe:V SecurePrefs:V OtaUpdater:V UpdateHook:V NilsMG4.Telemetry:V NilsMG4.Mongo:V NilsMG4.SteeringKey:V EVABRP.Boot:V EVABRP.Wifi:V" [tasks.release] description = "APK release des deux canaux (stable sans OTA, unstable avec OTA)" diff --git a/server/README.md b/server/README.md index 53a2fd9..0f565f3 100644 --- a/server/README.md +++ b/server/README.md @@ -31,18 +31,22 @@ The ABRP credentials never reach the car. ## Endpoints The ingest endpoints accept `Authorization: Bearer ` with either the shared `INGEST_TOKEN` -or a paired car's own token. +or a paired car's own token. A paired token only works for the installation id it was paired +with (403 otherwise); the shared token works for any. - `POST /v1/telemetry` — `{ installationId, samples: [...] }`, up to 500 samples in ABRP's `tlm` field names and units (`utc` in epoch seconds; flags as 0/1 or booleans). Unknown fields are - dropped, known ones bounded. Stored, then the newest sample is forwarded to ABRP if it is under - 5 minutes old. + dropped, known ones bounded: a value out of range is dropped on its own, and a sample without + a plausible time (before 2001, or more than an hour ahead) is dropped whole. The rest is stored + and the answer is `200 {stored, droppedCount, dropped}`, even when nothing was usable, since + the car would only send it again. The newest sample is forwarded to ABRP if it is under + 5 minutes old and not ahead of the server's clock. - `PUT /v1/consumption/daily` — one delta chunk of a day's consumption. - `PUT /v1/charging-sessions/{sessionId}` — a finished DC session with its curve. - `GET /v1/consumption/history/{installationId}` — the car reads its own totals back. - `GET /v1/health` — `{"status":"ok"}`. -A bad request is a 400 naming the field; a database failure is a 500 without detail. +A malformed request is a 400 naming the field; a database failure is a 500 without detail. ## Pairing a car by QR code @@ -52,13 +56,15 @@ on only when `APPROVAL_PASSWORD` is set. 1. The car calls `POST /v1/pair/start` `{installationId}` and shows the returned `verificationUriComplete` as a QR code, plus the short `userCode` (e.g. `K7F-3QX`). 2. The phone opens `/pair?code=…`, the owner checks the code and approves with the approval - password. Five wrong passwords lock approvals for a minute. + password. Five wrong passwords from one address lock that address out for a minute; other + addresses are unaffected (the address is the last `X-Forwarded-For` entry, which Traefik sets). + An address may hold three open requests; a car starting again replaces its own. 3. The car polls `POST /v1/pair/poll` `{deviceCode}`: 202 pending, 200 with its own token (handed out once), 403 denied, 410 expired (after 10 minutes) or unknown. Each car gets its own 43-character token; only its SHA-256 is stored (measurement `device` in -the same bucket), and the device is persisted before the car can collect the token, so a -restart never loses a paired car. Manage cars with the approval password: +the same bucket), and the token is only handed out, and only accepted, once the device is +persisted, so a restart never loses a paired car. Manage cars with the approval password: ```sh curl -H "Authorization: Bearer $APPROVAL_PASSWORD" https://car.k8s.stinnesbeck.com/v1/devices diff --git a/server/abrp.go b/server/abrp.go index 9869cc6..50df2d6 100644 --- a/server/abrp.go +++ b/server/abrp.go @@ -37,6 +37,8 @@ const ( abrpEndpoint = "https://api.iternio.com/1/tlm/send" // Older than this, a sample is history rather than live state, and ABRP gets nothing. abrpMaxAge = 5 * time.Minute + // Newer than now by more than this, a sample's clock is wrong, and ABRP gets nothing. + abrpMaxAhead = 30 * time.Second ) func (f *Forwarder) configured() bool { @@ -46,16 +48,17 @@ func (f *Forwarder) configured() bool { // pick chooses the sample to forward, or false when there is nothing new or it is too // soon. It records the choice, so two batches arriving together cannot both forward. func (f *Forwarder) pick(samples []TelemetrySample, now time.Time) (TelemetrySample, bool) { - if len(samples) == 0 { - return TelemetrySample{}, false - } - newest := samples[0] - for _, s := range samples[1:] { - if s.UTC > newest.UTC { - newest = s + // A sample stamped ahead of now has a wrong clock. Taking it would also set lastUTC into + // the future, and every real sample after it would look older and never be sent. + latest := now.Add(abrpMaxAhead).Unix() + var newest TelemetrySample + found := false + for _, s := range samples { + if s.UTC <= latest && (!found || s.UTC > newest.UTC) { + newest, found = s, true } } - if now.Sub(time.Unix(newest.UTC, 0)) > abrpMaxAge { + if !found || now.Sub(time.Unix(newest.UTC, 0)) > abrpMaxAge { return TelemetrySample{}, false } f.mu.Lock() diff --git a/server/api.go b/server/api.go index ce1390e..7b7bb83 100644 --- a/server/api.go +++ b/server/api.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "strings" + "time" ) // API serves the car. Every endpoint but /v1/health needs the ingest token. @@ -45,30 +46,49 @@ func (a *API) Routes() http.Handler { return mux } -// hasIngestToken accepts the shared token (compared in constant time; main refuses to start +// caller is who sent an ingest request. A paired car's token is bound to the installation +// id it was paired with; the shared INGEST_TOKEN may write and read for any. +type caller struct { + installationID string // empty for the shared token +} + +// mayUse reports whether the caller may write or read data stored under id. +func (c caller) mayUse(id string) bool { + return c.installationID == "" || strings.EqualFold(c.installationID, id) +} + +// errOtherInstallation is a 403: a paired car's token used for another car's data. +var errOtherInstallation = errors.New("this token belongs to another installation; pair again after changing the installation id") + +// authenticate accepts the shared token (compared in constant time; main refuses to start // with one under 24 characters, so an empty or short one never matches) or a paired car's // own, unrevoked token. -func (a *API) hasIngestToken(r *http.Request) bool { +func (a *API) authenticate(r *http.Request) (caller, bool) { header := r.Header.Get("Authorization") supplied, ok := strings.CutPrefix(header, "Bearer ") if !ok || supplied == "" { - return false + return caller{}, false } if len(a.IngestToken) >= minIngestTokenLength && subtle.ConstantTimeCompare([]byte(supplied), []byte(a.IngestToken)) == 1 { - return true + return caller{}, true } - return a.Pairing.Authorized(supplied) + d, ok := a.Pairing.Authorized(supplied) + if !ok { + return caller{}, false + } + return caller{installationID: d.InstallationID}, true } -type handler func(w http.ResponseWriter, r *http.Request, body []byte) error +type handler func(w http.ResponseWriter, r *http.Request, c caller, body []byte) error // ingest wraps a handler with the token check, the body limit and error mapping: a -// validation failure is a 400 naming the field; anything else is a 500 whose detail -// stays in the server log. +// validation failure is a 400 naming the field, another car's data a 403; anything else +// is a 500 whose detail stays in the server log. func (a *API) ingest(h handler) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !a.hasIngestToken(r) { + c, ok := a.authenticate(r) + if !ok { writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "unauthorized"}) return } @@ -81,7 +101,11 @@ func (a *API) ingest(h handler) http.HandlerFunc { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "body is too large"}) return } - if err := h(w, r, body); err != nil { + if err := h(w, r, c, body); err != nil { + if errors.Is(err, errOtherInstallation) { + writeJSON(w, http.StatusForbidden, map[string]string{"error": err.Error()}) + return + } var verr *ValidationError if errors.As(err, &verr) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": verr.Error()}) @@ -93,29 +117,53 @@ func (a *API) ingest(h handler) http.HandlerFunc { } } +// maxDroppedListed caps the dropped values named in a telemetry response and log line. +const maxDroppedListed = 20 + // telemetry stores the batch first; only once the write succeeded is the newest sample // handed to ABRP, so the car's retry of a failed request cannot leave ABRP with data the -// database never got. -func (a *API) telemetry(w http.ResponseWriter, r *http.Request, body []byte) error { - batch, err := validateTelemetryBatch(body) +// database never got. The answer says how many samples were stored and what was dropped; +// a batch with nothing usable is still a 200, since sending it again would not help. +func (a *API) telemetry(w http.ResponseWriter, r *http.Request, c caller, body []byte) error { + batch, err := validateTelemetryBatch(body, time.Now()) if err != nil { return err } - if err := a.Store.WriteTelemetry(r.Context(), batch.InstallationID, batch.Samples); err != nil { - return err + if !c.mayUse(batch.InstallationID) { + return errOtherInstallation } - a.Forwarder.Forward(batch.Samples) - w.WriteHeader(http.StatusNoContent) + if len(batch.Samples) > 0 { + if err := a.Store.WriteTelemetry(r.Context(), batch.InstallationID, batch.Samples); err != nil { + return err + } + a.Forwarder.Forward(batch.Samples) + } + dropped := batch.Dropped + if len(dropped) > maxDroppedListed { + dropped = dropped[:maxDroppedListed] + } + if len(batch.Dropped) > 0 { + slog.Warn("telemetry values dropped", "installation", batch.InstallationID, + "count", len(batch.Dropped), "first", dropped) + } + writeJSON(w, http.StatusOK, map[string]any{ + "stored": len(batch.Samples), + "droppedCount": len(batch.Dropped), + "dropped": append([]string{}, dropped...), + }) return nil } // consumption stores one delta chunk at its recordedAt. Reads add a day's chunks up, so a // chunk the car re-sends from its offline cache replaces itself instead of counting twice. -func (a *API) consumption(w http.ResponseWriter, r *http.Request, body []byte) error { +func (a *API) consumption(w http.ResponseWriter, r *http.Request, c caller, body []byte) error { chunk, err := validateConsumption(body) if err != nil { return err } + if !c.mayUse(chunk.InstallationID) { + return errOtherInstallation + } if err := a.Store.WriteConsumptionChunk(r.Context(), chunk); err != nil { return err } @@ -123,11 +171,14 @@ func (a *API) consumption(w http.ResponseWriter, r *http.Request, body []byte) e return nil } -func (a *API) chargingSession(w http.ResponseWriter, r *http.Request, body []byte) error { +func (a *API) chargingSession(w http.ResponseWriter, r *http.Request, c caller, body []byte) error { session, err := validateChargingSession(body, r.PathValue("sessionId")) if err != nil { return err } + if !c.mayUse(session.InstallationID) { + return errOtherInstallation + } if err := a.Store.WriteChargingSession(r.Context(), session); err != nil { return err } @@ -137,11 +188,14 @@ func (a *API) chargingSession(w http.ResponseWriter, r *http.Request, body []byt // history is the car's read-back of its own uploaded totals, so an app reinstall does not // blank its Consumption tab. Up to 400 days, newest first, plus the lifetime sum of all. -func (a *API) history(w http.ResponseWriter, r *http.Request, _ []byte) error { +func (a *API) history(w http.ResponseWriter, r *http.Request, c caller, _ []byte) error { id, err := validUUID(r.PathValue("installationId"), "installationId") if err != nil { return err } + if !c.mayUse(id) { + return errOtherInstallation + } chunks, err := a.Store.ReadConsumptionChunks(r.Context(), id) if err != nil { return err diff --git a/server/api_test.go b/server/api_test.go index c3832ae..4a69fa5 100644 --- a/server/api_test.go +++ b/server/api_test.go @@ -116,17 +116,32 @@ func TestEveryOtherEndpointNeedsTheIngestToken(t *testing.T) { } } -func TestTelemetryIsStoredAndBadInputIsA400(t *testing.T) { +func TestTelemetryIsStoredAndBadValuesAreDropped(t *testing.T) { api, store := newTestAPI() h := api.Routes() rec := do(t, h, "POST", "/v1/telemetry", testToken, string(telemetryBody(`{"utc":1790000000,"soc":80}`))) - if rec.Code != 204 || len(store.telemetry) != 1 { - t.Fatalf("%d, stored %d", rec.Code, len(store.telemetry)) + if rec.Code != 200 || len(store.telemetry) != 1 || !strings.Contains(rec.Body.String(), `"stored":1`) { + t.Fatalf("%d %s, stored %d", rec.Code, rec.Body, len(store.telemetry)) } - rec = do(t, h, "POST", "/v1/telemetry", testToken, string(telemetryBody(`{"utc":1790000000,"soc":120}`))) - if rec.Code != 400 || !strings.Contains(rec.Body.String(), "samples[0].soc is invalid") { + // One bad value costs that value, not the batch: the car drops what the server rejects. + rec = do(t, h, "POST", "/v1/telemetry", testToken, + string(telemetryBody(`{"utc":1790000000,"soc":120,"speed":-3},{"utc":1790000015,"soc":79}`))) + if rec.Code != 200 || len(store.telemetry) != 3 || !strings.Contains(rec.Body.String(), "samples[0].soc") || + !strings.Contains(rec.Body.String(), "samples[0].speed") || !strings.Contains(rec.Body.String(), `"stored":2`) { + t.Fatalf("%d %s, stored %d", rec.Code, rec.Body, len(store.telemetry)) + } + if _, ok := store.telemetry[1].Numbers["soc"]; ok { + t.Fatal("out-of-range soc stored") + } + // Nothing usable is still a 200: sending it again would not help. + rec = do(t, h, "POST", "/v1/telemetry", testToken, string(telemetryBody(`{"utc":5}`))) + if rec.Code != 200 || len(store.telemetry) != 3 || !strings.Contains(rec.Body.String(), `"stored":0`) { t.Fatalf("%d %s", rec.Code, rec.Body) } + // A malformed request is still a 400. + if rec := do(t, h, "POST", "/v1/telemetry", testToken, `{"installationId":"nope","samples":[]}`); rec.Code != 400 { + t.Fatalf("malformed: %d", rec.Code) + } if rec := do(t, h, "GET", "/v1/telemetry", testToken, ""); rec.Code != 405 { t.Fatalf("GET on a POST route: want 405, got %d", rec.Code) } diff --git a/server/logic_test.go b/server/logic_test.go index bf83bf9..6f8371c 100644 --- a/server/logic_test.go +++ b/server/logic_test.go @@ -75,6 +75,18 @@ func TestForwarderPicksOnlyTheNewestFreshSampleOncePerInterval(t *testing.T) { if _, ok := fresh.pick([]TelemetrySample{{UTC: testNowS - 600}}, now); ok { t.Fatal("forwarded a stale sample") } + // A sample from a clock a day ahead is skipped, and does not block the real ones after it. + ahead := &Forwarder{MinInterval: 5 * time.Second} + got, ok = ahead.pick([]TelemetrySample{{UTC: testNowS + 86400}, {UTC: testNowS - 1}}, now) + if !ok || got.UTC != testNowS-1 { + t.Fatalf("picked %+v %v", got, ok) + } + if _, ok := ahead.pick([]TelemetrySample{{UTC: testNowS + 86400}}, now.Add(time.Minute)); ok { + t.Fatal("forwarded a sample from the future") + } + if _, ok := ahead.pick([]TelemetrySample{{UTC: testNowS + 60}}, now.Add(time.Minute)); !ok { + t.Fatal("a real sample after a future one was not forwarded") + } } func TestForwarderPutsCredentialsInTheBodyNotTheURL(t *testing.T) { diff --git a/server/model.go b/server/model.go index 955f932..b121c87 100644 --- a/server/model.go +++ b/server/model.go @@ -105,4 +105,6 @@ type TelemetrySample struct { type TelemetryBatch struct { InstallationID string Samples []TelemetrySample + // Dropped names the values and samples validation left out, e.g. "samples[3].speed". + Dropped []string } diff --git a/server/pairing.go b/server/pairing.go index f67438c..4b698ee 100644 --- a/server/pairing.go +++ b/server/pairing.go @@ -21,12 +21,19 @@ import ( // replica. Approved devices are persisted (see Device) so their tokens survive a restart. const ( - pairingTTL = 10 * time.Minute - pairingInterval = 5 * time.Second - maxPendingPairing = 50 - // After this many wrong approval passwords in a window, approvals pause for the window. - maxFailedLogins = 5 - failedLoginsWindow = time.Minute + pairingTTL = 10 * time.Minute + pairingInterval = 5 * time.Second + // Pending requests per client address, and in all. A car needs one; a client that opens + // more only crowds out its own, and the total bounds memory. + maxPendingPerClient = 3 + maxPendingPairing = 1000 + // After this many wrong approval passwords from one client address in a window, that + // address waits for the window. Other addresses are not affected, so a stranger guessing + // cannot lock the owner out. The global cap only bounds guessing spread over many + // addresses; it is far above anything a person types. + maxFailedLogins = 5 + maxFailedLoginsGlobal = 100 + failedLoginsWindow = time.Minute // Unambiguous characters only: no 0/O, 1/I/L, so a code reads right off a car screen. userCodeAlphabet = "ABCDEFGHJKMNPQRSTUVWXYZ23456789" ) @@ -52,6 +59,9 @@ type pairingState int const ( pairingPending pairingState = iota + // pairingSaving: approved, and the device is being persisted. The car still sees + // pending, so it cannot collect a token whose device might not be saved. + pairingSaving pairingApproved pairingDenied ) @@ -60,6 +70,7 @@ type pairingRequest struct { deviceCode string userCode string installationID string + client string // address that opened the request expires time.Time state pairingState token string // set on approval, handed out once @@ -75,7 +86,9 @@ type Pairing struct { byDevice map[string]*pairingRequest byUser map[string]*pairingRequest devices map[string]Device // by token hash - failures []time.Time + // Wrong passwords: per client address, and all of them together. + failures map[string][]time.Time + globalFailures []time.Time } func NewPairing(password string, devices []Device) *Pairing { @@ -85,6 +98,7 @@ func NewPairing(password string, devices []Device) *Pairing { byDevice: map[string]*pairingRequest{}, byUser: map[string]*pairingRequest{}, devices: map[string]Device{}, + failures: map[string][]time.Time{}, } for _, d := range devices { p.devices[d.TokenHash] = d @@ -137,25 +151,43 @@ func hashToken(token string) string { func (p *Pairing) expireLocked(now time.Time) { for code, r := range p.byDevice { - if now.After(r.expires) { + // A request being saved is finished by Commit or Forget, not expired under them. + if now.After(r.expires) && r.state != pairingSaving { delete(p.byDevice, code) delete(p.byUser, r.userCode) } } } -// Start opens a pairing request for a car. -func (p *Pairing) Start(installationID string) (*pairingRequest, error) { +func (p *Pairing) dropLocked(r *pairingRequest) { + delete(p.byDevice, r.deviceCode) + delete(p.byUser, r.userCode) +} + +// Start opens a pairing request for a car. A new request replaces the same installation's +// undecided one; a client already holding maxPendingPerClient requests gets errTooMany. +func (p *Pairing) Start(installationID, client string) (*pairingRequest, error) { p.mu.Lock() defer p.mu.Unlock() now := p.Now() p.expireLocked(now) - if len(p.byDevice) >= maxPendingPairing { + perClient := 0 + for _, r := range p.byDevice { + if r.state == pairingPending && strings.EqualFold(r.installationID, installationID) { + p.dropLocked(r) + continue + } + if r.client == client { + perClient++ + } + } + if perClient >= maxPendingPerClient || len(p.byDevice) >= maxPendingPairing { return nil, errTooMany } r := &pairingRequest{ deviceCode: randomString(32), installationID: installationID, + client: client, expires: now.Add(pairingTTL), } for { @@ -182,32 +214,46 @@ func (p *Pairing) Lookup(userCode string) (pairingRequest, error) { return *r, nil } -// checkPassword counts failures and refuses everything while too many are recent. -func (p *Pairing) checkPasswordLocked(password string, now time.Time) error { - recent := p.failures[:0] - for _, t := range p.failures { +func recentOnly(times []time.Time, now time.Time) []time.Time { + recent := times[:0] + for _, t := range times { if now.Sub(t) < failedLoginsWindow { recent = append(recent, t) } } - p.failures = recent - if len(p.failures) >= maxFailedLogins { + return recent +} + +// checkPasswordLocked counts failures per client address and refuses a client while it +// has too many recent ones (or while too many arrive from everywhere at once). +func (p *Pairing) checkPasswordLocked(password, client string, now time.Time) error { + for c, times := range p.failures { + if recent := recentOnly(times, now); len(recent) == 0 { + delete(p.failures, c) + } else { + p.failures[c] = recent + } + } + p.globalFailures = recentOnly(p.globalFailures, now) + if len(p.failures[client]) >= maxFailedLogins || len(p.globalFailures) >= maxFailedLoginsGlobal { return errTooManyFailed } if subtle.ConstantTimeCompare([]byte(password), []byte(p.ApprovalPassword)) != 1 { - p.failures = append(p.failures, now) + p.failures[client] = append(p.failures[client], now) + p.globalFailures = append(p.globalFailures, now) return errWrongPassword } return nil } // Decide approves or denies a pending request. On approval it returns the new device, which -// the caller must persist before the car can collect the token. -func (p *Pairing) Decide(userCode, password string, approve bool) (Device, error) { +// the caller must persist and then Commit (or Forget, if saving failed): until Commit the +// car keeps seeing pending and the token is not accepted anywhere. +func (p *Pairing) Decide(userCode, password, client string, approve bool) (Device, error) { p.mu.Lock() defer p.mu.Unlock() now := p.Now() - if err := p.checkPasswordLocked(password, now); err != nil { + if err := p.checkPasswordLocked(password, client, now); err != nil { return Device{}, err } p.expireLocked(now) @@ -229,20 +275,29 @@ func (p *Pairing) Decide(userCode, password string, approve bool) (Device, error TokenHash: hashToken(token), CreatedAt: now, } - r.state = pairingApproved + r.state = pairingSaving r.token = token r.deviceID = d.ID - p.devices[d.TokenHash] = d return d, nil } +// Commit finishes an approval once its device is persisted: the token now works, and the +// car collects it at its next poll. +func (p *Pairing) Commit(userCode string, d Device) { + p.mu.Lock() + defer p.mu.Unlock() + p.devices[d.TokenHash] = d + if r, ok := p.byUser[normalizeUserCode(userCode)]; ok && r.deviceID == d.ID { + r.state = pairingApproved + } +} + // Forget undoes an approval whose device could not be persisted, so no token is handed out -// that would stop working at the next restart. +// that would stop working at the next restart. The request is pending again. func (p *Pairing) Forget(userCode string, d Device) { p.mu.Lock() defer p.mu.Unlock() - delete(p.devices, d.TokenHash) - if r, ok := p.byUser[normalizeUserCode(userCode)]; ok { + if r, ok := p.byUser[normalizeUserCode(userCode)]; ok && r.deviceID == d.ID { r.state = pairingPending r.token = "" r.deviceID = "" @@ -259,23 +314,27 @@ func (p *Pairing) Poll(deviceCode string) (state pairingState, token, deviceID s return 0, "", "", errUnknownCode } switch r.state { + case pairingSaving: + return pairingPending, "", "", nil case pairingApproved, pairingDenied: // Decided: hand the outcome out once, then forget the request. - delete(p.byDevice, r.deviceCode) - delete(p.byUser, r.userCode) + p.dropLocked(r) } return r.state, r.token, r.deviceID, nil } -// Authorized reports whether a token belongs to a paired, unrevoked device. -func (p *Pairing) Authorized(token string) bool { +// Authorized returns the paired, unrevoked device a token belongs to. +func (p *Pairing) Authorized(token string) (Device, bool) { if p == nil || token == "" { - return false + return Device{}, false } p.mu.Lock() defer p.mu.Unlock() d, ok := p.devices[hashToken(token)] - return ok && !d.Revoked + if !ok || d.Revoked { + return Device{}, false + } + return d, true } func (p *Pairing) Devices() []Device { @@ -304,8 +363,8 @@ func (p *Pairing) Revoke(id string) (Device, bool) { // CheckAdmin verifies the approval password for the device-management API, with the same // failure counting as the approval page. -func (p *Pairing) CheckAdmin(password string) error { +func (p *Pairing) CheckAdmin(password, client string) error { p.mu.Lock() defer p.mu.Unlock() - return p.checkPasswordLocked(password, p.Now()) + return p.checkPasswordLocked(password, client, p.Now()) } diff --git a/server/pairing_http.go b/server/pairing_http.go index f8341f1..2456969 100644 --- a/server/pairing_http.go +++ b/server/pairing_http.go @@ -5,6 +5,7 @@ import ( "html/template" "io" "log/slog" + "net" "net/http" "net/url" "strings" @@ -79,7 +80,7 @@ func (a *API) pairStart(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) return } - req, err := a.Pairing.Start(id) + req, err := a.Pairing.Start(id, clientAddress(r)) if err != nil { writeJSON(w, http.StatusTooManyRequests, map[string]string{"error": err.Error()}) return @@ -118,6 +119,25 @@ func (a *API) pairPoll(w http.ResponseWriter, r *http.Request) { } } +// clientAddress is who is asking, for the per-client limits: the last X-Forwarded-For entry, +// which is the one Traefik adds (anything left of it comes from the client and can be +// made up), or the connection's own address without a proxy. +func clientAddress(r *http.Request) string { + if forwarded := r.Header.Values("X-Forwarded-For"); len(forwarded) > 0 { + last := forwarded[len(forwarded)-1] + if i := strings.LastIndex(last, ","); i >= 0 { + last = last[i+1:] + } + if addr := strings.TrimSpace(last); addr != "" { + return addr + } + } + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host + } + return r.RemoteAddr +} + // ---------- Approval page ---------- type pageData struct { @@ -204,7 +224,7 @@ func (a *API) pairDecide(w http.ResponseWriter, r *http.Request) { } code := normalizeUserCode(r.PostForm.Get("code")) approve := r.PostForm.Get("action") != "deny" - device, err := a.Pairing.Decide(code, r.PostForm.Get("password"), approve) + device, err := a.Pairing.Decide(code, r.PostForm.Get("password"), clientAddress(r), approve) switch { case errors.Is(err, errWrongPassword): renderPage(w, http.StatusUnauthorized, pageData{Code: code, Error: "Wrong password."}) @@ -231,6 +251,7 @@ func (a *API) pairDecide(w http.ResponseWriter, r *http.Request) { renderPage(w, http.StatusInternalServerError, pageData{Code: code, Error: "The car could not be saved. Try again."}) return } + a.Pairing.Commit(code, device) slog.Info("car paired", "device", device.ID, "installation", device.InstallationID) renderPage(w, http.StatusOK, pageData{Done: true, Message: "Approved. The car finishes pairing on its own within a few seconds — you can close this page."}) } @@ -246,7 +267,7 @@ func (a *API) adminAuthorized(w http.ResponseWriter, r *http.Request) bool { writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "unauthorized"}) return false } - switch err := a.Pairing.CheckAdmin(supplied); { + switch err := a.Pairing.CheckAdmin(supplied, clientAddress(r)); { case errors.Is(err, errTooManyFailed): writeJSON(w, http.StatusTooManyRequests, map[string]string{"error": err.Error()}) return false diff --git a/server/pairing_test.go b/server/pairing_test.go index fce9b53..2be5f00 100644 --- a/server/pairing_test.go +++ b/server/pairing_test.go @@ -34,10 +34,19 @@ func poll(t *testing.T, h http.Handler, deviceCode string) (int, map[string]stri } func submit(t *testing.T, h http.Handler, code, password, action string) *httptest.ResponseRecorder { + t.Helper() + return submitFrom(t, h, "", code, password, action) +} + +// submitFrom submits the approval form as if from forwardedFor, the address Traefik adds. +func submitFrom(t *testing.T, h http.Handler, forwardedFor, code, password, action string) *httptest.ResponseRecorder { t.Helper() form := url.Values{"code": {code}, "password": {password}, "action": {action}} req := httptest.NewRequest("POST", "/pair", strings.NewReader(form.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } rec := httptest.NewRecorder() h.ServeHTTP(rec, req) return rec @@ -86,7 +95,7 @@ func TestPairingHappyPath(t *testing.T) { } // And it works on the ingest endpoints. rec := do(t, h, "POST", "/v1/telemetry", got["token"], string(telemetryBody(`{"utc":1790000000,"soc":80}`))) - if rec.Code != 204 { + if rec.Code != 200 { t.Fatalf("paired token on telemetry: %d %s", rec.Code, rec.Body) } } @@ -107,6 +116,11 @@ func TestPairingWrongPasswordAndLockout(t *testing.T) { if status, _ := poll(t, h, deviceCode); status != 202 || len(store.devices) != 0 { t.Fatal("a locked-out attempt changed the request") } + // The lockout is per address: someone guessing elsewhere does not lock the owner out. + if rec := submitFrom(t, h, "198.51.100.7", userCode, testApprovalPassword, "deny"); rec.Code != 200 { + t.Fatalf("other address locked out too: %d", rec.Code) + } + deviceCode, userCode, _ = startPairing(t, h) // After the window the right password works again. api.Pairing.Now = func() time.Time { return time.Now().Add(failedLoginsWindow + time.Second) } if rec := submit(t, h, userCode, testApprovalPassword, "approve"); rec.Code != 200 { @@ -186,7 +200,7 @@ func TestRevokedTokenIsRejected(t *testing.T) { t.Fatalf("revoked token accepted: %d", rec.Code) } // The shared ingest token is unaffected. - if rec := do(t, h, "POST", "/v1/telemetry", testToken, string(telemetryBody(`{"utc":1790000000,"soc":1}`))); rec.Code != 204 { + if rec := do(t, h, "POST", "/v1/telemetry", testToken, string(telemetryBody(`{"utc":1790000000,"soc":1}`))); rec.Code != 200 { t.Fatalf("shared token: %d", rec.Code) } } @@ -195,7 +209,9 @@ func TestPairedDevicesSurviveARestart(t *testing.T) { devices := []Device{{ID: "d1", TokenHash: hashToken("restored-token-0123456789abcdef"), CreatedAt: time.Now()}, {ID: "d2", TokenHash: hashToken("revoked-token-0123456789abcdef"), CreatedAt: time.Now(), Revoked: true}} p := NewPairing(testApprovalPassword, devices) - if !p.Authorized("restored-token-0123456789abcdef") || p.Authorized("revoked-token-0123456789abcdef") { + _, restored := p.Authorized("restored-token-0123456789abcdef") + _, revoked := p.Authorized("revoked-token-0123456789abcdef") + if !restored || revoked { t.Fatal("restored devices not honoured") } } @@ -232,3 +248,84 @@ func TestApprovalPasswordMustBeLongEnough(t *testing.T) { } var regexpUserCode = regexp.MustCompile(`^[` + userCodeAlphabet + `]{3}-[` + userCodeAlphabet + `]{3}$`) + +func TestPairedTokenOnlyServesItsOwnInstallation(t *testing.T) { + api, _ := newTestAPI() + h := api.Routes() + deviceCode, userCode, _ := startPairing(t, h) + submit(t, h, userCode, testApprovalPassword, "approve") + _, got := poll(t, h, deviceCode) + token := got["token"] + other := "7e2b3c4d-5e6f-4a70-9b8c-1d2e3f4a5b6c" + body := `{"installationId":"` + other + `","samples":[{"utc":1790000000,"soc":5}]}` + if rec := do(t, h, "POST", "/v1/telemetry", token, body); rec.Code != 403 { + t.Fatalf("telemetry for another car: %d", rec.Code) + } + if rec := do(t, h, "GET", "/v1/consumption/history/"+other, token, ""); rec.Code != 403 { + t.Fatalf("another car's history: %d", rec.Code) + } + if rec := do(t, h, "GET", "/v1/consumption/history/"+testInstallation, token, ""); rec.Code != 200 { + t.Fatalf("own history: %d", rec.Code) + } + // The shared token serves any installation. + if rec := do(t, h, "POST", "/v1/telemetry", testToken, body); rec.Code != 200 { + t.Fatalf("shared token: %d", rec.Code) + } +} + +func TestATokenIsCollectableOnlyOnceSaved(t *testing.T) { + p := NewPairing(testApprovalPassword, nil) + r, err := p.Start(testInstallation, "192.0.2.1") + if err != nil { + t.Fatal(err) + } + d, err := p.Decide(r.userCode, testApprovalPassword, "192.0.2.1", true) + if err != nil { + t.Fatal(err) + } + // Saving is in flight: the car still waits, and the token is not accepted yet. + if state, token, _, _ := p.Poll(r.deviceCode); state != pairingPending || token != "" { + t.Fatalf("collectable while saving: %v %q", state, token) + } + p.Forget(r.userCode, d) + if _, err := p.Decide(r.userCode, testApprovalPassword, "192.0.2.1", true); err != nil { + t.Fatalf("not approvable again after a failed save: %v", err) + } +} + +func TestPairingStartsAreLimitedPerAddress(t *testing.T) { + p := NewPairing(testApprovalPassword, nil) + ids := []string{"6f1a2b3c-4d5e-4f60-8a9b-0c1d2e3f4a51", "6f1a2b3c-4d5e-4f60-8a9b-0c1d2e3f4a52", + "6f1a2b3c-4d5e-4f60-8a9b-0c1d2e3f4a53", "6f1a2b3c-4d5e-4f60-8a9b-0c1d2e3f4a54"} + for _, id := range ids[:maxPendingPerClient] { + if _, err := p.Start(id, "203.0.113.9"); err != nil { + t.Fatal(err) + } + } + if _, err := p.Start(ids[3], "203.0.113.9"); !errors.Is(err, errTooMany) { + t.Fatalf("over the per-address limit: %v", err) + } + // The car on its own address is unaffected, and starting again replaces its request. + first, err := p.Start(testInstallation, "192.0.2.1") + if err != nil { + t.Fatal(err) + } + if _, err := p.Start(testInstallation, "192.0.2.1"); err != nil { + t.Fatal(err) + } + if _, _, _, err := p.Poll(first.deviceCode); !errors.Is(err, errUnknownCode) { + t.Fatal("the replaced request is still open") + } +} + +func TestClientAddressTakesTheProxysEntry(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("X-Forwarded-For", "10.9.9.9, 198.51.100.7") + if got := clientAddress(req); got != "198.51.100.7" { + t.Fatalf("got %q", got) + } + req.Header.Del("X-Forwarded-For") + if got := clientAddress(req); got != "192.0.2.1" { + t.Fatalf("without a proxy: %q", got) + } +} diff --git a/server/validate.go b/server/validate.go index 146a64e..c82cbe3 100644 --- a/server/validate.go +++ b/server/validate.go @@ -6,6 +6,7 @@ import ( "fmt" "math" "regexp" + "sort" "time" ) @@ -252,8 +253,13 @@ func validateChargingSession(body []byte, pathSessionID string) (ChargingSession // validateTelemetryBatch checks a batch of live samples in ABRP's tlm field names and // units. Unknown fields are dropped rather than rejected, so a newer app can add one -// without a server release; every known field is bounded; a flag may be 0/1 or a boolean. -func validateTelemetryBatch(body []byte) (TelemetryBatch, error) { +// without a server release; a flag may be 0/1 or a boolean. +// +// Only a malformed request is a 400. A bad value inside a sample costs that value, and a +// sample without a usable time costs that sample: the car drops a batch the server +// rejects, and one sensor reading out of range must not take hours of queued driving with +// it. What was dropped is listed in Dropped, for the response and the log. +func validateTelemetryBatch(body []byte, now time.Time) (TelemetryBatch, error) { o, err := decodeObject(body) if err != nil { return TelemetryBatch{}, err @@ -266,34 +272,45 @@ func validateTelemetryBatch(body []byte) (TelemetryBatch, error) { if !ok || len(raw) == 0 || len(raw) > maxSamples { return TelemetryBatch{}, invalid("samples is invalid") } - samples := make([]TelemetrySample, 0, len(raw)) + batch := TelemetryBatch{InstallationID: id, Samples: make([]TelemetrySample, 0, len(raw))} + latest := now.Add(maxClockAhead).Unix() for i, entry := range raw { - sample, err := validateSample(entry, fmt.Sprintf("samples[%d]", i)) - if err != nil { - return TelemetryBatch{}, err + name := fmt.Sprintf("samples[%d]", i) + sample, dropped, ok := validateSample(entry, name, latest) + batch.Dropped = append(batch.Dropped, dropped...) + if ok { + batch.Samples = append(batch.Samples, sample) } - samples = append(samples, sample) } - return TelemetryBatch{InstallationID: id, Samples: samples}, nil + return batch, nil } -func validateSample(entry any, name string) (TelemetrySample, error) { +// maxClockAhead is how far in the future a sample may be stamped. The head unit sets its +// clock from GPS; a sample from further ahead has a wrong time, and stored there it would +// pose as the car's latest state until that time came. +const maxClockAhead = time.Hour + +// validateSample keeps every valid field of one sample. It returns false when the sample +// as a whole is unusable: not an object, or no plausible time. dropped names what was left +// out. +func validateSample(entry any, name string, latestUTC int64) (sample TelemetrySample, dropped []string, ok bool) { o, err := asObject(entry, name) if err != nil { - return TelemetrySample{}, err + return TelemetrySample{}, []string{name}, false } utc, err := integer(o["utc"], name+".utc") - if err != nil || utc < utcMin || utc > utcMax { - return TelemetrySample{}, invalid("%s.utc is invalid", name) + if err != nil || utc < utcMin || utc > utcMax || utc > latestUTC { + return TelemetrySample{}, []string{name + ".utc"}, false } - sample := TelemetrySample{UTC: utc, Numbers: map[string]float64{}, Flags: map[string]bool{}} + sample = TelemetrySample{UTC: utc, Numbers: map[string]float64{}, Flags: map[string]bool{}} for field, limits := range telemetryNumbers { if !o.has(field) { continue } f, err := bounded(o[field], limits[0], limits[1], name+"."+field) if err != nil { - return TelemetrySample{}, err + dropped = append(dropped, name+"."+field) + continue } sample.Numbers[field] = f } @@ -311,17 +328,20 @@ func validateSample(entry any, name string) (TelemetrySample, error) { case "0": sample.Flags[field] = false default: - return TelemetrySample{}, invalid("%s.%s is invalid", name, field) + dropped = append(dropped, name+"."+field) } default: - return TelemetrySample{}, invalid("%s.%s is invalid", name, field) + dropped = append(dropped, name+"."+field) } } // A position is only a position as a pair. _, hasLat := sample.Numbers["lat"] _, hasLon := sample.Numbers["lon"] if hasLat != hasLon { - return TelemetrySample{}, invalid("%s.lat and lon must be sent together", name) + delete(sample.Numbers, "lat") + delete(sample.Numbers, "lon") + dropped = append(dropped, name+".lat/lon") } - return sample, nil + sort.Strings(dropped) + return sample, dropped, true } diff --git a/server/validate_test.go b/server/validate_test.go index baead87..8d05212 100644 --- a/server/validate_test.go +++ b/server/validate_test.go @@ -4,6 +4,7 @@ import ( "errors" "strings" "testing" + "time" ) const ( @@ -12,6 +13,9 @@ const ( testNowS = 1_790_000_000 ) +// testNow is the sample time the telemetry tests send, as "now" for the clock-ahead check. +var testNow = time.Unix(testNowS, 0) + func isValidationError(err error) bool { var v *ValidationError return errors.As(err, &v) @@ -125,7 +129,7 @@ func telemetryBody(samples string) []byte { func TestTelemetryKeepsOnlyWhatWasSent(t *testing.T) { b, err := validateTelemetryBatch(telemetryBody( - `{"utc":1790000000,"soc":81,"power":-12.5,"speed":0,"lat":52.1,"lon":7.6,"is_charging":1,"is_dcfc":false}`)) + `{"utc":1790000000,"soc":81,"power":-12.5,"speed":0,"lat":52.1,"lon":7.6,"is_charging":1,"is_dcfc":false}`), testNow) if err != nil { t.Fatal(err) } @@ -146,7 +150,7 @@ func TestTelemetryKeepsOnlyWhatWasSent(t *testing.T) { } func TestTelemetryDropsUnknownFields(t *testing.T) { - b, err := validateTelemetryBatch(telemetryBody(`{"utc":1790000000,"future_field":3}`)) + b, err := validateTelemetryBatch(telemetryBody(`{"utc":1790000000,"future_field":3}`), testNow) if err != nil { t.Fatal(err) } @@ -155,31 +159,49 @@ func TestTelemetryDropsUnknownFields(t *testing.T) { } } -func TestTelemetryRejectsBadSamples(t *testing.T) { - for name, sample := range map[string]string{ - "milliseconds": `{"utc":1790000000000}`, - "fractional utc": `{"utc":1790000000.5}`, - "soc above 100": `{"utc":1790000000,"soc":101}`, - "string speed": `{"utc":1790000000,"speed":"fast"}`, - "flag 2": `{"utc":1790000000,"is_parked":2}`, - "half position": `{"utc":1790000000,"lat":52.1}`, - "not an object": `5`, +func TestTelemetryDropsBadValuesAndKeepsTheRest(t *testing.T) { + for name, c := range map[string]struct { + sample string + dropped string + kept bool // the sample itself survives + }{ + "milliseconds": {`{"utc":1790000000000,"soc":5}`, "samples[0].utc", false}, + "fractional utc": {`{"utc":1790000000.5,"soc":5}`, "samples[0].utc", false}, + "clock unset": {`{"utc":0,"soc":5}`, "samples[0].utc", false}, + "clock a day ahead": {`{"utc":1790086400,"soc":5}`, "samples[0].utc", false}, + "not an object": {`5`, "samples[0]", false}, + "soc above 100": {`{"utc":1790000000,"soc":101,"speed":20}`, "samples[0].soc", true}, + "reversing": {`{"utc":1790000000,"soc":5,"speed":-4}`, "samples[0].speed", true}, + "string speed": {`{"utc":1790000000,"soc":5,"speed":"fast"}`, "samples[0].speed", true}, + "flag 2": {`{"utc":1790000000,"soc":5,"is_parked":2}`, "samples[0].is_parked", true}, + "half position": {`{"utc":1790000000,"soc":5,"lat":52.1}`, "samples[0].lat/lon", true}, } { - if _, err := validateTelemetryBatch(telemetryBody(sample)); !isValidationError(err) { - t.Errorf("%s: want ValidationError, got %v", name, err) + b, err := validateTelemetryBatch(telemetryBody(c.sample), testNow) + if err != nil { + t.Errorf("%s: the batch was rejected: %v", name, err) + continue + } + if len(b.Dropped) != 1 || b.Dropped[0] != c.dropped { + t.Errorf("%s: dropped %v, want %s", name, b.Dropped, c.dropped) + } + if (len(b.Samples) == 1) != c.kept { + t.Errorf("%s: %d samples kept", name, len(b.Samples)) + } + if c.kept && b.Samples[0].Numbers["soc"] != 5 && name != "soc above 100" { + t.Errorf("%s: the valid soc was lost", name) } } } func TestTelemetryRejectsEmptyAndOversizedBatches(t *testing.T) { - if _, err := validateTelemetryBatch(telemetryBody("")); !isValidationError(err) { + if _, err := validateTelemetryBatch(telemetryBody(""), testNow); !isValidationError(err) { t.Error("empty batch accepted") } many := strings.TrimSuffix(strings.Repeat(`{"utc":1790000000},`, 501), ",") - if _, err := validateTelemetryBatch(telemetryBody(many)); !isValidationError(err) { + if _, err := validateTelemetryBatch(telemetryBody(many), testNow); !isValidationError(err) { t.Error("501 samples accepted") } - if _, err := validateTelemetryBatch([]byte(`{"installationId":"nope","samples":[{"utc":1790000000}]}`)); !isValidationError(err) { + if _, err := validateTelemetryBatch([]byte(`{"installationId":"nope","samples":[{"utc":1790000000}]}`), testNow); !isValidationError(err) { t.Error("bad installation id accepted") } }