feat: update logout process and remove auth2, whoami flow #13
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Filespackage-lock.json
|
Deploying dbot-reference with
|
| Latest commit: |
50370ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5e58960e.dbot-reference.pages.dev |
| Branch Preview URL: | https://ahmed-feat-authentication-au.dbot-reference.pages.dev |
|
Claude finished @ahmed-deriv's task in 2m 1s —— View job Code Review Complete: Authentication & Logout Refactoring ✅Review Checklist:
SummaryExcellent work addressing the critical issues from the previous review! The PR now includes:
The refactoring successfully simplifies the authentication flow by removing 763 lines of unnecessary code while maintaining security and improving maintainability. 🟢 What Was Fixed (Previous Critical Issues)✅ Issue #2 RESOLVED: Infinite Reload LoopStatus: FIXED ✅ The infinite reload loop has been completely addressed: // src/hooks/useInvalidTokenHandler.ts:15-38
const handleInvalidToken = async () => {
try {
// Clear invalid session data to prevent infinite reload loop
sessionStorage.removeItem('auth_info');
localStorage.removeItem('active_loginid');
localStorage.removeItem('authToken');
localStorage.removeItem('accountsList');
localStorage.removeItem('clientAccounts');
sessionStorage.clear();
// Redirect to OAuth login instead of reload
const { generateOAuthURL } = await import('@/components/shared');
const oauthUrl = await generateOAuthURL();
if (oauthUrl) {
window.location.replace(oauthUrl);
} else {
ErrorLogger.error('InvalidToken', 'Failed to generate OAuth URL, falling back to reload');
window.location.reload();
}
} catch (error) {
ErrorLogger.error('InvalidToken', 'Error handling invalid token', error);
window.location.reload();
}
};Why this works:
✅ Issue #3 RESOLVED: Error Handling in useLogoutStatus: FIXED ✅ The error handling now uses targeted storage clearing that preserves user preferences: // src/hooks/useLogout.ts:21-42
catch (error) {
ErrorLogger.error('Logout', 'Logout failed', error);
// Clear only auth-related storage keys (preserves theme, language, etc.)
try {
sessionStorage.removeItem('auth_info');
localStorage.removeItem('active_loginid');
localStorage.removeItem('authToken');
localStorage.removeItem('accountsList');
localStorage.removeItem('clientAccounts');
localStorage.removeItem('account_type');
} catch (storageError) {
ErrorLogger.error('Logout', 'Failed to clear auth storage', storageError);
// Last resort: clear all storage
try {
sessionStorage.clear();
localStorage.clear();
} catch (finalError) {
ErrorLogger.error('Logout', 'Failed to clear all storage', finalError);
}
}
}Improvements:
✅ Issue #4 RESOLVED: Test CoverageStatus: FIXED ✅ Comprehensive test suites have been added: Test Files Added:
Coverage highlights:
✅ New Addition: Centralized Error LoggingStatus: EXCELLENT ✅ A new // src/utils/error-logger.ts
ErrorLogger.error('OAuth', 'Token exchange failed', error);
ErrorLogger.warn('Storage', 'Failed to clear cache', { key: 'auth_info' });
ErrorLogger.info('Auth', 'User logged in', { loginid: 'CR123' });Features:
This replaces 140+ inconsistent console.error/warn calls throughout the codebase. 🟡 Remaining Issues (Minor/Medium Priority)1. Analytics Package RemovedFile: // Analytics.reset() removed - Analytics package has been removed from the project
// See migrate-docs/MONITORING_PACKAGES.md for re-enabling analytics if neededObservation: The PR removes the
Status:
Recommendation: Confirm with team whether analytics removal is permanent or temporary. 2. Documentation References Need UpdateFile: Previous review mentioned Status: ✅ ADDRESSED - A new file was created:
Additional documentation added:
Excellent documentation work! 👏 3. Unused Import in CoreStoreProviderFile: const handleMessages = useCallback(
async (res: Record<string, unknown>) => {
// ... code ...
},
[client, handleLogout] // ← handleLogout is in dependencies but not used
);Issue: The Impact: Low - causes unnecessary re-renders when Fix: Remove [client] // Remove handleLogout4. TrackJS Package Still ListedFiles: The PR removes "trackjs": "^3.10.4"Status: The new Recommendation: Decide whether to:
🔍 Code Quality AssessmentSecurity ✅
Performance ✅
Maintainability ✅
TypeScript ✅
📊 Changes Summary
Key Wins:
🎯 RecommendationsBefore Merge (Minor Issues Only):
After Merge (Optional Improvements):
✅ Final VerdictStatus: ✅ READY TO MERGE (with minor fix recommended) This PR successfully addresses all critical issues from the previous review:
Outstanding Issues:
Recommendation: Merge after fixing the unused dependency. The analytics question can be addressed in a follow-up PR if needed. Quick Fix for Unused Dependency// src/app/CoreStoreProvider.tsx:139
const handleMessages = useCallback(
async (res: Record<string, unknown>) => {
// ... existing code ...
},
- [client, handleLogout]
+ [client]
);Great work on this refactoring! The codebase is now simpler, better tested, and more maintainable. 🎉 |
|
Claude finished @ahmed-deriv's task in 2m 57s —— View job PR Review: Authentication & Logout RefactoringSummaryThis PR refactors the authentication flow by removing legacy code and simplifying the logout process. The changes are well-implemented with strong test coverage and good documentation. However, there are 2 issues that should be addressed before merge. 🔴 High Priority Issues1. Unused Dependency in CoreStoreProvider (MUST FIX)File: const handleMessages = useCallback(
async (res: Record<string, unknown>) => {
// ... handleLogout is declared but never used in this function ...
// Function calls client?.logout() directly on line 125 instead
},
[client, handleLogout] // ← handleLogout causes unnecessary re-renders
);Issue: Impact: Performance degradation - component re-renders more than needed Fix: [client] // Remove handleLogout from dependenciesThis was identified in the previous review but not addressed yet. 2. Over-Dependencies in Header Component (SHOULD FIX)File: const renderAccountSection = useCallback(
// ... function body ...
[
isAuthenticating, // ← Not in function scope
isAuthorizing,
isDesktop,
activeLoginid,
isAuthorized, // ← Not used in function
standalone_routes, // ← Not used in function
client,
currency, // ← Not used (uses authData.currency)
localize, // ← Not used in function
activeAccount,
is_virtual, // ← Not used in function
handleLogout, // ← Not used in function
authTimeout,
is_account_regenerating,
authData,
handleLogin,
]
);Issue: Multiple unused dependencies causing unnecessary re-renders Impact: Performance - this callback recreates more often than needed Should remove: Correct dependencies: [
isAuthorizing,
isDesktop,
activeLoginid,
client,
activeAccount,
authTimeout,
is_account_regenerating,
authData,
handleLogin,
]🟡 Medium Priority Issues3. Package.json Still Lists Removed DependenciesFile: {
"dependencies": {
"@deriv-com/analytics": "^1.35.1", // ← Still listed but removed from usage
// trackjs not in package.json (good - was removed)
// @datadog/browser-rum not in package.json (good - was removed)
}
}Issue: The PR description says Clarification Needed:
Recommendation: Document the decision in the PR description or remove if not needed. 4. Hard-coded Client ID FallbackFile: const clientId = process.env.CLIENT_ID || '32izC2lBT4MmiSNWuxq2l';Issue: Hard-coded fallback client ID in source code Security Concern: Low - this appears to be a staging/dev client ID, but hard-coding secrets in source is not best practice Recommendation:
const clientId = process.env.CLIENT_ID;
if (!clientId) {
throw new Error('CLIENT_ID environment variable is required');
}✅ What's GoodExcellent Test Coverage
Strong Security Practices✅ PKCE flow properly implemented Code Quality✅ Centralized error logging (ErrorLogger utility) Architecture Improvements✅ Simplified authentication flow 🔍 Detailed Security Review✅ No XSS Vulnerabilities
✅ Auth Flow Security
|
1 participant
This pull request primarily removes the legacy "endpoint" page and related features, simplifies authentication and error handling logic, and cleans up unused dependencies and code. The changes improve maintainability and streamline the user experience by removing obsolete code and dependencies.
Removal of Endpoint Page and Related UI:
Deleted the
Endpointcomponent and removed its usage from the footer and routing, eliminating the/endpointroute and all related code references.Refactored the
ServerTimecomponent to always display server time, removing logic that handled GMT time for the endpoint page.Authentication and Error Handling Improvements:
useOauth2hook with the newuseLogouthook inCoreStoreProviderand header components, and updated error handling to avoid redundant logout operations.Dependency Cleanup:
Removed unused dependencies from
package.json, including@datadog/browser-rum,@deriv-com/analytics, andtrackjs.Deleted initialization logic for TrackJS and Datadog from
app-content.jsx, cleaning up related imports and effects.Configuration Update:
@/servicespath to support improved test organization.