Merge branch 'fix/directory-address-and-refusal-copy' into 'main'
Read the contact's address from the directory, and name the refusals See merge request paulhitt/flamenet-messenger-android!91
This commit was merged in pull request #118.
This commit is contained in:
@@ -1,7 +1,6 @@
|
|||||||
package io.flamenet.messenger.app
|
package io.flamenet.messenger.app
|
||||||
|
|
||||||
import androidx.compose.foundation.background
|
import androidx.compose.foundation.background
|
||||||
import androidx.compose.foundation.border
|
|
||||||
import androidx.compose.foundation.clickable
|
import androidx.compose.foundation.clickable
|
||||||
import androidx.compose.foundation.layout.Arrangement
|
import androidx.compose.foundation.layout.Arrangement
|
||||||
import androidx.compose.foundation.layout.Box
|
import androidx.compose.foundation.layout.Box
|
||||||
@@ -63,24 +62,24 @@ fun CallScreenBody(
|
|||||||
Column(
|
Column(
|
||||||
modifier = Modifier
|
modifier = Modifier
|
||||||
.fillMaxSize()
|
.fillMaxSize()
|
||||||
.background(MaterialTheme.colorScheme.background)
|
// ⚠️ The app's ground, not `colorScheme.background`. That token is
|
||||||
|
// a flat blue, and painting it full-screen here stamped a solid
|
||||||
|
// rectangle over the gradient every other screen shows — which
|
||||||
|
// read as "a blue box around the call area". The brush is used
|
||||||
|
// rather than transparency so this composable still stands on its
|
||||||
|
// own in the screenshot harness.
|
||||||
|
.background(groundBrush())
|
||||||
.padding(horizontal = Kit.margin)
|
.padding(horizontal = Kit.margin)
|
||||||
.testTag("call-screen"),
|
.testTag("call-screen"),
|
||||||
horizontalAlignment = Alignment.CenterHorizontally,
|
horizontalAlignment = Alignment.CenterHorizontally,
|
||||||
) {
|
) {
|
||||||
Spacer(Modifier.height(Space.xxl))
|
Spacer(Modifier.height(Space.xxl))
|
||||||
|
|
||||||
|
// ⚠️ No rings. A pair of translucent `primary` circles sat behind the
|
||||||
|
// avatar as a stand-in for the kit's animation, and against the
|
||||||
|
// gradient they read as banding — two hard edges around the one thing
|
||||||
|
// on screen. The avatar carries the centre on its own.
|
||||||
Box(contentAlignment = Alignment.Center) {
|
Box(contentAlignment = Alignment.Center) {
|
||||||
Box(
|
|
||||||
modifier = Modifier
|
|
||||||
.size(260.dp)
|
|
||||||
.border(24.dp, MaterialTheme.colorScheme.primary.copy(alpha = 0.10f), CircleShape)
|
|
||||||
)
|
|
||||||
Box(
|
|
||||||
modifier = Modifier
|
|
||||||
.size(190.dp)
|
|
||||||
.border(16.dp, MaterialTheme.colorScheme.primary.copy(alpha = 0.18f), CircleShape)
|
|
||||||
)
|
|
||||||
Box(
|
Box(
|
||||||
modifier = Modifier.size(128.dp).clip(CircleShape).background(Palette.AvatarFill),
|
modifier = Modifier.size(128.dp).clip(CircleShape).background(Palette.AvatarFill),
|
||||||
contentAlignment = Alignment.Center,
|
contentAlignment = Alignment.Center,
|
||||||
|
|||||||
@@ -5,6 +5,8 @@ import androidx.lifecycle.AndroidViewModel
|
|||||||
import androidx.lifecycle.viewModelScope
|
import androidx.lifecycle.viewModelScope
|
||||||
import io.flamenet.client.ChainRpc
|
import io.flamenet.client.ChainRpc
|
||||||
import io.flamenet.client.IdentityChangedException
|
import io.flamenet.client.IdentityChangedException
|
||||||
|
import io.flamenet.client.KeyLogForkException
|
||||||
|
import io.flamenet.client.KeyNotLoggedException
|
||||||
import io.flamenet.client.DeviceClock
|
import io.flamenet.client.DeviceClock
|
||||||
import io.flamenet.client.AccountsClient
|
import io.flamenet.client.AccountsClient
|
||||||
import io.flamenet.client.RelayClient
|
import io.flamenet.client.RelayClient
|
||||||
@@ -3901,6 +3903,24 @@ internal fun describeFailure(failure: Throwable): String = when (failure) {
|
|||||||
is IdentityChangedException ->
|
is IdentityChangedException ->
|
||||||
"Their security code changed. Open the conversation to review it " +
|
"Their security code changed. Open the conversation to review it " +
|
||||||
"before sending anything else."
|
"before sending anything else."
|
||||||
|
// 🔴 **The other two refusals reached the generic network line.**
|
||||||
|
// Only `IdentityChangedException` was named, and the send path throws
|
||||||
|
// these just as often — a key the log does not account for is the
|
||||||
|
// failure a person actually hits after a contact reinstalls. "Something
|
||||||
|
// went wrong reaching that server" sent them to check their signal for a
|
||||||
|
// decision this phone made, and the server was answering fine.
|
||||||
|
is KeyNotLoggedException -> if (failure.hadEntries) {
|
||||||
|
"This contact's device is offering a key that doesn't match what " +
|
||||||
|
"the server has on record for it. Open the conversation to " +
|
||||||
|
"review their security code before sending anything else."
|
||||||
|
} else {
|
||||||
|
"This contact's device is offering a key the server never recorded. " +
|
||||||
|
"Nothing was sent. Check their security code with them directly."
|
||||||
|
}
|
||||||
|
is KeyLogForkException ->
|
||||||
|
"This server's record of published keys doesn't add up, so nothing " +
|
||||||
|
"was sent. That can mean tampering \u2014 check this contact's " +
|
||||||
|
"security code with them another way before continuing."
|
||||||
is Vault.Corrupt -> "This phone's stored keys could not be read."
|
is Vault.Corrupt -> "This phone's stored keys could not be read."
|
||||||
|
|
||||||
// 🔴 **Anything not named above used to reach the screen verbatim.**
|
// 🔴 **Anything not named above used to reach the screen verbatim.**
|
||||||
|
|||||||
@@ -612,14 +612,21 @@ class Conversations(
|
|||||||
val theirs = bundles.filter { it.deviceId != identity.deviceId }
|
val theirs = bundles.filter { it.deviceId != identity.deviceId }
|
||||||
if (theirs.isEmpty()) return
|
if (theirs.isEmpty()) return
|
||||||
|
|
||||||
// The pinned address is passed back in so a CHANGED address is caught
|
// ⚠️ **The address compared has to be the one just SERVED.** This
|
||||||
// here too, not only a changed key.
|
// read the pin and passed it straight back into the comparison, which
|
||||||
|
// compares it against itself — so the "a different address is a
|
||||||
|
// different person" rule could never fire on a send. It reads as
|
||||||
|
// working code and tests as working code; only a relay actually
|
||||||
|
// swapping an address would have shown it.
|
||||||
val pinned = trust.pinnedFnid(toUserId)
|
val pinned = trust.pinnedFnid(toUserId)
|
||||||
if (trust.record(toUserId, theirs.map { it.identitySigning }, pinned)
|
if (trust.record(toUserId, theirs.map { it.identitySigning }, servedFnid ?: pinned)
|
||||||
== IdentityTrust.CHANGED
|
== IdentityTrust.CHANGED
|
||||||
) {
|
) {
|
||||||
throw IdentityChangedException(toUserId)
|
throw IdentityChangedException(toUserId)
|
||||||
}
|
}
|
||||||
|
// Only once the keys are accepted — an address learned alongside keys we
|
||||||
|
// are about to refuse is not one to pin.
|
||||||
|
servedFnid?.let { trust.rememberFnid(toUserId, it) }
|
||||||
// The address just served wins over the pinned one: a device that
|
// The address just served wins over the pinned one: a device that
|
||||||
// registered a moment ago is logged under whatever the account claims
|
// registered a moment ago is logged under whatever the account claims
|
||||||
// NOW, and the pin may predate the claim.
|
// NOW, and the pin may predate the claim.
|
||||||
@@ -664,7 +671,8 @@ class Conversations(
|
|||||||
// "Too many attempts". The directory costs nothing; full bundles are
|
// "Too many attempts". The directory costs nothing; full bundles are
|
||||||
// fetched only when some device has no session yet, which is the one
|
// fetched only when some device has no session yet, which is the one
|
||||||
// moment X3DH actually needs them.
|
// moment X3DH actually needs them.
|
||||||
val directory = relay.listDevices(toUserId)
|
val served = relay.listDirectory(toUserId)
|
||||||
|
val directory = served.devices
|
||||||
val stubs = (0 until directory.length()).map { i ->
|
val stubs = (0 until directory.length()).map { i ->
|
||||||
val o = directory.getJSONObject(i)
|
val o = directory.getJSONObject(i)
|
||||||
PeerBundle(
|
PeerBundle(
|
||||||
@@ -700,13 +708,18 @@ class Conversations(
|
|||||||
val needsEstablish = stubs.any {
|
val needsEstablish = stubs.any {
|
||||||
it.deviceId != identity.deviceId && existingSession(it.deviceId) == null
|
it.deviceId != identity.deviceId && existingSession(it.deviceId) == null
|
||||||
}
|
}
|
||||||
// ⚠️ The address comes back with the bundles and is REMEMBERED. The
|
// 🔴 **The directory serves the address too, and it has to.** A key
|
||||||
// directory route serves no fnid, so a later send that needs no bundle
|
// log leaf is written under `fnid:` once an account has claimed an
|
||||||
// would otherwise be unable to check the log again.
|
// address; a verifier that does not know the address computes
|
||||||
var servedFnid: String? = null
|
// `local:<audience>/<id>`, matches no leaf, and refuses the send as an
|
||||||
|
// unlogged key — reported as "something went wrong reaching that
|
||||||
|
// server". Bundles are fetched only when a session needs establishing,
|
||||||
|
// so every later send saw the directory alone; falling back to the pin
|
||||||
|
// failed outright for a contact pinned before addresses existed.
|
||||||
|
var servedFnid: String? = served.fnid
|
||||||
val bundles = if (!needsEstablish) stubs else {
|
val bundles = if (!needsEstablish) stubs else {
|
||||||
val keys = relay.fetchKeys(toUserId)
|
val keys = relay.fetchKeys(toUserId)
|
||||||
servedFnid = keys.fnid
|
servedFnid = keys.fnid ?: servedFnid
|
||||||
keys.fnid?.let { trust.rememberFnid(toUserId, it) }
|
keys.fnid?.let { trust.rememberFnid(toUserId, it) }
|
||||||
val full = keys.bundles.associateBy { it.deviceId }
|
val full = keys.bundles.associateBy { it.deviceId }
|
||||||
// Union: every device the directory knows, carrying a full bundle
|
// Union: every device the directory knows, carrying a full bundle
|
||||||
|
|||||||
@@ -201,7 +201,30 @@ class RelayClient(
|
|||||||
|
|
||||||
/** `GET /e2e/devices/{user}` — who a user is, costing no prekey. */
|
/** `GET /e2e/devices/{user}` — who a user is, costing no prekey. */
|
||||||
fun listDevices(userId: Long): JSONArray =
|
fun listDevices(userId: Long): JSONArray =
|
||||||
getJson("/e2e/devices/$userId").optJSONArray("devices") ?: JSONArray()
|
listDirectory(userId).devices
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The directory, with the account's address alongside it.
|
||||||
|
*
|
||||||
|
* ⚠️ The address is what makes the key log checkable: a leaf is written
|
||||||
|
* under `fnid:` once an account has claimed one, and a verifier that does
|
||||||
|
* not know it computes the wrong subject and refuses the send. The send
|
||||||
|
* path deliberately fetches full bundles only when a session needs
|
||||||
|
* establishing, so on every other message this is the only place the
|
||||||
|
* address can come from.
|
||||||
|
*/
|
||||||
|
fun listDirectory(userId: Long): Directory {
|
||||||
|
val o = getJson("/e2e/devices/$userId")
|
||||||
|
return Directory(
|
||||||
|
devices = o.optJSONArray("devices") ?: JSONArray(),
|
||||||
|
// Absent on a relay older than this field, which is a relay whose
|
||||||
|
// accounts have no addresses either — null is then correct, not a
|
||||||
|
// degraded answer.
|
||||||
|
fnid = o.optString("fnid").takeIf { it.isNotEmpty() },
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
data class Directory(val devices: JSONArray, val fnid: String?)
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* `DELETE /e2e/devices/{id}` — remove one of the ACCOUNT'S OWN devices.
|
* `DELETE /e2e/devices/{id}` — remove one of the ACCOUNT'S OWN devices.
|
||||||
|
|||||||
@@ -259,6 +259,16 @@ class Trust(
|
|||||||
fun rememberFnid(userId: Long, fnid: String) {
|
fun rememberFnid(userId: Long, fnid: String) {
|
||||||
val existing = vault.loadTrust(userId) ?: return
|
val existing = vault.loadTrust(userId) ?: return
|
||||||
if (existing.fnid == fnid) return
|
if (existing.fnid == fnid) return
|
||||||
|
// 🔴 **The paragraph above was documentation, not behaviour.** This
|
||||||
|
// overwrote a DIFFERENT address in place — the one thing it says it must
|
||||||
|
// never do — and it went unnoticed while only the bundle route carried
|
||||||
|
// an address, because that route is reached once per conversation. The
|
||||||
|
// directory carries it on every send, so a silent replacement here
|
||||||
|
// would erase the change the pin exists to notice.
|
||||||
|
if (existing.fnid != null) {
|
||||||
|
markChanged(userId, existing)
|
||||||
|
return
|
||||||
|
}
|
||||||
vault.saveTrust(userId, existing.copy(fnid = fnid))
|
vault.saveTrust(userId, existing.copy(fnid = fnid))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -95,6 +95,39 @@ class TrustTest {
|
|||||||
assertEquals(listOf(Wire.b64(key(1))), vault.loadTrust(7)?.identities)
|
assertEquals(listOf(Wire.b64(key(1))), vault.loadTrust(7)?.identities)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 🔴 **The doc comment on `rememberFnid` said this and the code did the
|
||||||
|
* opposite.** It overwrote a different address in place — the one thing an
|
||||||
|
* address pin exists to prevent, since an fnid is derived from the account
|
||||||
|
* key, so a new address is not a new device but a different account.
|
||||||
|
*
|
||||||
|
* ⚠️ It went unnoticed while only the bundle route carried an address:
|
||||||
|
* that route is reached once per conversation, when a session is first
|
||||||
|
* established. The directory carries it on every send.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `a different address is a change, not a quiet replacement`() {
|
||||||
|
val t = trust()
|
||||||
|
t.record(7, listOf(key(1)), fnid = "fnid:first")
|
||||||
|
t.rememberFnid(7, "fnid:second")
|
||||||
|
|
||||||
|
assertEquals("fnid:first", vault.loadTrust(7)?.fnid,
|
||||||
|
"the pinned address was replaced instead of challenged")
|
||||||
|
assertEquals(IdentityTrust.CHANGED, t.state(7))
|
||||||
|
}
|
||||||
|
|
||||||
|
/** An address learned for the first time is simply pinned. */
|
||||||
|
@Test
|
||||||
|
fun `an address is adopted when none was pinned`() {
|
||||||
|
val t = trust()
|
||||||
|
t.record(7, listOf(key(1)))
|
||||||
|
assertNull(vault.loadTrust(7)?.fnid)
|
||||||
|
|
||||||
|
t.rememberFnid(7, "fnid:first")
|
||||||
|
assertEquals("fnid:first", vault.loadTrust(7)?.fnid)
|
||||||
|
assertEquals(IdentityTrust.PINNED, t.state(7))
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `a change drops the verified tick`() {
|
fun `a change drops the verified tick`() {
|
||||||
val t = trust()
|
val t = trust()
|
||||||
|
|||||||
Reference in New Issue
Block a user