feat: harper login and logout - #527
Conversation
|
Reviewed; no blockers found. Prior finding (untested JWT auth paths in |
65f263d to
50b332f
Compare
|
Should we put it in |
c875ab6 to
d51d387
Compare
|
@kriszyp the harper-agent puts its stuff in that ~/.harper/ folder already, and I thought we had some other stuff that can get put in there. If that's where we want it to be eventually, might as well stick it there now, that's my thinking at least! |
1efb3f6 to
b46db0c
Compare
|
@Ethan-Arrowood I shifted to TypeScript and shuffled a few things around per your recommendations. The dotenv configs are also clearer now, happening when login, logout or cliOperations are invoked. Those are the 3 cases where we'll be purely shuttling requests to a targeted instance, and not handling them locally. |
|
The unit test failure isn't related to the changes made here. |
kriszyp
left a comment
There was a problem hiding this comment.
This is an awesome feature and I love it. I will include my AI review of this because it seems like it has some good suggestions, but ultimately I approve of this (if you want ~/.harper, that's ok with me too):
-
.harper vs .harperdb inconsistency — user-visible
bin/cliCredentials.ts writes to path.join(getHomeDir(), '.harperdb', 'credentials.json') (correct — matches terms.HDB_HOME_DIR_NAME = '.harperdb'), but the welcome banner in bin/login.ts tells the user the file lives in ~/.harper/credentials.json, and the PR description also says ~/.harper. Users following the printed instructions will look in the wrong place. Use terms.HDB_HOME_DIR_NAME everywhere and update the prompt text. Even better: hard-code neither — print the resolved path. -
saveCredentials silently swallows write failures
saveCredentials catches its own errors and only console.errors. bin/login.ts:152 then prints Successfully logged in to ... and process.exit(0) regardless. If the home dir is unwritable, the user is told they're logged in but nothing was persisted. saveCredentials should throw (or return a boolean) and login should surface failure and non-zero exit. -
cliOperations fallback branch writes malformed data
In saveCredentials:
} else {
// Fallback for when target is not provided (shouldn't happen with new login)
Object.assign(allCredentials, tokens);
}
This sprays operation_token / refresh_token onto the top-level TargetedCredentials object, violating the declared type and producing a file no other code knows how to read. Since the caller guarantees a target, just delete the branch (or throw) — don't leave a broken fallback as a trap for future reads.
-
clearCredentials only writes back when targets existed
If a user runs harper logout some-stale-target and that key isn't present, the function silently does nothing — no message, no error, success exit. At minimum print a "no credentials found for X" message and exit non-zero. The "altTarget without trailing slash" double-lookup is also a sign the data isn't being normalized on the way in (see point 5). -
Centralize target normalization
normalizeTarget exists in bin/login.ts but saveCredentials, clearCredentials, and the cliOperations lookup each re-implement "append a slash, then also try without one." Pick one canonical form, normalize once at the entry point, and stop carrying the variant matching everywhere. The trailing-slash fallback chains (allCredentials.targets[lookupKey] ?? allCredentials.targets[target.resolvedTarget.replace(//$/, '')]) get harder to reason about as the surface grows.
Medium
6. Hand-rolled raw-mode password prompt
The 30-line setRawMode block in login.ts (the author already flagged this) doesn't handle Unicode well (char += data.toString('utf-8') accumulates byte-wise, breaking multi-byte chars split across chunks), doesn't disable echo via the normal Node pattern, and process.exit(1) on Ctrl+C skips the finally { rl.close() }. The conventional fix is a tiny writable wrapper passed as output that swallows writes after the prompt — readline then handles backspace, Ctrl+C, paste, and Unicode for free. If you keep the hand-rolled version, at least: decode with StringDecoder, restore the TTY before exiting, and clear password from memory after use.
-
Lockfile churn looks unintentional
The package-lock.json portion of the diff flips peer: true / dev: true flags on a large number of unrelated packages (smithy, eslint, typescript-eslint, @types/node, acorn, etc.) and adds an optional bufferutil dependency that doesn't appear motivated by login/logout. This looks like a lockfile regenerated under a different npm or Node version, not a deliberate dependency change for this PR. Worth regenerating against the project's canonical npm version and dropping the noise — otherwise it'll trigger spurious diffs for everyone else on rebase. -
isJWTExpired import
bin/cliOperations.ts imports isJWTExpired from security/tokenAuthentication.ts. I couldn't find it in the current source — confirm it exists on the target branch (or is being added in this PR) and isn't a Junie hallucination. If it's new, make sure it's not double-checking expiry of an unverified JWT (i.e., decoding without verifying signature is fine for "should I bother refreshing", but worth a comment). -
Refresh-on-expiry: no retry on the original op
If operation_token is expired and the refresh succeeds, the new token is saved — but the original request continues with options.headers.Authorization = Bearer ${tokens.operation_token}. That line is set after refresh, so it works on the happy path. However, if the refresh fails (network error, 401), the code still attaches the expired token below; the server will 401 and the user gets the raw error. Consider: if refresh fails, prompt "session expired, run harper login again" instead of letting the underlying call surface a generic 401. -
cliOperations signature lost its type annotation
Was async function cliOperations(req: any), now async function cliOperations(req, skipResponseLog = false). Either restore the any or type properly — Harper enforces TypeStrip-clean type annotations on signatures. -
require('dotenv').config() inside cliOperations
Every CLI op now re-parses .env from disk. The login and logout functions also each call dotenv.config(). Move this to a single early init point. Also: cliOperations.ts mixes import at the top with require('dotenv').config() inline — pick one style. -
.env mutation on first login
The login flow appends HARPER_CLI_TARGET=... to a .env file in process.cwd(). This is surprising — harper login from any random directory will scribble in that directory's .env. If the user is in their app repo, fine; if they're in /, they get a stray .env. At minimum: only do this if a .env already exists, or prompt before creating one. Also, it doesn't check whether HARPER_CLI_TARGET is already set in .env under a different value — appending a second line silently shadows.
Minor
require('./cliOperations.js') from login.ts uses the .js extension while the rest of the new code uses .ts imports. Inconsistent under TypeStrip.
clearCredentials picks remainingTargets[0] as the new last_target — arbitrary; consider null instead so the next op falls back to env vars rather than a surprise different host.
process.exit(0) at the end of logout skips any pending async work; for this command it's fine, but exiting from a library-style function is a smell.
Comment "shouldn't happen with new login" in bin/cliCredentials.ts — if it shouldn't happen, throw; don't leave the malformed-write fallback.
This saves jwts, per target, in your ~/.harperdb directory.
This saves jwts, per target, in your ~/.harper directory under credentials.json. There are likely common bits that are already implemented that Junie missed, and wasted time recreating. It got a bit creative with the readline stuff too, I think that could be tighter, but the experience is decent.
Will follow up with a documentation PR and adjustments to create-harper if we like this pattern.
To verify this, I
npm link'd harper, and went through various use cases.