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

Feature ETP-801: Fix claims generation #550

Merged
merged 3 commits into from
Dec 6, 2024
+10 −10
Merged

Feature ETP-801: Fix claims generation #550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
456a017
Feature ETP-801: Fix claims generation
RomanMagnoli Dec 4, 2024
673757d
Merge branch 'develop' into feature/ETP-801
valeg-etendo Dec 6, 2024
eb83697
Merge branch 'develop' into feature/ETP-801
RomanMagnoli Dec 6, 2024
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
8 changes: 4 additions & 4 deletions ...com.smf.securewebservices/src/com/smf/securewebservices/utils/SecureWebServicesUtils.java
View file
Open in desktop
RomanMagnoli marked this conversation as resolved.
Show resolved Hide resolved
RomanMagnoli marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

GPT Review for SecureWebServicesUtils.java

Review

  • Estimated effort to review [1-5]:
    1, because the changes are straightforward and involve renaming claims in a JWT builder method, which is simple to review.
  • Score: 95

Code feedback

  • File:
    modules_core/com.smf.securewebservices/src/com/smf/securewebservices/utils/SecureWebServicesUtils.java
  • Language:
    java
  • Suggestion:
    Consider adding a comment to explain the rationale behind renaming the JWT claims. This can help future developers understand the context and reason for these changes. [medium]
  • Label:
    documentation
  • Existing code:
.withClaim("user", user.getId())
.withClaim("client", selectedRole.getClient().getId())
.withClaim("role", selectedRole.getId())
.withClaim("organization", selectedOrg.getId())
  • Improved code:
// Renamed claims for better clarity and consistency
.withClaim("user", user.getId())
.withClaim("client", selectedRole.getClient().getId())
.withClaim("role", selectedRole.getId())
.withClaim("organization", selectedOrg.getId())

Original file line number Diff line number Diff line change
Expand Up @@ -691,10 +691,10 @@ private static Builder getJwtBuilder(User user, Role selectedRole, Organization
return JWT.create()
.withIssuer("sws")
.withAudience("sws")
.withClaim("ad_user_id", user.getId())
.withClaim("ad_client_id", selectedRole.getClient().getId())
.withClaim("ad_role_id", selectedRole.getId())
.withClaim("ad_org_id", selectedOrg.getId())
.withClaim("user", user.getId())
.withClaim("client", selectedRole.getClient().getId())
.withClaim("role", selectedRole.getId())
.withClaim("organization", selectedOrg.getId())
.withClaim("warehouse", selectedWarehouse.getId())
.withIssuedAt(new Date());
}
Expand Down
12 changes: 6 additions & 6 deletions src-test/src/com/smf/securewebservices/utils/SecureWebServicesUtilsTest.java
View file
Open in desktop
RomanMagnoli marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

GPT Review for SecureWebServicesUtilsTest.java

Review

  • Estimated effort to review [1-5]:
    2, because the changes are straightforward and involve updating the claim keys in test assertions, which is a simple and clear modification.
  • Score: 95

Code feedback

  • File:
    src-test/src/com/smf/securewebservices/utils/SecureWebServicesUtilsTest.java
  • Language:
    java
  • Suggestion:
    Consider adding a comment or updating documentation to reflect the change in claim keys from ad_user_id, ad_role_id, and ad_org_id to user, role, and organization. This will help maintain clarity for future developers who may work with this code. [medium]
  • Label:
    documentation
  • Existing code:
assertEquals(user.getId(), decodedToken.getClaim("user").asString());
assertEquals(role.getId(), decodedToken.getClaim("role").asString());
assertEquals(org.getId(), decodedToken.getClaim("organization").asString());
  • Improved code:
// Updated claim keys to match new token structure
assertEquals(user.getId(), decodedToken.getClaim("user").asString());
assertEquals(role.getId(), decodedToken.getClaim("role").asString());
assertEquals(org.getId(), decodedToken.getClaim("organization").asString());

Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,9 @@ public void testGenerateAndDecodeTokenWithHS256Algorithm() throws Exception {
String token = SecureWebServicesUtils.generateToken(user, role, org, warehouse);
DecodedJWT decodedToken = SecureWebServicesUtils.decodeToken(token);

assertEquals(user.getId(), decodedToken.getClaim("ad_user_id").asString());
assertEquals(role.getId(), decodedToken.getClaim("ad_role_id").asString());
assertEquals(org.getId(), decodedToken.getClaim("ad_org_id").asString());
assertEquals(user.getId(), decodedToken.getClaim("user").asString());
assertEquals(role.getId(), decodedToken.getClaim("role").asString());
assertEquals(org.getId(), decodedToken.getClaim("organization").asString());
}

/**
Expand All @@ -121,9 +121,9 @@ public void testGenerateAndDecodeTokenWithES256Algorithm() throws Exception {
String token = SecureWebServicesUtils.generateToken(user, role, org, warehouse);
DecodedJWT decodedToken = SecureWebServicesUtils.decodeToken(token);

assertEquals(user.getId(), decodedToken.getClaim("ad_user_id").asString());
assertEquals(role.getId(), decodedToken.getClaim("ad_role_id").asString());
assertEquals(org.getId(), decodedToken.getClaim("ad_org_id").asString());
assertEquals(user.getId(), decodedToken.getClaim("user").asString());
assertEquals(role.getId(), decodedToken.getClaim("role").asString());
assertEquals(org.getId(), decodedToken.getClaim("organization").asString());
}

/**
Expand Down
Loading