feat: user authentication flow #9
Conversation
- Move auth URLs to environment variables - Remove hardcoded URLs from app.config.js - Add auth configuration documentation in README - Update .env.example with generic examples - Improve security by not exposing URLs in git-tracked files
Reviewer's Guide by SourceryThis pull request introduces a comprehensive user authentication flow, integrating Telegram WebApp and external OAuth for user login and session management. It also includes logic for handling trading accounts and default account settings. Sequence diagram for the new authentication flowsequenceDiagram
actor User
participant UI as Login Page
participant Auth as Auth Service
participant Storage as Local Storage
participant External as OAuth/Telegram
User->>UI: Access application
UI->>Auth: initialize()
Auth->>Storage: Check stored session
alt Has valid session
Storage-->>Auth: Return session data
Auth-->>UI: Return authenticated
UI->>UI: Redirect to Dashboard
else No valid session
alt Has Telegram user
External-->>Auth: Get Telegram user data
Auth->>Storage: Store session
Auth-->>UI: Return authenticated
else OAuth login
User->>UI: Click 'Login with existing account'
UI->>External: Redirect to OAuth URL
External-->>UI: Return with OAuth data
UI->>Auth: handleOAuthCallback()
Auth->>Storage: Store session & accounts
Auth-->>UI: Return authenticated
end
end
Class diagram for the updated AuthServiceclassDiagram
class AuthService {
-storageKey: string
-tradingAccountsKey: string
-defaultAccountKey: string
+initialize(): Promise<boolean>
+setSession(session: Object): Promise<boolean>
+getStoredSession(): Promise<Object>
+validateSession(session: Object): Promise<boolean>
+setTradingAccounts(accounts: Array): Promise<boolean>
+getTradingAccounts(): Promise<Array>
+setDefaultAccount(account: Object): Promise<boolean>
+getDefaultAccount(): Promise<Object>
+clearSession(): Promise<boolean>
+isAuthenticated(): boolean
}
class useAuth {
-user: Object
-defaultAccount: Object
-isAuthenticated: boolean
-isLoading: boolean
+initialize(): Promise<boolean>
+login(telegramUser, oauthData): Promise<boolean>
+handleOAuthCallback(searchParams): Promise<boolean>
+logout(): Promise<boolean>
}
useAuth ..> AuthService : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@aliakbar-deriv is attempting to deploy a commit to the Deriv Team on Vercel. A member of the Team first needs to authorize it. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aliakbar-deriv - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Sensitive data like trading accounts and tokens should not be stored as plaintext in localStorage (link)
Overall Comments:
- Consider implementing encryption for sensitive data in localStorage before deploying to production. Storing auth tokens and account details in plaintext is a security risk.
- The trading account object stored in localStorage contains more information than necessary. Consider storing only essential identifiers and fetching full account details from the API when needed.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 1 blocking issue, 1 other issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/services/auth.service.js
Outdated
| */ | ||
| async setTradingAccounts(accounts) { | ||
| try { | ||
| // In production, encrypt sensitive data before storing |
There was a problem hiding this comment.
🚨 issue (security): Sensitive data like trading accounts and tokens should not be stored as plaintext in localStorage
Consider using a secure encryption method or a more secure storage mechanism for sensitive data. Web Storage is vulnerable to XSS attacks and should not store sensitive information unencrypted.
| // Close dropdown immediately to prevent UI glitches | ||
| setIsDropdownOpen(false); | ||
|
|
||
| // Small delay to allow dropdown animation to complete |
There was a problem hiding this comment.
suggestion (performance): Unnecessary setTimeout could cause UI lag
Consider using CSS transitions instead of setTimeout for animations. This would be more performant and avoid artificial delays in the UI.
Suggested implementation:
setIsDropdownOpen(false);You'll need to add CSS styles to handle the animation. Add this to your CSS/SCSS file (likely AppBar.scss or similar):
.dropdown {
opacity: 1;
transform: translateY(0);
transition: opacity 0.1s ease-out, transform 0.1s ease-out;
}
.dropdown.closed {
opacity: 0;
transform: translateY(-10px);
pointer-events: none;
}Then update your dropdown JSX element to include these classes:
<div
ref={dropdownRef}
className={`dropdown ${isDropdownOpen ? '' : 'closed'}`}
>
{/* dropdown content */}
</div>This will create a smooth fade-out and slide-up animation when closing the dropdown, without needing any artificial delays.
src/hooks/useAuth.js
Outdated
| const account = searchParams.get(`acct${index}`); | ||
| const token = searchParams.get(`token${index}`); | ||
| const currency = searchParams.get(`cur${index}`); | ||
|
|
||
| if (!account || !token || !currency) break; | ||
|
|
||
| tradingAccounts.push({ account, token, currency }); |
There was a problem hiding this comment.
🚨 suggestion (security): OAuth callback should validate data format before using values
Add validation for the expected format of account, token, and currency values to prevent potential issues with malformed data.
| const account = searchParams.get(`acct${index}`); | |
| const token = searchParams.get(`token${index}`); | |
| const currency = searchParams.get(`cur${index}`); | |
| if (!account || !token || !currency) break; | |
| tradingAccounts.push({ account, token, currency }); | |
| const account = searchParams.get(`acct${index}`); | |
| const token = searchParams.get(`token${index}`); | |
| const currency = searchParams.get(`cur${index}`); | |
| if (!account || !token || !currency) break; | |
| // Validate account format (alphanumeric, max 50 chars) | |
| if (!/^[a-zA-Z0-9]{1,50}$/.test(account)) { | |
| console.warn(`Invalid account format at index ${index}`); | |
| index++; | |
| continue; | |
| } | |
| // Validate token format (JWT format: three base64 sections separated by dots) | |
| if (!/^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*$/.test(token)) { | |
| console.warn(`Invalid token format at index ${index}`); | |
| index++; | |
| continue; | |
| } | |
| // Validate currency format (3 uppercase letters, ISO 4217) | |
| if (!/^[A-Z]{3}$/.test(currency)) { | |
| console.warn(`Invalid currency format at index ${index}`); | |
| index++; | |
| continue; | |
| } | |
| tradingAccounts.push({ account, token, currency }); |
| // Process OAuth callback if present | ||
| if (location.search) { | ||
| const searchParams = new URLSearchParams(location.search); | ||
| handleOAuthCallback(searchParams).then(success => { |
There was a problem hiding this comment.
suggestion (bug_risk): Add cleanup to prevent navigation after component unmount
Use a cleanup function with useEffect to prevent navigation attempts if the component unmounts during the OAuth callback processing.
Suggested implementation:
// Process OAuth callback if present
useEffect(() => {
let mounted = true;
if (location.search) {
const searchParams = new URLSearchParams(location.search);
handleOAuthCallback(searchParams).then(success => {
if (success && mounted) {
navigate(ROUTES.DASHBOARD, { replace: true });
}
});
}
return () => {
mounted = false;
};
}, [location.search, navigate]);You'll need to ensure that useEffect is imported from 'react' at the top of the file:
import { useEffect } from 'react';
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @aliakbar-deriv - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid hardcoding the default account key. (link)
- Avoid hardcoding the token. (link)
- Avoid hardcoding the token. (link)
Overall Comments:
- Consider consolidating the auth state management - currently there are multiple interrelated state variables (isAuthenticated, isLoading, isSwitchingAccount) that could be simplified into a single state enum to make the logic more maintainable.
- Error handling should be more consistent throughout the codebase. Consider creating a centralized error handling utility that standardizes how errors are caught, logged and propagated across the WebSocket and auth services.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🔴 Security: 3 blocking issues
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| this.connect(); | ||
| }, this.reconnectTimeout); | ||
| this.reconnectAttempts++; | ||
| const delay = this.reconnectTimeout * Math.pow(2, this.reconnectAttempts - 1); |
There was a problem hiding this comment.
suggestion (performance): Consider adding a maximum delay cap for exponential backoff
The exponential backoff could lead to very long delays. Consider adding something like: const maxDelay = 30000; const delay = Math.min(this.reconnectTimeout * Math.pow(2, this.reconnectAttempts - 1), maxDelay);
Suggested implementation:
this.reconnectAttempts++;
const maxDelay = 30000; // 30 seconds maximum delay
const delay = Math.min(this.reconnectTimeout * Math.pow(2, this.reconnectAttempts - 1), maxDelay);
this.emit('reconnecting', {You might want to consider:
- Making maxDelay configurable by adding it as a class property that can be set in the constructor
- Adding the maxDelay value to the reconnecting event payload for logging/debugging purposes
| */ | ||
| async setSession(userData) { | ||
| async validateSession(session) { |
There was a problem hiding this comment.
suggestion: Consider refactoring complex validation logic into smaller functions
The validation logic is quite nested. Consider breaking it into separate functions like validateSessionStructure(), validateTradingAccounts(), etc.
Suggested implementation:
/**
* Validates basic session structure
* @private
*/
_validateSessionStructure(session) {
return session && typeof session === 'object';
}
/**
* Validates session data
* @private
*/
async validateSession(session) {
try {
// Clear any corrupted data
if (!session) {
await this.clearSession();
return false;
}
// Validate basic session structure
if (!this._validateSessionStructure(session)) {
await this.clearSession();
return false;
}
// Continue with remaining validation
const isValidSession = (Since we can only see part of the code, you'll need to:
- Continue breaking down the remaining validation logic inside the
isValidSessioncheck into separate private methods based on what's being validated (e.g.,_validateTradingAccounts(),_validateUserProfile(), etc.) - Each validation method should return a boolean and handle one specific aspect of validation
- The main
validateSessionmethod should orchestrate these individual validations
The naming and exact structure of additional validation methods will depend on what's being validated in the rest of the isValidSession check that we can't see.
| if (mounted) { | ||
| setIsAuthenticated(isAuth); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
suggestion (bug_risk): Consider enhancing error handling to preserve error context
Instead of just returning false, consider throwing a custom error with the original error as cause. This would help with debugging and error reporting while maintaining the boolean return type for the public API.
| console.error('Failed to switch account:', error); | ||
| return false; | ||
| } finally { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
suggestion: Avoid hardcoded delay in switchAccount
The 300ms delay seems arbitrary. Consider making this configurable or removing it if not strictly necessary. If it's needed for UI purposes, document the reason.
Suggested implementation:
setTimeout(() => {
setIsSwitchingAccount(false);
}, ACCOUNT_SWITCH_TRANSITION_DELAY);At the top of src/hooks/useAuth.js, add this constant:
// Delay (in ms) to ensure UI state updates complete and animations finish smoothly
// when switching between accounts. This prevents visual glitches during the transition.
const ACCOUNT_SWITCH_TRANSITION_DELAY = 300;If this delay is used in other components or might need to be adjusted based on the environment, consider moving it to a shared constants file (e.g., src/constants/ui.js) where it can be imported where needed.
| let attempts = 0; | ||
| const maxAttempts = 40; // 2 seconds total | ||
|
|
||
| const checkState = () => { |
There was a problem hiding this comment.
issue (bug_risk): Potential race condition in OAuth callback state updates
The state check loop could miss updates or continue running after component unmount. Consider using a more robust approach like React's useEffect or a proper state management solution.
| error.details = errorResponse.details; | ||
|
|
||
| // Map error codes to specific error types | ||
| switch (error.code) { |
There was a problem hiding this comment.
suggestion: Expand error code handling in BaseApi
Consider adding more error codes to handle common API scenarios like network errors, server errors, and timeout conditions. This would improve error reporting and handling throughout the application.
Suggested implementation:
switch (error.code) {
// Authentication & Authorization
case 'AuthorizationRequired':
error.name = 'AuthorizationError';
break;
case 'InvalidAppID':
error.name = 'ConfigurationError';
break;
case 'InputValidationFailed':
error.name = 'ValidationError';
break;
case 'PermissionDenied':
error.name = 'PermissionError';
break;
// Network Related
case 'NetworkError':
error.name = 'NetworkError';
break;
case 'ConnectionLost':
error.name = 'NetworkError';
break;
case 'RequestTimeout':
error.name = 'TimeoutError';
break;
// Server Errors
case 'InternalServerError':
error.name = 'ServerError';
break;
case 'ServiceUnavailable':
error.name = 'ServerError';
break;
case 'MaintenanceError':
error.name = 'ServerError';
break;
// Rate Limiting
case 'RateLimit':
error.name = 'RateLimitError';
break;
case 'ConcurrentRequestLimit':
error.name = 'RateLimitError';
break;
// Data Related
case 'ResourceNotFound':
error.name = 'NotFoundError';
break;
case 'DataError':
error.name = 'DataProcessingError';
break;The developer should also:
- Update any error handling documentation to reflect these new error types
- Ensure error handling middleware/catch blocks can handle these new error types appropriately
- Consider adding specific error handling logic for each new error type where they are caught
| import { useTelegram, useAuth } from '@/hooks'; | ||
| import styles from './LoginPage.module.css'; | ||
|
|
||
| const LoginPage = () => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the OAuth callback handling into a custom hook and simplifying the LoginPage useEffect to make auth state transitions more explicit.
The login flow can be simplified by extracting the OAuth callback handling and using a more declarative state approach. Here's how:
- Create a custom hook for OAuth:
const useOAuthCallback = () => {
const { handleOAuthCallback } = useAuth();
const location = useLocation();
const [state, setState] = useState({
success: false,
processing: false
});
useEffect(() => {
if (!location.search) return;
const processOAuth = async () => {
setState({ success: false, processing: true });
const searchParams = new URLSearchParams(location.search);
const success = await handleOAuthCallback(searchParams);
setState({ success, processing: false });
};
processOAuth();
}, [location.search, handleOAuthCallback]);
return state;
};- Simplify LoginPage useEffect:
const LoginPage = () => {
const { isLoading, isAuthenticated, initialize } = useAuth();
const { success, processing } = useOAuthCallback();
const [initializing, setInitializing] = useState(false);
useEffect(() => {
const init = async () => {
if (sessionStorage.getItem('logout_in_progress')) {
sessionStorage.removeItem('logout_in_progress');
return;
}
setInitializing(true);
await initialize();
setInitializing(false);
};
init();
}, [initialize]);
useEffect(() => {
if (success || isAuthenticated) {
navigate(ROUTES.DASHBOARD, { replace: true });
}
}, [success, isAuthenticated, navigate]);
if (isLoading || processing || initializing) {
return <LoadingSpinner text="Processing login..." />;
}
// ... rest of render code
};This approach:
- Separates OAuth handling into its own hook
- Removes nested conditionals
- Makes the auth state transitions more explicit
- Maintains all existing functionality
- Reduces cognitive load by isolating concerns
|
|
||
| class AuthService { | ||
| constructor() { | ||
| this.storageKey = 'auth_session'; | ||
| this.tradingAccountsKey = 'trading_accounts'; | ||
| this.defaultAccountKey = 'default_account'; |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding the default account key.
Storing the default account key directly in the code can pose a security risk. Consider storing it securely, such as in a secrets management service or environment variables, and retrieving it during runtime.
| let account = existingAccount; | ||
|
|
||
| if (!existingAccount && success) { | ||
| account = { |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding the token.
Hardcoding tokens directly in the code can be a security vulnerability. Consider using a more secure approach, such as fetching the token from a secrets management service or environment variables.
| if (!existingAccount && success) { | ||
| account = { | ||
| account: `TG${telegramUser.id}`, | ||
| token: 'telegram-token', |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding the token.
Storing tokens directly within the code is not recommended. Explore more secure options like environment variables or a dedicated secrets management service.
Summary by Sourcery
Implement user authentication with enhanced security and session management. Securely store user data with encryption and handle authentication through Telegram Web App or OAuth. Manage multiple trading accounts and authorize WebSocket connections using account tokens. Implement comprehensive session validation and error handling.
New Features:
Tests: