CLI: Implement auth logout command
#2027
Conversation
📊 Performance Test ResultsComparing 4465a21 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
gcsecsey
left a comment
gcsecsey
left a comment
There was a problem hiding this comment.
Thanks @fredrikekelund, this looks good to me and works well. 👍
The command's output is as described:
| Logged in | Logged out |
|---|---|
 |
 |
When switching back to Studio, I'm also logged out:
The new tests are also all green:
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 finished @fredrikekelund's task —— View job Comprehensive Code Review CompleteI've completed a thorough review of the 🔴 Critical Issues (Must Fix)1. Race Condition with Lock AcquisitionLocation: The Impact: High - Can result in deleting a newly created token from a concurrent login operation. 2. Inconsistent Error State HandlingLocation: The current error handling allows the system to enter an inconsistent state:
Impact: High - Users will be confused about their actual authentication state. 🟡 Important Issues (Should Fix)3. API Error Handling Too GenericLocation: The Impact: Medium - Poor developer experience and user experience with error messages. 4. Missing Test CoverageLocation: No tests for:
Impact: Medium - Edge cases are not validated. 5. Test Validates Buggy BehaviorLocation: The test on line 75 asserts that Impact: Medium - Test will fail when the race condition is fixed. 🟢 Minor Issues (Nice to Have)6. Redundant Method ParameterLocation: The 7. Fragile Test AssertionLocation: The assertion 8. Inconsistent Error Handling PatternLocation: The error handling for 9. Missing DocumentationLocation: The file documents 10. Missing JSDoc CommentsLocation: Adding JSDoc to the enum would help explain its purpose, especially given the comment about Webpack issues. ✅ What's Good
📋 RecommendationsPriority 1 (Before Merge)
Priority 2 (Follow-up PR)
Priority 3 (Optional)
🔍 Cross-Platform Review✅ No cross-platform issues found - The code properly uses:
🛡️ Security Review✅ Security is generally good:
📊 Performance✅ No performance concerns - The command is lightweight and performs only necessary operations. Overall AssessmentThis is a solid implementation that follows the codebase patterns well. The main concerns are around error handling edge cases and a race condition that could cause issues in concurrent scenarios. The tests are comprehensive but validate some buggy behavior. Recommendation: Address the critical issues (race condition and error state handling) before merging. The other issues can be addressed in follow-up PRs if time is constrained. Great work overall! The code is clean, well-structured, and follows the established patterns. 🎉 |
|
Claude Code is working… I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await lockAppdata(); | ||
| const userData = await readAppdata(); | ||
| delete userData.authToken; | ||
| await saveAppdata( userData ); |
There was a problem hiding this comment.
Security Issue: Token remains valid after local deletion
The current implementation has a critical security flaw: if the revokeAuthToken() call succeeds but the local deletion fails (lines 30-33), the user will see an error message, but their token has already been revoked on the server. This means:
- The token is revoked remotely (server-side)
- The local deletion fails (e.g., file permission issue)
- User sees "Failed to log out" error
- The invalid token remains in local appdata
Suggested fix:
try {
await revokeAuthToken( token.accessToken );
} catch ( error ) {
logger.reportError( new LoggerError( __( 'Failed to revoke token on server' ), error ) );
return; // Don't proceed with local deletion if server revocation failed
}
// Now attempt local cleanup
try {
await lockAppdata();
const userData = await readAppdata();
delete userData.authToken;
await saveAppdata( userData );
logger.reportSuccess( __( 'Successfully logged out' ) );
} catch ( error ) {
// Token is revoked on server but local cleanup failed
logger.reportError( new LoggerError( __( 'Token revoked but failed to update local config. Please try again.' ), error ) );
} finally {
await unlockAppdata();
}This ensures the error messages accurately reflect the state and prevents confusion.
| try { | ||
| await revokeAuthToken( token.accessToken ); | ||
|
|
||
| await lockAppdata(); |
There was a problem hiding this comment.
Race Condition: Lock acquired AFTER API call
The lockAppdata() is called after revokeAuthToken(), which creates a race condition. Consider this scenario:
- User A runs
studio auth logout(Process A starts) - Process A calls
revokeAuthToken()- succeeds - User A runs
studio auth loginfrom another terminal (Process B starts) - Process B locks appdata, writes new token
- Process A locks appdata (waiting for Process B)
- Process A deletes the NEW token that was just created
Fix: Move the lock acquisition before the revoke call:
try {
await lockAppdata();
await revokeAuthToken( token.accessToken );
const userData = await readAppdata();
delete userData.authToken;
await saveAppdata( userData );
logger.reportSuccess( __( 'Successfully logged out' ) );
} catch ( error ) {
// error handling
} finally {
await unlockAppdata();
}This ensures the entire logout operation is atomic.
There was a problem hiding this comment.
It's unlikely, but I think it's a good approach.
There was a problem hiding this comment.
I agree the case that Claude describes is very unlikely, but I still realize that lockAppdata should always be the first thing we call in a try..catch block to prevent cases where we unlock appdata without actually having acquired the lock.
| await runCommand(); | ||
|
|
||
| expect( getAuthToken ).toHaveBeenCalled(); | ||
| expect( lockAppdata ).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Test Issue: Incorrect assertion - lock should NOT be called
This test expects lockAppdata to not be called when token revocation fails, but the current implementation (line 30 in logout.ts) calls lockAppdata() AFTER revokeAuthToken().
If revokeAuthToken() throws an error, the code jumps to the catch block at line 36, and lockAppdata() is never reached. So this assertion is actually testing the bug mentioned in the other comment about the race condition.
When the race condition is fixed (by moving lockAppdata() before revokeAuthToken()), this test will fail and needs to be updated to:
expect( lockAppdata ).toHaveBeenCalled();
expect( unlockAppdata ).toHaveBeenCalled();Because the lock should be acquired before attempting revocation to ensure atomicity.
| expect( mockLogger.reportSuccess ).toHaveBeenCalledWith( 'Already logged out' ); | ||
| } ); | ||
|
|
||
| it( 'should unlock appdata even if save fails', async () => { |
There was a problem hiding this comment.
Missing Test Coverage: Lock failure scenario
There's no test for when lockAppdata() itself fails. This is an important edge case since file locking can fail due to:
- Another process holding the lock
- File system permissions
- Stale lock files
Suggested additional test:
it( 'should report an error if locking appdata fails', async () => {
( lockAppdata as jest.Mock ).mockRejectedValue( new Error( 'Lock acquisition failed' ) );
const { runCommand } = await import( '../logout' );
await runCommand();
expect( getAuthToken ).toHaveBeenCalled();
expect( lockAppdata ).toHaveBeenCalled();
expect( revokeAuthToken ).not.toHaveBeenCalled(); // Should not proceed
expect( unlockAppdata ).toHaveBeenCalled(); // Should still unlock
expect( mockLogger.reportError ).toHaveBeenCalled();
} );
| it( 'should report an error if revoking the token fails', async () => { | ||
| ( revokeAuthToken as jest.Mock ).mockRejectedValue( new Error( 'Failed to revoke token' ) ); |
There was a problem hiding this comment.
Test Gap: Testing against implementation bug
This test verifies the current buggy behavior where token revocation failure prevents local cleanup. However, there's a better approach:
Scenario 1: Server revocation fails
- Token remains valid on server
- Local token should remain (current behavior is correct)
Scenario 2: Server revocation succeeds, local cleanup fails
- Token is invalid on server
- Local token should be cleaned up in a retry
The current implementation doesn't handle Scenario 2 well. Consider adding a test:
it( 'should handle partial failure gracefully (server revoke success, local cleanup fails)', async () => {
( revokeAuthToken as jest.Mock ).mockResolvedValue( undefined );
( saveAppdata as jest.Mock ).mockRejectedValue( new Error( 'Disk full' ) );
const { runCommand } = await import( '../logout' );
await runCommand();
expect( revokeAuthToken ).toHaveBeenCalled();
expect( mockLogger.reportError ).toHaveBeenCalled();
// The error message should indicate the token is revoked on server
// but local cleanup failed
} );
| export async function revokeAuthToken( token: string ): Promise< void > { | ||
| const wpcom = wpcomFactory( token, wpcomXhrRequest ); | ||
| try { | ||
| await wpcom.req.del( { | ||
| apiNamespace: 'wpcom/v2', | ||
| path: '/studio-app/token', | ||
| method: 'DELETE', | ||
| } ); | ||
| } catch ( error ) { | ||
| throw new LoggerError( __( 'Failed to revoke token' ), error ); | ||
| } | ||
| } |
There was a problem hiding this comment.
API Issue: Error handling masks important information
The current error handling wraps all errors in a generic "Failed to revoke token" message, which makes debugging difficult. Consider these scenarios:
- Network timeout - User needs to retry
- Token already revoked (404) - This is actually success
- Invalid token format (400) - Token is corrupted
- Server error (500) - WordPress.com API issue
Suggested improvement:
export async function revokeAuthToken( token: string ): Promise< void > {
const wpcom = wpcomFactory( token, wpcomXhrRequest );
try {
await wpcom.req.del( {
apiNamespace: 'wpcom/v2',
path: '/studio-app/token',
method: 'DELETE',
} );
} catch ( error ) {
// If token is already revoked, treat as success
if ( error instanceof Error && 'statusCode' in error && error.statusCode === 404 ) {
return;
}
// Preserve the original error for better debugging
if ( error instanceof Error ) {
throw new LoggerError(
__( 'Failed to revoke token on WordPress.com' ),
error
);
}
throw new LoggerError( __( 'Failed to revoke token' ), error );
}
}This provides better error messages and handles the idempotent case (token already revoked).
| await wpcom.req.del( { | ||
| apiNamespace: 'wpcom/v2', | ||
| path: '/studio-app/token', | ||
| method: 'DELETE', |
There was a problem hiding this comment.
Minor: Redundant method parameter
The wpcom.req.del() method already implies DELETE, so the method: 'DELETE' parameter is redundant. Looking at other uses of wpcom.req.del() in this file (line 125), the method parameter is not included.
Suggested fix:
await wpcom.req.del( {
apiNamespace: 'wpcom/v2',
path: '/studio-app/token',
} );This keeps the code consistent with the rest of the file.
| logger.reportStart( LoggerAction.LOGOUT, __( 'Logging out…' ) ); | ||
| let token: Awaited< ReturnType< typeof getAuthToken > >; | ||
|
|
||
| try { | ||
| token = await getAuthToken(); | ||
| } catch ( error ) { | ||
| logger.reportSuccess( __( 'Already logged out' ) ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Code Quality: Inconsistent with login command pattern
The login command (lines 24-30 in login.ts) silently catches errors from getAuthToken() and proceeds with authentication. The logout command does the same but reports "Already logged out" as success.
However, there's an inconsistency in the user experience:
- Login: If already authenticated, shows "Already authenticated with WordPress.com" (line 26)
- Logout: If not authenticated, shows "Already logged out" (line 23)
But getAuthToken() can fail for multiple reasons:
- No token exists (truly logged out) ✅
- Token expired (should we revoke expired tokens?)
- Token invalid/corrupted (edge case)
- Network error during validation (line 156 in appdata.ts calls
validateAccessToken) - Appdata file missing/corrupted
Suggestion: Consider being more specific about the reason:
try {
token = await getAuthToken();
} catch ( error ) {
// Check if it's specifically "no token" vs other errors
if ( error instanceof LoggerError && error.message.includes( 'Authentication required' ) ) {
logger.reportSuccess( __( 'Already logged out' ) );
return;
}
// For other errors (corrupted file, network issues), report them
logger.reportError( error instanceof LoggerError ? error : new LoggerError( __( 'Failed to check authentication status' ), error ) );
return;
}
| beforeEach( () => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| mockLogger = { | ||
| reportStart: jest.fn(), | ||
| reportSuccess: jest.fn(), | ||
| reportError: jest.fn(), | ||
| }; | ||
|
|
||
| ( Logger as jest.Mock ).mockReturnValue( mockLogger ); | ||
| ( getAuthToken as jest.Mock ).mockResolvedValue( mockAppdata.authToken ); | ||
| ( revokeAuthToken as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( lockAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( unlockAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( readAppdata as jest.Mock ).mockResolvedValue( JSON.parse( JSON.stringify( mockAppdata ) ) ); | ||
| ( saveAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| } ); |
There was a problem hiding this comment.
Test Quality: Mock setup could be more maintainable
The mock setup is good, but using JSON.parse( JSON.stringify( mockAppdata ) ) on line 47 to deep clone is a bit obscure. Consider using a factory function for clarity:
const createMockAppdata = () => ( {
authToken: {
accessToken: 'existing-token',
id: 999,
email: '[email protected]',
displayName: 'Existing User',
expiresIn: 1209600,
expirationTime: Date.now() + 1209600000,
},
} );
beforeEach( () => {
// ...
( readAppdata as jest.Mock ).mockResolvedValue( createMockAppdata() );
} );This makes the intent clearer and is easier to maintain. Minor nitpick though - current code works fine.
| expect( revokeAuthToken ).toHaveBeenCalled(); | ||
| expect( lockAppdata ).toHaveBeenCalled(); | ||
| expect( readAppdata ).toHaveBeenCalled(); | ||
| expect( saveAppdata ).toHaveBeenCalledWith( {} ); |
There was a problem hiding this comment.
Test Issue: Fragile assertion
The test asserts saveAppdata is called with an empty object {}, but this is fragile because:
- The
readAppdata()mock returns an object withauthTokenproperty - The code deletes
authTokenfrom the object - The resulting object is passed to
saveAppdata()
However, the mock on line 47 returns a new object each time (due to deep clone), so the assertion works. But if readAppdata were to return additional properties in the future (like version, sites, etc.), this assertion would fail incorrectly.
Better assertion:
expect( saveAppdata ).toHaveBeenCalledWith(
expect.not.objectContaining( { authToken: expect.anything() } )
);Or even better, verify the token was removed:
const savedData = ( saveAppdata as jest.Mock ).mock.calls[0][0];
expect( savedData ).not.toHaveProperty( 'authToken' );This tests the actual requirement: "authToken should be deleted from appdata".
|
|
||
| export enum AuthCommandLoggerAction { | ||
| LOGIN = 'login', | ||
| LOGOUT = 'logout', |
There was a problem hiding this comment.
Documentation: Consider adding JSDoc
Since this enum is shared between CLI and main app (as noted in the comment), it would be helpful to document what each action represents:
/**
* Logger actions for authentication commands.
* These actions are used for telemetry and progress reporting.
*/
export enum AuthCommandLoggerAction {
/** User login to WordPress.com */
LOGIN = 'login',
/** User logout from WordPress.com */
LOGOUT = 'logout',
}This is especially helpful given the note about avoiding Webpack issues - future developers will understand why this file exists separately.
| @@ -0,0 +1,55 @@ | |||
| import { __ } from '@wordpress/i18n'; | |||
There was a problem hiding this comment.
Documentation Missing: CLAUDE.md needs update
The CLAUDE.md file documents the auth login command (lines 52-82) but doesn't include documentation for the new auth logout command.
Suggested addition after line 82 in CLAUDE.md:
#### `studio auth logout`
Log out from WordPress.com and revoke the access token.
**Usage:**
```bash
node dist/cli/main.js auth logoutDescription:
This command logs you out from WordPress.com by:
- Revoking the access token on the WordPress.com server
- Removing the token from your local app data
- Syncing the logout state with the Studio desktop app
Options:
- None required
Example:
npm run cli:build
node dist/cli/main.js auth logout
# Output: ✓ Successfully logged outNotes:
- If already logged out, the command will notify you without error
- Logout is shared between the CLI and the Studio desktop app
- The token is revoked on WordPress.com, invalidating all sessions using that token
This maintains consistency with the existing documentation style and helps users understand the command.
|
Claude finished @fredrikekelund's task —— View job Documentation Update CompleteI've successfully reviewed the CLI changes and updated the documentation for the new Tasks
What Was DoneCommand Implementation ReviewThe new
Documentation UpdatesAdded comprehensive documentation for the
The documentation follows the same format as the existing Changes CommittedCreated commit Note on PushThe documentation changes have been committed locally. To push them to the remote branch, you'll need to run: git pull --rebase origin f26d/cli-auth-logout-command
git push origin f26d/cli-auth-logout-commandThis is needed because there are remote changes that aren't in the local copy yet (the push was rejected with "fetch first" message). |
|
Claude finished @fredrikekelund's task —— View job Documentation UpdatedI've reviewed the new Tasks Completed
Changes MadeAdded documentation for the
Notes
Commit: ffd836f - "docs: Update CLI documentation" |
Add documentation for the new `studio auth logout` command, following the same format as the existing `auth login` command documentation. Co-authored-by: Fredrik Rombach Ekelund <[email protected]>
|
Claude finished @fredrikekelund's task —— View job Documentation Review CompleteI've reviewed the CLI changes and documentation for the new Tasks
SummaryThe documentation for the What I FoundCommand Implementation (
Documentation Status (
File Structure:
ConclusionNo documentation updates are required. The CLI documentation comprehensively covers the new |
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
LGTM 👍
Tested and worked as described on macOS and Windows.
Added some non-blocking considerations
cli/commands/auth/logout.ts
Outdated
| } | ||
|
|
||
| try { | ||
| await revokeAuthToken( token.accessToken ); |
There was a problem hiding this comment.
In Studio, failing to revoke the auth token doesn't fail the logout operation.
Have you considered keeping the same behavior in the CLI?
There was a problem hiding this comment.
If we consider the revoke of the token as non-critical we should do the api call after removing the token from appdata, preventing the unlikely scenario of having a revoked token store in appdata
| try { | ||
| await revokeAuthToken( token.accessToken ); | ||
|
|
||
| await lockAppdata(); |
There was a problem hiding this comment.
It's unlikely, but I think it's a good approach.
|
Claude finished @fredrikekelund's task —— View job CLI Implementation Review CompleteI've reviewed the new Tasks
SummaryThe documentation is already complete and accurate. The Documentation Status ✅The documentation includes:
The documentation is comprehensive and follows the established format in the repository. Implementation ReviewWhat's Good ✅
Implementation DetailsFiles Changed:
Command Flow:
Observations from Review CommentsI noticed reviewer @bcotrim raised an interesting consideration about the revocation behavior:
The current CLI implementation treats server revocation failure as a hard error (line 28-32 in
However, if you prefer to match the desktop app's behavior (local-first logout), that would be a valid design choice too. Code Quality NotesThe implementation has one minor observation:
This is cosmetic and doesn't affect functionality. Conclusion✅ Documentation is complete - No updates needed The PR successfully implements the |
|
Claude finished @fredrikekelund's task —— View job Documentation Status: Up to Date ✅I've reviewed the recent changes to this PR and verified the documentation state. Tasks
AnalysisRecent Commits:
Documentation Status:
Implementation Review:
ConclusionNo documentation updates are needed. The recent changes are internal implementation improvements that don't affect the documented behavior or usage of the The PR has comprehensive documentation, proper test coverage, and has received multiple approvals from reviewers. The implementation correctly handles:
|
Related issues
Proposed Changes
This PR is based on @youknowriad's work in #1958. It implements a
studio auth logoutcommand that:Testing Instructions
npm run cli:buildnode dist/cli/main.js auth logoutnode dist/cli/main.js auth logoutPre-merge Checklist