diff --git a/echolot-app/app/src/main/kotlin/app/echo_lot/app/RunViewModel.kt b/echolot-app/app/src/main/kotlin/app/echo_lot/app/RunViewModel.kt index f09e834..319ad66 100644 --- a/echolot-app/app/src/main/kotlin/app/echo_lot/app/RunViewModel.kt +++ b/echolot-app/app/src/main/kotlin/app/echo_lot/app/RunViewModel.kt @@ -387,17 +387,54 @@ class RunViewModel(app: Application) : AndroidViewModel(app) { } /** - * Was IPv6 actually provisioned on any network? A global (non-link-local) v6 address or a - * v6 default route means the network claims to offer IPv6 — link-local only does not count. + * Was IPv6 provisioned on the network a test actually ran over? + * + * This deliberately asks about one network rather than about the device. Answering "does any + * network here have IPv6" produces a real false positive on a phone, and it is not hypothetical: + * an IPv4-only wifi with working cellular alongside it reports "IPv6 is configured, but ICMPv6 + * gets no reply" — configured on cellular, pinged over wifi, and the two never met. + * + * A global (non-link-local) address or a v6 default route means the network claims to offer + * IPv6; link-local only does not count, since every interface has one. */ - private fun ipv6Provisioned(networks: List): Boolean = - networks.any { n -> + private fun ipv6Provisioned( + networks: List, + networkRef: String?, + ): Boolean { + // No reference means the test was not per-network; fall back to the device-wide reading + // rather than silently reporting nothing. + val scope = networks.filter { networkRef == null || it.id == networkRef } + return scope.any { n -> n.link.addresses.any { a -> a.addr.contains(':') && !a.addr.startsWith("fe80", ignoreCase = true) && !a.addr.startsWith("::1") } || n.link.routes.any { it.dst == "::/0" } } + } + + /** + * Per-network ICMP outcomes, keyed by network id. + * + * Reads the structured evidence the probe records rather than its prose detail — a finding + * that depended on the wording of a human-readable string would break silently the first time + * that wording improved. + */ + private fun icmpResults(t: Test): Map { + val out = HashMap() + val ev = t.evidence ?: return out + for ((_, v) in ev) { + val o = v as? kotlinx.serialization.json.JsonObject ?: continue + val ref = (o["network_ref"] as? kotlinx.serialization.json.JsonPrimitive)?.content ?: continue + val ok = (o["ok"] as? kotlinx.serialization.json.JsonPrimitive)?.content == "true" + out[ref] = ok + } + return out + } + + /** Human-facing name for the network a test ran over; falls back to something readable. */ + private fun ifaceOf(networks: List, ref: String?): String = + networks.firstOrNull { it.id == ref }?.iface?.takeIf { it.isNotBlank() } ?: "this network" /** Minimal first-pass findings from device-tier evidence; the registry grows with the suite. */ private fun deriveFindings(tests: List, networks: List): List { @@ -526,31 +563,55 @@ class RunViewModel(app: Application) : AndroidViewModel(app) { ) } } - if (t.type == TestType.ICMP_PING6 && t.status == TestStatus.FAILED) { - // A network with no IPv6 at all is NORMAL — most networks are still IPv4-only, - // and that is not a defect. What IS a defect is IPv6 that the network claims to - // provide (a global address or a default route from RA/DHCPv6) but that does not - // work: that causes Happy-Eyeballs delays, timeouts and hangs. So the severity - // depends on whether v6 was provisioned at all. - if (ipv6Provisioned(networks)) { + if (t.type == TestType.ICMP_PING6) { + // A network with no IPv6 at all is NORMAL — most networks are still IPv4-only, and + // that is not a defect. What IS a defect is IPv6 the network claims to provide (a + // global address or a default route from RA/DHCPv6) that does not work: that causes + // Happy-Eyeballs delays, timeouts and hangs. + // + // Judged per network, from the per-network evidence rather than the aggregate + // status. The aggregate can only say "some network answered", and on a phone with + // wifi and cellular up at once that is how "IPv6 is configured but gets no reply" + // ends up describing a network where IPv6 was never configured in the first place. + val results = icmpResults(t) + var anyV6Network = false + for (n in networks) { + val provisioned = ipv6Provisioned(networks, n.id) + if (provisioned) anyV6Network = true + val ok = results[n.id] ?: continue + if (!provisioned || ok) continue + val where = n.iface?.takeIf { it.isNotBlank() } ?: "this network" out.add( Finding( id = ids.uuid(), code = FindingRegistry.V6_NO_ICMP_REPLY.code, category = FindingRegistry.V6_NO_ICMP_REPLY.category, - severity = FindingRegistry.V6_NO_ICMP_REPLY.severity, confidence = Confidence.MEDIUM, - title = "IPv6 is configured, but ICMPv6 gets no reply", - description = "This network advertises IPv6 (a global address and/or a default route), but ICMPv6 echo got no reply on any network. That has two explanations which look identical from here: IPv6 is broken, or ICMPv6 is filtered while IPv6 itself works. Filtering is common and is a fault in its own right — it breaks Path MTU Discovery, so large packets vanish rather than being reported as too big.", + severity = FindingRegistry.V6_NO_ICMP_REPLY.severity, + confidence = Confidence.MEDIUM, + title = "IPv6 is configured, but ICMPv6 gets no reply ($where)", + description = "$where advertises IPv6 (a global address and/or a " + + "default route), but ICMPv6 echo got no reply over it. That has " + + "two explanations which look identical from here: IPv6 is broken, " + + "or ICMPv6 is filtered while IPv6 itself works. Filtering is " + + "common and is a fault in its own right — it breaks Path MTU " + + "Discovery, so large packets vanish rather than being reported as " + + "too big.", evidenceRefs = listOf(EvidenceRef(t.id)), ) ) - } else { + } + if (!anyV6Network) { + // Said once for the device, not once per interface: "this network is IPv4-only" + // repeated per interface reads as several problems instead of one observation. out.add( Finding( id = ids.uuid(), code = FindingRegistry.V6_NOT_OFFERED.code, category = FindingRegistry.V6_NOT_OFFERED.category, - severity = FindingRegistry.V6_NOT_OFFERED.severity, confidence = Confidence.HIGH, + severity = FindingRegistry.V6_NOT_OFFERED.severity, + confidence = Confidence.HIGH, title = "IPv4-only network (no IPv6 offered)", - description = "No IPv6 address or default route was provisioned, so IPv6 tests could not run. This is normal — many networks are still IPv4-only and it is not a fault.", + description = "No IPv6 address or default route was provisioned on " + + "any active network, so IPv6 tests could not run. This is normal " + + "— many networks are still IPv4-only and it is not a fault.", evidenceRefs = listOf(EvidenceRef(t.id)), ) ) diff --git a/echolot-app/core-probe/src/main/kotlin/app/echo_lot/probe/IcmpProbe.kt b/echolot-app/core-probe/src/main/kotlin/app/echo_lot/probe/IcmpProbe.kt index ddd2bff..af9f3dc 100644 --- a/echolot-app/core-probe/src/main/kotlin/app/echo_lot/probe/IcmpProbe.kt +++ b/echolot-app/core-probe/src/main/kotlin/app/echo_lot/probe/IcmpProbe.kt @@ -40,24 +40,35 @@ class IcmpProbe( override suspend fun run(ctx: Context, ids: ProbeIds): Test = withContext(Dispatchers.IO) { val b = TestBuilder(type, tier, ids) - val perNetwork = LinkedHashMap() + val perNetwork = LinkedHashMap>() var anyOk = false val rtts = ArrayList() // Default network first, then each active network explicitly. attempt(null).let { (ok, detail, rtt) -> - perNetwork["default"] = detail; if (ok) { anyOk = true; rtt?.let(rtts::add) } + perNetwork["default"] = Triple(null, ok, detail); if (ok) { anyOk = true; rtt?.let(rtts::add) } } for (e in entries) { val label = "${e.model.transport.name.lowercase()}:${e.model.id}" val (ok, detail, rtt) = attempt(e.handle) - perNetwork[label] = detail + perNetwork[label] = Triple(e.model.id, ok, detail) if (ok) { anyOk = true; rtt?.let(rtts::add) } } + // Per-network results are recorded structurally, not just as prose. The aggregate status + // can only say "some network answered"; a finding needs to know *which* network failed, + // and recovering that by parsing a human-readable detail string would be a trap waiting to + // spring the first time the wording changes. val evidence: JsonObject = buildJsonObject { put("target", target) - for ((k, v) in perNetwork) put(k, v) + for ((label, r) in perNetwork) { + val (netId, ok, detail) = r + put(label, buildJsonObject { + netId?.let { put("network_ref", it) } + put("ok", ok) + put("detail", detail) + }) + } } val metrics: JsonObject = buildJsonObject { put("networks_ok", rtts.size) diff --git a/server/internal/adminauth/adminauth.go b/server/internal/adminauth/adminauth.go index 9f06e8c..270fd9a 100644 --- a/server/internal/adminauth/adminauth.go +++ b/server/internal/adminauth/adminauth.go @@ -166,12 +166,21 @@ func (t *Throttle) Succeeded() { // ---- sessions --------------------------------------------------------------------------- -// Session is an authenticated admin, however they proved it. +// Session is an authenticated account, however it proved itself. Not necessarily an admin: +// signing in and being allowed to administer the server are separate questions, and a plain user +// gets a session so they can manage their own uploads. type Session struct { // Subject is the account id: "local:" or "#" from OIDC. Subject string // Display is what the UI shows. Display string + // Admin is authorisation, decided at sign-in and carried inside the signed payload. + // + // Inside, specifically — not derived later from the subject, and not stored beside the MAC. + // A flag outside the signature is a privilege escalation anyone can perform with a text + // editor, and re-deriving it per request would mean re-reading group membership from the IdP + // on a path that has no token to do it with. + Admin bool Expires time.Time } @@ -203,12 +212,16 @@ func NewSecret() ([]byte, error) { var ErrSession = errors.New("session is not valid") -// Issue returns the cookie value for a newly authenticated admin. -func (s *Sessions) Issue(subject, display string) string { +// Issue returns the cookie value for a newly authenticated account. +func (s *Sessions) Issue(subject, display string, admin bool) string { exp := time.Now().Add(s.ttl).Unix() + role := "u" + if admin { + role = "a" + } payload := base64.RawURLEncoding.EncodeToString([]byte(subject)) + "." + base64.RawURLEncoding.EncodeToString([]byte(display)) + "." + - strconv.FormatInt(exp, 10) + strconv.FormatInt(exp, 10) + "." + role return payload + "." + s.mac(payload) } @@ -225,7 +238,7 @@ func (s *Sessions) Parse(value string) (*Session, error) { return nil, ErrSession } parts := strings.Split(payload, ".") - if len(parts) != 3 { + if len(parts) != 4 { return nil, ErrSession } subject, err := base64.RawURLEncoding.DecodeString(parts[0]) @@ -243,7 +256,13 @@ func (s *Sessions) Parse(value string) (*Session, error) { if time.Now().After(time.Unix(exp, 0)) { return nil, fmt.Errorf("%w: expired", ErrSession) } - return &Session{Subject: string(subject), Display: string(display), Expires: time.Unix(exp, 0)}, nil + // Anything that is not exactly the admin marker is a user. A malformed role must fail closed: + // the safe reading of an unparseable privilege claim is the smaller privilege. + admin := parts[3] == "a" + return &Session{ + Subject: string(subject), Display: string(display), + Admin: admin, Expires: time.Unix(exp, 0), + }, nil } func (s *Sessions) mac(payload string) string { diff --git a/server/internal/adminauth/adminauth_test.go b/server/internal/adminauth/adminauth_test.go index 845870a..a371fd1 100644 --- a/server/internal/adminauth/adminauth_test.go +++ b/server/internal/adminauth/adminauth_test.go @@ -4,6 +4,8 @@ package adminauth import ( + "encoding/base64" + "strconv" "strings" "testing" "time" @@ -96,7 +98,7 @@ func TestAnEmptyCredentialNeverVerifies(t *testing.T) { func TestSessionRoundTrip(t *testing.T) { secret, _ := NewSecret() s := NewSessions(secret, time.Hour) - got, err := s.Parse(s.Issue("local:admin", "Admin")) + got, err := s.Parse(s.Issue("local:admin", "Admin", true)) if err != nil { t.Fatal(err) } @@ -110,7 +112,7 @@ func TestSessionRoundTrip(t *testing.T) { func TestTamperedSessionsAreRejected(t *testing.T) { secret, _ := NewSecret() s := NewSessions(secret, time.Hour) - good := s.Issue("local:admin", "Admin") + good := s.Issue("local:admin", "Admin", true) parts := strings.Split(good, ".") tampered := []string{ @@ -131,7 +133,7 @@ func TestTamperedSessionsAreRejected(t *testing.T) { func TestSessionsFromAnotherSecretAreRejected(t *testing.T) { a, _ := NewSecret() b, _ := NewSecret() - issued := NewSessions(a, time.Hour).Issue("local:admin", "Admin") + issued := NewSessions(a, time.Hour).Issue("local:admin", "Admin", true) if _, err := NewSessions(b, time.Hour).Parse(issued); err == nil { t.Fatal("a session signed with a different secret was accepted — rotating the secret " + "must invalidate every existing session") @@ -143,7 +145,7 @@ func TestExpiredSessionsAreRejected(t *testing.T) { // A negative TTL is not reachable through NewSessions, so issue with a real one and check // the boundary via a session that has already run out. s := NewSessions(secret, time.Millisecond) - v := s.Issue("local:admin", "Admin") + v := s.Issue("local:admin", "Admin", true) time.Sleep(10 * time.Millisecond) if _, err := s.Parse(v); err == nil { t.Fatal("an expired session was accepted") @@ -201,3 +203,46 @@ func TestThrottleForgivesAfterAQuietPeriod(t *testing.T) { t.Fatalf("an operator returning later was still throttled: %v", d) } } + +// The admin flag is an authorisation decision carried in a cookie the client holds, so the +// interesting cases are all about what happens when the client lies about it. +func TestSessionAdminFlag(t *testing.T) { + s := NewSessions([]byte("secret"), time.Hour) + + t.Run("round trips both ways", func(t *testing.T) { + admin, err := s.Parse(s.Issue("local:admin", "Admin", true)) + if err != nil || !admin.Admin { + t.Fatalf("admin session did not survive: %+v err=%v", admin, err) + } + user, err := s.Parse(s.Issue("oidc#1", "Markus", false)) + if err != nil || user.Admin { + t.Fatalf("user session came back as admin: %+v err=%v", user, err) + } + }) + + t.Run("promoting yourself invalidates the cookie", func(t *testing.T) { + // The whole point of putting the flag inside the MAC: editing it must break the signature + // rather than produce a valid admin session. + v := s.Issue("oidc#1", "Markus", false) + i := strings.LastIndex(v, ".") + tampered := strings.TrimSuffix(v[:i], ".u") + ".a" + v[i:] + if got, err := s.Parse(tampered); err == nil { + t.Fatalf("a self-promoted cookie was accepted as %+v", got) + } + }) + + t.Run("an unparseable role is not an admin", func(t *testing.T) { + // Fail closed: whatever a malformed privilege claim means, it does not mean "more access". + // Signed by us, so it passes the MAC — only the role parsing stands between it and admin. + exp := strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10) + payload := base64.RawURLEncoding.EncodeToString([]byte("oidc#1")) + "." + + base64.RawURLEncoding.EncodeToString([]byte("Markus")) + "." + exp + ".ADMIN" + sess, err := s.Parse(payload + "." + s.mac(payload)) + if err != nil { + t.Fatalf("unexpected parse error: %v", err) + } + if sess.Admin { + t.Fatal("a role of \"ADMIN\" was treated as the admin marker") + } + }) +} diff --git a/server/internal/adminui/auth.go b/server/internal/adminui/auth.go index bcb4130..752ef76 100644 --- a/server/internal/adminui/auth.go +++ b/server/internal/adminui/auth.go @@ -83,13 +83,17 @@ func (s *Server) Handler() http.Handler { mux.HandleFunc("GET /admin/callback", s.oidcCallback) mux.HandleFunc("POST /logout", s.logout) + // Any signed-in account. These handlers scope what they show to the session themselves — + // an admin sees everything, a user sees their own devices and runs. mux.HandleFunc("GET /", s.guard(s.dashboard)) mux.HandleFunc("GET /devices", s.guard(s.devices)) - mux.HandleFunc("POST /devices/{id}/revoke", s.guard(s.revokeDevice)) - mux.HandleFunc("POST /enroll-tokens", s.guard(s.mintToken)) mux.HandleFunc("GET /runs", s.guard(s.runsList)) mux.HandleFunc("GET /runs/{device}/{id}", s.guard(s.runView)) mux.HandleFunc("POST /runs/{device}/{id}/delete", s.guard(s.runDelete)) + // Deleting your own upload is yours to do; revoking a device or minting an enrolment token + // affects the whole server, so those stay with the admin. + mux.HandleFunc("POST /devices/{id}/revoke", s.guard(s.adminOnly(s.revokeDevice))) + mux.HandleFunc("POST /enroll-tokens", s.guard(s.adminOnly(s.mintToken))) return mux } @@ -140,10 +144,28 @@ func (s *Server) csrfOK(r *http.Request, sess *adminauth.Session) bool { return r.PostFormValue(csrfField) == s.csrfToken(sess) } -func (s *Server) setSession(w http.ResponseWriter, subject, display string) { +// adminOnly refuses a handler to a signed-in account that is not an administrator. +// +// A separate wrapper rather than a check inside each handler: an authorisation rule that has to be +// remembered in every handler is one that will eventually be forgotten in a new one, and the route +// table is where someone looks to find out who may do what. +func (s *Server) adminOnly( + h func(http.ResponseWriter, *http.Request, *adminauth.Session), +) func(http.ResponseWriter, *http.Request, *adminauth.Session) { + return func(w http.ResponseWriter, r *http.Request, sess *adminauth.Session) { + if !sess.Admin { + slog.Info("admin action refused", "account", sess.Subject, "path", r.URL.Path) + http.Error(w, "that action needs an administrator account", http.StatusForbidden) + return + } + h(w, r, sess) + } +} + +func (s *Server) setSession(w http.ResponseWriter, subject, display string, admin bool) { http.SetCookie(w, &http.Cookie{ Name: sessionCookie, - Value: s.Sessions.Issue(subject, display), + Value: s.Sessions.Issue(subject, display, admin), Path: "/", HttpOnly: true, // the cookie is a bearer credential; script has no business reading it Secure: s.Secure, @@ -186,7 +208,7 @@ func (s *Server) loginSubmit(w http.ResponseWriter, r *http.Request) { } s.Throttle.Succeeded() slog.Info("admin login", "user", user, "method", "local", "from", clientIP(r)) - s.setSession(w, "local:"+cred.Username, cred.Username) + s.setSession(w, "local:"+cred.Username, cred.Username, true) http.Redirect(w, r, "/", http.StatusSeeOther) } @@ -271,18 +293,14 @@ func (s *Server) oidcCallback(w http.ResponseWriter, r *http.Request) { http.Error(w, "the identity token was not accepted", http.StatusForbidden) return } - if !s.OIDC.IsAdmin(claims) { - // Named explicitly: "you signed in but you are not an admin" is a different problem from - // "your password is wrong", and the group is the thing to go and check. - slog.Info("admin access denied: not in group", "account", claims.AccountID(), - "want_group", s.OIDC.Config().AdminGroup, "have", claims.Groups) - http.Error(w, fmt.Sprintf( - "Signed in as %s, but that account is not in the %q group, so it cannot administer "+ - "this server.", claims.Display(), s.OIDC.Config().AdminGroup), http.StatusForbidden) - return - } - slog.Info("admin login", "account", claims.AccountID(), "method", "oidc", "from", clientIP(r)) - s.setSession(w, claims.AccountID(), claims.Display()) + // Authentication and authorisation are answered separately here. Someone who is not in the + // admin group has still proved who they are, and their own uploads are their business to + // manage — refusing them a session outright, as this used to, left a legitimate account with + // no way to see or delete the data it had sent. + admin := s.OIDC.IsAdmin(claims) + slog.Info("login", "account", claims.AccountID(), "method", "oidc", "admin", admin, + "from", clientIP(r)) + s.setSession(w, claims.AccountID(), claims.Display(), admin) http.Redirect(w, r, "/", http.StatusSeeOther) } diff --git a/server/internal/adminui/pages.go b/server/internal/adminui/pages.go index bf06674..d15136a 100644 --- a/server/internal/adminui/pages.go +++ b/server/internal/adminui/pages.go @@ -16,6 +16,46 @@ import ( "echo-lot.app/server/internal/store" ) +// visibleDevices returns the devices a session may see: everything for an administrator, and for +// anyone else the devices linked to their own account. +// +// Every page goes through this rather than filtering for itself. Scoping applied per-page is +// scoping that will be missing from the next page someone adds, and the failure is silent — a +// listing that quietly shows other people's uploads looks exactly like one that does not. +func (s *Server) visibleDevices(sess *adminauth.Session) []store.Device { + all := s.Store.Devices() + if sess.Admin { + return all + } + owned := make(map[string]bool) + for _, id := range s.Store.DeviceIDsForAccount(sess.Subject) { + owned[id] = true + } + out := make([]store.Device, 0, len(owned)) + for _, d := range all { + if owned[d.ID] { + out = append(out, d) + } + } + return out +} + +// mayTouchRun reports whether this session may read or delete a given run. +// +// Checked against the device list rather than against the run's own metadata, so an unlinked or +// revoked device stops granting access the moment the link is gone. +func (s *Server) mayTouchRun(sess *adminauth.Session, device string) bool { + if sess.Admin { + return true + } + for _, d := range s.visibleDevices(sess) { + if d.ID == device { + return true + } + } + return false +} + func (s *Server) loginForm(w http.ResponseWriter, r *http.Request) { if s.session(r) != nil { http.Redirect(w, r, "/", http.StatusSeeOther) @@ -29,7 +69,7 @@ func (s *Server) loginForm(w http.ResponseWriter, r *http.Request) { } func (s *Server) dashboard(w http.ResponseWriter, r *http.Request, sess *adminauth.Session) { - devices := s.Store.Devices() + devices := s.visibleDevices(sess) linked := 0 for _, d := range devices { if d.LinkedToAccount() { @@ -37,7 +77,9 @@ func (s *Server) dashboard(w http.ResponseWriter, r *http.Request, sess *adminau } } var selftest any - if s.SelfTest != nil { + // The self-test describes the server's own health, which is an operator's concern; a user + // looking at their uploads has no use for it and no ability to act on it. + if s.SelfTest != nil && sess.Admin { selftest = s.SelfTest() } s.render(w, r, "dashboard", map[string]any{ @@ -48,6 +90,7 @@ func (s *Server) dashboard(w http.ResponseWriter, r *http.Request, sess *adminau "Runs": s.totalRuns(devices), "SelfTest": selftest, "Version": s.Version, + "Admin": sess.Admin, }) } @@ -63,7 +106,7 @@ func (s *Server) totalRuns(devices []store.Device) int { } func (s *Server) devices(w http.ResponseWriter, r *http.Request, sess *adminauth.Session) { - devices := s.Store.Devices() + devices := s.visibleDevices(sess) // Newest first: the device someone is looking for is almost always the one just enrolled. sort.Slice(devices, func(i, j int) bool { return devices[i].Enrolled.After(devices[j].Enrolled) }) @@ -81,7 +124,7 @@ func (s *Server) devices(w http.ResponseWriter, r *http.Request, sess *adminauth } s.render(w, r, "devices", map[string]any{ "Session": sess, "CSRF": s.csrfToken(sess), "Rows": rows, - "Link": r.URL.Query().Get("link"), + "Link": r.URL.Query().Get("link"), "Admin": sess.Admin, }) } @@ -118,7 +161,7 @@ func (s *Server) runsList(w http.ResponseWriter, r *http.Request, sess *adminaut DeviceName string } var rows []row - for _, d := range s.Store.Devices() { + for _, d := range s.visibleDevices(sess) { if s.Runs == nil { break } @@ -134,10 +177,18 @@ func (s *Server) runsList(w http.ResponseWriter, r *http.Request, sess *adminaut if len(rows) > 200 { rows = rows[:200] // a page, not the archive; the count is on the dashboard } - s.render(w, r, "runs", map[string]any{"Session": sess, "CSRF": s.csrfToken(sess), "Rows": rows}) + s.render(w, r, "runs", map[string]any{ + "Session": sess, "CSRF": s.csrfToken(sess), "Rows": rows, "Admin": sess.Admin, + }) } func (s *Server) runView(w http.ResponseWriter, r *http.Request, sess *adminauth.Session) { + // 404 rather than 403 for someone else's run: a distinguishable "you may not see this" tells + // an unauthorised caller that the run exists, which is itself something they should not learn. + if !s.mayTouchRun(sess, r.PathValue("device")) { + http.NotFound(w, r) + return + } body, err := s.Runs.Get(r.PathValue("device"), r.PathValue("id")) if err != nil { http.NotFound(w, r) @@ -151,7 +202,7 @@ func (s *Server) runView(w http.ResponseWriter, r *http.Request, sess *adminauth out = body } s.render(w, r, "run", map[string]any{ - "Session": sess, "CSRF": s.csrfToken(sess), + "Session": sess, "CSRF": s.csrfToken(sess), "Admin": sess.Admin, "Device": r.PathValue("device"), "ID": r.PathValue("id"), "JSON": string(out), }) @@ -159,6 +210,10 @@ func (s *Server) runView(w http.ResponseWriter, r *http.Request, sess *adminauth func (s *Server) runDelete(w http.ResponseWriter, r *http.Request, sess *adminauth.Session) { device, id := r.PathValue("device"), r.PathValue("id") + if !s.mayTouchRun(sess, device) { + http.NotFound(w, r) + return + } if err := s.Runs.Delete(device, id); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/server/internal/adminui/render.go b/server/internal/adminui/render.go index e9f582f..ffb6528 100644 --- a/server/internal/adminui/render.go +++ b/server/internal/adminui/render.go @@ -67,7 +67,7 @@ const baseHTML = `

Echolot

- {{.Session.Display}} + {{.Session.Display}}{{if not .Session.Admin}} (your account){{end}}
@@ -94,18 +94,28 @@ const baseHTML = ` {{else if eq .Page "dashboard"}}
-
{{.Devices}}devices
-
{{.Linked}}signed in
-
{{.Runs}}stored runs
+
{{.Devices}}{{if .Admin}}devices{{else}}your devices{{end}}
+ {{if .Admin}}
{{.Linked}}signed in
{{end}} +
{{.Runs}}{{if .Admin}}stored runs{{else}}your runs{{end}}
+ {{if not .Admin}} +
+

This is your account. You can see the devices you have signed in on, review everything + they have uploaded, and delete any of it.

+

Administering the server — enrolling devices, revoking them, and + seeing other people's uploads — needs an administrator account.

+
+ {{end}} + {{if .Admin}}

Server

version {{.Version}}

{{with .SelfTest}}
{{printf "%+v" .}}
{{end}}
+ {{end}} {{else if eq .Page "devices"}} -

Devices

+

{{if .Admin}}Devices{{else}}Your devices{{end}}

{{with .Link}}

Enrolment link — single use, valid 24 hours. Treat it like a password until spent.

@@ -114,10 +124,12 @@ const baseHTML = ` adb shell am start -a android.intent.action.VIEW -d "{{.}}"

{{end}} + {{if .Admin}}
+ {{end}} {{range .Rows}} @@ -127,17 +139,17 @@ const baseHTML = ` - + {{end}} {{else}} - + {{end}}
DeviceNameAccountEnrolledRuns
{{if .LinkedToAccount}}{{.AccountName}}{{else}}not signed in{{end}} {{.Enrolled.Format "2006-01-02 15:04"}} {{.Runs}}
+
{{if $.Admin}} -
No devices enrolled.
{{if $.Admin}}No devices enrolled.{{else}}You have not signed in on any device yet. Sign in from the Echolot app to link one.{{end}}
{{else if eq .Page "runs"}} -

Uploaded runs

+

{{if .Admin}}Uploaded runs{{else}}Your uploaded runs{{end}}

Shown exactly as uploaded, at the privacy level the uploader chose. Nothing here can un-redact a run.

diff --git a/server/internal/adminui/scope_test.go b/server/internal/adminui/scope_test.go new file mode 100644 index 0000000..1d40387 --- /dev/null +++ b/server/internal/adminui/scope_test.go @@ -0,0 +1,119 @@ +// SPDX-FileCopyrightText: 2026 Echolot contributors +// SPDX-License-Identifier: GPL-3.0-or-later + +package adminui + +import ( + "testing" + "time" + + "echo-lot.app/server/internal/adminauth" + "echo-lot.app/server/internal/runs" + "echo-lot.app/server/internal/store" +) + +// Two accounts, one device each, plus an unlinked device nobody owns. +func fixture(t *testing.T) (*Server, string, string, string) { + t.Helper() + dir := t.TempDir() + st, err := store.Open(dir) + if err != nil { + t.Fatal(err) + } + rs, err := runs.Open(dir, runs.DefaultPolicy()) + if err != nil { + t.Fatal(err) + } + enroll := func(name string) string { + tok, err := st.NewEnrollToken(time.Hour, "test") + if err != nil { + t.Fatal(err) + } + d, err := st.Redeem(tok, name) + if err != nil { + t.Fatal(err) + } + return d.ID + } + mine, theirs, orphan := enroll("mine"), enroll("theirs"), enroll("orphan") + if err := st.LinkAccount(mine, "oidc#me", "Me"); err != nil { + t.Fatal(err) + } + if err := st.LinkAccount(theirs, "oidc#you", "You"); err != nil { + t.Fatal(err) + } + for _, d := range []string{mine, theirs, orphan} { + if _, err := rs.Put(d, []byte(`{"run":{"id":"r"}}`), true); err != nil { + t.Fatal(err) + } + } + return &Server{Store: st, Runs: rs}, mine, theirs, orphan +} + +func user() *adminauth.Session { return &adminauth.Session{Subject: "oidc#me", Display: "Me"} } +func admin() *adminauth.Session { return &adminauth.Session{Subject: "local:a", Admin: true} } + +func TestVisibleDevicesScopesToAccount(t *testing.T) { + s, mine, theirs, orphan := fixture(t) + + got := s.visibleDevices(user()) + if len(got) != 1 || got[0].ID != mine { + t.Fatalf("a user should see only their own device, got %+v", got) + } + + all := s.visibleDevices(admin()) + if len(all) != 3 { + t.Fatalf("an admin should see every device, got %d", len(all)) + } + _ = theirs + _ = orphan +} + +func TestUnlinkedDevicesBelongToNobody(t *testing.T) { + // An enrolled but never-signed-in device is not "everyone's" — a user must not inherit it + // just because no account claimed it. + s, _, _, orphan := fixture(t) + if s.mayTouchRun(user(), orphan) { + t.Fatal("an unlinked device was treated as the user's own") + } +} + +func TestRunAccessFollowsDeviceOwnership(t *testing.T) { + s, mine, theirs, _ := fixture(t) + + if !s.mayTouchRun(user(), mine) { + t.Fatal("a user cannot reach their own run") + } + if s.mayTouchRun(user(), theirs) { + t.Fatal("a user reached someone else's run") + } + if !s.mayTouchRun(admin(), theirs) { + t.Fatal("an admin should reach any run") + } +} + +func TestAccessEndsWhenTheLinkDoes(t *testing.T) { + // Ownership is read from the device list on every request rather than captured at sign-in, + // so unlinking takes effect immediately — a session issued while linked must not keep working. + s, mine, _, _ := fixture(t) + sess := user() + if !s.mayTouchRun(sess, mine) { + t.Fatal("precondition: the device should start out owned") + } + if err := s.Store.LinkAccount(mine, "", ""); err != nil { + t.Fatal(err) + } + if s.mayTouchRun(sess, mine) { + t.Fatal("access survived the account link being removed") + } +} + +func TestEmptySubjectMatchesNothing(t *testing.T) { + // The dangerous degenerate case: a session with no subject must own nothing, not everything + // that happens to have an empty account id. + s, _, _, _ := fixture(t) + anon := &adminauth.Session{Subject: "", Display: ""} + if got := s.visibleDevices(anon); len(got) != 0 { + t.Fatalf("an empty subject matched %d devices", len(got)) + } +}