CLI: Implement auth login command
#2017
Conversation
| cmd = 'PowerShell'; | ||
| args = [ | ||
| '-NoProfile', | ||
| '-NonInteractive', | ||
| '-ExecutionPolicy', | ||
| 'Bypass', | ||
| '-Command', | ||
| 'Start', | ||
| `"${ url }"`, | ||
| ]; |
There was a problem hiding this comment.
I have yet to test this. This was taken from Sindre Sorhus's open module.
📊 Performance Test ResultsComparing 28ef7cd vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
|
Working on a test right now. |
|
Looking at how many changes I had to make today, I see that I opened this PR a little early. In any case, it's good for a full review now 👍 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a CLI authentication command (studio auth login) to allow users to authenticate with WordPress.com directly from the command line. The implementation opens a browser for OAuth authentication and stores the resulting token with user information.
Key changes:
- Adds
@inquirer/promptsdependency for interactive token input - Implements OAuth-based login flow with browser integration
- Enhances auth token storage to include expiration time and user details
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json, package-lock.json | Added @inquirer/prompts dependency for interactive CLI prompts |
| common/logger-actions.ts | Added AuthCommandLoggerAction enum for login/logout tracking |
| common/lib/oauth.ts | Made redirect URI configurable for CLI authentication flow |
| cli/lib/i18n.ts | Extracted getAppLocale() function for reusability |
| cli/lib/browser.ts | New utility to open default browser across platforms |
| cli/lib/appdata.ts | Enhanced auth token schema with expiration, email, and display name |
| cli/lib/api.ts | Added getUserInfo() function to fetch user details from WordPress.com |
| cli/index.ts | Registered auth command group with login subcommand |
| cli/commands/auth/login.ts | Main login command implementation |
| cli/commands/auth/tests/login.test.ts | Comprehensive test coverage for login command |
| cli/lib/tests/snapshots.test.ts, appdata.test.ts | Updated test mocks to include new auth token fields |
Comments suppressed due to low confidence (1)
cli/lib/browser.ts:2
- Unused import sprintf.
import { __, sprintf } from '@wordpress/i18n';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cli/commands/auth/login.ts
Outdated
| email: user.email, | ||
| displayName: user.display_name, | ||
| expiresIn: twoWeeksInSeconds, | ||
| expirationTime: now.getTime() + twoWeeksInSeconds, |
There was a problem hiding this comment.
The expirationTime calculation is incorrect. twoWeeksInSeconds is in seconds (1209600), but getTime() returns milliseconds. This should be now.getTime() + twoWeeksInSeconds * 1000 to convert seconds to milliseconds, or the calculation should use twoWeeksInSeconds * 1000 directly.
| expirationTime: now.getTime() + twoWeeksInSeconds, | |
| expirationTime: now.getTime() + twoWeeksInSeconds * 1000, |
| let cmd: string; | ||
| let args: string[]; |
There was a problem hiding this comment.
Variables cmd and args are not initialized and will be undefined if process.platform doesn't match 'darwin', 'win32', or 'linux'. This will cause the spawn() call on line 37 to fail. Add a default case to the switch statement that throws an error for unsupported platforms.
cli/lib/browser.ts
Outdated
| @@ -0,0 +1,49 @@ | |||
| import { spawn } from 'child_process'; | |||
| import { __, sprintf } from '@wordpress/i18n'; | |||
There was a problem hiding this comment.
The sprintf function is imported but never used in this file. Remove the unused import.
| import { __, sprintf } from '@wordpress/i18n'; | |
| import { __ } from '@wordpress/i18n'; |
cli/lib/browser.ts
Outdated
| import { LoggerError } from 'cli/logger'; | ||
|
|
There was a problem hiding this comment.
Unused import LoggerError.
| import { LoggerError } from 'cli/logger'; |
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
I confirm that the login works as expected, and I was able to authenticate by copying and pasting the token. Here are a couple of suggestions to improve the experience:
- We could avoid hiding the token so the user can see they pasted the correct value.
- We could automatically try three times when the user enters an invalid token, similarly to SSH connections. Currently the feedback is very rough
✖ Failed to fetch user info: The OAuth2 token is invalid., we could at least invite the user to try again.
common/logger-actions.ts
Outdated
|
|
||
| export enum AuthCommandLoggerAction { | ||
| LOGIN = 'login', | ||
| LOGOUT = 'logout', |
There was a problem hiding this comment.
Nit: We don't really need the logout at this point.
cli/commands/auth/login.ts
Outdated
| const userData = await readAppdata(); | ||
|
|
||
| const now = new Date(); | ||
| const twoWeeksInSeconds = 2 * 7 * 24 * 60 * 60; |
There was a problem hiding this comment.
I just saw that the expiresIn in appData is 1209600 seconds or two weeks, but I always thought the token was valid for months. 🤔
There was a problem hiding this comment.
This is governed by the back-end. We could look into this further if we think two weeks is too short
|
I like how simple this is. Good work :) |
|
Claude finished @fredrikekelund's task —— View job Code Review Complete ✓I've performed a comprehensive code review of the 🔴 Critical Issues1. Uninitialized Variables in
|
|
Claude finished @fredrikekelund's task —— View job Documentation updated for auth login command
SummaryI've updated the documentation in Added new "CLI Commands" section with:
Updated directory structure section to reflect:
The documentation now provides clear guidance for anyone using or working with the authentication CLI command. |
Add comprehensive documentation for the new `studio auth login` command including usage instructions, flow description, and important notes about token lifetime and authentication sharing between CLI and desktop app. Co-authored-by: Fredrik Rombach Ekelund <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export async function openBrowser( url: string ): Promise< void > { | ||
| const platform = process.platform; | ||
| let cmd: string; | ||
| let args: string[]; |
There was a problem hiding this comment.
🔴 Critical: Variables cmd and args are declared but not initialized. If process.platform doesn't match any case in the switch statement, these will be undefined causing spawn() to fail on line 38.
Recommendation: Add a default case to the switch statement:
default:
throw new Error(__('Unsupported platform'));This was also flagged in the Copilot review.
cli/lib/browser.ts
Outdated
| @@ -0,0 +1,50 @@ | |||
| import { spawn } from 'child_process'; | |||
| import { __ } from '@wordpress/i18n'; | |||
There was a problem hiding this comment.
🟡 Minor: sprintf is imported but never used in this file. Should be removed:
import { __ } from '@wordpress/i18n';
| logger.reportError( loggerError ); | ||
| } | ||
|
|
||
| console.log( |
There was a problem hiding this comment.
🟢 Code quality: Consider using the logger for consistency instead of console.log directly. This ensures uniform output formatting and enables better control over logging behavior.
| user = await getUserInfo( accessToken ); | ||
| logger.reportSuccess( __( 'Authentication completed successfully!' ) ); | ||
| } catch ( error ) { | ||
| logger.reportError( new LoggerError( __( 'Authentication failed. Please try again.' ) ) ); |
There was a problem hiding this comment.
🟡 UX Enhancement: As noted in the PR reviews, consider:
- Adding retry logic (e.g., 3 attempts like SSH)
- Providing more specific error messages from the API
- Preserving the original error context:
catch (error) {
const message = error instanceof LoggerError
? error.message
: __('Authentication failed. Please try again.');
logger.reportError(new LoggerError(message, error));
return;
}
There was a problem hiding this comment.
I agree that a retry logic would improve the UX to account for a mistake paste or enter
There was a problem hiding this comment.
Another idea would be to have a "click here if you aren't automatically redirected" to allow users to manually open the browser tab.
Neither of these are blockers in my opinion, just considerations.
|
Claude finished @fredrikekelund's task —— View job Documentation Review CompleteI've reviewed the CLI changes in this PR and verified the documentation status. Here are my findings: SummaryNo additional documentation updates are needed. The What's Already DocumentedThe documentation already includes:
VerificationI verified that:
ConclusionThe documentation is up-to-date and complete. No commit is needed since the previous Claude run already handled the documentation updates appropriately when the CLI command was implemented. |
|
Claude finished @fredrikekelund's task —— View job Code Review Complete ✓I've performed a comprehensive code review of the 🟢 Strengths
🔴 Critical Issues1. Linux Path Handling in
|
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @fredrikekelund's task —— View job Documentation Review for CLI Changes
SummaryNo documentation updates are needed. The Verification Findings
ConclusionThe documentation accurately reflects the implementation. The minor code quality improvement in commit |
| user = await getUserInfo( accessToken ); | ||
| logger.reportSuccess( __( 'Authentication completed successfully!' ) ); | ||
| } catch ( error ) { | ||
| logger.reportError( new LoggerError( __( 'Authentication failed. Please try again.' ) ) ); |
There was a problem hiding this comment.
I agree that a retry logic would improve the UX to account for a mistake paste or enter
| user = await getUserInfo( accessToken ); | ||
| logger.reportSuccess( __( 'Authentication completed successfully!' ) ); | ||
| } catch ( error ) { | ||
| logger.reportError( new LoggerError( __( 'Authentication failed. Please try again.' ) ) ); |
There was a problem hiding this comment.
Another idea would be to have a "click here if you aren't automatically redirected" to allow users to manually open the browser tab.
Neither of these are blockers in my opinion, just considerations.
Related issues
Proposed Changes
This PR is based on @youknowriad's work in #1958. It implements a
studio auth logincommand that:Testing Instructions
developer.wordpress.comnpm run cli:buildnode dist/cli/main.js auth loginPre-merge Checklist