Conversation
Code Review: Add interactive login
|
8908deb to
6b05689
Compare
|
Claude finished @ramilamparo's task in 8m 37s —— View job Code Review: Add interactive login
The latest push ( Ran four specialized subagents against the full PR diff. No new noteworthy findings beyond what prior reviews have already raised. Five open items remain unaddressed but none are blocking:
Resolved 3 now-addressed bot threads, minimized 14 outdated bot review summaries, and deleted 1 progress-tracking comment. Submitted as APPROVE. |
hi-rai
left a comment
There was a problem hiding this comment.
PR looks okay. Few minor nits
| console.log(` Status: ${chalk.red('invalid or expired')}`) | ||
| } | ||
|
|
||
| if (result.authType === 'bearer') { |
There was a problem hiding this comment.
We should show this only if token is still valid. Also we should show the refresh token expiry instead of access token expiry ?
There was a problem hiding this comment.
Also we should show the refresh token expiry instead of access token expiry ?
Yes I think that's a more useful. But currently the API expires_in only returns the expiry time in seconds for access token. Should we just hardcoded constant 90 day expiry, or should we get this from backend?
There was a problem hiding this comment.
Okay only the ListAuthorizations API returns it - which we use to show the expiry on UI. The /oauth/token authorization endpoint currently doesn't return it
The spec doesn't require the refresh token expiry to be present. But we can add it as a non-standard extension. Many providers do it as well. I will open a PR for it
| keyring: 'the system keyring', | ||
| 'credentials.json': 'a credentials file (~/.config/qasphere/credentials.json)', |
There was a problem hiding this comment.
Should we expose the internal details of how the oauth credentials are stored to the user ? Or simply mention oauth ?
There was a problem hiding this comment.
I don't see the harm. Showing "OAuth" could be a bit misleading. Since keyring also uses OAuth.
| const url = verification_uri_complete || `${verification_uri}?code=${normalizedCode}` | ||
| console.log(`\nIf the browser didn't open, visit:\n ${url}\n`) | ||
| openBrowser(url) | ||
| console.log(`Verify the code displayed in the browser: ${chalk.bold(normalizedCode)}\n`) |
There was a problem hiding this comment.
nit: I guess the non-normalized code would be better for readability ?
There was a problem hiding this comment.
I was thinking because verification_url_complete returns non-hyphenated code in the url http://tenant1.localhost:5173/settings/oauth/device?code=M4NGNW8R
So frontend will show the non-hyphenated code as well. User might confuse that hyphenated code missing is invalid. So I normalized it to non-hyphenated code.
There was a problem hiding this comment.
The input on the UI currently automatically adds the hyphens. I will do the same for the final confirmation (it currently doesn't show hyphens). I guess we can ignore the URL
|
Claude finished @ramilamparo's task in 10m 18s —— View job Code Review: Add interactive login
The latest commit ( Ran four specialized subagents against the full PR diff. No new noteworthy findings beyond what prior reviews have already raised. Two previously-open test gaps are now addressed ( |
|
Claude finished @ramilamparo's task in 10m 25s —— View job Code Review: Add interactive login
The latest commit ( Ran four specialized subagents against the full PR diff. Two new findings posted as inline comments:
Minimized 16 outdated bot review summaries. Three open items from prior reviews remain unaddressed but none are blocking. Submitted as APPROVE. |
Code Review: Add interactive login
|
|
Claude finished @ramilamparo's task in 14m 7s —— View job Code Review: Add interactive login
The latest commits ( Ran four specialized subagents against the full PR diff. One new finding posted as an inline comment: Minimized 17 outdated bot review summaries. Three open items from prior reviews remain unaddressed but none are blocking. Submitted as APPROVE. |
There was a problem hiding this comment.
The latest commits (5ce6d76 — "Nits", 90bac86 — "Add refresh token expiration") address both findings from the prior review:
tenantUrlschema now usesz.string().url()for proper URL validation ✓resolveAutherror message now leads with "Not authenticated" and suggestsauth loginfirst ✓- Status now shows refresh token expiry in days instead of access token expiry in minutes ✓
- User code shown as-is from server without normalization ✓
One new finding posted inline — refresh_token_expires_in is used without a fallback default, which could crash login if the backend hasn't been updated to return this field yet.
Three open items from prior reviews remain unaddressed but none are blocking:
oauth.ts:134—refreshAccessTokenomitsclient_id(consistency with other OAuth calls)browser.ts:15— Windowscmd /c startshell metacharacter handling (URLs are server-controlled, low practical risk)keyring.ts:22—loadKeyringModule()cache opportunity
LGTM.

Summary
Adds interactive
qasphere authcommands for login, logout, and status — replacing the need to manually manage API keys and environment variables.For testing purposes, you can override the
QAS_LOGIN_SERVICE_URL.Login methods
Credential storage
Credentials are persisted in priority order:
@napi-rs/keyring) — preferred, used when the OS keyring daemon is available~/.config/qasphere/credentials.json) — restricted permissions (0600), used when keyring is unavailableCredential resolution
resolveCredentialSource()checks sources in priority order:QAS_TOKEN+QAS_URLenvironment variables.envfile in cwd~/.config/qasphere/credentials.json.qasphereclifile (searched up directory tree)Auth subcommands
qasphere auth loginqasphere auth statusqasphere auth logout