feat: handle unauthorized error to redirect to login #351
Conversation
WalkthroughThe changes in this pull request involve modifications to several components and services within the application. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)apps/portal/src/app/views/notifications/notifications.component.ts (2)
The AuthService integration follows Angular best practices with proper import and dependency injection. Also applies to: 87-87
While the error handling implementation is good, consider these improvements:
- if (applicationResponse?.errors?.length) {
+ if (applicationResponse?.errors?.length > 0) {
const unauthorizedError = applicationResponse.errors.find(
(error) => error.message === 'Unauthorized',
);
this.applications = [];
this.selectedApplication = null;
if (unauthorizedError) {
this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
- detail: 'Unauthorized access. Please log in again.',
+ detail: 'Your session has expired. Please log in again to continue.',
});
this.authService.logoutUser();
} else {
applicationResponse.errors.forEach((error) => {
this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
- detail: `GraphQL Error - Get Applications: ${error.message}`,
+ detail: `Unable to load applications: ${error.message}`,
});
});
}
}Let's verify the error message consistency across the application: 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: 3
🧹 Outside diff range and nitpick comments (4)
apps/portal/src/app/auth/auth.service.ts (1)
Line range hint
1-50: Consider enhancing security measures.The authentication service could benefit from additional security measures:
- Consider implementing CSRF protection for authentication endpoints
- Add refresh token mechanism for better session management
- Consider using a secure storage mechanism (like cookies with httpOnly flag) instead of localStorage for sensitive data
- Add events/observables for authentication state changes to better manage UI updates across the application
Would you like assistance in implementing any of these security enhancements?
apps/portal/src/app/views/applications/applications.service.ts (2)
Line range hint
35-39: Standardize error handling approach.The current implementation mixes two error handling approaches: returning errors in the response object and throwing errors in catchError. This could lead to inconsistent error handling across the application.
Consider standardizing the error handling:
catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); + return [{ + applications: [], + total: 0, + offset: 0, + limit: 0, + errors: [{ + message: this.sanitizeErrorMessage(error.message), + code: error.extensions?.code || 'UNKNOWN_ERROR' + }] + }]; }),
Security concern confirmed: Unsafe token handling detected
The
decodeTokenmethod inApplicationsServiceis indeed problematic as:
- It's only used for decoding without any signature validation
- The auth service doesn't show any token validation methods
- The method is exposed at class level, potentially allowing unauthorized access to token payloads
Recommendations:
- Move token handling to the
AuthService- Implement proper JWT verification using a cryptographic library
- If payload inspection is needed, ensure it's done after signature validation
- Consider marking sensitive payload data as private
🔗 Analysis chain
Line range hint
42-60: Review token handling security implications.While not part of the current changes, given this PR's focus on unauthorized errors, it's worth noting that the
decodeTokenmethod:
- Doesn't validate the token signature
- Could expose sensitive information in the decoded payload
- Shouldn't be used for authentication purposes
Let's check if this method is used for authentication:
The previous command failed due to typescript extension not being recognized. Let's try a broader search to understand the usage context and potential authentication flows.
Let's verify if this is just a utility method or if it's actually used in authentication flows by checking the service implementations.
We need to understand the actual usage context of decodeToken and verify if proper token validation exists in the auth service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of decodeToken method # Expected: Should not be used in authentication flows rg -t typescript "decodeToken" -A 5Length of output: 91
Script:
#!/bin/bash # Let's try different approaches to find usages of decodeToken # Search for decodeToken using ripgrep without file type restriction rg "decodeToken" -B 2 -A 5 # Also search for potential authentication-related files rg -l "auth|login|authenticate"Length of output: 3010
Script:
#!/bin/bash # Let's check how this decodeToken is being used in the applications service rg -B 10 -A 10 "class ApplicationsService" apps/portal/src/app/views/applications/applications.service.ts # Also check if there's a proper auth service handling token validation rg -B 5 -A 10 "class AuthService" apps/portal/src/app/auth/auth.service.tsLength of output: 1216
Script:
#!/bin/bash # Let's check where this decodeToken is actually used within the applications service rg -B 5 -A 10 "decodeToken\(" apps/portal/src/app/views/applications/applications.service.ts # And check for any token validation in the auth service rg "validateToken|verifyToken|jwt\.verify" apps/portal/src/app/auth/auth.service.tsLength of output: 622
apps/portal/src/app/views/notifications/notifications.component.ts (1)
126-142: Consider implementing retry logic for non-auth errors.While the unauthorized error handling is good, other transient errors might benefit from a retry mechanism.
Consider implementing a retry strategy using RxJS operators for non-auth errors:
import { retry, retryWhen, delay, take } from 'rxjs/operators'; // In your service: getApplications(variables, loginToken).pipe( retryWhen(errors => errors.pipe( delay(1000), // Wait 1s between retries take(3) // Max 3 retries ) ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/portal/src/app/auth/auth.service.ts (3 hunks)
- apps/portal/src/app/views/applications/application.model.ts (2 hunks)
- apps/portal/src/app/views/applications/applications.service.ts (1 hunks)
- apps/portal/src/app/views/notifications/notifications.component.ts (3 hunks)
🧰 Additional context used
🪛 Biome
apps/portal/src/app/views/notifications/notifications.component.ts
[error] 152-152: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
apps/portal/src/app/views/applications/application.model.ts (2)
17-17: LGTM! Well-structured error handling.The addition of the optional
errorsproperty usingReadonlyArray<GraphQLFormattedError>is a good practice for GraphQL error handling. This immutable approach ensures type safety and aligns with GraphQL specifications.
1-1: Verify GraphQL package dependency.Please ensure that the
graphqlpackage is listed as a dependency in the project's package.json.✅ Verification successful
GraphQL package dependency is properly configured
The
graphqlpackage is correctly listed as a dependency inapps/portal/package.jsonwith version^16.9.0, which supports the importedGraphQLFormattedErrortype.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if graphql is listed as a dependency grep -r '"graphql"' package.jsonLength of output: 80
Script:
#!/bin/bash # Search for package.json files in the repository fd package.json # After finding package.json files, let's check their content for graphql dependency fd package.json --exec grep -l "graphql" {}Length of output: 161
Script:
#!/bin/bash # Check the content of portal's package.json for graphql dependency details cat apps/portal/package.json | grep -A 5 -B 5 '"graphql"'Length of output: 404
apps/portal/src/app/auth/auth.service.ts (2)
4-4: LGTM!The Router import is correctly added and necessary for the new navigation functionality.
17-20: LGTM!The Router service is properly injected following Angular dependency injection patterns.
apps/portal/src/app/views/applications/applications.service.ts (1)
32-32: Verify ApplicationResponse type definition.The
errorsproperty has been added to the response object, but we need to ensure theApplicationResponseinterface inapplication.model.tsis properly updated.Let's verify the type definition:
#!/bin/bash # Description: Check if ApplicationResponse interface includes the errors property # Expected: Should find the interface definition with errors property rg -t typescript "interface ApplicationResponse" -A 10apps/portal/src/app/views/notifications/notifications.component.ts (2)
5-5: LGTM: Clean dependency injection implementation.The AuthService integration follows Angular's dependency injection best practices.
Also applies to: 87-87
141-141: Verify AuthService.logoutUser implementation.Ensure that the AuthService.logoutUser() method properly handles:
- Clearing all relevant session data
- Redirecting to the login page
- Cleaning up any active subscriptions
✅ Verification successful
Let me try a different search pattern to locate the AuthService implementation.
AuthService.logoutUser implementation is verified and properly handles logout
The implementation in
apps/portal/src/app/auth/auth.service.tscorrectly handles:
- ✓ Clearing session data via
localStorage.clear()- ✓ Redirecting to login page via
router.navigate(['/login'])The implementation is simple but sufficient for basic logout functionality. No issues found with the integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AuthService implementation echo "Searching for AuthService implementation..." ast-grep --pattern 'class AuthService { $$$ logoutUser() { $$$ } $$$ }'Length of output: 182
Script:
#!/bin/bash # Search for AuthService class and logoutUser method with more flexible patterns echo "Searching for AuthService class..." rg -l "class AuthService" -g "*.ts" echo -e "\nSearching for logoutUser method..." rg "logoutUser\(" -g "*.ts" -A 5Length of output: 1331
apps/portal/src/app/views/notifications/notifications.component.ts
Outdated
Show resolved
Hide resolved
xixas
changed the title
Portal PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login page)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Handle GraphQL response errors. Logout when
unauthorizederror is encounteredRelated changes:
GraphQLFormattedErrorobject inApplicationResponselogoutUserservice functionunauthorizederror is encounteredScreenshots:
Unauthorized token
https://github.com/user-attachments/assets/f41325b0-690d-4f78-8cf7-699576b2cafb
Non admin user login
https://github.com/user-attachments/assets/29b1e4ba-9fce-402a-9db2-20b12164634f
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
New Features
Bug Fixes
Documentation