diff --git a/README.md b/README.md index 37f77f3..1703ef0 100644 --- a/README.md +++ b/README.md @@ -397,6 +397,14 @@ as being offline (see `bdrive status`): pulled, pushed, or written: revoking access never deletes or reverts a file on someone's disk. Re-granting resumes on the next tick. +`bdrive status` and `bdrive sync` print the hub's own sentence under either +one, as `reason:`. Read it: not every refused push is a permissions +question. `this device is not registered to your account on this hub` means +this machine's device identity was never bound to your account — update +`bdrive` and run `bdrive login` here. Project settings will show `write` and +explain nothing. Both states are recorded only by a cycle that actually +reached the hub, so the local-only ticks in between never revise the answer. + Public `/s/` share links are **unaffected** by any of this: they are anonymous by design and keep serving until revoked, so cutting someone's access does not kill links they already minted. diff --git a/architecture/cli-sync.md b/architecture/cli-sync.md index 662fc27..39880fe 100644 --- a/architecture/cli-sync.md +++ b/architecture/cli-sync.md @@ -37,8 +37,10 @@ classDiagram +Conflicts +Pruned +Materialized +Pushed +Offline +OfflineErr +ReadOnly +NoAccess +AccessErr + +Reason() string } note for Result "Offline / ReadOnly / NoAccess are three different answers: unreachable (retry all), push refused (pull-only), pull refused (pause, touch nothing)" + note for Result "Reason() is accessReason(AccessErr): the hub's own sentence for a refusal, minus the wrapper chain, dropped unless it passes journal.SafeText. 'read-only' summarizes the STATUS CODE — the sentence is the only thing that tells a device-registration 403 from a project the user really is a reader on" class Filter { +Skip(rel) bool @@ -52,8 +54,10 @@ classDiagram class SyncState { +Lamport +PushedOps +Access + +AccessReason +IgnoreAccepted +IgnorePulled } + note for SyncState "Access/AccessReason are written ONLY by the leg that asked the hub — pull clears no-access, push records read-only or clears it. A cycle with no remote leg leaves the last answer standing, so the daemon's local-only ticks stop alternating 'read-only' / 'access restored' and bdrive status stops reporting healthy sync moments after a refused push" note for SyncState "store — IgnoreAccepted is the .bdriveignore scope THIS device consented to; IgnorePulled is the text a peer's version last wrote here. The pair is what tells a locally authored rule change from one that arrived over the wire. vouchedFloor seeds it once on upgrade: keep every exclusion, drop each `!` that no already-materialized path vouches for" class SafePath { diff --git a/cmd/bdrive/cmds.go b/cmd/bdrive/cmds.go index 52fb127..1bd04fa 100644 --- a/cmd/bdrive/cmds.go +++ b/cmd/bdrive/cmds.go @@ -255,6 +255,12 @@ func statusCmd() *cobra.Command { case store.AccessNone: fmt.Printf(" access: no access to this project — sync paused\n") } + // `status` is the command someone runs when sync is stuck, and + // it never talks to the hub — so the refusal it reports is only + // as useful as the reason the last cycle recorded with it. + if st.AccessReason != "" { + fmt.Printf(" reason: %s\n", safeField(st.AccessReason, 300)) + } } } return nil diff --git a/cmd/bdrive/helpers.go b/cmd/bdrive/helpers.go index 2c74f94..01caa81 100644 --- a/cmd/bdrive/helpers.go +++ b/cmd/bdrive/helpers.go @@ -179,6 +179,16 @@ func humanBytes(n int64) string { return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGTPE"[exp]) } +// printReason prints the hub's stated reason under a refusal line. "read-only" +// is a summary of the STATUS code; the sentence under it is the only thing that +// tells a device-registration 403 apart from a project the user really is a +// reader on — and only one of those is fixed by signing in again. +func printReason(reason string) { + if reason != "" { + fmt.Printf(" %s\n", safeField(reason, 300)) + } +} + func printCycle(res *syncer.Result) { fmt.Printf(" local changes: %d\n", res.LocalOps) fmt.Printf(" pulled changes: %d\n", res.PulledOps) @@ -192,8 +202,10 @@ func printCycle(res *syncer.Result) { switch { case res.NoAccess: fmt.Printf(" remote: no access — sync paused (ask a project admin for access)\n") + printReason(res.Reason()) case res.ReadOnly: fmt.Printf(" remote: read-only (pull only) — local changes stay on this device\n") + printReason(res.Reason()) case res.Pushed && res.OfflineErr != nil: // Offline is a report, not a gate (see syncer.Result): a content-level // problem with one object no longer withholds this device's push, so diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index cad7702..84eaeaf 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -449,7 +449,11 @@ func Run(folder string, scanInterval, remoteInterval time.Duration) error { lastRemote = time.Now() case res.ReadOnly: if lastAccess != store.AccessReadOnly { - log.Printf("read-only on this project, pulling only; local changes stay on this device") + if reason := res.Reason(); reason != "" { + log.Printf("read-only on this project, pulling only; local changes stay on this device (%s)", reason) + } else { + log.Printf("read-only on this project, pulling only; local changes stay on this device") + } lastAccess = store.AccessReadOnly } lastRemote = time.Now() @@ -461,7 +465,12 @@ func Run(folder string, scanInterval, remoteInterval time.Duration) error { } lastRemote = time.Now() default: - if lastAccess != store.AccessOK { + // "access restored" is a claim about the hub, and a local-only tick + // never asked it one. Announcing it there is what made a refused + // device alternate between this line and the read-only one every few + // seconds: three cheap scan ticks run between remote passes, each + // cleared the flag, and the next remote pass set it again. + if doRemote && lastAccess != store.AccessOK { log.Printf("access restored; syncing normally") lastAccess = store.AccessOK } diff --git a/internal/store/store.go b/internal/store/store.go index 28df149..22d8e3d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -246,6 +246,12 @@ type SyncState struct { Lamport int64 `json:"lamport"` PushedOps int64 `json:"pushed_ops"` // how many of our own ops the remote has Access string `json:"access,omitempty"` // "", "read-only", or "no-access" + // AccessReason is the hub's own words for the last refusal. Without it every + // 403 renders as the same "read-only (pull only)" line, and the hub's most + // actionable answer — "this device is not registered to your account on this + // hub; run `bdrive login`" — reached nobody: the one state a user cannot + // diagnose from the outside was the one the CLI summarized away. + AccessReason string `json:"access_reason,omitempty"` // IgnoreAccepted is the .bdriveignore text whose scan scope THIS device has // accepted, and IgnorePulled is the text a peer's version last wrote here. diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index cbfd80f..78b82d9 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -127,6 +127,38 @@ type Result struct { AccessErr error } +// accessReason renders a hub refusal as the sentence a person can act on. The +// hub answers a device-registration 403 with what to DO about it, and every +// caller used to collapse that into "read-only (pull only)" — the one refusal +// that is not about project permissions at all, reported as if it were, so the +// user re-checked their access, saw `write`, and had nowhere left to look. +// +// The wrapper chain the CLI itself added ("forbidden: server: 403 Forbidden: ") +// is dropped; it tells a reader nothing the line does not already say. +// +// The remainder is the HUB's text landing in a log file, a terminal and an +// agent's context, so it passes journal.SafeText — the rule this repo already +// applies to every other peer-written string it renders — or it does not travel +// at all. Bounded for the same reason: this is persisted in sync.json and +// printed on one status line. +func accessReason(err error) string { + if err == nil { + return "" + } + msg := err.Error() + if _, rest, ok := strings.Cut(msg, "Forbidden: "); ok && rest != "" { + msg = rest + } + if len([]rune(msg)) > 300 || !journal.SafeText(msg) { + return "" + } + return msg +} + +// Reason is the hub's own words for a ReadOnly or NoAccess answer, empty when +// the hub gave none. It is what the CLI prints under the summary line. +func (r *Result) Reason() string { return accessReason(r.AccessErr) } + func (r *Result) Activity() bool { return r.LocalOps > 0 || r.PulledOps > 0 || r.Conflicts > 0 || r.Pruned > 0 || r.Materialized > 0 } @@ -254,13 +286,18 @@ func (s *Session) Cycle(ctx context.Context) (*Result, error) { pulled, gone, err = s.pull(ctx) switch { case err == nil: + if st.Access == store.AccessNone { + // The pull that was refused now succeeds, so read access is back. + // Whether writes are back is the push leg's to answer below. + st.Access, st.AccessReason = store.AccessOK, "" + } case errors.Is(err, remote.ErrForbidden): // Access to this project was revoked. Stop here: materializing a // replay we can no longer refresh would look like the hub // reverting the user's files. Nothing is pushed, nothing is // deleted, and the next cycle re-checks. res.NoAccess, res.AccessErr = true, err - st.Access = store.AccessNone + st.Access, st.AccessReason = store.AccessNone, accessReason(err) return res, s.finish(cache, st) case errors.Is(err, errBlobContent): // Reported — it is the only signal a device ever gets that its hub @@ -422,6 +459,7 @@ func (s *Session) Cycle(ctx context.Context) (*Result, error) { switch err := s.push(ctx, myOps, &st); { case err == nil: res.Pushed = true + st.Access, st.AccessReason = store.AccessOK, "" case errors.Is(err, remote.ErrForbidden): // Read-only on this project: pull and materialize already ran, so // pull-only is the steady state. Our own ops stay in the local @@ -429,6 +467,7 @@ func (s *Session) Cycle(ctx context.Context) (*Result, error) { // attempted once per remote interval (no hot loop, and a re-grant // self-heals). res.ReadOnly, res.AccessErr = true, err + st.Access, st.AccessReason = store.AccessReadOnly, accessReason(err) default: res.Offline = true res.OfflineErr = err @@ -451,10 +490,14 @@ func (s *Session) Cycle(ctx context.Context) (*Result, error) { } } - st.Access = store.AccessOK - if res.ReadOnly { - st.Access = store.AccessReadOnly - } + // st.Access is NOT recomputed here. Only the leg that actually asked the hub + // knows how it answered, so each one records its own verdict above and a + // cycle that never asked leaves the last one standing. Resetting it to OK on + // every cycle meant the daemon's cheap local-only ticks — three of them + // between remote passes — each declared "access restored; syncing normally", + // so a device the hub was refusing alternated between the two log lines + // forever and `bdrive status` reported healthy sync moments after the push + // it had just refused. if err := s.finish(cache, st); err != nil { return nil, err } diff --git a/internal/syncer/syncer_test.go b/internal/syncer/syncer_test.go index 72ca514..90a986a 100644 --- a/internal/syncer/syncer_test.go +++ b/internal/syncer/syncer_test.go @@ -491,6 +491,98 @@ func TestReadOnlyDevicePullsOnly(t *testing.T) { } } +// refusingPush is a hub that answers every push with a real 403 body. The one +// that matters is the device-registration refusal: it is not about project +// permissions at all, so the user who checks their permissions finds `write` +// and has nowhere left to look — the hub's sentence is the only thing that +// points at the fix. +type refusingPush struct { + remote.Backend + msg string +} + +func (p refusingPush) Put(ctx context.Context, key string, r io.Reader, size int64) error { + return fmt.Errorf("%w: server: 403 Forbidden: %s", remote.ErrForbidden, p.msg) +} + +// A refused device must stay refused in its own records until the hub says +// otherwise — and it must be able to say WHY. +// +// Both halves come from one report: a mount whose journal pushes 403'd on every +// remote pass, while `bdrive status` said access was fine and the daemon log +// alternated "read-only on this project" / "access restored; syncing normally" +// every few seconds. The cycle recomputed access from scratch at the end of +// every pass, including the cheap local-only ones the daemon runs three of +// between remote passes, so the hub's answer was overwritten by a cycle that +// never asked it anything. And the answer itself — "this device is not +// registered to your account on this hub; run `bdrive login`" — was summarized +// into "read-only (pull only)" and lost. +func TestRefusedPushKeepsItsVerdictAndItsReason(t *testing.T) { + const refusal = "this device is not registered to your account on this hub; run `bdrive login` on this machine" + be := sharedRemote(t) + b := newDevice(t, "devb", refusingPush{Backend: be, msg: refusal}) + + write(t, b.Folder, "mine.md", "local only") + res, err := b.Cycle(context.Background()) + if err != nil { + t.Fatal(err) + } + if !res.ReadOnly { + t.Fatalf("a refused push should report ReadOnly: %+v", res) + } + if res.Reason() != refusal { + t.Fatalf("Result.Reason() = %q, want the hub's own sentence %q", res.Reason(), refusal) + } + st, err := b.Store.LoadSync() + if err != nil { + t.Fatal(err) + } + if st.Access != store.AccessReadOnly || st.AccessReason != refusal { + t.Fatalf("persisted access = %q/%q, want read-only + the hub's reason", st.Access, st.AccessReason) + } + + // The daemon's local-only tick: same session, no backend at all. It learns + // nothing about the hub, so it must not clear the hub's last answer. + b.Backend = nil + write(t, b.Folder, "second.md", "still local") + if res := cycle(t, b); res.ReadOnly { + t.Fatalf("a cycle with no remote leg cannot discover a refusal: %+v", res) + } + st, _ = b.Store.LoadSync() + if st.Access != store.AccessReadOnly || st.AccessReason != refusal { + t.Fatalf("a local-only tick reset access to %q/%q — `bdrive status` reports healthy "+ + "sync moments after the push the hub refused, and the daemon logs "+ + "\"access restored\" between every pair of remote passes", st.Access, st.AccessReason) + } + + // Only the hub clears it: a push that lands does, and takes the stale + // reason with it. + b.Backend = be + if res := cycle(t, b); !res.Pushed { + t.Fatalf("re-granted device did not push: %+v", res) + } + if st, _ := b.Store.LoadSync(); st.Access != store.AccessOK || st.AccessReason != "" { + t.Fatalf("after a successful push, access = %q/%q, want cleared", st.Access, st.AccessReason) + } +} + +// A hub message that could repaint the terminal it lands in is dropped rather +// than rendered: it reaches a log file, `bdrive status`, and an agent's context +// verbatim, and the hub is the one string source here nobody local vouches for. +func TestAccessReasonRefusesTerminalControls(t *testing.T) { + esc := fmt.Errorf("%w: server: 403 Forbidden: nope\x1b[2Kaccess restored", remote.ErrForbidden) + if got := accessReason(esc); got != "" { + t.Errorf("accessReason kept a control sequence: %q", got) + } + long := fmt.Errorf("%w: server: 403 Forbidden: %s", remote.ErrForbidden, strings.Repeat("x", 400)) + if got := accessReason(long); got != "" { + t.Errorf("accessReason kept a %d-rune message", len([]rune(got))) + } + if got := accessReason(nil); got != "" { + t.Errorf("accessReason(nil) = %q", got) + } +} + // A device whose access is revoked entirely pauses: the cycle reports // NoAccess (not Offline), the working folder is left byte-for-byte alone — // revoking access must never look like the hub deleting someone's files — and diff --git a/internal/webapp/store.go b/internal/webapp/store.go index a9b06b0..5c8bfbb 100644 --- a/internal/webapp/store.go +++ b/internal/webapp/store.go @@ -133,9 +133,17 @@ func (s *Server) ownJournal(w http.ResponseWriter, r *http.Request, key string) // machine's token (DeviceRegistry.Bind), which is a moment the hub // authenticates and the machine cannot forge. So an unowned id is // simply not anybody's to write, and the remedy is to sign in. + // "run `bdrive login`" alone sent one user in a circle for an + // afternoon: the binding is made by the login request naming its + // device, which a CLI older than this gate does not do, so signing in + // again succeeded, changed nothing, and every push kept 403ing with + // the same sentence. The hub and the CLI deploy separately, so the + // skew is the expected state right after this ships — the refusal has + // to name the upgrade, not just the command. http.Error(w, "this device is not registered to your account on this hub; "+ - "run `bdrive login` on this machine (if the id belongs to someone else, "+ - "delete device.json in your BearDrive home first, or ask a project admin)", + "update bdrive, then run `bdrive login` on this machine (an older CLI signs in "+ + "without registering its device). If the id belongs to someone else, delete "+ + "device.json in your BearDrive home first, or ask a project admin", http.StatusForbidden) return false } diff --git a/web/docs/src/content/docs/reference/cli.md b/web/docs/src/content/docs/reference/cli.md index b005674..8cd2f09 100644 --- a/web/docs/src/content/docs/reference/cli.md +++ b/web/docs/src/content/docs/reference/cli.md @@ -206,19 +206,30 @@ touches your files: ``` pending: 3 local change(s) not yet pushed access: read-only (pull only) — 3 local change(s) stay on this device + reason: this device is not registered to your account on this hub; update bdrive, then run `bdrive login` on this machine ``` -- **`read-only (pull only)`** — you have `read` on the project. The daemon keeps - pulling teammates' changes; your own edits stay journaled locally, never - pushed and never dropped. They go out if you are granted `write` again. +- **`read-only (pull only)`** — the hub refused this device's push. Usually you + have `read` on the project: the daemon keeps pulling teammates' changes, your + own edits stay journaled locally, never pushed and never dropped, and they go + out if you are granted `write` again. - **`no access to this project — sync paused`** — your access was revoked. Nothing is pulled, pushed, or written; the working folder is left exactly as it is. Re-granting resumes on the next tick with no manual step. +Always read the `reason:` line under them — it is the hub's own sentence, and +not every refusal is about project permissions. The common non-permission one +is `this device is not registered to your account on this hub`: your device +identity was never bound to your account, which is fixed by updating `bdrive` +and running `bdrive login` on that machine, not in Project settings. Checking +your permissions there will show `write` and tell you nothing. + `bdrive sync` shows the same two as `remote: read-only (pull only)` / -`remote: no access — sync paused`, and the daemon logs each once on -transition rather than on every tick. Both are permission answers: they are -fixed in the hub's Project settings → People, not on the device. See +`remote: no access — sync paused` with the reason on the line below, and the +daemon logs each once on transition rather than on every tick — including the +cheap local-only ticks between remote passes, which never ask the hub anything +and so never revise its last answer. For the permission answers, the fix is in +the hub's Project settings → People; see [Project permissions](/concepts/permissions/). ### `bdrive login` and switching hubs