Scheduler wasa#43
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive migration from Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (34)
package.json (1)
47-49: Document storage strategy changesThe addition of these dependencies indicates a significant change in the application's storage strategy, moving from localStorage to a more secure approach.
Please update the documentation to include:
- Rationale for the storage strategy change
- Security considerations and configurations
- Migration guide for other developers
src/app/app-modules/core/services/auth.service.ts (2)
103-107: LGTM! Good security improvement.Moving from localStorage to sessionStorage is a security improvement as session data will be automatically cleared when the browser session ends.
Consider documenting this security consideration in the code comments to help future maintainers understand the rationale behind using sessionStorage.
Line range hint
1-107: Consider adding migration guide for breaking change.The switch from localStorage to sessionStorage is a breaking change that could affect session persistence across browser tabs and session lifetime. Consider documenting:
- The rationale behind this security improvement
- Impact on existing functionality
- Migration steps for dependent components
Would you like me to help draft documentation for this breaking change?
src/environments/environment.development.ts (1)
Line range hint
1-99: Review security configurations for sensitive endpointsThe configuration file contains multiple sensitive endpoints related to:
- User authentication and session management
- Healthcare data access
- SMS templates and parameters
Please ensure that:
- All endpoints are properly secured with appropriate authentication mechanisms
- Healthcare data endpoints comply with relevant privacy regulations
- Rate limiting and access controls are implemented at the API gateway level
Consider:
- Implementing API gateway level security controls
- Adding request rate limiting
- Implementing proper CORS policies
- Using API key authentication for sensitive endpoints
src/environments/environment.ts (1)
27-42: Security Advisory: Review HIPAA compliance measuresGiven that this is a medical records system (AMRIT), please ensure:
- All API endpoints enforce proper authentication and authorization
- Data transmission is properly encrypted (currently using HTTPS which is good)
- Proper audit logging is implemented for all sensitive operations
- Environment-specific security controls are properly configured
Consider documenting security measures and conducting a security review before deployment.
src/app/app-modules/scheduler/sms-template/sms-template-list/sms-template-list.component.ts (1)
94-94: Consider implementing a centralized storage strategyThe migration from localStorage to SessionStorage would benefit from:
- A centralized storage service that abstracts the storage mechanism
- Consistent key management across the application
- Type-safe storage operations
- Clear documentation of storage lifetime expectations
This would make future storage mechanism changes easier to implement and maintain.
src/app/app-modules/scheduler/mystaff/mystaff.component.ts (3)
47-47: Consider following Angular naming conventionsThe parameter name
sessionstorageshould be camelCase and more descriptive, likesessionStorageService, to align with Angular style guide.- readonly sessionstorage: SessionStorageService, + readonly sessionStorageService: SessionStorageService,
86-86: Maintain consistent key naming convention
- The storage key 'supervisor-specialistID' doesn't follow the 'tm-' prefix convention used elsewhere.
- Consider adding type safety for the userID value.
- this.sessionstorage.setItem('supervisor-specialistID', userID); + this.sessionstorage.setItem('tm-supervisor-specialistID', userID?.toString() ?? '');
Storage strategy needs standardization across the codebase
The verification reveals inconsistent storage patterns that need attention:
- Multiple components still use
localStoragedirectly, particularly inhttp-interceptor.service.tsand test files- Inconsistent key prefix usage: Some keys use 'tm-' prefix while others don't
- Mix of direct
sessionStorageaccess andSessionStorageServiceusageKey areas needing standardization:
src/app/app-modules/core/services/http-interceptor.service.tsstill uses directlocalStoragecallssrc/app/redir-open/redir-open.component.tsuses directsessionStoragecalls instead of the service- Test files need to be updated to use the storage service for consistency
🔗 Analysis chain
Line range hint
1-144: Consider documenting storage strategy decisionsThe migration from localStorage to SessionStorageService appears to be part of a larger security enhancement. Consider:
- Documenting the storage strategy (what goes in sessionStorage vs localStorage)
- Adding comments explaining the 'tm-' prefix convention
- Creating a constants file for storage keys to prevent typos and ensure consistency
Let's verify the consistency of storage usage across the application:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining localStorage usage rg "localStorage\." --type ts # Check for consistent session storage key prefixes rg "sessionStorage\.setItem|getItem" --type tsLength of output: 9665
src/app/app-modules/scheduler/specialization-calander-view/specialization-calander-view.component.ts (2)
26-26: Use Angular path aliases instead of src-relative importsConsider updating the import to use Angular's path aliases or relative paths instead of src-relative paths for better maintainability and consistency.
-import { SessionStorageService } from 'src/app/app-modules/core/services/session-storage.service'; +import { SessionStorageService } from '@core/services/session-storage.service';
68-71: Consider extracting session storage keys as constantsThe hardcoded session storage keys could lead to maintenance issues. Consider extracting them into named constants.
+private readonly USER_ID_KEY = 'tm-userID'; +private readonly PROVIDER_SERVICE_MAP_ID_KEY = 'tm-providerServiceMapID'; ... - userID: this.sessionstorage.getItem('tm-userID'), - providerServiceMapID: this.sessionstorage.getItem( - 'tm-providerServiceMapID', - ), + userID: this.sessionstorage.getItem(this.USER_ID_KEY), + providerServiceMapID: this.sessionstorage.getItem(this.PROVIDER_SERVICE_MAP_ID_KEY),src/app/app-modules/core/services/http-interceptor.service.ts (2)
52-54: Improve constructor parameter organizationThe constructor parameters should follow a consistent pattern:
- Use consistent access modifiers (either all private or specify readonly where necessary)
- Group similar services together (http-related, storage-related)
Consider reorganizing like this:
constructor( private spinnerService: SpinnerService, private router: Router, private confirmationService: ConfirmationService, + private http: HttpClient, + private cookieService: CookieService, + private sessionStorage: SessionStorageService, - readonly sessionstorage: SessionStorageService, - private http: HttpClient, - private cookieService: CookieService, ) {}
Storage strategy needs to be migrated to SessionStorageService
Based on the verification results, there are clear inconsistencies in the storage strategy:
- The codebase has a proper
SessionStorageServicethat provides encrypted storage usingng-cryptostore, but it's not being utilized in the http-interceptor service- Direct
localStorageandsessionStorageaccess is scattered across multiple files, bypassing the secure storage service- The http-interceptor is using unencrypted storage for sensitive auth tokens
Key locations needing updates:
src/app/app-modules/core/services/http-interceptor.service.ts: Replace direct storage access with SessionStorageServicesrc/app/app-modules/core/services/auth.service.ts: Multiple direct sessionStorage calls for auth-related operationssrc/app/redir-open/redir-open.component.ts: Direct sessionStorage usage for authentication🔗 Analysis chain
Line range hint
91-150: Ensure consistent storage strategy across the applicationThe code shows mixed usage of storage mechanisms:
- Direct sessionStorage access instead of using the injected SessionStorageService
- Both localStorage and sessionStorage are cleared on session expiry
- Magic number (27 minutes) for session timeout should be configurable
Consider these improvements:
- Use the injected SessionStorageService consistently
- Move timeout duration to environment configuration
- Document the session management strategy
Let's verify the storage usage across the application:
Let me generate additional verification scripts to check the implementation of SessionStorageService and its usage patterns:
This will help us understand:
- The actual implementation of SessionStorageService
- Where it's properly injected and used
- If auth-related keys are handled by the service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct localStorage/sessionStorage access rg "localStorage\." --type ts rg "sessionStorage\." --type ts # Search for SessionStorageService usage ast-grep --pattern 'new SessionStorageService()' ast-grep --pattern 'SessionStorageService'Length of output: 8106
Script:
#!/bin/bash # Check SessionStorageService implementation cat src/app/app-modules/core/services/session-storage.service.ts # Check for constructor injections of SessionStorageService ast-grep --pattern 'constructor($$$, private sessionStorageService: SessionStorageService, $$$)' # Check specific auth-related storage keys in SessionStorageService usage rg "tm-key|tm-isAuthenticated|authenticationToken" src/app/app-modules/core/services/session-storage.service.tsLength of output: 972
src/app/app-modules/scheduler/shared/services/scheduler.service.ts (2)
29-32: Consider renaming for consistency with Angular naming conventionsThe constructor injection is well-structured with appropriate readonly modifier. However, consider renaming
sessionstoragetosessionStoragefor consistency with Angular's camelCase naming convention.constructor( private http: HttpClient, - readonly sessionstorage: SessionStorageService, + readonly sessionStorage: SessionStorageService, ) {}
Line range hint
1-191: Consider improving overall service architectureSeveral architectural improvements could enhance maintainability and type safety:
- Remove commented-out code (e.g.,
.map(res => res.json()))- Add consistent error handling across all HTTP methods
- Add return type annotations for all methods using appropriate interfaces
- Consider implementing a request interceptor for common error handling
Would you like me to provide a detailed example of these improvements for one of the methods as a template for the others?
src/app/app-modules/scheduler/reports/daily-report/daily-report.component.ts (1)
54-54: Consider reordering constructor parametersWhile the readonly modifier is good practice, consider moving the SessionStorageService before the ConfirmationService to maintain a consistent alphabetical ordering of dependencies.
constructor( private formBuilder: FormBuilder, private schedulerService: SchedulerService, public httpServiceService: HttpServiceService, + readonly sessionstorage: SessionStorageService, private confirmationService: ConfirmationService, - readonly sessionstorage: SessionStorageService, ) {}src/app/redir-open/redir-open.component.ts (2)
Line range hint
86-95: Add input sanitization for URL parameters.The code directly uses URL parameters without proper sanitization, which could lead to XSS attacks.
Consider adding a sanitization method:
private sanitizeUrlParam(param: string | undefined): string { if (!param || param === 'undefined') return ''; return this.sanitizer.sanitize(SecurityContext.URL, param) || ''; }Then use it in the parameter extraction:
-this.externalSession.protocol = params['protocol'] === 'undefined' ? undefined : params['protocol']; +this.externalSession.protocol = this.sanitizeUrlParam(params['protocol']);Don't forget to inject the DomSanitizer:
constructor( private sanitizer: DomSanitizer, // ... other dependencies ) {}
179-205: Consider encrypting sensitive session data.The component stores sensitive information like authentication tokens and user roles without encryption.
Consider:
- Implementing encryption/decryption in the SessionStorageService
- Using a dedicated secure storage service for sensitive data
- Adding data expiration mechanisms
Would you like me to provide an example implementation of these security enhancements?
src/app/app-modules/scheduler/reports/chief-complaint-report/chief-complaint-report.component.ts (1)
68-71: Consider using constants for storage keys.Storage keys like 'tm-providerServiceMapID' and 'tm-userID' should be defined as constants to prevent typos and improve maintainability.
+private readonly STORAGE_KEYS = { + PROVIDER_SERVICE_MAP_ID: 'tm-providerServiceMapID', + USER_ID: 'tm-userID' +} as const; -this.providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID'); -this.userID = this.sessionstorage.getItem('tm-userID'); +this.providerServiceMapID = this.sessionstorage.getItem(this.STORAGE_KEYS.PROVIDER_SERVICE_MAP_ID); +this.userID = this.sessionstorage.getItem(this.STORAGE_KEYS.USER_ID);src/app/app-modules/core/components/camera-dialog/camera-dialog.component.ts (1)
Line range hint
266-288: Consider additional security measures for PII dataWhile the migration to sessionStorage improves security, consider implementing these additional measures:
- Encrypt sensitive data before storing in sessionStorage
- Implement session timeout handling
- Add data sanitization before using in DOM operations
Would you like guidance on implementing any of these security measures?
src/app/app-modules/core/components/app-header/app-header.component.ts (2)
Line range hint
194-203: Add error handling for role parsing.The current implementation might throw an error if the stored roles value is invalid JSON. Consider adding try-catch block for safer parsing.
Apply this change to add error handling:
- const role: any = this.sessionstorage.getItem('tm-roles'); - console.log('role', role); - this.roles = JSON.parse(role); - console.log('roles', this.roles); - if (this.roles) { + try { + const role = this.sessionstorage.getItem('tm-roles'); + this.roles = role ? JSON.parse(role) : []; + if (this.roles?.length) { this.filteredNavigation = this.navigation.filter((item: any) => { return this.roles.includes(item.role); }); + } + } catch (error) { + console.error('Error parsing roles:', error); + this.roles = []; } - }
Line range hint
1-284: Additional improvements for consideration.
- There are still direct
sessionStorageaccesses in the language-related methods that should be migrated toSessionStorageServicefor consistency.- Remove or replace console.log statements with proper logging service before deployment.
Consider applying these changes:
- Replace
sessionStorage.getItem('setLanguage')withthis.sessionstorage.getItem('setLanguage')- Replace
sessionStorage.setItem('setLanguage', language)withthis.sessionstorage.setItem('setLanguage', language)- Replace console.log statements with a proper logging service
src/app/app-modules/scheduler/reports/total-consultation-report/total-consultation-report.component.ts (3)
55-55: Consider following Angular naming conventionsWhile the injection is correct, consider renaming
sessionstoragetosessionStorageServiceto match Angular's style guide for service naming.- readonly sessionstorage: SessionStorageService, + readonly sessionStorageService: SessionStorageService,
Line range hint
123-157: Add input validation and sanitization for report parametersThe
searchReportmethod processes user input without proper validation. Consider adding the following safeguards:
- Validate date ranges
- Add maximum date range limit to prevent large data exports
- Sanitize provider and user IDs before API calls
searchReport() { + // Validate date range + const maxRangeDays = 31; // Example: limit to 1 month + const daysDiff = moment(this.toDate).diff(moment(this.fromDate), 'days'); + + if (daysDiff > maxRangeDays) { + this.confirmationService.alert( + `Date range cannot exceed ${maxRangeDays} days`, + 'error' + ); + return; + } + const reqObjForTotalConsultationReport = { fromDate: new Date( this.fromDate.valueOf() -
Line range hint
158-234: Optimize Excel export performance and securityThe Excel export functionality has several areas for improvement:
- Large datasets might cause performance issues
- Sensitive data might be included in exports
- Memory usage could be optimized
Consider implementing these improvements:
- Add pagination or data size limits
- Implement data masking for sensitive fields
- Stream large files instead of loading entirely in memory
exportToxlsx(criteria: any) { + const MAX_ROWS = 10000; // Prevent memory issues + if (this.totalConsultationList.length > MAX_ROWS) { + this.confirmationService.alert( + `Export limited to ${MAX_ROWS} records. Please reduce date range.`, + 'error' + ); + return; + } + + // Mask sensitive data + const maskData = (data: any) => { + const maskedData = {...data}; + if (maskedData.userID) maskedData.userID = '****' + maskedData.userID.slice(-4); + return maskedData; + }; + const cleanNullValues = (obj: any) => { for (const key in obj) { if (obj[key] === null) { obj[key] = ''; } } - return obj; + return maskData(obj); };src/app/app-modules/scheduler/appointment-view/appointment-view.component.ts (3)
92-95: Consider using constants for storage keys.While the migration to SessionStorage is correct, consider defining the storage keys as constants to improve maintainability and prevent typos.
Consider creating a constants file:
// storage-keys.constants.ts export const STORAGE_KEYS = { PROVIDER_SERVICE_MAP_ID: 'tm-providerServiceMapID', USER_ID: 'tm-userID', USER_NAME: 'tm-userName', SPECIALIST_ID: 'supervisor-specialistID' } as const;
115-118: Consider extracting common storage retrieval logic.Multiple methods are retrieving the same storage items. This could be centralized into a service method.
Consider creating a service method:
// user-context.service.ts export class UserContextService { constructor(private sessionStorage: SessionStorageService) {} getUserContext() { return { providerServiceMapID: this.sessionStorage.getItem('tm-providerServiceMapID'), userID: this.sessionStorage.getItem('tm-userID'), userName: this.sessionStorage.getItem('tm-userName') }; } }
Line range hint
251-264: LGTM! Consider adding loading state.The cancellation logic is well-implemented with proper confirmation and error handling. Consider adding a loading state during the API call to prevent multiple submissions.
Example implementation:
private isSubmitting = false; cancelTCRequest(beneficiary: any) { if (this.isSubmitting) return; this.confirmationService.confirm(...).subscribe((res) => { if (res) { this.isSubmitting = true; this.schedulerService.cancelBeneficiaryTCRequest(...).subscribe({ next: (res: any) => { this.isSubmitting = false; // existing logic }, error: (error: any) => { this.isSubmitting = false; // existing error handling } }); } }); }src/app/app-modules/scheduler/reports/consultation-report/consultation-report.component.ts (1)
Line range hint
1-324: Consider implementing session timeout handling.Since we're now using session storage, which is cleared when the browser tab is closed, we should consider implementing proper session timeout handling across the application.
Consider:
- Implementing a session timeout service that monitors user activity
- Adding session refresh mechanisms for long-running operations
- Gracefully handling session expiration during report generation
Would you like me to help create a GitHub issue to track this architectural improvement?
src/app/app-modules/scheduler/sms-template/create-sms-template/create-sms-template.component.ts (4)
Line range hint
77-79: Fix the broken change detection implementationThe current implementation has critical issues:
- The
ngDoCheckmethod throws an error which will break Angular's change detection- There's confusion between
ngDoCheck(the interface method) andDoCheck(custom method)Apply this fix:
- ngDoCheck(): void { - throw new Error('Method not implemented.'); - } + ngDoCheck(): void { + this.fetchLanguageResponse(); + if ( + this.currentLanguageSet !== undefined && + this.currentLanguageSet !== null && + this.fullSMSTemplate !== undefined && + this.fullSMSTemplate !== null + ) { + // this.createViewSMSTemplate() + } else { + this.heading = this.currentLanguageSet.createSMSTemplate; + } + } - // Existing DoCheck method can be removed - DoCheck() { - ... - }
Line range hint
41-53: Remove duplicate displayedColumns definitionThere's a duplicate definition of
displayedColumnsarray - one active and one commented out with the same values.- displayedColumns: string[] = [ - 'sNo', - 'smsParameterName', - 'smsParameterType', - 'smsParameterValue', - 'action', - ]; - // displayedColumns: string[] = [ - // 'sNo', - // 'smsParameterName', - // 'smsParameterType', - // 'smsParameterValue', - // 'action', - // ];
Line range hint
146-166: Refactor parameter extraction logicThe parameter extraction logic in
extractParametersmethod can be simplified using regular expressions and array methods.- let string_contents = []; - const regex = /[!?.,\n]/g; - string_contents = this.smsTemplate.replace(regex, ' ').split(' '); - for (const element of string_contents) { - if (element.startsWith('$$') && element.endsWith('$$')) { - const item = element.substr(2).slice(0, -2); - console.log(item); - tempParameters.push(item); - } - } + const regex = /\$\$(.*?)\$\$/g; + const matches = this.smsTemplate.matchAll(regex); + const tempParameters = Array.from(matches, m => m[1]);
Line range hint
208-231: Improve validation in addSMSParameterTemplateThe validation in
addSMSParameterTemplatecould be more robust and consistent with other methods.+ interface SMSParameter { + smsParameterName: string; + smsParameterType: string; + smsParameterID: number; + smsParameterValue: string; + createdBy: string; + modifiedBy: string; + } addSMSParameterTemplate() { - const reqObj = { + const reqObj: SMSParameter = { createdBy: this.sessionstorage.getItem('tm-userName'), modifiedBy: this.sessionstorage.getItem('tm-userName'), smsParameterName: this.parameter, smsParameterType: this.smsParameterType.smsParameterType, smsParameterID: this.smsParameterValue.smsParameterID, smsParameterValue: this.smsParameterValue.smsParameterName, }; - if ( - reqObj.smsParameterName !== undefined && - reqObj.smsParameterType !== undefined && - reqObj.smsParameterID !== undefined - ) { + if (Object.values(reqObj).every(value => value !== undefined && value !== null)) { this.mappedSMSParameter.data.push(reqObj); + this.clearParameterForm(); } else { this.confirmationService.alert( this.currentLanguageSet.ValueTypeAndValueShouldBeSelected, 'info' ); } } + private clearParameterForm(): void { + this.parameters.splice(this.parameters.indexOf(this.parameter), 1); + this.parametersLength = this.parameters.length; + this.smsTemplateCreationForm.patchValue({ + parameter: null, + smsParameterType: null, + smsParameterValue: null, + }); + this.masterSMSParameter = []; + this.selectedParameterValues = []; + }src/app/app-modules/scheduler/timesheet/timesheet.component.ts (1)
Line range hint
43-348: Consider centralizing session storage accessThe current implementation directly accesses session storage across multiple methods. This could lead to maintenance issues and inconsistent error handling.
Consider creating a dedicated user service that:
- Centralizes all session storage access
- Provides type-safe methods for retrieving user data
- Implements consistent error handling
- Caches frequently accessed values
Example:
@Injectable({providedIn: 'root'}) class UserService { constructor(private sessionStorage: SessionStorageService) {} getUserId(): string { const userId = this.sessionStorage.getItem('tm-userID'); if (!userId) throw new Error('User ID not found in session'); return userId; } // Similar methods for other user data... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.gitignore(1 hunks)WEB-INF/jboss-web.xml(1 hunks)package.json(1 hunks)src/app/app-modules/core/components/app-header/app-header.component.ts(4 hunks)src/app/app-modules/core/components/camera-dialog/camera-dialog.component.ts(4 hunks)src/app/app-modules/core/services/auth.service.ts(1 hunks)src/app/app-modules/core/services/http-interceptor.service.ts(3 hunks)src/app/app-modules/core/services/session-storage.service.spec.ts(1 hunks)src/app/app-modules/core/services/session-storage.service.ts(1 hunks)src/app/app-modules/scheduler/appointment-view/appointment-view.component.ts(5 hunks)src/app/app-modules/scheduler/mystaff/mystaff.component.ts(3 hunks)src/app/app-modules/scheduler/reports/chief-complaint-report/chief-complaint-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/consultation-report/consultation-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/daily-report/daily-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/monthly-report/monthly-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/total-consultation-report/total-consultation-report.component.ts(3 hunks)src/app/app-modules/scheduler/shared/services/scheduler.service.ts(2 hunks)src/app/app-modules/scheduler/sms-template/create-sms-template/create-sms-template.component.ts(4 hunks)src/app/app-modules/scheduler/sms-template/sms-template-list/sms-template-list.component.ts(3 hunks)src/app/app-modules/scheduler/specialization-calander-view/specialization-calander-view.component.ts(3 hunks)src/app/app-modules/scheduler/timesheet/timesheet.component.ts(5 hunks)src/app/redir-open/redir-open.component.ts(5 hunks)src/environments/enckey.ts(1 hunks)src/environments/environment.development.ts(1 hunks)src/environments/environment.prod.ts(1 hunks)src/environments/environment.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- WEB-INF/jboss-web.xml
- src/environments/enckey.ts
🔇 Additional comments (35)
src/app/app-modules/core/services/session-storage.service.spec.ts (1)
1-16: Verify session storage security implementation
Since this service is handling user session data, we need to ensure proper security measures are in place.
Consider implementing or verifying:
- Data encryption for sensitive information
- Session timeout handling
- Cross-tab communication if required
- Clear documentation of security measures
✅ Verification successful
Security implementation verified with some recommendations
The service is using ng-cryptostore library with environment-specific encryption keys, providing a secure base implementation. However, there are some areas that could be enhanced:
- The service uses
anytype for user-related properties (userID, userName, username) which could be better typed - The session timeout handling exists in the HTTP interceptor but could be more comprehensive
- The
SECRET_KEYcould be made private to prevent external access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for encryption/security implementations in the service
rg -A 5 "encrypt|decrypt|crypto" src/app/app-modules/core/services/session-storage.service.ts
# Check if sensitive data handling is documented
rg -l "sensitive|security|encryption" docs/
Length of output: 313
Script:
#!/bin/bash
# Let's check the actual implementation of SessionStorageService
cat src/app/app-modules/core/services/session-storage.service.ts
# Check if ng-cryptostore is properly configured and used
rg -A 5 "StorageService" src/app/app-modules/core/services/
# Look for any security-related configurations
rg "StorageService.*(config|options)" src/app/app-modules/core/
Length of output: 4579
Script:
#!/bin/bash
# Let's check the environment configuration for the encryption key
rg -A 2 "encKey" src/environments/
# Check for any additional security configurations in the app
rg -A 5 "ng-cryptostore" src/app/
# Look for any session timeout implementations
rg "timeout|expire" src/app/app-modules/core/
Length of output: 1306
src/app/app-modules/core/services/session-storage.service.ts (2)
1-8: LGTM! Service setup and imports are correct.
The service is properly configured as a root-level provider, which is appropriate for session management.
14-14: LGTM! Constructor dependency injection is properly implemented.
package.json (1)
47-49: Verify security implications of storage changes
The addition of cryptographic storage and cookie management libraries suggests a shift in how user data is stored and managed. Please ensure:
- Proper encryption of sensitive data
- Secure cookie configurations
- Compliance with security requirements
✅ Verification successful
Let me gather more information about the security configurations and usage patterns.
Let me check one more aspect to understand the encryption configuration and environment setup.
Security implementation appears robust and properly configured
The verification reveals a well-structured security implementation:
- Proper encryption using
ng-cryptostorewith environment-specific encryption keys - Secure cookie handling in HTTP interceptor for JWT tokens
- Encrypted storage service implementation with proper key management
- Environment-specific encryption configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for storage/cookie related security configurations
rg -A 5 "CryptoStore|CookieService"
# Look for any security-related configurations
ast-grep --pattern 'new CookieService($$$)'
Length of output: 1100
Script:
#!/bin/bash
# Search for CryptoStore usage and configurations
rg -A 5 "ng-cryptostore|CryptoStore"
# Look for cookie configurations and usage
rg -A 5 "cookieService\."
# Search for security-related configurations in environment files
fd "environment" --exec rg -l "secure|encrypt|cookie"
# Look for storage service implementations
rg -A 10 "class.*Storage"
Length of output: 3776
Script:
#!/bin/bash
# Check environment configuration for encryption key
rg -A 5 "encKey|SECRET_KEY"
# Look for storage service methods and their usage
ast-grep --pattern 'store.$_($$$)'
# Check for any additional security configurations
rg -A 5 "StorageService.*(set|get|encrypt)"
Length of output: 2129
src/app/app-modules/core/services/auth.service.ts (1)
103-107: Verify consistent storage mechanism usage.
Let's ensure all session-related storage operations consistently use sessionStorage instead of localStorage throughout the service.
✅ Verification successful
Based on the search results, I can now provide a final response:
Storage mechanism usage is consistent across the codebase
The codebase shows a clear and consistent pattern:
sessionStorageis used exclusively for storing user authentication and session-related data with thetm-prefix (tm-designation, tm-roles, tm-userName, etc.)localStorageis used only in specific cases:- In http-interceptor for clearing storage during logout/session expiry
- In app-header component tests for test data setup
- One isolated case in appointment-view component for supervisor-specialist related data
The changes being made to use sessionStorage for these authentication-related items follow the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining localStorage usage in auth service
# and related components that might need to be migrated to sessionStorage
# Search for localStorage usage in auth service and related files
rg -A 2 "localStorage\." --type ts
# Search for sessionStorage usage to compare
rg -A 2 "sessionStorage\." --type ts
# Look for potential session-related constants or configurations
rg -A 2 "(userID|userName|roles|designation|providerServiceMapID)" --type ts
Length of output: 45214
src/environments/environment.development.ts (1)
27-29:
Verify environment configuration alignment
The configuration appears to be using development domain (amritwprdev) in what seems to be a production environment file (based on production: true). This could lead to development services being accessed in production.
Let's verify the environment configuration setup:
src/environments/environment.prod.ts (2)
Line range hint 1-99: Security: Audit endpoint exposure and access controls
The configuration file contains numerous API endpoints. Please ensure:
- All endpoints are properly secured with appropriate authentication
- Rate limiting is in place
- CORS policies are properly configured
- PII data is properly protected in transit
Let's check for potential security configurations:
✅ Verification successful
Security review: Adequate authentication and session controls in place
Based on the code analysis:
-
Authentication is properly implemented:
- Login/logout endpoints use HTTPS
- Session validation is enforced via AuthGuard
- Security questions and password reset flows are in place
-
Session management is secure:
- Redis session management is implemented
- Session validation before protected routes
- Proper session cleanup on logout
-
API endpoints are protected:
- All sensitive endpoints use authenticated COMMON_API
- Public endpoints use COMMON_API_OPEN appropriately
- HTTPS is enforced for all API communication
-
No critical security issues found:
- No hardcoded credentials
- Environment variables properly segregated
- Encryption keys managed separately in enckey file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security-related configurations
# Look for security-related files
echo "Checking for security configurations..."
fd -t f "security|auth|cors"
# Check for exposed environment files
echo "Checking for exposed environment files..."
fd -t f "environment" -E "*.spec.ts"
Length of output: 512
Script:
#!/bin/bash
# Check auth service implementation and security configurations
echo "Checking auth service implementation..."
ast-grep --pattern 'class AuthService {
$$$
}' src/app/app-modules/core/services/auth.service.ts
echo "\nChecking auth guard implementation..."
ast-grep --pattern 'class AuthGuard {
$$$
}' src/app/app-modules/core/services/auth-guard.service.ts
echo "\nChecking for CORS configurations..."
rg -i "cors" -A 3 -B 3
echo "\nChecking environment files for potential sensitive data exposure..."
cat src/environments/environment.ts
Length of output: 19122
32-35: Verify API version compatibility
The API versions have been updated from v1.0 to v3.0.0 across all services. This is a major version change that could introduce breaking changes.
Let's verify the API versions are documented and available:
src/environments/environment.ts (1)
32-35: Verify API version compatibility
The significant version upgrade from v1.0 to v3.0.0 across all APIs suggests breaking changes. Please ensure:
- All API calls in the application are compatible with v3.0.0
- Migration guide or changelog has been reviewed
✅ Verification successful
API version update appears consistent across environments
The version upgrade to v3.0.0 has been consistently applied across all environment files (development, production). The only reference to v1.0 appears in a commented-out line, suggesting an intentional migration. Since we couldn't find any active API endpoint usage patterns or version inconsistencies in the codebase, the change appears to be systematically applied.
- All API version declarations in
environment.ts,environment.development.ts, andenvironment.prod.tsconsistently use v3.0.0 - The old v1.0 reference exists only in a commented line:
// const TM_API = ${tmIP}tmapi-v1.0/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining v1.0 API references that might need updating
rg "api-v1.0" --type ts
# Search for API call patterns to verify compatibility
ast-grep --pattern 'this.http.$_(${COMMON_API}'
Length of output: 145
Script:
#!/bin/bash
# Let's check for all API version patterns to get a complete picture
rg "(commonapi|schedulerapi|tmapi)-v\d+\.?\d*\.?\d*" --type ts -A 2
# Also search for HTTP service usage patterns
ast-grep --pattern 'this.http.$_($SCHEDULER_API'
ast-grep --pattern 'this.http.$_($TM_API'
# Check for any migration or changelog files
fd -i -g "*change*log*"
fd -i -g "*migrat*"
Length of output: 1682
Script:
#!/bin/bash
# Let's check for actual API endpoint usage patterns to ensure all are using v3.0.0
rg "commonapi|schedulerapi|tmapi" --type ts -g "!environment*" -B 1 -A 1
# Also check for any configuration or setup files that might contain version information
fd -e json -e yaml -e yml | xargs rg "api.*v\d+\.?\d*\.?\d*"
Length of output: 194
src/app/app-modules/scheduler/sms-template/sms-template-list/sms-template-list.component.ts (2)
29-29: LGTM!
The SessionStorageService import follows Angular's best practices and maintains consistent import path structure.
56-56: LGTM!
Good use of the readonly modifier for the injected SessionStorageService, ensuring immutability of the service reference.
src/app/app-modules/scheduler/mystaff/mystaff.component.ts (1)
27-27: LGTM: Clean import statement addition
The SessionStorageService import follows Angular's best practices.
src/app/app-modules/scheduler/specialization-calander-view/specialization-calander-view.component.ts (2)
45-45: LGTM! Good use of readonly modifier
The constructor injection is well-implemented with the readonly modifier, following Angular best practices.
Line range hint 31-31: Fix typo in component name: "Calander" should be "Calendar"
The component name contains a spelling error. While fixing this would require significant refactoring (component class, selector, file names), it's important for maintaining professional code quality.
The following files would need to be renamed and updated:
- specialization-calander-view.component.ts → specialization-calendar-view.component.ts
- specialization-calander-view.component.html → specialization-calendar-view.component.html
- specialization-calander-view.component.css → specialization-calendar-view.component.css
And the following changes would be needed in the code:
- selector: 'app-specialization-calander-view',
+ selector: 'app-specialization-calendar-view',
-export class SpecializationCalanderViewComponent implements OnInit, DoCheck {
+export class SpecializationCalendarViewComponent implements OnInit, DoCheck {src/app/app-modules/core/services/http-interceptor.service.ts (1)
39-40: LGTM: Import statements are correctly structured
The new imports for SessionStorageService and CookieService align with the architectural change from localStorage to session-based storage.
src/app/app-modules/scheduler/shared/services/scheduler.service.ts (2)
25-25: LGTM: Import statement follows Angular conventions
The SessionStorageService import is correctly added and follows proper Angular conventions.
Line range hint 136-141: Add error handling and type safety for session storage retrieval
While the migration from localStorage to sessionStorage improves security, consider adding error handling and type safety:
getAllSMSTemplates(smsTypeID?: any) {
- const providerServiceMapID = this.sessionstorage.getItem(
+ const providerServiceMapID = this.sessionStorage.getItem<string>(
'tm-providerServiceMapID',
);
+ if (!providerServiceMapID) {
+ throw new Error('Provider service map ID not found in session');
+ }
return this.http.post(environment.getAllSMSTemplatesUrl, {
providerServiceMapID: providerServiceMapID,
smsTemplateTypeID: smsTypeID || undefined,
});
}Let's verify if this key is consistently used across the codebase:
✅ Verification successful
Consider adding type safety, but error handling may not be necessary
The verification shows that tm-providerServiceMapID is consistently used across the codebase with sessionstorage service, and it's a critical authentication-related value that's properly managed:
- Set during login (redir-open.component.ts)
- Cleaned up during logout (auth.service.ts)
- Used consistently across multiple components
- No remaining localStorage usage for this key
The suggested error handling might be too defensive since this value is:
- Always set during authentication
- Cleared only on logout
- Required for the application to function
A simpler type-safe approach would suffice:
getAllSMSTemplates(smsTypeID?: any) {
- const providerServiceMapID = this.sessionstorage.getItem(
+ const providerServiceMapID = this.sessionstorage.getItem<string>(
'tm-providerServiceMapID',
);
return this.http.post(environment.getAllSMSTemplatesUrl, {
providerServiceMapID: providerServiceMapID,
smsTemplateTypeID: smsTypeID || undefined,
});
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of this session key to ensure consistent naming
rg "tm-providerServiceMapID"
# Search for potential localStorage usages that might need migration
rg "localStorage.getItem.*providerServiceMapID"
Length of output: 1653
Script:
#!/bin/bash
# Let's check the actual usage context of this key in these files
# to verify if they're using sessionStorage or localStorage
for file in $(rg -l "tm-providerServiceMapID"); do
echo "=== $file ==="
rg -B2 -A2 "tm-providerServiceMapID" "$file"
done
# Also check for any remaining localStorage usage patterns
rg "localStorage\.getItem|localStorage\.setItem"
Length of output: 8026
src/app/app-modules/scheduler/reports/daily-report/daily-report.component.ts (1)
29-29: LGTM: Import statement is correctly structured
The SessionStorageService import follows the application's established import pattern.
src/app/redir-open/redir-open.component.ts (2)
34-34: LGTM! Good use of readonly for dependency injection.
The SessionStorageService is properly imported and injected into the constructor with the readonly modifier, which is a good practice for preventing accidental reassignment.
Also applies to: 60-60
Line range hint 116-166: Verify error handling in authentication flow.
The authentication flow contains error handling, but we should verify that all error scenarios are properly handled and users can't get stuck in an invalid state.
Let's verify the error handling implementation:
✅ Verification successful
Error handling is properly implemented with appropriate fallbacks
The verification shows comprehensive error handling across the authentication flow:
- HTTP interceptor properly catches and processes all API errors
- Authentication errors trigger
goBackToOriginwhich:- Shows error message to user via confirmation service
- Cleans up session data
- Redirects user to safe state
- Auth guard prevents unauthorized access to protected routes
- Session state is properly managed with clear authentication flags
The implementation follows security best practices:
- Proper session cleanup on errors
- Protected routes with auth guards
- Clear user feedback on failures
- No possibility of getting stuck in invalid states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for comprehensive error handling in authentication-related files
# Look for error handling patterns in authentication-related files
echo "Checking error handling patterns in authentication files..."
rg -A 2 "catch|error:|reject|throw" --type ts
# Look for potential authentication state handling
echo "Checking authentication state handling..."
rg -A 2 "isAuthenticated|loggedIn|authState" --type ts
# Look for navigation guards that might prevent invalid states
echo "Checking navigation guards..."
rg -A 5 "canActivate|guard" --type ts
Length of output: 17202
src/app/app-modules/scheduler/reports/chief-complaint-report/chief-complaint-report.component.ts (3)
30-30: LGTM!
The import statement for SessionStorageService follows proper Angular conventions using absolute imports.
54-54: LGTM!
Good use of readonly modifier for dependency injection, following Angular best practices.
30-30: Verify consistent storage service usage across the application.
Let's verify that other components handling similar session data have been updated to use SessionStorageService consistently.
Also applies to: 54-54, 68-71
✅ Verification successful
Storage service usage is consistent across the scheduler module
The verification shows that:
- All report components in the scheduler module consistently use SessionStorageService for handling session data
- The session keys 'tm-providerServiceMapID' and 'tm-userID' are consistently accessed via SessionStorageService
- The only remaining localStorage usages are in:
- http-interceptor.service.ts (for clearing on logout/errors)
- appointment-view.component.ts (for a different key: 'supervisor-specialistID')
- app-header.component.spec.ts (test cases only)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for components still using localStorage and for consistent SessionStorageService usage
# Find components still using localStorage
echo "Components still using localStorage:"
rg "localStorage\." "src/app" -t ts
# Find other usages of these session keys
echo "\nOther components using these session keys:"
rg "tm-providerServiceMapID|tm-userID" "src/app" -t ts
# Find components already using SessionStorageService
echo "\nComponents using SessionStorageService:"
rg "SessionStorageService" "src/app" -t ts
Length of output: 10846
src/app/app-modules/core/components/camera-dialog/camera-dialog.component.ts (1)
41-41: LGTM: Clean service integration
The SessionStorageService integration follows Angular best practices with proper import path and readonly injection.
Also applies to: 97-97
src/app/app-modules/core/components/app-header/app-header.component.ts (2)
30-30: LGTM! Proper dependency injection setup.
The SessionStorageService is correctly imported and injected following Angular best practices. The readonly modifier ensures immutability of the service reference.
Also applies to: 62-62
Line range hint 212-229: LGTM! Well-structured navigation methods.
The navigation methods are well-implemented with:
- Proper session storage access
- Error handling in the logout flow
- Clear single responsibility for each method
src/app/app-modules/scheduler/reports/total-consultation-report/total-consultation-report.component.ts (1)
30-30: LGTM: Clean import addition
The SessionStorageService import is properly added using absolute path.
src/app/app-modules/scheduler/appointment-view/appointment-view.component.ts (2)
17-17: LGTM! Good use of readonly modifier.
The SessionStorageService is properly imported and injected with readonly modifier, which is a good practice for dependency injection in Angular services.
Also applies to: 63-63
Line range hint 1-282: Verify data sanitization for beneficiary details.
The component handles sensitive beneficiary information. Ensure all displayed data is properly sanitized to prevent XSS attacks.
✅ Verification successful
No XSS vulnerabilities found in beneficiary data handling
The code review shows that:
- All template interpolations are properly bound using Angular's
{{}}syntax which automatically escapes HTML - No unsafe
innerHTMLbindings are present - Data transformations use safe Angular pipes (
titlecase) - The component uses Angular Material components which have built-in XSS protection
- All beneficiary data is displayed through standard text interpolation without any direct DOM manipulation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential XSS vulnerabilities in template
rg -g '*.html' --no-heading '{{.*}}|[\s\"]innerHTML[\s\"]' src/app/app-modules/scheduler/appointment-view/
# Check for DomSanitizer usage
ast-grep --pattern 'DomSanitizer' src/app/app-modules/scheduler/appointment-view/
Length of output: 294
Script:
#!/bin/bash
# Search for potential XSS vulnerabilities in template with corrected regex
rg -g '*.html' --no-heading '\{\{.*\}\}|innerHTML' src/app/app-modules/scheduler/appointment-view/
# Check for sanitization imports and usage
rg -g '*.ts' 'DomSanitizer|sanitize' src/app/app-modules/scheduler/appointment-view/
# Check template file content
cat src/app/app-modules/scheduler/appointment-view/appointment-view.component.html
Length of output: 11243
src/app/app-modules/scheduler/reports/monthly-report/monthly-report.component.ts (2)
30-30: LGTM!
The SessionStorageService import is correctly added and follows Angular's import organization practices.
56-56: LGTM!
The SessionStorageService is properly injected as a readonly dependency, following Angular's dependency injection best practices.
src/app/app-modules/scheduler/reports/consultation-report/consultation-report.component.ts (3)
30-30: LGTM! Clean import statement.
The import follows Angular conventions and is properly placed with other imports.
55-55: LGTM! Constructor injection follows best practices.
Good use of readonly modifier for immutability.
67-70: Verify the session storage key names across the application.
While the switch from localStorage to SessionStorageService is a good security practice, we should ensure consistency in key naming conventions.
Let's verify the usage of these session storage keys across the codebase:
✅ Verification successful
The session storage key names are consistently used across the application
Based on the search results, both tm-providerServiceMapID and tm-userID are used consistently throughout the application:
- The keys are used with the same naming convention across multiple components in the scheduler module
- The auth service properly handles cleanup of these keys during logout
- No inconsistent variations of these key names were found in the codebase
- The only exception is in camera-dialog.component.ts which uses 'providerServiceID', but this appears to be for a different purpose
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the session storage keys to ensure consistent usage
echo "Searching for tm-providerServiceMapID usage..."
rg "tm-providerServiceMapID"
echo -e "\nSearching for tm-userID usage..."
rg "tm-userID"
# Search for any other potential variations of these keys
echo -e "\nSearching for potential variations..."
rg -i "provider.*map.*id"
rg -i "user.*id"
Length of output: 16930
src/app/app-modules/scheduler/timesheet/timesheet.component.ts (1)
43-43: LGTM: Clean implementation of SessionStorageService
The SessionStorageService is properly imported and injected as a readonly dependency.
Also applies to: 85-85
| it('should be created', () => { | ||
| expect(service).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Add test coverage for core functionality
The current test suite only verifies service creation. Since this service is replacing localStorage functionality, we need comprehensive test coverage for:
- Setting session storage items
- Getting session storage items
- Removing items
- Clearing storage
- Error handling
- Data encryption/decryption if implemented
Here's a suggested structure for additional test cases:
it('should set and get session storage items', () => {
service.setItem('testKey', 'testValue');
expect(service.getItem('testKey')).toBe('testValue');
});
it('should remove items from session storage', () => {
service.setItem('testKey', 'testValue');
service.removeItem('testKey');
expect(service.getItem('testKey')).toBeNull();
});
it('should clear all items from session storage', () => {
service.setItem('key1', 'value1');
service.setItem('key2', 'value2');
service.clear();
expect(service.getItem('key1')).toBeNull();
expect(service.getItem('key2')).toBeNull();
});
it('should handle non-existent keys', () => {
expect(service.getItem('nonExistentKey')).toBeNull();
});| beforeEach(() => { | ||
| TestBed.configureTestingModule({}); | ||
| service = TestBed.inject(SessionStorageService); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
TestBed configuration needs StorageService provider
The service has a dependency on StorageService in its constructor which needs to be configured in the TestBed. Update the test setup to include this provider:
beforeEach(() => {
TestBed.configureTestingModule({
providers: [StorageService]
});
service = TestBed.inject(SessionStorageService);
});🔗 Analysis chain
Configure TestBed with necessary providers
The empty TestBed configuration might be insufficient if the service has dependencies. Consider adding any required providers or dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the service has any dependencies that need to be mocked
ast-grep --pattern 'class SessionStorageService {
constructor($_) {
$$$
}
}'
Length of output: 86
Script:
#!/bin/bash
# Let's try to find the service implementation first
rg -l "class SessionStorageService" --type ts
Length of output: 108
Script:
#!/bin/bash
# Let's also check the content of the service file to understand its implementation
rg "class SessionStorageService" -A 10 --type ts
Length of output: 992
| userID: any; | ||
| userName: any; | ||
| username: any; | ||
| SECRET_KEY = environment.encKey; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and property access modifiers.
The current implementation could be improved for better type safety and encapsulation:
- Avoid using
anytype - Add appropriate access modifiers
- Make the encryption key private
Consider applying these changes:
- userID: any;
- userName: any;
- username: any;
- SECRET_KEY = environment.encKey;
+ private userID: string | null = null;
+ private userName: string | null = null;
+ private username: string | null = null;
+ private readonly SECRET_KEY = environment.encKey;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userID: any; | |
| userName: any; | |
| username: any; | |
| SECRET_KEY = environment.encKey; | |
| private userID: string | null = null; | |
| private userName: string | null = null; | |
| private username: string | null = null; | |
| private readonly SECRET_KEY = environment.encKey; |
| safeToString(value: any): any { | ||
| if (value === null || value === undefined) { | ||
| return ''; | ||
| } | ||
| return value.toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and robustness of safeToString method.
The current implementation has potential issues:
- Uses
anytype - Could throw errors when calling
toString()on complex objects - Lacks type checking before toString conversion
Consider this safer implementation:
- safeToString(value: any): any {
+ safeToString(value: unknown): string {
if (value === null || value === undefined) {
return '';
}
- return value.toString();
+ if (typeof value === 'object') {
+ return JSON.stringify(value);
+ }
+ return String(value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| safeToString(value: any): any { | |
| if (value === null || value === undefined) { | |
| return ''; | |
| } | |
| return value.toString(); | |
| } | |
| safeToString(value: unknown): string { | |
| if (value === null || value === undefined) { | |
| return ''; | |
| } | |
| if (typeof value === 'object') { | |
| return JSON.stringify(value); | |
| } | |
| return String(value); | |
| } |
| setItem(key: string, value: any): void { | ||
| this.store.set(key, value, this.SECRET_KEY); | ||
| } | ||
|
|
||
| getItem(key: string): any | null { | ||
| return this.store.get(key, this.SECRET_KEY); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety for storage operations.
The storage methods need:
- Error handling for encryption/decryption operations
- Key validation
- More specific return types
Consider this improved implementation:
- setItem(key: string, value: any): void {
- this.store.set(key, value, this.SECRET_KEY);
+ setItem(key: string, value: unknown): void {
+ if (!key?.trim()) {
+ throw new Error('Invalid storage key');
+ }
+ try {
+ this.store.set(key, value, this.SECRET_KEY);
+ } catch (error) {
+ console.error('Failed to store encrypted data:', error);
+ throw new Error('Storage operation failed');
+ }
}
- getItem(key: string): any | null {
- return this.store.get(key, this.SECRET_KEY);
+ getItem<T>(key: string): T | null {
+ if (!key?.trim()) {
+ throw new Error('Invalid storage key');
+ }
+ try {
+ return this.store.get(key, this.SECRET_KEY) as T;
+ } catch (error) {
+ console.error('Failed to retrieve encrypted data:', error);
+ return null;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setItem(key: string, value: any): void { | |
| this.store.set(key, value, this.SECRET_KEY); | |
| } | |
| getItem(key: string): any | null { | |
| return this.store.get(key, this.SECRET_KEY); | |
| } | |
| setItem(key: string, value: unknown): void { | |
| if (!key?.trim()) { | |
| throw new Error('Invalid storage key'); | |
| } | |
| try { | |
| this.store.set(key, value, this.SECRET_KEY); | |
| } catch (error) { | |
| console.error('Failed to store encrypted data:', error); | |
| throw new Error('Storage operation failed'); | |
| } | |
| } | |
| getItem<T>(key: string): T | null { | |
| if (!key?.trim()) { | |
| throw new Error('Invalid storage key'); | |
| } | |
| try { | |
| return this.store.get(key, this.SECRET_KEY) as T; | |
| } catch (error) { | |
| console.error('Failed to retrieve encrypted data:', error); | |
| return null; | |
| } | |
| } |
| this.providerServiceMapID = this.sessionstorage.getItem( | ||
| 'tm-providerServiceMapID', | ||
| ); | ||
| this.userID = this.sessionstorage.getItem('tm-userID'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for missing session data.
The session storage values are used in critical API calls (see searchReport method), but there's no error handling if these values are missing.
Consider adding validation:
ngOnInit() {
this.providerServiceMapID = this.sessionstorage.getItem(
'tm-providerServiceMapID',
);
this.userID = this.sessionstorage.getItem('tm-userID');
+ if (!this.providerServiceMapID || !this.userID) {
+ this.confirmationService.alert('Session expired. Please login again.', 'error');
+ // Add navigation to login if needed
+ return;
+ }
this.fetchLanguageResponse();
this.createConsultationForm();
/* Set Max date*/
this.maxEndDate = new Date();
this.today = new Date();
this.maxEndDate.setDate(this.today.getDate() - 1);
}Also applies to: 144-156
| import { SetLanguageComponent } from '../../../core/components/set-language.component'; | ||
| import { HttpServiceService } from 'src/app/app-modules/core/services/http-service.service'; | ||
| import { MatTableDataSource } from '@angular/material/table'; | ||
| import { SessionStorageService } from 'src/app/app-modules/core/services/session-storage.service'; |
There was a problem hiding this comment.
💡 Codebase verification
Migration to SessionStorageService needs attention
Several components still use direct localStorage access that should be migrated to SessionStorageService:
appointment-view.component.ts: UseslocalStorage.removeItem('supervisor-specialistID')http-interceptor.service.ts: UseslocalStorage.clear()in multiple placesapp-header.component.spec.ts: Contains test cases using direct localStorage access
The migration is incomplete and needs to be consistent across the codebase. The SessionStorageService is being used correctly in the SMS template components, but other areas need to be updated to maintain consistency.
🔗 Analysis chain
Verify session storage migration completeness
The migration from localStorage to SessionStorageService looks good, but let's ensure all required session storage keys exist and are properly set.
Also applies to: 74-74, 212-213, 255-258
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all localStorage references are migrated and session storage keys exist
# Check for any remaining localStorage references
echo "Checking for remaining localStorage references..."
rg "localStorage\." --type ts
# Check for all sessionstorage.getItem calls to verify key consistency
echo "Checking session storage key usage..."
rg "sessionstorage\.getItem\(" --type ts
Length of output: 7888
| availabilityFormValue.createdBy = | ||
| this.sessionstorage.getItem('tm-userName'); |
There was a problem hiding this comment.
Add validation for createdBy field
The userName retrieval from session storage lacks validation. If the userName is missing, the form submission might fail silently.
Consider adding validation:
-availabilityFormValue.createdBy = this.sessionstorage.getItem('tm-userName');
+const userName = this.sessionstorage.getItem('tm-userName');
+if (!userName) {
+ this.confirmationService.alert('User name not found in session', 'error');
+ return;
+}
+availabilityFormValue.createdBy = userName;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| availabilityFormValue.createdBy = | |
| this.sessionstorage.getItem('tm-userName'); | |
| const userName = this.sessionstorage.getItem('tm-userName'); | |
| if (!userName) { | |
| this.confirmationService.alert('User name not found in session', 'error'); | |
| return; | |
| } | |
| availabilityFormValue.createdBy = userName; |
| userInfo = { userID: this.sessionstorage.getItem('tm-userID') }; | ||
| } else { | ||
| userInfo = { userID: localStorage.getItem('supervisor-specialistID') }; | ||
| userInfo = { | ||
| userID: this.sessionstorage.getItem('supervisor-specialistID'), | ||
| }; |
There was a problem hiding this comment.
Add null checks for session storage retrieval
The code retrieves user IDs from session storage without checking if they exist. This could lead to runtime errors if the session data is missing.
Consider adding null checks:
-userInfo = { userID: this.sessionstorage.getItem('tm-userID') };
+const userId = this.sessionstorage.getItem('tm-userID');
+if (!userId) {
+ this.confirmationService.alert('User ID not found in session', 'error');
+ return;
+}
+userInfo = { userID: userId };Similar checks should be added for 'supervisor-specialistID'.
Committable suggestion skipped: line range outside the PR's diff.
| const providerServiceMapID = this.sessionstorage.getItem( | ||
| 'tm-providerServiceMapID', | ||
| ); | ||
| const userID = localStorage.getItem('tm-userID'); | ||
| const userID = this.sessionstorage.getItem('tm-userID'); |
There was a problem hiding this comment.
Add comprehensive validation for specialist retrieval
The getSpecialist method retrieves multiple session storage items without validation before making an API call.
Consider adding comprehensive validation:
-const providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID');
-const userID = this.sessionstorage.getItem('tm-userID');
+const providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID');
+const userID = this.sessionstorage.getItem('tm-userID');
+
+if (!providerServiceMapID || !userID) {
+ this.confirmationService.alert('Required session data is missing', 'error');
+ return;
+}
+
+if (!this.selectedSpecialization?.specializationID) {
+ this.confirmationService.alert('Please select a specialization', 'error');
+ return;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const providerServiceMapID = this.sessionstorage.getItem( | |
| 'tm-providerServiceMapID', | |
| ); | |
| const userID = localStorage.getItem('tm-userID'); | |
| const userID = this.sessionstorage.getItem('tm-userID'); | |
| const providerServiceMapID = this.sessionstorage.getItem( | |
| 'tm-providerServiceMapID', | |
| ); | |
| const userID = this.sessionstorage.getItem('tm-userID'); | |
| if (!providerServiceMapID || !userID) { | |
| this.confirmationService.alert('Required session data is missing', 'error'); | |
| return; | |
| } | |
| if (!this.selectedSpecialization?.specializationID) { | |
| this.confirmationService.alert('Please select a specialization', 'error'); | |
| return; | |
| } |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
All changes I have reviewed. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/environments/environment.ci.ts.template (1)
32-32: Good use of template variable for encryption key, consider extending to API endpoints.The use of template variable for
sessionStorageEncKeyis a good security practice. Consider extending this pattern to API endpoints for better environment configuration management.Apply this pattern to API endpoints:
-const COMMON_API_OPEN = `${commonIP}commonapi-3.0.0/`; +const COMMON_API_OPEN = '<%= COMMON_API_OPEN_URL %>';Also applies to: 35-35
src/environments/environment.ts (1)
32-35: Remove commented out legacy API version.The commented out legacy API version (
tmapi-v1.0) should be removed as it's no longer needed and could cause confusion.Apply this diff:
-// const TM_API = `${tmIP}tmapi-v1.0/`;src/app/app-modules/scheduler/sms-template/create-sms-template/create-sms-template.component.ts (1)
Line range hint
77-79: Critical: Fix ngDoCheck implementation.The current ngDoCheck implementation throws an error which will break the component's change detection. The correct implementation exists as 'DoCheck' at the bottom of the file.
Apply this fix:
- ngDoCheck(): void { - throw new Error('Method not implemented.'); - } + ngDoCheck() { + this.fetchLanguageResponse(); + if ( + this.currentLanguageSet !== undefined && + this.currentLanguageSet !== null && + this.fullSMSTemplate !== undefined && + this.fullSMSTemplate !== null + ) { + // this.createViewSMSTemplate() + } else { + this.heading = this.currentLanguageSet.createSMSTemplate; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
src/app/app-modules/core/components/app-header/app-header.component.ts(4 hunks)src/app/app-modules/core/components/camera-dialog/camera-dialog.component.ts(4 hunks)src/app/app-modules/core/services/http-interceptor.service.ts(3 hunks)src/app/app-modules/scheduler/appointment-view/appointment-view.component.ts(5 hunks)src/app/app-modules/scheduler/mystaff/mystaff.component.ts(3 hunks)src/app/app-modules/scheduler/reports/chief-complaint-report/chief-complaint-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/consultation-report/consultation-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/daily-report/daily-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/monthly-report/monthly-report.component.ts(3 hunks)src/app/app-modules/scheduler/reports/total-consultation-report/total-consultation-report.component.ts(3 hunks)src/app/app-modules/scheduler/shared/services/scheduler.service.ts(2 hunks)src/app/app-modules/scheduler/sms-template/create-sms-template/create-sms-template.component.ts(4 hunks)src/app/app-modules/scheduler/sms-template/sms-template-list/sms-template-list.component.ts(3 hunks)src/app/app-modules/scheduler/specialization-calander-view/specialization-calander-view.component.ts(3 hunks)src/app/app-modules/scheduler/timesheet/timesheet.component.ts(5 hunks)src/app/redir-open/redir-open.component.ts(5 hunks)src/environments/environment.ci.ts.template(1 hunks)src/environments/environment.development.ts(1 hunks)src/environments/environment.prod.ts(1 hunks)src/environments/environment.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/app/app-modules/scheduler/sms-template/sms-template-list/sms-template-list.component.ts
- src/app/app-modules/scheduler/specialization-calander-view/specialization-calander-view.component.ts
- src/app/app-modules/scheduler/reports/total-consultation-report/total-consultation-report.component.ts
- src/app/app-modules/scheduler/reports/daily-report/daily-report.component.ts
- src/app/app-modules/core/components/camera-dialog/camera-dialog.component.ts
- src/app/redir-open/redir-open.component.ts
- src/app/app-modules/scheduler/shared/services/scheduler.service.ts
- src/app/app-modules/scheduler/reports/consultation-report/consultation-report.component.ts
- src/app/app-modules/core/components/app-header/app-header.component.ts
- src/app/app-modules/scheduler/mystaff/mystaff.component.ts
- src/app/app-modules/scheduler/reports/monthly-report/monthly-report.component.ts
- src/app/app-modules/scheduler/reports/chief-complaint-report/chief-complaint-report.component.ts
- src/app/app-modules/core/services/http-interceptor.service.ts
- src/environments/environment.prod.ts
🔇 Additional comments (12)
src/environments/environment.development.ts (3)
27-29:
Critical: Environment configuration needs immediate attention.
The development environment configuration has several issues:
- All services point to the same development domain
- URLs are hardcoded instead of using environment variables
36-36:
Critical: Encryption keys need secure management.
The encryption key implementation raises security concerns.
Also applies to: 40-40
32-35:
Fix inconsistent API version format.
The API version format is inconsistent:
commonapi-3.0.0vscommonapi-v3.0.0
Apply this diff to standardize the version format:
-const COMMON_API_OPEN = `${commonIP}commonapi-3.0.0/`;
+const COMMON_API_OPEN = `${commonIP}commonapi-v3.0.0/`;Likely invalid or redundant comment.
src/environments/environment.ts (2)
27-29:
Critical: Environment configuration needs immediate attention.
Environment configuration issues persist in the default environment file.
36-36:
Critical: Encryption keys need secure management.
The encryption key implementation raises security concerns.
Also applies to: 42-42
src/app/app-modules/scheduler/sms-template/create-sms-template/create-sms-template.component.ts (2)
31-31: LGTM! Proper service injection.
The SessionStorageService is correctly imported and injected as readonly in the constructor.
Also applies to: 74-74
212-213: Verify session storage keys on component initialization.
The migration to SessionStorageService looks good, but we should verify that required keys exist when the component initializes.
Run this script to verify the session storage keys are properly set:
Consider adding this validation in ngOnInit:
ngOnInit() {
const userName = this.sessionstorage.getItem('tm-userName');
const providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID');
if (!userName || !providerServiceMapID) {
this.confirmationService.alert('Session expired. Please login again.');
// Handle navigation to login
return;
}
this.smsTemplateCreationForm = this.createsmsTemplateCreationForm();
this.getSMSType();
this.fetchLanguageResponse();
}Also applies to: 255-258
src/app/app-modules/scheduler/appointment-view/appointment-view.component.ts (1)
Line range hint 79-79: Fix inconsistent storage mechanism.
The code retrieves 'supervisor-specialistID' from SessionStorage in ngOnInit but removes it from localStorage in ngOnDestroy. This inconsistency could lead to cleanup issues.
src/app/app-modules/scheduler/timesheet/timesheet.component.ts (4)
43-43: Verify the import path.
The import path 'Common-UI/src/registrar/services/session-storage.service' seems to be from a shared library. Ensure this path is correctly configured in your build system.
121-125:
Add null checks for session storage retrieval in getUserDesignation.
The code retrieves user IDs from session storage without validation. This could lead to runtime errors if the session data is missing.
165-166:
Add validation for createdBy field.
The userName retrieval from session storage lacks validation. If the userName is missing, the form submission might fail silently.
345-348:
Add comprehensive validation for specialist retrieval.
The getSpecialist method retrieves multiple session storage items without validation before making an API call.
| const providerServiceMapID = this.sessionstorage.getItem( | ||
| 'tm-providerServiceMapID', | ||
| ); | ||
| const userID = localStorage.getItem('tm-userID'); | ||
| const userID = this.sessionstorage.getItem('tm-userID'); |
There was a problem hiding this comment.
Add null checks for session storage retrieval.
Multiple session storage items are retrieved without validation. This could lead to runtime errors if the session data is missing.
Apply this diff to add proper validation:
-const providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID');
-const userID = this.sessionstorage.getItem('tm-userID');
+const providerServiceMapID = this.sessionstorage.getItem('tm-providerServiceMapID');
+const userID = this.sessionstorage.getItem('tm-userID');
+
+if (!providerServiceMapID || !userID) {
+ this.confirmationService.alert('Required session data is missing', 'error');
+ return;
+}Also applies to: 115-118
| visitCode: beneficiary.visitCode, | ||
| userID: beneficiary.tCSpecialistUserID, | ||
| modifiedBy: localStorage.getItem('tm-userName'), | ||
| modifiedBy: this.sessionstorage.getItem('tm-userName'), |
There was a problem hiding this comment.
Add validation for modifiedBy field.
The userName retrieval from session storage lacks validation. If the userName is missing, the API call might fail silently.
Apply this diff to add proper validation:
-modifiedBy: this.sessionstorage.getItem('tm-userName'),
+const userName = this.sessionstorage.getItem('tm-userName');
+if (!userName) {
+ this.confirmationService.alert('User name not found in session', 'error');
+ return;
+}
+modifiedBy: userName,Committable suggestion skipped: line range outside the PR's diff.
| import { HttpServiceService } from '../../core/services/http-service.service'; | ||
| import * as moment from 'moment'; | ||
| import { MatTableDataSource } from '@angular/material/table'; | ||
| import { SessionStorageService } from 'Common-UI/src/registrar/services/session-storage.service'; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
The import path 'Common-UI/src/registrar/services/session-storage.service' needs attention
The import is widely used across multiple components in the codebase, but:
- The
Common-UIpackage is not listed in package.json dependencies - The directory
Common-UIis not present in the project - There are no path mappings configured in tsconfig files to resolve this import
This suggests the import path is incorrect or the Common-UI dependency is missing from the project setup.
🔗 Analysis chain
Verify the import path.
The import path 'Common-UI/src/registrar/services/session-storage.service' seems to be from a shared library. Ensure this path is correctly configured in your build system.
Let me gather more information about this import path and its usage.
Let me check if the Common-UI directory exists and look for path mappings in the tsconfig files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the import path is correctly configured
fd -e json "package.json" -x jq -r '.dependencies["Common-UI"] // .devDependencies["Common-UI"]'
Length of output: 101
Script:
#!/bin/bash
# Search for any references to Common-UI in tsconfig or angular configuration files
fd -e json "tsconfig|angular" -x cat {}
# Search for similar imports across the codebase
rg "from.*Common-UI.*session-storage.service" -A 2
Length of output: 12707
Script:
#!/bin/bash
# Check if Common-UI directory exists
fd -t d "Common-UI"
# Look for path mappings in tsconfig files
fd -e json "tsconfig" -x jq -r '.compilerOptions.paths // empty'
Length of output: 255
| const COMMON_API = `${commonIP}commonapi-v3.0.0/`; | ||
| const SCHEDULER_API = `${schedulerIP}schedulerapi-v3.0.0/`; | ||
| const TM_API = `${tmIP}tmapi-v3.0.0/`; | ||
| const sessionStorageEncKey = '<%= SESSION_STORAGE_ENC_KEY %>'; |
There was a problem hiding this comment.
@NKOKKILIGADDA87 please refer to how it's done in other places.
This notation is only required in the ci template.
Please leave it empty in other env files.
There was a problem hiding this comment.
ok, so we'll keep the same line only in "ci template" and other environment files will have like below
const sessionStorageEncKey = <>;
is that correct? @drtechie
There was a problem hiding this comment.
Yes, that's fine.
Only ci.template passes through the build script (check scripts folder) where the pattern is replaced by an environment variable set in CI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/environments/environment.ts (1)
36-36:⚠️ Potential issueCritical: Encryption keys are exposed in source code
The session storage encryption configuration has critical security issues:
- Empty encryption key is committed (
'<>')- Key is hardcoded rather than being managed through environment variables
Immediate actions needed:
- Remove hardcoded key and use environment variables
- Implement proper key management
- Ensure different keys for different environments
Also applies to: 42-42
🧹 Nitpick comments (1)
src/environments/environment.ts (1)
27-35: Enhance API security configurationWhile HTTPS is now used, additional security measures should be considered:
- CORS configuration for these endpoints
- Security headers (HSTS, CSP, etc.)
- Rate limiting
- API versioning strategy
Consider implementing an API gateway pattern to centralize these security concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/environments/environment.ts(1 hunks)
🔇 Additional comments (2)
src/environments/environment.ts (2)
27-29: Environment configuration needs immediate attention
While HTTPS migration is a positive security improvement, the current configuration still has issues:
- Development domain (amritwprdev) is hardcoded
- All services point to the same domain
Recommend:
- Using environment variables for API endpoints
- Implementing a configuration management system
32-35: Verify API version compatibility
The upgrade from v1.0 to v3.0.0 is a major version change that likely includes breaking changes.
Let's verify the API version compatibility:
|
sandipkarmakar3
left a comment
sandipkarmakar3
left a comment
There was a problem hiding this comment.
All changes I have reviewed.
Everything is as per the requirement.
We used sessionstorage.service to store the data to session storage.
We removed all localstorage code and used session storage.
We used jwt token for validate user's session.
|
|
||
| // Without API MAN Configuration | ||
| const COMMON_API_OPEN = `${commonIP}commonapi-v1.0/`; | ||
| const COMMON_API_OPEN = `${commonIP}commonapi-1.0/`; |
There was a problem hiding this comment.
@sandipkarmakar3 Please keep common api version as "v1.0"
|
5 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
Release Notes
New Dependencies
ng-cryptostoreandngx-cookie-serviceto project dependenciesSession Management
localStoragetoSessionStorageServiceacross multiple componentsEnvironment Updates
https://amritwprdev.piramalswasthya.org/Project Structure
.gitignoreto exclude sensitive environment files