-
Notifications
You must be signed in to change notification settings - Fork 51
CLI: Implement auth logout command
#2027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 26 commits
999b2a8
8650b02
fc661e0
c8b228c
9fcc45f
36e6059
a228ae9
b838c0c
c1799fe
23aa26c
ed4f216
49966e3
e13fd34
c72a70d
edb407f
6ba6a05
2908170
fc816bf
5748109
7b1c743
d8feb6b
20c7566
58c6aa7
3247311
ffd836f
d660ff0
c11f125
b6d4c68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { AuthCommandLoggerAction as LoggerAction } from 'common/logger-actions'; | ||
| import { revokeAuthToken } from 'cli/lib/api'; | ||
| import { | ||
| readAppdata, | ||
| saveAppdata, | ||
| lockAppdata, | ||
| unlockAppdata, | ||
| getAuthToken, | ||
| } from 'cli/lib/appdata'; | ||
| import { Logger, LoggerError } from 'cli/logger'; | ||
| import { StudioArgv } from 'cli/types'; | ||
|
|
||
| export async function runCommand(): Promise< void > { | ||
| const logger = new Logger< LoggerAction >(); | ||
|
|
||
| logger.reportStart( LoggerAction.LOGOUT, __( 'Logging out…' ) ); | ||
| let token: Awaited< ReturnType< typeof getAuthToken > >; | ||
|
|
||
| try { | ||
| token = await getAuthToken(); | ||
| } catch ( error ) { | ||
| logger.reportSuccess( __( 'Already logged out' ) ); | ||
| return; | ||
| } | ||
|
Comment on lines
+17
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Quality: Inconsistent with login command pattern The login command (lines 24-30 in However, there's an inconsistency in the user experience:
But
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;
} |
||
|
|
||
| try { | ||
| await revokeAuthToken( token.accessToken ); | ||
|
||
|
|
||
| await lockAppdata(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race Condition: Lock acquired AFTER API call The
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unlikely, but I think it's a good approach.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the case that Claude describes is very unlikely, but I still realize that |
||
| const userData = await readAppdata(); | ||
| delete userData.authToken; | ||
| await saveAppdata( userData ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Token remains valid after local deletion The current implementation has a critical security flaw: if the
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. |
||
|
|
||
| logger.reportSuccess( __( 'Successfully logged out' ) ); | ||
| } catch ( error ) { | ||
| if ( error instanceof LoggerError ) { | ||
| logger.reportError( error ); | ||
| } else { | ||
| logger.reportError( new LoggerError( __( 'Failed to log out' ), error ) ); | ||
| } | ||
| } finally { | ||
| await unlockAppdata(); | ||
| } | ||
| } | ||
|
|
||
| export const registerCommand = ( yargs: StudioArgv ) => { | ||
| return yargs.command( { | ||
| command: 'logout', | ||
| describe: __( 'Log out and clear WordPress.com authentication' ), | ||
| handler: async () => { | ||
| await runCommand(); | ||
| }, | ||
| } ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { revokeAuthToken } from 'cli/lib/api'; | ||
| import { | ||
| getAuthToken, | ||
| lockAppdata, | ||
| readAppdata, | ||
| saveAppdata, | ||
| unlockAppdata, | ||
| } from 'cli/lib/appdata'; | ||
| import { Logger, LoggerError } from 'cli/logger'; | ||
|
|
||
| jest.mock( 'cli/lib/appdata' ); | ||
| jest.mock( 'cli/logger' ); | ||
| jest.mock( 'cli/lib/api' ); | ||
|
|
||
| describe( 'Auth Logout Command', () => { | ||
| function getMockAppdata() { | ||
| return { | ||
| authToken: { | ||
| accessToken: 'existing-token', | ||
| id: 999, | ||
| email: '[email protected]', | ||
| displayName: 'Existing User', | ||
| expiresIn: 1209600, | ||
| expirationTime: Date.now() + 1209600000, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| let mockLogger: { | ||
| reportStart: jest.Mock; | ||
| reportSuccess: jest.Mock; | ||
| reportError: jest.Mock; | ||
| }; | ||
|
|
||
| beforeEach( () => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| mockLogger = { | ||
| reportStart: jest.fn(), | ||
| reportSuccess: jest.fn(), | ||
| reportError: jest.fn(), | ||
| }; | ||
|
|
||
| ( Logger as jest.Mock ).mockReturnValue( mockLogger ); | ||
| ( getAuthToken as jest.Mock ).mockResolvedValue( getMockAppdata().authToken ); | ||
| ( revokeAuthToken as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( lockAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( unlockAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| ( readAppdata as jest.Mock ).mockResolvedValue( getMockAppdata() ); | ||
| ( saveAppdata as jest.Mock ).mockResolvedValue( undefined ); | ||
| } ); | ||
|
|
||
| afterEach( () => { | ||
| jest.restoreAllMocks(); | ||
| } ); | ||
|
|
||
| it( 'should complete the logout process successfully', async () => { | ||
| const { runCommand } = await import( '../logout' ); | ||
| await runCommand(); | ||
|
|
||
| expect( getAuthToken ).toHaveBeenCalled(); | ||
| expect( revokeAuthToken ).toHaveBeenCalled(); | ||
| expect( lockAppdata ).toHaveBeenCalled(); | ||
| expect( readAppdata ).toHaveBeenCalled(); | ||
| expect( saveAppdata ).toHaveBeenCalledWith( | ||
| expect.not.objectContaining( { authToken: expect.anything() } ) | ||
| ); | ||
| expect( unlockAppdata ).toHaveBeenCalled(); | ||
| expect( mockLogger.reportSuccess ).toHaveBeenCalledWith( 'Successfully logged out' ); | ||
| } ); | ||
|
|
||
| it( 'should report an error if revoking the token fails', async () => { | ||
| ( revokeAuthToken as jest.Mock ).mockRejectedValue( new Error( 'Failed to revoke token' ) ); | ||
|
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Scenario 2: Server revocation succeeds, local cleanup fails
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
} ); |
||
|
|
||
| const { runCommand } = await import( '../logout' ); | ||
| await runCommand(); | ||
|
|
||
| expect( getAuthToken ).toHaveBeenCalled(); | ||
| expect( lockAppdata ).not.toHaveBeenCalled(); | ||
|
||
| expect( readAppdata ).not.toHaveBeenCalled(); | ||
| expect( saveAppdata ).not.toHaveBeenCalledWith( {} ); | ||
| expect( unlockAppdata ).toHaveBeenCalled(); | ||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockLogger.reportError ).toHaveBeenCalledWith( expect.any( LoggerError ) ); | ||
| } ); | ||
|
|
||
| it( 'should report already logged out if no auth token exists', async () => { | ||
| ( getAuthToken as jest.Mock ).mockRejectedValue( new Error( 'No auth token' ) ); | ||
|
|
||
| const { runCommand } = await import( '../logout' ); | ||
| await runCommand(); | ||
|
|
||
| expect( getAuthToken ).toHaveBeenCalled(); | ||
| expect( revokeAuthToken ).not.toHaveBeenCalled(); | ||
| expect( lockAppdata ).not.toHaveBeenCalled(); | ||
| expect( readAppdata ).not.toHaveBeenCalled(); | ||
| expect( saveAppdata ).not.toHaveBeenCalled(); | ||
| expect( mockLogger.reportSuccess ).toHaveBeenCalledWith( 'Already logged out' ); | ||
| } ); | ||
|
|
||
| it( 'should unlock appdata even if save fails', async () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Test Coverage: Lock failure scenario There's no test for when
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();
} ); |
||
| ( saveAppdata as jest.Mock ).mockRejectedValue( new Error( 'Failed to save' ) ); | ||
|
|
||
| const { runCommand } = await import( '../logout' ); | ||
| await runCommand(); | ||
|
|
||
| expect( revokeAuthToken ).toHaveBeenCalled(); | ||
| expect( lockAppdata ).toHaveBeenCalled(); | ||
| expect( unlockAppdata ).toHaveBeenCalled(); | ||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockLogger.reportError ).toHaveBeenCalledWith( expect.any( LoggerError ) ); | ||
| } ); | ||
| } ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,3 +164,16 @@ export async function getUserInfo( | |
| throw new LoggerError( __( 'Failed to fetch user info' ), error ); | ||
| } | ||
| } | ||
|
|
||
| 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', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Redundant The Suggested fix: await wpcom.req.del( {
apiNamespace: 'wpcom/v2',
path: '/studio-app/token',
} );This keeps the code consistent with the rest of the file. |
||
| } ); | ||
| } catch ( error ) { | ||
| throw new LoggerError( __( 'Failed to revoke token' ), error ); | ||
| } | ||
| } | ||
|
Comment on lines
+168
to
+179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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). |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| export enum AuthCommandLoggerAction { | ||
| LOGIN = 'login', | ||
| LOGOUT = 'logout', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| export enum PreviewCommandLoggerAction { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation Missing: CLAUDE.md needs update
The
CLAUDE.mdfile documents theauth logincommand (lines 52-82) but doesn't include documentation for the newauth logoutcommand.Suggested addition after line 82 in CLAUDE.md:
Description:
This command logs you out from WordPress.com by:
Options:
Example:
Notes: