fix(aprs): stop inventing a transmit passcode, and let the report notices appear
AprsReporter derived a passcode from the callsign whenever the operator's entry was
unusable:
passcode = cfg.passcode.toIntOrNull()?.takeIf { it >= 0 } ?: AprsPacket.passcode(cfg.callsign)
Measured against the shipped algorithm for BG7NTA, whose passcode is 21162, four
inputs produced a transmit passcode the operator never obtained: blank, whitespace,
non-numeric, and an explicit -1. That last one is the documented receive-only value,
so `takeIf { it >= 0 }` also made receive-only unreachable - and a receive-only login
is the one way to confirm a setup works without putting anything on the network,
which is exactly how this feature was supposed to be validated before release.
This is a policy question more than a bug. APRS-IS states that supplying the correct
passcode to a user is the software author's responsibility, and the passcode functions
as a licence check for transmitting. APRSdroid carries the same algorithm in the same
source file and deliberately does not use it to fill a blank, validating the operator's
entry instead. AprsPasscode follows that: it classifies an entry as Transmit,
ReceiveOnly, Mismatch or NotANumber, and anything not usable logs in as -1. The
connection still works and the operator is told separately that reports are not being
forwarded, but no packet goes out under a code the app made up.
AprsPacket.passcode stays, because validating an entry means recomputing the expected
value. Nothing substitutes it for a missing one.
Separately, and worse than the line above: the report notices never appeared at all.
onReport is invoked from AprsReporter's Dispatchers.IO scope, where constructing a
Toast throws because the thread has no Looper - and the surrounding runCatching
swallowed it. So the whole reporting path, including the unverified-login warning
added in the previous commit, was writing messages nobody could see. They now post to
the main looper. The one in startReporting is left alone: onStartCommand already runs
on the main thread.
Still outstanding for APRS, and not addressed here: the 0N 0E position fallback, the
missing TCPIP* path, the unbounded status field, the coroutine delay that does not fire
in Doze, and the dataSync foreground service type. Also unaddressed is the "Compute
passcode" button in AprsCard, which offers the operator the derived value directly and
so contradicts the policy this commit establishes - it was added by request, so it needs
a decision rather than a quiet removal.
This commit is contained in:
1 parent
7ac54f0a37
commit
262ae45432
4 files changed
+222
-2
No files matched your search
@@ -11,6 +11,8 @@ import android.content.SharedPreferences
|
||||
import android.widget.Toast
|
||||
import android.content.pm.ServiceInfo
|
||||
import android.os.Build
|
||||
import android.os.Handler
|
||||
import android.os.Looper
|
||||
import android.os.IBinder
|
||||
import com.rtbishop.look4sat.MainApplication
|
||||
import com.rtbishop.look4sat.core.presentation.R
|
||||
@@ -39,6 +41,16 @@ class AprsForegroundService : Service() {
|
||||
|
||||
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
|
||||
private var reporter: AprsReporter? = null
|
||||
|
||||
/**
|
||||
* Handler on the main looper, for anything that must not run on the reporter's IO thread.
|
||||
*
|
||||
* onReport is invoked from AprsReporter's Dispatchers.IO scope, and Toast construction there
|
||||
* throws because that thread has no Looper - an exception the surrounding runCatching then
|
||||
* swallowed, so every report notice was silently discarded. The messages existed and no
|
||||
* operator ever saw one.
|
||||
*/
|
||||
private val mainHandler = Handler(Looper.getMainLooper())
|
||||
private var lastState: AprsState = AprsState.Idle
|
||||
|
||||
override fun onBind(intent: Intent?): IBinder? = null
|
||||
@@ -99,7 +111,7 @@ class AprsForegroundService : Service() {
|
||||
!report.verified -> getString(R.string.aprs_toast_unverified)
|
||||
else -> getString(R.string.aprs_toast_fail, report.detail)
|
||||
}
|
||||
runCatching {
|
||||
mainHandler.post {
|
||||
Toast.makeText(this, msg,
|
||||
if (report.ok) Toast.LENGTH_SHORT else Toast.LENGTH_LONG).show()
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package com.rtbishop.look4sat.core.data.aprs
|
||||
|
||||
import com.rtbishop.look4sat.core.domain.aprs.AprsPacket
|
||||
import com.rtbishop.look4sat.core.domain.aprs.AprsPasscode
|
||||
import com.rtbishop.look4sat.core.domain.aprs.AprsPosition
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
@@ -101,7 +102,9 @@ class AprsReporter(
|
||||
port = cfg.port,
|
||||
callsign = cfg.callsign,
|
||||
ssid = cfg.ssid,
|
||||
passcode = cfg.passcode.toIntOrNull()?.takeIf { it >= 0 } ?: AprsPacket.passcode(cfg.callsign),
|
||||
// Never derives one: a blank or wrong entry logs in receive-only rather than
|
||||
// transmitting under a passcode the app invented for an unchecked licence.
|
||||
passcode = AprsPasscode.loginValue(cfg.callsign, cfg.passcode),
|
||||
version = "Look4Sat 4.5.4"
|
||||
).also { client = it }
|
||||
if (!c.isConnected) c.connect()
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
/*
|
||||
* Look4Sat. Amateur radio satellite tracker and pass predictor.
|
||||
* Copyright (C) 2019-2026 Arty Bishop and contributors.
|
||||
*
|
||||
* This program is free software: you can redistribute it and/or modify
|
||||
* it under the terms of the GNU General Public License as published by
|
||||
* the Free Software Foundation, either version 3 of the License, or
|
||||
* (at your option) any later version.
|
||||
*
|
||||
* This program is distributed in the hope that it will be useful,
|
||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
* GNU General Public License for more details.
|
||||
*
|
||||
* You should have received a copy of the GNU General Public License
|
||||
* along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
*/
|
||||
package com.rtbishop.look4sat.core.domain.aprs
|
||||
|
||||
/**
|
||||
* Decides what passcode to present to APRS-IS, and whether the operator's entry is usable.
|
||||
*
|
||||
* The app must not derive a transmit passcode for the operator. APRS-IS states that supplying
|
||||
* the correct passcode to a user is the software author's responsibility, and the passcode
|
||||
* exists as a licence check - deriving it in-app and shipping the algorithm defeats the point.
|
||||
* APRSdroid has the same algorithm in the same file and deliberately does not use it for this
|
||||
* reason, validating the operator's entry instead and linking out to request one.
|
||||
*
|
||||
* So this validates. [AprsPacket.passcode] stays, because checking an entry means recomputing
|
||||
* the expected value, but nothing here substitutes a derived code for a missing one.
|
||||
*/
|
||||
object AprsPasscode {
|
||||
|
||||
/** Value that asks APRS-IS for a receive-only connection. Always legitimate. */
|
||||
const val RECEIVE_ONLY = -1
|
||||
|
||||
/** What the operator's passcode entry amounts to. */
|
||||
sealed interface Entry {
|
||||
|
||||
/** A passcode that matches the callsign. Reports will be forwarded. */
|
||||
data class Transmit(val passcode: Int) : Entry
|
||||
|
||||
/**
|
||||
* An explicit -1, or a blank entry.
|
||||
*
|
||||
* A blank entry lands here rather than being filled in with a derived code: connecting
|
||||
* receive-only is honest about what an operator without a passcode can do, where a
|
||||
* derived code silently claims a licence check that was never performed.
|
||||
*/
|
||||
data object ReceiveOnly : Entry
|
||||
|
||||
/** Something was typed but it is not this callsign's passcode. */
|
||||
data class Mismatch(val expectedFor: String) : Entry
|
||||
|
||||
/** Something was typed that is not a number at all. */
|
||||
data object NotANumber : Entry
|
||||
}
|
||||
|
||||
/**
|
||||
* Classify what the operator typed.
|
||||
*
|
||||
* A mismatch is reported rather than corrected, so the UI can refuse to save and say why.
|
||||
* Silently swapping in a derived code is how an operator ends up believing they are
|
||||
* transmitting under a passcode they never obtained.
|
||||
*/
|
||||
fun classify(callsign: String, entry: String): Entry {
|
||||
val trimmed = entry.trim()
|
||||
if (trimmed.isEmpty()) return Entry.ReceiveOnly
|
||||
val value = trimmed.toIntOrNull() ?: return Entry.NotANumber
|
||||
// Checked before the callsign comparison: -1 is the documented receive-only value and
|
||||
// is never anyone's passcode, so comparing it would report a deliberate choice as a typo.
|
||||
if (value == RECEIVE_ONLY) return Entry.ReceiveOnly
|
||||
val call = callsign.trim()
|
||||
if (call.isEmpty()) return Entry.Mismatch("")
|
||||
return if (value == AprsPacket.passcode(call)) {
|
||||
Entry.Transmit(value)
|
||||
} else {
|
||||
Entry.Mismatch(call.uppercase())
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The number to send in the login line for this entry.
|
||||
*
|
||||
* Anything not usable becomes [RECEIVE_ONLY]: the connection still works, the operator is
|
||||
* told separately that their reports are not being forwarded, and no packet goes out under
|
||||
* a passcode the app invented. The previous code sent a derived transmit passcode here,
|
||||
* and `takeIf { it >= 0 }` additionally made an explicit -1 impossible to use - which also
|
||||
* blocked the one safe way to test a setup, since a receive-only login is how you confirm
|
||||
* the connection works without putting anything on the network.
|
||||
*/
|
||||
fun loginValue(callsign: String, entry: String): Int =
|
||||
when (val classified = classify(callsign, entry)) {
|
||||
is Entry.Transmit -> classified.passcode
|
||||
else -> RECEIVE_ONLY
|
||||
}
|
||||
|
||||
/** True when this entry lets the operator's reports reach the network. */
|
||||
fun canTransmit(callsign: String, entry: String): Boolean =
|
||||
classify(callsign, entry) is Entry.Transmit
|
||||
}
|
||||
@@ -0,0 +1,104 @@
|
||||
package com.rtbishop.look4sat.core.domain.aprs
|
||||
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* The rule these pin down: the app never invents a transmit passcode.
|
||||
*
|
||||
* APRS-IS treats the passcode as a licence check and says supplying it to a user is the software
|
||||
* author's job. The previous code derived one from the callsign whenever the entry was blank or
|
||||
* unparseable, so an operator who had never obtained a passcode transmitted anyway - under a
|
||||
* check that was never performed.
|
||||
*/
|
||||
class AprsPasscodeTest {
|
||||
|
||||
/** Recomputed rather than hardcoded: the point is agreement with the shipped algorithm. */
|
||||
private val callsign = "BG7NTA"
|
||||
private val correct = AprsPacket.passcode(callsign)
|
||||
|
||||
@Test
|
||||
fun `the callsign's own passcode allows transmitting`() {
|
||||
assertEquals(
|
||||
AprsPasscode.Entry.Transmit(correct),
|
||||
AprsPasscode.classify(callsign, correct.toString())
|
||||
)
|
||||
assertTrue(AprsPasscode.canTransmit(callsign, correct.toString()))
|
||||
assertEquals(correct, AprsPasscode.loginValue(callsign, correct.toString()))
|
||||
}
|
||||
|
||||
/**
|
||||
* A blank entry becomes receive-only rather than a derived code. This is the defect: the old
|
||||
* line was `cfg.passcode.toIntOrNull()?.takeIf { it >= 0 } ?: AprsPacket.passcode(callsign)`,
|
||||
* so leaving the field empty transmitted under a computed passcode.
|
||||
*/
|
||||
@Test
|
||||
fun `a blank entry connects receive-only instead of deriving one`() {
|
||||
assertEquals(AprsPasscode.Entry.ReceiveOnly, AprsPasscode.classify(callsign, ""))
|
||||
assertEquals(AprsPasscode.Entry.ReceiveOnly, AprsPasscode.classify(callsign, " "))
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue(callsign, ""))
|
||||
assertFalse(AprsPasscode.canTransmit(callsign, ""))
|
||||
}
|
||||
|
||||
/**
|
||||
* -1 is the documented receive-only value and has to survive. `takeIf { it >= 0 }` used to
|
||||
* discard it and substitute a transmit passcode, which removed the only safe way to test a
|
||||
* setup - connecting receive-only puts nothing on the network.
|
||||
*/
|
||||
@Test
|
||||
fun `an explicit -1 stays receive-only`() {
|
||||
assertEquals(AprsPasscode.Entry.ReceiveOnly, AprsPasscode.classify(callsign, "-1"))
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue(callsign, "-1"))
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue(callsign, " -1 "))
|
||||
}
|
||||
|
||||
/** A wrong passcode is reported, not quietly corrected to the right one. */
|
||||
@Test
|
||||
fun `a mismatched passcode is reported rather than fixed`() {
|
||||
val wrong = (correct + 1).toString()
|
||||
assertEquals(AprsPasscode.Entry.Mismatch("BG7NTA"), AprsPasscode.classify(callsign, wrong))
|
||||
assertFalse(AprsPasscode.canTransmit(callsign, wrong))
|
||||
// And it must not be sent: a mismatch logs in receive-only, never as the derived value.
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue(callsign, wrong))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `something that is not a number is its own case`() {
|
||||
assertEquals(AprsPasscode.Entry.NotANumber, AprsPasscode.classify(callsign, "abcde"))
|
||||
assertEquals(AprsPasscode.Entry.NotANumber, AprsPasscode.classify(callsign, "12a45"))
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue(callsign, "abcde"))
|
||||
}
|
||||
|
||||
/** Surrounding whitespace from a paste must not turn a valid passcode into a mismatch. */
|
||||
@Test
|
||||
fun `whitespace around a valid passcode is tolerated`() {
|
||||
assertTrue(AprsPasscode.canTransmit(callsign, " $correct "))
|
||||
}
|
||||
|
||||
/** The passcode depends on the callsign, so it must not validate against a different one. */
|
||||
@Test
|
||||
fun `a passcode for one callsign does not validate another`() {
|
||||
assertFalse(AprsPasscode.canTransmit("W1AW", correct.toString()))
|
||||
assertEquals(
|
||||
AprsPasscode.Entry.Mismatch("W1AW"),
|
||||
AprsPasscode.classify("W1AW", correct.toString())
|
||||
)
|
||||
}
|
||||
|
||||
/** Case must not matter: the algorithm upper-cases before hashing. */
|
||||
@Test
|
||||
fun `a lower case callsign validates the same passcode`() {
|
||||
assertTrue(AprsPasscode.canTransmit("bg7nta", correct.toString()))
|
||||
}
|
||||
|
||||
/** The SSID is not part of the hash, so the base callsign's code is the right one. */
|
||||
@Test
|
||||
fun `no callsign cannot transmit`() {
|
||||
assertEquals(AprsPasscode.Entry.Mismatch(""), AprsPasscode.classify("", "12345"))
|
||||
assertEquals(AprsPasscode.RECEIVE_ONLY, AprsPasscode.loginValue("", "12345"))
|
||||
// A blank entry with a blank callsign is still just receive-only, not a mismatch.
|
||||
assertEquals(AprsPasscode.Entry.ReceiveOnly, AprsPasscode.classify("", ""))
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user