Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It is correct when decode token ? #137

Closed
wants to merge 1 commit into from
Closed

It is correct when decode token ? #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions sep490-frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions sep490-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"@angular/platform-browser": "^18.2.0",
"@angular/platform-browser-dynamic": "^18.2.0",
"@angular/router": "^18.2.0",
"@auth0/angular-jwt": "^5.2.0",
"@ngx-translate/core": "^16.0.4",
"@ngx-translate/http-loader": "^16.0.1",
"@primeng/themes": "^18.0.2",
Expand Down
43 changes: 42 additions & 1 deletion sep490-frontend/src/app/modules/core/services/application.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import {
import {SubscriptionAwareComponent} from '../subscription-aware.component';
import {BehaviorSubject, Observable, filter, map, switchMap} from 'rxjs';
import {JwtPayload} from 'jwt-decode';
import {JwtHelperService} from '@auth0/angular-jwt';
import {UserRole} from '../../authorization/enums/role-names';
import {UUID} from '../../../../types/uuid';

interface UserInfoEmailScope {
email: string;
Expand Down Expand Up @@ -50,7 +53,7 @@ export class ApplicationService
implements OnDestroy
{
public readonly userInfoSubject;

jwtHelperService = new JwtHelperService();
constructor(private readonly oidcSecurityService: OidcSecurityService) {
super();
this.userInfoSubject = new BehaviorSubject<UserInfoData | null>(null);
Expand Down Expand Up @@ -79,6 +82,7 @@ export class ApplicationService

login(): void {
this.oidcSecurityService.authorize();
this.getAccessToken();
}

isMobile(): boolean {
Expand All @@ -97,9 +101,46 @@ export class ApplicationService
);
}

getStoredAccessToken(): string | null {
return localStorage.getItem('access_token') ?? '';
}
Comment on lines +104 to +106
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use secure storage for access tokens.

Storing tokens in localStorage makes them vulnerable to XSS attacks.

Consider using more secure alternatives:

  1. HttpOnly cookies for token storage
  2. In-memory storage with refresh token pattern

getUserId(): UUID | null {
const token = this.getStoredAccessToken();
if (!token) {
return null;
}
const userObject = this.jwtHelperService.decodeToken(token);
return 'userId' in userObject ? userObject.userId : '';
}
Comment on lines +107 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}
}


getUserRole(): UserRole {
const token = this.getStoredAccessToken();
if (!token) {
return UserRole.ENTERPRISE_OWNER;
}
const userObject = this.jwtHelperService.decodeToken(token);
return 'role' in userObject ? userObject.role : '';
}
Comment on lines +116 to +123
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}
}


private postLogout(): void {
sessionStorage.clear();
localStorage.clear();
// localStorage.removeItem('access_token');
this.userInfoSubject.next(null);
}

private getAccessToken(): void {
this.oidcSecurityService.getAccessToken().subscribe((token: string) => {
if (token) {
localStorage.setItem('access_token', token);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct way to store access_token.
By the way we already store it in session/cookies

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never manually put secret to localStorage, we need to handle access token in application lifecycle instead

}
});
}

private isTokenExpired(): boolean {
if (this.getStoredAccessToken() == null) {
return false;
}
return this.jwtHelperService.isTokenExpired(this.getStoredAccessToken());
}
}
14 changes: 9 additions & 5 deletions sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
<img
ngSrc="assets/images/payment-method/momo.svg"
alt="Logo"
width="20"
height="20"
width="30"
height="30"
priority
/>
<img
Expand All @@ -21,27 +21,31 @@
width="30"
height="30"
priority
class="border"
/>
<img
ngSrc="assets/images/payment-method/napas.svg"
alt="Logo"
width="30"
height="30"
priority
class="border"
/>
<img
ngSrc="assets/images/payment-method/visa.svg"
alt="Logo"
width="30"
height="30"
priority
class="border"
/>
<img
ngSrc="assets/images/payment-method/master-card.svg"
alt="Logo"
width="30"
height="30"
priority
class="border"
/>
</div>
</div>
Expand All @@ -68,12 +72,12 @@
>
<div class="flex flex-col gap-16">
<div
class="bg-white rounded-md border-[#E5E7EB] border px-4 py-2 mt-8 mx-6 flex justify-center items-center lg:justify-start"
class="bg-white rounded-md border-[#E5E7EB] border px-4 py-2 mt-8 mx-6 flex justify-center items-center lg:justify-start text-black"
>
<p>{{ "purchaseCredit.selectCreditPackage" | translate }}</p>
</div>
<div
class="flex lg:flex-row flex-col gap-16 items-center justify-center mx-6"
class="flex lg:flex-row flex-col gap-16 items-center justify-center mx-6 text-black"
>
@for (package of creditPackages; track package.id) {
<card-template
Expand Down Expand Up @@ -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">{{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tab.title | translate
}}</p-accordion-header>
<p-accordion-content>
Expand Down
2 changes: 2 additions & 0 deletions sep490-idp/src/main/java/green_buildings/idp/service/impl/UserInfoService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ public Map<String, Object> getCustomClaimsForJwtAuthenticationToken(String email
.map(UserEnterpriseEntity::getEnterprise)
.map(UUID::toString)
.orElse(null));
claims.put("userId", user.getId());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userId need to config as sub in access token instead of put userId

claims.put("role", user.getEnterprise().getRole());
Comment on lines +59 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate and sanitize JWT claims.

The new claims (userId and role) are added without validation:

  1. userId is exposed directly which could be sensitive
  2. 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.

Suggested change
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");
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorities already include user roles, why need we put separately?

claims.put("permissions", buildingPermissions);
return claims;
}
Expand Down