It is correct when decode token ? #137
Conversation
WalkthroughThe changes integrate JWT support across the project. In the frontend, a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ApplicationService
participant oidcSecurityService
participant JwtHelperService
participant LocalStorage
Client->>ApplicationService: login(credentials)
ApplicationService->>oidcSecurityService: getAccessToken()
oidcSecurityService-->>ApplicationService: accessToken
ApplicationService->>LocalStorage: store(accessToken)
Client->>ApplicationService: getUserId() / getUserRole()
ApplicationService->>JwtHelperService: decode(accessToken)
JwtHelperService-->>ApplicationService: user data
ApplicationService->>Client: return userId / role
sequenceDiagram
autonumber
participant AuthToken
participant UserInfoService
participant UserEntity
AuthToken->>UserInfoService: getCustomClaimsForJwtAuthenticationToken(email)
UserInfoService->>UserEntity: fetch user details
UserEntity-->>UserInfoService: user data (id, role)
UserInfoService->>AuthToken: return claims {userId, role}
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ 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 (
|
| private getAccessToken(): void { | ||
| this.oidcSecurityService.getAccessToken().subscribe((token: string) => { | ||
| if (token) { | ||
| localStorage.setItem('access_token', token); |
There was a problem hiding this comment.
I'm not sure this is correct way to store access_token.
By the way we already store it in session/cookies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.html (1)
11-25: Enhance payment method images accessibility.The payment method images need better accessibility attributes.
Add descriptive alt text and aria labels:
<img ngSrc="assets/images/payment-method/momo.svg" - alt="Logo" + alt="MoMo Payment Method" + aria-label="Pay with MoMo" width="30" height="30" priority /> <img ngSrc="assets/images/payment-method/vnpay.svg" - alt="Logo" + alt="VNPay Payment Method" + aria-label="Pay with VNPay" width="30" height="30" priority class="border" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sep490-frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
sep490-frontend/package.json(1 hunks)sep490-frontend/src/app/modules/core/services/application.service.ts(4 hunks)sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.html(4 hunks)sep490-idp/src/main/java/green_buildings/idp/service/impl/UserInfoService.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Java Modules (sep490-enterprise)
- GitHub Check: Build Java Modules (sep490-idp)
- GitHub Check: Build Frontend
🔇 Additional comments (1)
sep490-frontend/package.json (1)
22-22:❌ Incorrect review comment
Update @auth0/angular-jwt to the latest version.
Version 5.2.0 might have known vulnerabilities.
🌐 Web query:
What is the latest version of @auth0/angular-jwt?💡 Result:
The latest version of @auth0/angular-jwt is v5.2.0, released on October 31, 2023[10][9]. This version dropped support for Angular versions below 12 and aligns with Google’s maintenance policies for Angular frameworks[3][10].
Key details:
- Available via npm with installation command:
npm install @auth0/angular-jwt[10]- Includes updates like Ivy partial compilation mode and Angular 12+ compatibility[3][6]
- Earlier versions (e.g., v1.x) targeted AngularJS (1.x), while newer versions support Angular 2+[1][5][7].
For projects using Angular ≥12, v5.x remains the recommended stable release[10].
Citations:
- 1: https://github.com/auth0/angular-jwt
- 2: https://community.auth0.com/t/updates-to-angular-documentation-samples-needed/9809
- 3: https://github.com/auth0/angular2-jwt/blob/main/CHANGELOG.md
- 4: https://forum.ionicframework.com/t/suddenly-the-project-doesnt-build-auth0-angular-jwt-mjs-using-angular-version-14-3-0/242251
- 5: https://community.auth0.com/t/auth0-angular-bower-package-which-previously-supported-angularjs-1-x-apps-has-been-removed/47900
- 6: https://community.auth0.com/t/new-major-versions-for-auth0-angular-auth0-react-and-auth0-vue-released-in-ga/98751
- 7: https://community.auth0.com/t/authenticate-angular-1-jwt/140084
- 8: https://unpkg.com/browse/@auth0/angular-jwt@1.0.0/package.json
- 9: https://security.snyk.io/package/npm/@auth0%2Fangular-jwt
- 10: https://www.npmjs.com/package/@auth0/angular-jwt
Dependency is up-to-date: No update required for @auth0/angular-jwt
The current package.json entry already uses v5.2.0, which is confirmed as the latest stable release.
sep490-frontend/package.jsonat line 22 correctly specifies"@auth0/angular-jwt": "^5.2.0".- There is no need to update, as v5.2.0 is the current supported and recommended version for Angular ≥12.
Likely an incorrect or invalid review comment.
| claims.put("userId", user.getId()); | ||
| claims.put("role", user.getEnterprise().getRole()); |
There was a problem hiding this comment.
Validate and sanitize JWT claims.
The new claims (userId and role) are added without validation:
userIdis exposed directly which could be sensitive- No validation on role values which could lead to security issues
Add validation and consider using a more secure approach:
- claims.put("userId", user.getId());
- claims.put("role", user.getEnterprise().getRole());
+ claims.put("sub", user.getId().toString()); // Use standard JWT claim
+ if (user.getEnterprise() != null && user.getEnterprise().getRole() != null) {
+ claims.put("role", user.getEnterprise().getRole().name());
+ } else {
+ claims.put("role", "GUEST");
+ }📝 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.
| claims.put("userId", user.getId()); | |
| claims.put("role", user.getEnterprise().getRole()); | |
| claims.put("sub", user.getId().toString()); // Use standard JWT claim | |
| if (user.getEnterprise() != null && user.getEnterprise().getRole() != null) { | |
| claims.put("role", user.getEnterprise().getRole().name()); | |
| } else { | |
| claims.put("role", "GUEST"); | |
| } |
| getStoredAccessToken(): string | null { | ||
| return localStorage.getItem('access_token') ?? ''; | ||
| } |
There was a problem hiding this comment.
Use secure storage for access tokens.
Storing tokens in localStorage makes them vulnerable to XSS attacks.
Consider using more secure alternatives:
- HttpOnly cookies for token storage
- In-memory storage with refresh token pattern
| getUserRole(): UserRole { | ||
| const token = this.getStoredAccessToken(); | ||
| if (!token) { | ||
| return UserRole.ENTERPRISE_OWNER; | ||
| } | ||
| const userObject = this.jwtHelperService.decodeToken(token); | ||
| return 'role' in userObject ? userObject.role : ''; | ||
| } |
There was a problem hiding this comment.
Avoid default role assignment.
Defaulting to ENTERPRISE_OWNER when token is missing could lead to privilege escalation.
Modify to use a safer default:
getUserRole(): UserRole {
const token = this.getStoredAccessToken();
if (!token) {
- return UserRole.ENTERPRISE_OWNER;
+ return UserRole.GUEST;
}
+ try {
const userObject = this.jwtHelperService.decodeToken(token);
return 'role' in userObject ? userObject.role : '';
+ } catch (error) {
+ console.error('Invalid token:', error);
+ return UserRole.GUEST;
+ }
}📝 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.
| getUserRole(): UserRole { | |
| const token = this.getStoredAccessToken(); | |
| if (!token) { | |
| return UserRole.ENTERPRISE_OWNER; | |
| } | |
| const userObject = this.jwtHelperService.decodeToken(token); | |
| return 'role' in userObject ? userObject.role : ''; | |
| } | |
| getUserRole(): UserRole { | |
| const token = this.getStoredAccessToken(); | |
| if (!token) { | |
| return UserRole.GUEST; | |
| } | |
| try { | |
| const userObject = this.jwtHelperService.decodeToken(token); | |
| return 'role' in userObject ? userObject.role : ''; | |
| } catch (error) { | |
| console.error('Invalid token:', error); | |
| return UserRole.GUEST; | |
| } | |
| } |
| getUserId(): UUID | null { | ||
| const token = this.getStoredAccessToken(); | ||
| if (!token) { | ||
| return null; | ||
| } | ||
| const userObject = this.jwtHelperService.decodeToken(token); | ||
| return 'userId' in userObject ? userObject.userId : ''; | ||
| } |
There was a problem hiding this comment.
Add token validation before decoding.
The token is decoded without validating its integrity or expiration.
Add validation before using the token:
getUserId(): UUID | null {
const token = this.getStoredAccessToken();
- if (!token) {
+ if (!token || this.isTokenExpired()) {
return null;
}
+ try {
const userObject = this.jwtHelperService.decodeToken(token);
return 'userId' in userObject ? userObject.userId : '';
+ } catch (error) {
+ console.error('Invalid token:', 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.
| getUserId(): UUID | null { | |
| const token = this.getStoredAccessToken(); | |
| if (!token) { | |
| return null; | |
| } | |
| const userObject = this.jwtHelperService.decodeToken(token); | |
| return 'userId' in userObject ? userObject.userId : ''; | |
| } | |
| getUserId(): UUID | null { | |
| const token = this.getStoredAccessToken(); | |
| if (!token || this.isTokenExpired()) { | |
| return null; | |
| } | |
| try { | |
| const userObject = this.jwtHelperService.decodeToken(token); | |
| return 'userId' in userObject ? userObject.userId : ''; | |
| } catch (error) { | |
| console.error('Invalid token:', error); | |
| return null; | |
| } | |
| } |
| @@ -56,6 +56,8 @@ public Map<String, Object> getCustomClaimsForJwtAuthenticationToken(String email | |||
| .map(UserEnterpriseEntity::getEnterprise) | |||
| .map(UUID::toString) | |||
| .orElse(null)); | |||
| claims.put("userId", user.getId()); | |||
There was a problem hiding this comment.
userId need to config as sub in access token instead of put userId
| @@ -56,6 +56,8 @@ public Map<String, Object> getCustomClaimsForJwtAuthenticationToken(String email | |||
| .map(UserEnterpriseEntity::getEnterprise) | |||
| .map(UUID::toString) | |||
| .orElse(null)); | |||
| claims.put("userId", user.getId()); | |||
| claims.put("role", user.getEnterprise().getRole()); | |||
There was a problem hiding this comment.
authorities already include user roles, why need we put separately?
| @@ -126,7 +130,7 @@ | |||
| <p-accordion [value]="0"> | |||
| @for (tab of tabs; track tab.title) { | |||
| <p-accordion-panel [value]="tab.value"> | |||
| <p-accordion-header class="bg-[#F4FCFD]">{{ | |||
| <p-accordion-header class="bg-[#F4FCFD] text-black">{{ | |||
There was a problem hiding this comment.
although UX/UI isn't the most important to put more effort, but hard code color like #F4FCFD is very hard to maintain later, in addition, use text-black will display fail on dark mode, by the way, create task fix dark mode config with primeNG to fix later for this point
| private getAccessToken(): void { | ||
| this.oidcSecurityService.getAccessToken().subscribe((token: string) => { | ||
| if (token) { | ||
| localStorage.setItem('access_token', token); |
There was a problem hiding this comment.
never manually put secret to localStorage, we need to handle access token in application lifecycle instead
This is test code need review to it is correct
Summary by CodeRabbit
New Features
Style