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.

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
Merged

Conversation

RomanMagnoli
Copy link
Contributor

No description provided.

@RomanMagnoli RomanMagnoli added the bug Something isn't working label Dec 4, 2024
@RomanMagnoli RomanMagnoli self-assigned this Dec 4, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 0.00% Coverage (0.00% Estimated after merge)
  • Duplications 60.00% Duplicated Code (13.20% Estimated after merge)

Project ID: etendosoftware_etendo_core_AYOKvQCAuJ79WHyLB97g

View in SonarQube

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())

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());

@RomanMagnoli RomanMagnoli merged commit 7f599cf into develop Dec 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants