Skip to content

Add interactive login#64

Open
ramilamparo wants to merge 21 commits intomainfrom
dev/ram/interactive-login
Open

Add interactive login#64
ramilamparo wants to merge 21 commits intomainfrom
dev/ram/interactive-login

Conversation

@ramilamparo
Copy link
Copy Markdown
Collaborator

@ramilamparo ramilamparo commented Apr 8, 2026

Summary

Adds interactive qasphere auth commands 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

  • Device Authorization Grant (RFC 8628) — default flow. Opens a browser to the QA Sphere verification page, displays a user code, and polls until the user approves. No secrets typed into the terminal.

Credential storage

Credentials are persisted in priority order:

  1. System keyring (@napi-rs/keyring) — preferred, used when the OS keyring daemon is available
  2. File fallback (~/.config/qasphere/credentials.json) — restricted permissions (0600), used when keyring is unavailable

Credential resolution

resolveCredentialSource() checks sources in priority order:

  1. QAS_TOKEN + QAS_URL environment variables
  2. .env file in cwd
  3. System keyring
  4. ~/.config/qasphere/credentials.json
  5. .qaspherecli file (searched up directory tree)

Auth subcommands

Command Description
qasphere auth login Device authorization flow (browser-based)
qasphere auth status Shows current auth source and validates the API key
qasphere auth logout Clears keyring or file credentials; warns if env-based credentials remain

chatgpt-codex-connector[bot]

This comment was marked as outdated.

gemini-code-assist[bot]

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 8, 2026

Code Review: Add interactive login

  • Read review instructions
  • Gather full PR diff and context
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings and post review with inline comments
  • Clean up outdated bot comments and threads

Running specialized review subagents...

View job run

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

@ramilamparo ramilamparo requested a review from a team April 9, 2026 13:40
@ramilamparo ramilamparo force-pushed the dev/ram/interactive-login branch from 8908deb to 6b05689 Compare April 9, 2026 14:10
claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

Claude finished @ramilamparo's task in 8m 37s —— View job


Code Review: Add interactive login

  • Gather PR context and full diff
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings and post review
  • Clean up outdated bot comments and threads

The latest push (9837ba6 — "Fixes") addresses three open items from prior reviews: replace('-', '')replaceAll, sourceLabels now includes keyring/credentials.json, and resolveAuth error output uses console.error.

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:

  1. oauth.ts:134refreshAccessToken omits client_id
  2. browser.ts:15 — Windows cmd /c start shell metacharacter handling
  3. keyring.ts:22loadKeyringModule() cache opportunity
  4. oauth.ts — OAuth responses lack Zod runtime validation
  5. auth-e2e.spec.ts — test gaps (default OAuth error branch, polling network error, keyring-backed refresh persistence, clearCredentials failure in logout)

Resolved 3 now-addressed bot threads, minimized 14 outdated bot review summaries, and deleted 1 progress-tracking comment. Submitted as APPROVE.

claude[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@hi-rai hi-rai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks okay. Few minor nits

Comment thread README.md
Comment thread src/api/oauth.ts
Comment thread src/commands/auth.ts
Comment thread src/commands/auth.ts Outdated
Comment thread src/commands/auth.ts Outdated
Comment thread src/commands/auth.ts Outdated
Comment thread src/commands/auth.ts Outdated
console.log(` Status: ${chalk.red('invalid or expired')}`)
}

if (result.authType === 'bearer') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should show this only if token is still valid. Also we should show the refresh token expiry instead of access token expiry ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/commands/auth.ts
Comment on lines +194 to +195
keyring: 'the system keyring',
'credentials.json': 'a credentials file (~/.config/qasphere/credentials.json)',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we expose the internal details of how the oauth credentials are stored to the user ? Or simply mention oauth ?

Copy link
Copy Markdown
Collaborator Author

@ramilamparo ramilamparo Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the harm. Showing "OAuth" could be a bit misleading. Since keyring also uses OAuth.

Comment thread src/commands/auth.ts Outdated
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`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I guess the non-normalized code would be better for readability ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/commands/auth.ts Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @ramilamparo's task in 10m 18s —— View job


Code Review: Add interactive login

  • Gather PR context and full diff
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings, deduplicate against existing comments, and post review
  • Clean up outdated bot comments and threads

The latest commit (b103da8 — "Tweaks") addresses hi-rai's review feedback: REFRESH_TOKEN_GRANT_TYPE constant added, status message changed to "Credentials connected via", token expiry only shown when status is valid, more specific revocation instructions, and simplified "Remove them manually." text.

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 (clearCredentials failure path at line 1001, keyring-backed refresh persistence at line 945). Resolved 2 now-addressed bot threads and minimized 13 outdated bot review summaries. Submitted as APPROVE.

claude[bot]

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @ramilamparo's task in 10m 25s —— View job


Code Review: Add interactive login

  • Gather PR context and full diff
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings, deduplicate against existing comments, and post review
  • Clean up outdated bot comments and threads

The latest commit (ba1fe61 — "Improve command description") updates the auth status describe text to better reflect its behavior, addressing hi-rai's feedback.

Ran four specialized subagents against the full PR diff. Two new findings posted as inline comments:

  1. types.ts:9tenantUrl schema uses .min(1) instead of .url(), missing URL format validation at the credential boundary
  2. resolvers.ts:153resolveAuth() error message leads with "Missing required environment variables" when OAuth is now the primary auth path

Minimized 16 outdated bot review summaries. Three open items from prior reviews remain unaddressed but none are blocking. Submitted as APPROVE.

claude[bot]

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review: Add interactive login

  • Gather PR context and full diff
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings, deduplicate against existing comments, and post review
  • Clean up outdated bot comments and threads

Running specialized review subagents...

View job run

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @ramilamparo's task in 14m 7s —— View job


Code Review: Add interactive login

  • Gather PR context and full diff
  • Run specialized review subagents (code quality, security, performance, test coverage)
  • Aggregate findings, deduplicate against existing comments, and post review
  • Clean up outdated bot comments and threads

The latest commits (5ce6d76 — "Nits", 90bac86 — "Add refresh token expiration") address both findings from the prior review: tenantUrl schema now uses .url() for proper URL validation, and resolveAuth error message now leads with "Not authenticated" and suggests auth login first. Status now shows refresh token expiry in days, and user code is shown as-is without normalization.

Ran four specialized subagents against the full PR diff. One new finding posted as an inline comment: refresh_token_expires_in is used without a fallback default — if the server hasn't been updated to return this field yet, login will crash with a RangeError.

Minimized 17 outdated bot review summaries. Three open items from prior reviews remain unaddressed but none are blocking. Submitted as APPROVE.

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commits (5ce6d76 — "Nits", 90bac86 — "Add refresh token expiration") address both findings from the prior review:

  1. tenantUrl schema now uses z.string().url() for proper URL validation ✓
  2. resolveAuth error message now leads with "Not authenticated" and suggests auth login first ✓
  3. Status now shows refresh token expiry in days instead of access token expiry in minutes ✓
  4. 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:

  1. oauth.ts:134refreshAccessToken omits client_id (consistency with other OAuth calls)
  2. browser.ts:15 — Windows cmd /c start shell metacharacter handling (URLs are server-controlled, low practical risk)
  3. keyring.ts:22loadKeyringModule() cache opportunity

LGTM.

Comment thread src/commands/auth.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants