From abf368f00930980fed1259a8a9539627e0197a9f Mon Sep 17 00:00:00 2001 From: Pfriedrix Date: Sat, 8 Aug 2026 18:57:08 +0300 Subject: [PATCH] fix(auth): tell a refused Keychain apart from a missing login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing n on the lcc open dashboard answered "Not authenticated. Run `lcc auth` first." while the token sat in the Keychain, valid, and then took the dashboard down with it. oauth.getToken collapsed three answers into one null: nothing stored, a Keychain that refused the read, and a stored value that will not parse. Only the first means the user has to authenticate. The refusal is the likeliest of the three on a machine that rebuilds lcc, because a renewed signing certificate changes the designated requirement and macOS asks once more — a dialog that can be denied, escaped, or missed behind a full-screen terminal. Sending that user through a browser round trip fixes it only by accident. readToken separates the three, and keychain.describeLast, which nothing had ever called, names the OSStatus so the next occurrence is diagnosable from the one line it prints. The second failure is that start.bail exited the process. lcc open runs start.run in-process for n and catches its errors precisely so the dashboard survives, so one bad answer ended every other session's row along with the picker. bail now returns error.Failed under returns_to_caller, the flag cancel already used for the same reason. --- CLAUDE.md | 19 +++++- README.md | 14 ++++- src/commands/auth.zig | 18 +++++- src/commands/issue.zig | 13 ++++- src/commands/list.zig | 15 ++++- src/commands/start.zig | 129 +++++++++++++++++++++++++++++------------ src/commands/watch.zig | 2 +- src/keychain.zig | 40 +++++++++++++ src/oauth.zig | 71 +++++++++++++++++++++-- 9 files changed, 267 insertions(+), 54 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a9c0237..d1841e9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ library plus CoreFoundation/Security. Run from the repo root: ```bash -zig build test --summary all # unit tests (~3s, 300 at last count) +zig build test --summary all # unit tests (~3s, 303 at last count) zig build # debug binary → zig-out/bin/lcc zig build -Doptimize=ReleaseFast # what PATH should be serving zig build run -- list # run without installing @@ -124,9 +124,26 @@ Do not "simplify" `build.zig`'s separate `test_mod`: reusing the executable's mo code signature, so `build.zig` signs the installed binary to keep one "Always Allow" valid across rebuilds. Removing or bypassing that (`-Dsign=none`) brings back a login password prompt on every rebuild, from a process that blocks with no output. +- **"No token" and "the Keychain would not give it to me" are different answers.** + `oauth.readToken` separates `.missing` from `.unreadable`, and every caller has to keep + them apart — collapsing them back into `getToken() == null` compiles, reads tidier, and + tells a user whose token is right there to run `lcc auth` again: a browser round trip for + a Keychain dialog that only had to be answered. The refusal is the *likelier* of the two + on a machine that rebuilds lcc, because a renewed signing certificate changes the + designated requirement and macOS asks once more — and that prompt can be denied, escaped, + or missed behind a full-screen terminal. `keychain.describeLast` is what turns the + OSStatus into that sentence; leaving it uncalled is how the distinction quietly dies. - **`src/keychain.zig` imports five narrow C headers on purpose.** The umbrella `CoreFoundation.h` / `Security.h` do not translate on this SDK. Do not tidy them into one import. +- **`start.zig`'s `bail` only exits when nobody is waiting on it.** It returns + `error{Failed}` under `opts.returns_to_caller`, which is what `lcc open`'s `n` sets: the + dashboard calls `start.run` in-process, so a `std.process.exit` in there takes the whole + dashboard down — every other session's row with it — over one bad answer. That is why + every call site reads `return bail(…)` and why `cancel` has the same shape. A new call + site written as a bare `bail(…)` is a compile error rather than a silent fall-through, + but only because the returned error value cannot be discarded; do not "fix" that by + ignoring it. - **A hook event that reports no `permission_mode` must not clear the one already known.** Only some events carry it — `Notification` does not (see the test in `watch_hooks.zig`). `watch_session.setPlan` is guarded on `permission_mode.len > 0` for that reason, and diff --git a/README.md b/README.md index b3eef9c..2fcfacc 100644 --- a/README.md +++ b/README.md @@ -350,7 +350,7 @@ Failures come back in the same shape, on stdout, with exit code 1: { "error": { "code": "issue_not_found", "message": "No issue PE-999 in Linear." } } ``` -Codes: `usage`, `not_authenticated`, `auth_failed`, `bad_identifier`, `issue_not_found`, `linear_failed`, `worktree_path_exists`, `git_failed`, `bad_repo`, `plan_not_found`, `plan_unreadable`, `repo_unconfirmed` — the last one is the picker above in a mode with nobody to ask: pass `--repo `, or run it once interactively and the answer is remembered. Progress lines and the human-readable error go to stderr, so stdout holds nothing but the payload — including git's own output, which is captured rather than inherited in this mode. +Codes: `usage`, `not_authenticated`, `keychain_unreadable`, `auth_failed`, `bad_identifier`, `issue_not_found`, `linear_failed`, `worktree_path_exists`, `git_failed`, `bad_repo`, `plan_not_found`, `plan_unreadable`, `repo_unconfirmed` — the last one is the picker above in a mode with nobody to ask: pass `--repo `, or run it once interactively and the answer is remembered. Progress lines and the human-readable error go to stderr, so stdout holds nothing but the payload — including git's own output, which is captured rather than inherited in this mode. ### `lcc issue` @@ -521,7 +521,7 @@ Some details that are easy to get wrong, and are decided here rather than left t `gh` is optional throughout: without it rule 4 falls back to the commit-distance contest, which needs no network, no auth and no remote — the absence costs one note. -Failures take the same shape as `lcc start --json` — JSON on stdout, the human line on stderr, exit 1. Codes shared by every subcommand: `usage`, `not_authenticated`, `auth_failed`, `bad_identifier`, `issue_not_found`, `linear_failed`. `comment` adds `body_not_found`, `body_unreadable`, `body_empty`, `body_too_large`. +Failures take the same shape as `lcc start --json` — JSON on stdout, the human line on stderr, exit 1. Codes shared by every subcommand: `usage`, `not_authenticated`, `keychain_unreadable`, `auth_failed`, `bad_identifier`, `issue_not_found`, `linear_failed`. `comment` adds `body_not_found`, `body_unreadable`, `body_empty`, `body_too_large`. ### `lcc open` @@ -805,6 +805,16 @@ The designated requirement becomes `identifier lcc and … certificate leaf[subj A self-signed certificate works and never expires on someone else's schedule (Keychain Access → Certificate Assistant → *Create a Certificate*, type *Code Signing*); name it `lcc-dev` and it is preferred automatically. An Apple Development certificate is equally fine, with the caveat that it expires — the requirement changes with the certificate, so the first run after a renewal asks once more. +That one prompt is answerable with *Deny*, or with Escape, or it can be missed entirely behind a full-screen terminal — and a refused read is not a missing login. It is reported as one: + +``` +✗ The Linear token is in the Keychain, but reading it failed: authorization failed + (keychain prompt denied?) (OSStatus -25293). Answer `Always Allow` if macOS asks + again; `lcc auth` re-stores it if it stays refused. +``` + +`lcc auth --status` says the same thing rather than "Not authenticated", and `--json` calls it `keychain_unreadable` rather than `not_authenticated`. Re-running `lcc auth` does fix it, by storing the item afresh under the current signature — but it is a full browser round trip for a dialog that only had to be answered, which is why the two are told apart. + ### MCP servers Symlinks cannot solve the same problem for MCP servers, because they are not in the repository. `claude mcp add` without `-s user` stores a server under `projects[""].mcpServers` in `~/.claude.json` — the key *is* the directory. A worktree is a different directory, so it starts with none of them: the checkout where `linear-server` was added is the only place it exists. diff --git a/src/commands/auth.zig b/src/commands/auth.zig index f05ffe0..de9a7e8 100644 --- a/src/commands/auth.zig +++ b/src/commands/auth.zig @@ -83,9 +83,17 @@ fn withPersonalToken(app: app_mod.App, pat: []const u8) !void { } fn status(app: app_mod.App) !void { - const stored = oauth.getToken(app.gpa) orelse { - app.ui.warn("Not authenticated. Run `lcc auth`.", .{}); - return; + const stored = switch (oauth.readToken(app.gpa)) { + .token => |t| t, + .missing => { + app.ui.warn("Not authenticated. Run `lcc auth`.", .{}); + return; + }, + .unreadable => |why| { + app.ui.fail("The Linear token is stored, but reading it failed: {s}", .{why}); + app.ui.hint("Answer `Always Allow` if macOS asks again; `lcc auth` re-stores it if it stays refused.", .{}); + std.process.exit(1); + }, }; const cfg = try config.load(app.gpa, app.io, app.environ); @@ -123,6 +131,10 @@ fn reportAuthError(app: app_mod.App, err: anyerror) noreturn { error.CallbackTimedOut => app.ui.fail("Timed out waiting for browser authorization", .{}), error.AuthorizationDenied => app.ui.fail("Linear returned error: {s}", .{detail}), error.NotAuthenticated => app.ui.fail("Not authenticated. Run `lcc auth` first.", .{}), + error.KeychainUnreadable => app.ui.fail( + "The Linear token is stored, but reading it failed: {s}", + .{detail}, + ), error.TokenExpiredNoRefresh => app.ui.fail( "Access token expired and no refresh token available. Run `lcc auth` again.", .{}, diff --git a/src/commands/issue.zig b/src/commands/issue.zig index c3642e0..79537eb 100644 --- a/src/commands/issue.zig +++ b/src/commands/issue.zig @@ -85,8 +85,17 @@ pub fn run(app: app_mod.App, opts: Opts) !void { fn authorize(app: app_mod.App, opts: Opts) !oauth.Token { app.ui.hint("Reading the Linear token from the Keychain...", .{}); app.ui.flush(); - if (oauth.getToken(app.gpa) == null) { - bail(app, opts.json, "not_authenticated", "Not authenticated. Run `lcc auth` first.", .{}); + switch (oauth.readToken(app.gpa)) { + .token => {}, + .missing => bail(app, opts.json, "not_authenticated", "Not authenticated. Run `lcc auth` first.", .{}), + .unreadable => |why| bail( + app, + opts.json, + "keychain_unreadable", + "The Linear token is in the Keychain, but reading it failed: {s}. " ++ + "Answer `Always Allow` if macOS asks again; `lcc auth` re-stores it if it stays refused.", + .{why}, + ), } const cfg = try config.load(app.gpa, app.io, app.environ); diff --git a/src/commands/list.zig b/src/commands/list.zig index 3f6ea89..ac47152 100644 --- a/src/commands/list.zig +++ b/src/commands/list.zig @@ -199,9 +199,18 @@ fn issueTask(app: app_mod.App, refs: []const linear.Ref, out: *IssueColumn) void out.note = "Linear column skipped — could not read the config."; return; }; - if (oauth.getToken(app.gpa) == null) { - out.note = "Linear column needs `lcc auth`."; - return; + switch (oauth.readToken(app.gpa)) { + .token => {}, + .missing => { + out.note = "Linear column needs `lcc auth`."; + return; + }, + .unreadable => |why| { + out.note = std.fmt.allocPrint(app.gpa, "Linear column skipped — the Keychain refused the token: {s}", .{ + why, + }) catch "Linear column skipped — the Keychain refused the token."; + return; + }, } const token = oauth.ensureFreshToken(app.gpa, app.io, cfg.clientId) catch { out.note = "Could not refresh the Linear token — run `lcc auth`."; diff --git a/src/commands/start.zig b/src/commands/start.zig index fb6db8f..4ad683a 100644 --- a/src/commands/start.zig +++ b/src/commands/start.zig @@ -28,37 +28,46 @@ pub const Opts = struct { plan_mode: bool = true, watch: bool = true, no_attach: bool = false, - cancel_returns: bool = false, + returns_to_caller: bool = false, }; fn cancel(opts: Opts) error{Cancelled} { - if (opts.cancel_returns) return error.Cancelled; + if (opts.returns_to_caller) return error.Cancelled; std.process.exit(app_mod.cancelled_exit_code); } pub fn run(app: app_mod.App, opts: Opts) !void { if (opts.json and opts.issue == null) { - bail(app, opts.json, "usage", "--json needs an issue to resolve, e.g. `lcc start PE-256 --json`.", .{}); + return bail(app, opts, "usage", "--json needs an issue to resolve, e.g. `lcc start PE-256 --json`.", .{}); } if (opts.json and opts.all) { - bail(app, opts.json, "usage", "--all only affects the picker, which --json does not use.", .{}); + return bail(app, opts, "usage", "--all only affects the picker, which --json does not use.", .{}); } const plan_path: ?[]const u8 = if (opts.plan) |raw| blk: { const resolved = Io.Dir.cwd().realPathFileAlloc(app.io, raw, app.gpa) catch |err| switch (err) { - error.FileNotFound => bail(app, opts.json, "plan_not_found", "No plan file at {s}.", .{raw}), - else => bail(app, opts.json, "plan_unreadable", "Cannot read plan at {s}: {s}", .{ raw, @errorName(err) }), + error.FileNotFound => return bail(app, opts, "plan_not_found", "No plan file at {s}.", .{raw}), + else => return bail(app, opts, "plan_unreadable", "Cannot read plan at {s}: {s}", .{ raw, @errorName(err) }), }; const info = Io.Dir.cwd().statFile(app.io, resolved, .{}) catch |err| - bail(app, opts.json, "plan_unreadable", "Cannot read plan at {s}: {s}", .{ raw, @errorName(err) }); + return bail(app, opts, "plan_unreadable", "Cannot read plan at {s}: {s}", .{ raw, @errorName(err) }); if (info.kind != .file) { - bail(app, opts.json, "plan_not_found", "{s} is a {s}, not a plan file.", .{ raw, @tagName(info.kind) }); + return bail(app, opts, "plan_not_found", "{s} is a {s}, not a plan file.", .{ raw, @tagName(info.kind) }); } break :blk resolved; } else null; app.ui.hint("Reading the Linear token from the Keychain...", .{}); app.ui.flush(); - if (oauth.getToken(app.gpa) == null) { - bail(app, opts.json, "not_authenticated", "Not authenticated. Run `lcc auth` first.", .{}); + switch (oauth.readToken(app.gpa)) { + .token => {}, + .missing => return bail(app, opts, "not_authenticated", "Not authenticated. Run `lcc auth` first.", .{}), + .unreadable => |why| return bail( + app, + opts, + "keychain_unreadable", + "The Linear token is in the Keychain, but reading it failed: {s}. " ++ + "Answer `Always Allow` if macOS asks again; `lcc auth` re-stores it if it stays refused.", + .{why}, + ), } const cfg = try config.load(app.gpa, app.io, app.environ); @@ -66,9 +75,9 @@ pub fn run(app: app_mod.App, opts: Opts) !void { const plan_mode = plan_path == null and opts.plan_mode; if (plan_path != null and !templateCarriesPlan(cfg.startTaskCommand)) { - bail( + return bail( app, - opts.json, + opts, "usage", "--plan needs {{plan}} in startTaskCommand — nothing else carries it to the agent. Add it with `lcc setup`.", .{}, @@ -76,7 +85,7 @@ pub fn run(app: app_mod.App, opts: Opts) !void { } const token = oauth.ensureFreshToken(app.gpa, app.io, cfg.clientId) catch |err| { - bail(app, opts.json, "auth_failed", "{s}: {s}", .{ @errorName(err), oauth.last_detail }); + return bail(app, opts, "auth_failed", "{s}: {s}", .{ @errorName(err), oauth.last_detail }); }; const selected = if (opts.issue) |raw| @@ -182,9 +191,9 @@ fn fetchNamed( raw: []const u8, ) !linear.Issue { const trimmed = std.mem.trim(u8, raw, " \t"); - const ref = linear.refFromBranch(trimmed) orelse bail( + const ref = linear.refFromBranch(trimmed) orelse return bail( app, - opts.json, + opts, "bad_identifier", "'{s}' is not an issue identifier — expected something like PE-256.", .{trimmed}, @@ -195,14 +204,15 @@ fn fetchNamed( app.ui.flush(); } - const found = linear.fetchIssue(app.gpa, app.io, token, ref) catch |err| bail( + const found = linear.fetchIssue(app.gpa, app.io, token, ref) catch |err| return bail( app, - opts.json, + opts, "linear_failed", "Linear request failed ({s}, HTTP {d}): {s}", .{ @errorName(err), linear.last_status, linear.last_message }, ); - return found orelse bail(app, opts.json, "issue_not_found", "No issue {s} in Linear.", .{trimmed}); + if (found) |issue| return issue; + return bail(app, opts, "issue_not_found", "No issue {s} in Linear.", .{trimmed}); } fn pickFromActive( @@ -218,9 +228,9 @@ fn pickFromActive( app.ui.step("Fetching Linear issues ({s})...", .{fetch_label}); app.ui.flush(); - const result = linear.fetchActiveIssues(app.gpa, app.io, token, cfg.activeStates, opts.all) catch |err| bail( + const result = linear.fetchActiveIssues(app.gpa, app.io, token, cfg.activeStates, opts.all) catch |err| return bail( app, - opts.json, + opts, "linear_failed", "Linear request failed ({s}, HTTP {d}): {s}", .{ @errorName(err), linear.last_status, linear.last_message }, @@ -358,17 +368,17 @@ fn bootstrap( } const wt = repo.createWorktree(branch, path, base.?) catch |err| switch (err) { - git.Error.WorktreePathExists => bail( + git.Error.WorktreePathExists => return bail( app, - opts.json, + opts, "worktree_path_exists", "Worktree path already exists but no worktree is registered there: {s}\nRemove it first with: git worktree remove {s} (or delete the directory).", .{ path, path }, ), git.Error.GitFailed => if (git.last_error.len > 0) - bail(app, opts.json, "git_failed", "git worktree add failed: {s}", .{git.last_error}) + return bail(app, opts, "git_failed", "git worktree add failed: {s}", .{git.last_error}) else - bail(app, opts.json, "git_failed", "git worktree add failed.", .{}), + return bail(app, opts, "git_failed", "git worktree add failed.", .{}), else => return err, }; created = wt.created; @@ -417,13 +427,15 @@ fn bootstrap( } fn resolveRepo(app: app_mod.App, opts: Opts, identifier: []const u8) !git.Repo { - if (opts.repo) |given| return app.repoAt(given) catch bail( - app, - opts.json, - "bad_repo", - "--repo {s} is not inside a git repository.", - .{given}, - ); + if (opts.repo) |given| { + return app.repoAt(given) catch return bail( + app, + opts, + "bad_repo", + "--repo {s} is not inside a git repository.", + .{given}, + ); + } const state = repos.load(app.gpa, app.io, app.environ); @@ -452,9 +464,9 @@ fn resolveRepo(app: app_mod.App, opts: Opts, identifier: []const u8) !git.Repo { return found; } - if (opts.json) bail( + if (opts.json) return bail( app, - opts.json, + opts, "repo_unconfirmed", "Nothing says which repository {s} belongs to: no answer remembered for it, and " ++ "no branch for it in any repository lcc knows{s}. --json will not fall back to the " ++ @@ -675,13 +687,13 @@ fn isCwd(app: app_mod.App, path: []const u8) bool { fn bail( app: app_mod.App, - json: bool, + opts: Opts, code: []const u8, comptime fmt: []const u8, args: anytype, -) noreturn { +) error{Failed} { const message = std.fmt.allocPrint(app.gpa, fmt, args) catch ""; - if (json) { + if (opts.json) { const body = std.json.Stringify.valueAlloc(app.gpa, .{ .@"error" = .{ .code = code, .message = message }, }, .{ .whitespace = .indent_2 }) catch "{\"error\":{\"code\":\"internal\"}}"; @@ -689,6 +701,7 @@ fn bail( } app.ui.fail("{s}", .{message}); app.ui.flush(); + if (opts.returns_to_caller) return error.Failed; std.process.exit(1); } @@ -741,6 +754,50 @@ fn pickBaseBranch(app: app_mod.App, repo: git.Repo) !?[]const u8 { return branches[index]; } +test "a refusal on the dashboard's new-session path comes back as an error, instead of taking the dashboard down with it" { + const gpa = std.testing.allocator; + const io = std.testing.io; + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var environ: std.process.Environ.Map = .init(arena); + + var out_buf: [4096]u8 = undefined; + var err_buf: [4096]u8 = undefined; + var out_w: Io.Writer = .fixed(&out_buf); + var err_w: Io.Writer = .fixed(&err_buf); + const app: app_mod.App = .{ + .gpa = arena, + .io = io, + .environ = &environ, + .ui = .{ .io = io, .out = &out_w, .err = &err_w }, + }; + + const missing = "/nowhere/lcc-has-no-plan-here.md"; + const answer = run(app, .{ .plan = missing, .returns_to_caller = true }); + + if (answer) |_| { + std.debug.print( + "start.run accepted a plan file that is not there, so the refusal this test " ++ + "watches never happened and nothing was proven about how it comes back.\n", + .{}, + ); + return error.TestUnexpectedResult; + } else |err| { + try std.testing.expectEqual(error.Failed, err); + } + + if (std.mem.indexOf(u8, err_w.buffered(), missing) == null) { + std.debug.print( + "the refusal came back without naming {s} anywhere the user can see it: pressing " ++ + "n on the dashboard would fail silently and redraw as if nothing had happened.\n", + .{missing}, + ); + return error.TestExpectedEqual; + } +} + test "findWorktree prefers the exact branch, then the issue behind it" { const entries = [_]git.WorktreeEntry{ .{ .path = "/r", .branch = "main", .head = "a", .locked = false, .prunable = false, .is_main = true }, diff --git a/src/commands/watch.zig b/src/commands/watch.zig index 798d5e8..3817ee1 100644 --- a/src/commands/watch.zig +++ b/src/commands/watch.zig @@ -354,7 +354,7 @@ fn newSession(app: app_mod.App, screen: *term.Screen, terminal: term.Terminal) ! .no_attach = true, .all = cfg.allIssues, .plan_mode = cfg.planMode, - .cancel_returns = true, + .returns_to_caller = true, }) catch {}; _ = try term.Terminal.enterRaw(); diff --git a/src/keychain.zig b/src/keychain.zig index 76c28f5..d9dc86d 100644 --- a/src/keychain.zig +++ b/src/keychain.zig @@ -19,6 +19,8 @@ const err_sec_success: c.OSStatus = 0; const err_sec_item_not_found: c.OSStatus = -25300; const err_sec_duplicate_item: c.OSStatus = -25299; const err_sec_auth_failed: c.OSStatus = -25293; +const err_sec_interaction_not_allowed: c.OSStatus = -25308; +const err_sec_interaction_required: c.OSStatus = -25315; const err_sec_user_canceled: c.OSStatus = -128; pub var last_status: c.OSStatus = err_sec_success; @@ -139,7 +141,45 @@ pub fn describeStatus(status: c.OSStatus) []const u8 { err_sec_item_not_found => "item not found", err_sec_duplicate_item => "duplicate item", err_sec_auth_failed => "authorization failed (keychain prompt denied?)", + err_sec_interaction_not_allowed, err_sec_interaction_required => "the keychain is locked and no prompt could be shown", err_sec_user_canceled => "user canceled the keychain prompt", else => "unmapped OSStatus", }; } + +pub fn describeLast(gpa: std.mem.Allocator) []const u8 { + return std.fmt.allocPrint(gpa, "{s} (OSStatus {d})", .{ + describeStatus(last_status), + last_status, + }) catch describeStatus(last_status); +} + +test "a refused read is described by what the Keychain answered, not left as a bare failure" { + const gpa = std.testing.allocator; + + const refusals = [_]c.OSStatus{ + err_sec_auth_failed, + err_sec_interaction_not_allowed, + err_sec_user_canceled, + }; + for (refusals) |status| { + last_status = status; + const said = describeLast(gpa); + defer gpa.free(said); + + if (std.mem.indexOf(u8, said, "unmapped") != null) { + std.debug.print( + "OSStatus {d} came back as an unmapped number: the one line telling the user " ++ + "why the Keychain refused reads as noise, and the only thing left to act on " ++ + "is the wrong advice to authenticate again.\n", + .{status}, + ); + return error.TestUnexpectedResult; + } + const number = try std.fmt.allocPrint(gpa, "{d}", .{status}); + defer gpa.free(number); + try std.testing.expect(std.mem.indexOf(u8, said, number) != null); + } + + last_status = err_sec_success; +} diff --git a/src/oauth.zig b/src/oauth.zig index 09fe3cf..238a6eb 100644 --- a/src/oauth.zig +++ b/src/oauth.zig @@ -17,6 +17,7 @@ pub const Token = struct { pub const Error = error{ NotAuthenticated, + KeychainUnreadable, TokenExpiredNoRefresh, TokenEndpointFailed, CallbackFailed, @@ -373,12 +374,31 @@ pub fn refreshAccessToken( return token; } -pub fn getToken(gpa: std.mem.Allocator) ?Token { - const raw = keychain.get(gpa, service, account) catch return null; - const value = raw orelse return null; - return std.json.parseFromSliceLeaky(Token, gpa, value, .{ +pub const Stored = union(enum) { + token: Token, + missing, + unreadable: []const u8, +}; + +pub fn decodeStored(gpa: std.mem.Allocator, raw: ?[]const u8) Stored { + const value = raw orelse return .missing; + const parsed = std.json.parseFromSliceLeaky(Token, gpa, value, .{ .ignore_unknown_fields = true, - }) catch null; + }) catch return .{ .unreadable = "the stored token is not the JSON lcc wrote" }; + return .{ .token = parsed }; +} + +pub fn readToken(gpa: std.mem.Allocator) Stored { + const raw = keychain.get(gpa, service, account) catch + return .{ .unreadable = keychain.describeLast(gpa) }; + return decodeStored(gpa, raw); +} + +pub fn getToken(gpa: std.mem.Allocator) ?Token { + return switch (readToken(gpa)) { + .token => |t| t, + else => null, + }; } pub fn setToken(gpa: std.mem.Allocator, token: Token) !void { @@ -393,8 +413,47 @@ pub fn clearToken() void { keychain.delete(service, account) catch {}; } +test "a token the Keychain would not give up is a different answer from never having logged in" { + const gpa = std.testing.allocator; + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + try std.testing.expectEqual( + @as(std.meta.Tag(Stored), .missing), + decodeStored(arena, null), + ); + + const good = decodeStored(arena, "{\"access_token\":\"lin_oauth_x\",\"scope\":\"read write\"}"); + try std.testing.expectEqualStrings("lin_oauth_x", good.token.access_token); + + const damaged = [_][]const u8{ "", " ", "{", "null", "{\"scope\":\"read\"}" }; + for (damaged) |raw| { + switch (decodeStored(arena, raw)) { + .unreadable => {}, + .missing => { + std.debug.print( + "a Keychain entry holding {s} was reported as no entry at all: the user is " ++ + "sent to `lcc auth` to fix a login that is already there, and the damaged " ++ + "value that actually needs replacing is never named.\n", + .{if (raw.len == 0) "an empty value" else raw}, + ); + return error.TestUnexpectedResult; + }, + .token => return error.TestUnexpectedResult, + } + } +} + pub fn ensureFreshToken(gpa: std.mem.Allocator, io: Io, client_id: []const u8) Error!Token { - const token = getToken(gpa) orelse return Error.NotAuthenticated; + const token = switch (readToken(gpa)) { + .token => |t| t, + .missing => return Error.NotAuthenticated, + .unreadable => |why| { + last_detail = why; + return Error.KeychainUnreadable; + }, + }; if (token.is_pat orelse false) return token; const expired = if (token.expires_at) |at| at <= nowSeconds(io) else false;