Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Code Review: Add interactive login
|
|
Claude finished @ramilamparo's task in 5m 56s —— View job Code Review: Add interactive loginSolid implementation of device authorization grant (RFC 8628) with keyring/file credential storage. The architecture is clean — well-separated API layer, credential management, and CLI commands. The test suite is comprehensive with good MSW-based coverage. A few issues worth addressing: Security1. Server-returned
Fix: validate the origin — at minimum enforce const origin = new URL(data.redirectUrl).origin
if (!origin.startsWith('https://')) {
throw new Error(`Unexpected non-HTTPS tenant URL: ${origin}`)
}2. CONFIG_DIR created with default umask —
mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 })Code Quality3. Type-only imports should use
import { saveCredentials, clearCredentials, type CredentialSource } from '../utils/credentials'
// ...
import { checkTenant, requestDeviceCode, pollDeviceToken, type DeviceCodeResponse } from '../api/deviceAuth'4. Duplicated The header is manually specified in all three exported functions. Bake it into const createFetcher = (baseUrl: string) => {
const fetcher = withHttpRetry(withJson(withBaseUrl(fetch, baseUrl)))
return (url: string, init?: RequestInit) =>
fetcher(url, {
...init,
headers: { 'User-Agent': `qas-cli/${CLI_VERSION}`, ...init?.headers },
})
}Test Coverage Gaps5. The catch block for 6. Keyring There's no test for a keyring that loads successfully (module import + |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8908deb to
6b05689
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Clean implementation. The latest commits have addressed all prior review findings (tenantUrl origin extraction, DRY warning constant, type-only imports for CredentialSource/DeviceCodeResponse, middleware composition for User-Agent). The architecture is well-separated — credential storage with keyring/file fallback, composable fetch middlewares, and comprehensive E2E tests with proper isolation via vi.doMock/resetModules.
One minor import type nit in an inline comment — otherwise LGTM.
|
Claude finished @ramilamparo's task in 7m 3s —— View job Code Review: Add interactive login
Ran four specialized subagents (code quality, security, performance, test coverage) against the full diff. All prior review findings have been addressed in the latest commits — Minimized 5 outdated bot review summaries. Could not delete progress tracking comments due to insufficient permissions. Verdict: LGTM — clean implementation, well-structured architecture, comprehensive test suite. |

Summary
Adds interactive
qasphere authcommands for login, logout, and status — replacing the need to manually manage API keys and environment variables.Login methods
--api-keyflag) — prompts for team name and API key directly (hidden input).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 login --api-keyqasphere auth statusqasphere auth logout