Conversation
…in some dependencies, added needed jakarta libraries
- adjusted the code to use slf4j imports
… logic, and use the new CsrfGuard methods - verifyPageToken() - verifySessionToken() - rotateTokens()
…uts2.ActionSupport since it's deprecated in struts 6.7.0
…ts to jakarta version
- upgrade hibernate version from 5.6.15 to 6.4.10 - update DAOs' methods using new methods in Hibernate 6
- fix setAllowMTOM() doesn't exist error by adding config to JaxWsProxyFactoryBean
- Changed the oauth library to ScribeJava, refactored the OscarRequestTokenService and OscarRequestTokenHandler classes
- Added Auth.java and ServletUtils.java to the customize directory - Changed javax to jakarta
- direct dependencies calls on the same transitive dependencies which causes duplicates and conflicts in versions - removed/excluded those dependencies
Struts 6 and Jakarta Migration
- replace jakarta.servlet.ServletContext with javax.servlet.ServletContext
…s API and methods
Reviewer's Guide by SourceryThis pull request includes several updates: the removal of the PHR module, dependency cleanup in pom.xml, axis2 upgrade, removal of myoscar_client_utils, replacement of org.apache.cxf.rs.security.oauth with scribejava, replacement of joda-time with java.time, replacement of javax.servlet with jakarta.servlet, and replacement of com.onelogin:java-saml with com.onelogin:java-saml:customize. The changes primarily involve dependency management, library upgrades, and code refactoring to adapt to the new libraries. Updated class diagram for OscarOAuthDataProviderclassDiagram
class OscarOAuthDataProvider {
-Logger logger
-ServiceRequestTokenDao serviceRequestTokenDao
-ServiceAccessTokenDao serviceAccessTokenDao
-ServiceClientDao serviceClientDao
-OAuth1Client oauthClient
+initializeOAuthClient(String apiKey, String apiSecret, String callbackUrl) void
+getClient(String clientId) ServiceClient
+createRequestToken(String clientId, String callbackUrl, List~String~ scopes) OAuth1RequestToken
+getRequestToken(String tokenId) OAuth1RequestToken
+finalizeAuthorization(String tokenId) String
+createAccessToken(String tokenId, String verifier) OAuth1AccessToken
+getAccessToken(String tokenId) OAuth1AccessToken
+removeToken(String tokenId) void
+sendSignedRequest(OAuth1AccessToken accessToken, String url) String
}
class OAuth1Client {
+OAuth1Client(String apiKey, String apiSecret, String callbackUrl)
+getAuthorizationUrl(OAuth1RequestToken requestToken) String
+getAccessToken(OAuth1RequestToken requestToken, String verifier) OAuth1AccessToken
+sendSignedRequest(OAuth1AccessToken accessToken, String url) String
}
OscarOAuthDataProvider -- OAuth1Client : uses
note for OscarOAuthDataProvider "Replaced CXF OAuth with ScribeJava OAuth1.0a"
Updated class diagram for AbstractServiceImplclassDiagram
class AbstractServiceImpl {
-OAuth10aService oauthService
+setOAuthService(OAuth10aService oauthService) void
+getHttpServletRequest() HttpServletRequest
+getOAuthAccessToken() OAuth1AccessToken
+getCurrentProvider() Provider
+getLoggedInInfo() LoggedInInfo
}
note for AbstractServiceImpl "Replaced CXF OAuth with ScribeJava OAuth1.0a"
Updated class diagram for StayclassDiagram
class Stay {
-Instant admissionInstant
-Instant dischargeInstant
-Duration stayDuration
+Stay(Date admission, Date discharge, Date start, Date end)
+getStayDuration() Duration
+getAdmissionInstant() Instant
+getDischargeInstant() Instant
}
note for Stay "Replaced Joda-Time with java.time"
Updated class diagram for AgeCalculatorclassDiagram
class AgeCalculator {
+calculateAge(Calendar birthDate) Age
}
class Age {
-int days
-int months
-int years
}
AgeCalculator -- Age : returns
note for AgeCalculator "Replaced Joda-Time with java.time"
Updated class diagram for PreventionclassDiagram
class Prevention {
+isHistorical(int days) boolean
}
note for Prevention "Replaced Joda-Time with java.time"
Updated class diagram for PersonaServiceclassDiagram
class PersonaService {
}
PersonaService -- AbstractServiceImpl : extends
note for PersonaService "Removed MyOscarUtils.isMyOscarEnabled"
Updated class diagram for LoginFilterclassDiagram
class LoginFilter {
}
note for LoginFilter "Replaced javax.servlet with jakarta.servlet"
Updated class diagram for AuthclassDiagram
class Auth {
-Instant sessionExpiration
}
note for Auth "Replaced Joda-Time with java.time"
Updated class diagram for SamlMessageFactoryclassDiagram
class SamlMessageFactory {
}
note for SamlMessageFactory "Added SamlMessageFactory interface"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kateyang1998 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using managed dependencies in your pom.xml to centralize dependency version management and ensure consistency across modules.
- It looks like you're using a lot of exclusions; consider if there's a better way to manage dependencies to avoid needing so many.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| // We need to Remove the saml:EncryptedAssertion Node | ||
| NodeList AssertionDataNodes = Util.query(dom, "/samlp:Response/saml:EncryptedAssertion/saml:Assertion"); |
There was a problem hiding this comment.
issue (bug_risk): Potential bug: wrong NodeList variable used for assertion check.
Immediately after fetching AssertionDataNodes, the code checks the length of encryptedDataNodes instead of AssertionDataNodes. This appears to be a mistake and may lead to unexpected behavior if the assertion element is missing.
There was a problem hiding this comment.
@kateyang1998 ? Did you miss following up with sourcery on this?
There was a problem hiding this comment.
This is the original code from OneLogin source code, I didn't change anything of that instead of the namespaces from javax to jakarta to make it compatible. We can make it open to be discussed to see if it's better to take sourcery's suggestion?
There was a problem hiding this comment.
Based on the conversation, I'll provide a constructive response:
The Sourcery review highlights a potential issue in the code where the wrong NodeList is being used for assertion checking. While the code is originally from the OneLogin source, it's still worth carefully reviewing the suggestion.
Here's a detailed analysis:
-
The current code checks the length of
encryptedDataNodesafter fetchingAssertionDataNodes, which could indeed lead to unexpected behavior. -
A potential fix would be to modify the code to check the length of
AssertionDataNodesinstead:
NodeList encryptedDataNodes = Util.query(dom, "/samlp:Response/saml:EncryptedAssertion/xenc:EncryptedData");
NodeList AssertionDataNodes = Util.query(dom, "/samlp:Response/saml:EncryptedAssertion/saml:Assertion");
if (AssertionDataNodes.getLength() == 0) {
throw new ValidationError("No decrypted Assertion found", ValidationError.MISSING_ASSERTION_ELEMENT);
}- This change would ensure that the code checks for the presence of the decrypted Assertion element, which seems to be the intent of the original code.
I recommend discussing this suggestion with the team. Even though the code is from an existing library, it's always good to review and potentially improve error handling and validation logic.
Would you like me to elaborate on the potential implications of this code change or provide more context about the assertion decryption process?
yingbull
left a comment
yingbull
left a comment
There was a problem hiding this comment.
I am going to let this go forward but be sure that we have followed up to removed those other MyOSCAR items you added (and any others) and make sure an issue ticket tracks this till done. Also, remember to not add commented out code unless there is a clear statement why this must be commented out and under what circumstances it would be uncommented/when.
|
This needs to be updated / merge issue. Please review @kateyang1998 |
Sure, I will look into it on Monday morning. |
|
@kateyang1998 This is also out of date, can you update what should be reviewed for merge? |
|
@yingbull I just updated the PR description, right now is updated to-date. The 20 commits are the latest changes we made to this branch. (struts6-upg) |
| @Autowired | ||
| OAuth1Client oauthClient; | ||
|
|
||
| protected SecurityContext getSecurityContext() { |
There was a problem hiding this comment.
Can you help me understand why this is removed, @kateyang1998?
| //private ServiceClientDao serviceClientDao = SpringUtils.getBean(ServiceClientDao.class); | ||
|
|
||
| @Override | ||
| public Client getClient(String clientId) throws OAuthServiceException { |
There was a problem hiding this comment.
Can you explain this removal, @kateyang1998 ?
| */ | ||
| public ServiceClient getClient(String clientId) { | ||
| logger.debug("getClient() called"); | ||
| ServiceClient sc = serviceClientDao.findByKey(clientId); |
There was a problem hiding this comment.
This is a good case for adding docs to code as we do it as it helps with review... can you justify/explain this change @kateyang1998 ?
| StringBuilder sb = new StringBuilder(); | ||
| List<OAuthPermission> perms = new ArrayList<>(); | ||
| for (String scope : reg.getScopes()) { | ||
| OAuthPermission p = new OAuthPermission(scope, scope); |
There was a problem hiding this comment.
Are permissions being kept in how we are doing OAuth @kateyang1998
| long issuedAt = System.currentTimeMillis() / 1000; | ||
| AccessToken accessToken = new AccessToken(client, accessTokenString, | ||
| tokenSecretString, 3600, issuedAt); | ||
| UserSubject subject = new UserSubject(srt.getProviderNo(), new ArrayList<>()); |
There was a problem hiding this comment.
Where are we capturing this level of detail now @kateyang1998 ?
| public OAuth1AccessToken getAccessToken(String tokenId) { | ||
| ServiceAccessToken sat = serviceAccessTokenDao.findByTokenId(tokenId); | ||
| if (sat == null) { | ||
| throw new OAuthServiceException("Invalid access token."); |
| ResourceBundle props = ResourceBundle.getBundle("oscarResources", request.getLocale()); | ||
| try { | ||
| MessageTransfer3 messageTransfer = MyOscarMessagesHelper.readMessage(request.getSession(), | ||
| MessageTransfer3 messageTransfer = readMessage(request.getSession(), |
There was a problem hiding this comment.
Why this change? If we aren't going myoscarmsgs, maybe this whole block goes away?
There was a problem hiding this comment.
This is because of myoscar dependency removal. After we removed the dependency, we had to do this refactor (The refactor of myoscar is not finished yet, we stopped and then switch to the 404/500 error fix in coyote)
| String dateStr = ""; | ||
|
|
||
| if (request.getParameter("remyoscarmsg") != null) { | ||
| MessageTransfer3 messageTransferOrig = MyOscarMessagesHelper.readMessage(request.getSession(), |
|
|
||
| import org.apache.commons.fileupload2.core.FileItem; | ||
| import org.apache.commons.fileupload2.core.FileUploadException; | ||
| import org.apache.commons.fileupload.servlet.ServletFileUpload; |
There was a problem hiding this comment.
This is the relpaced import:
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload;
| return null; | ||
| } | ||
| bean.clearPairPHRMed(); | ||
| //bean.clearPairPHRMed(); |
There was a problem hiding this comment.
if not needed, let's remove, but feels like more code might be involved?
There was a problem hiding this comment.
Removed since it's calling the PHR method, we've already deleted PHR modules; this line of code caused an error.
|
@yingbull for the comments I haven't replied - Q2: Where are we capturing this level of detail now? Q3: What is token lifetime in this new setup? Critical Missing Pieces in Refactored Code Recommendations |
|
@yingbull I will start to look into this branch today, please let me know if there is any other issue you want me to work on first, or I will just focus on this PR now :-) |
|
No, work on open issues there are many |
|
@yingbull Sure, will pick from the ticket board. |
|
@sebastian-j-ibanez this PR might have some of the jakarta related changes that you might be able to point claude at to summarize as part of moving them to experimental/next update. |
|
To generate Unit Tests for this PR, please click here. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
|
The majority of changes in this PR have either already made their way into the code base or aren't directly useful. However there were several classes that were migrated to Jakarta that might be useful.
|
|
This PR is being closed because it is stale. A lot of the work has been in the following PRs: PHR / MyOscar removal:
OAuth: CXF → ScribeJava migration:
Axis2 upgrade (1.8.0 → 1.8.2):
POM cleanup:
The remaining work are epics that are still in the backlog. This includes things like the jakarta migration and struts 6 upgrade. |
5 participants
What included in this PR for reviewing:
PHRmodule and fixed relevant codes.axis2from version 1.8.0 to 2.0.0-SNAPSHOT.myoscar_client_utilslibrary, fixed some compilation errors caused by removal.org.apache.cxf.rs.security.oauthtoscribejava, refactored the relevant classes to make them compatible withscribejava oauth1.0aSummary by Sourcery
This pull request updates dependencies, removes unused modules, and refactors OAuth implementation to improve compatibility and maintainability.
Enhancements:
axis2from version 1.8.0 to 2.0.0-SNAPSHOT to be compatible with Jakarta EE.scribejavainstead oforg.apache.cxf.rs.security.oauthfor OAuth1.0a support.antdependency to version 1.10.15.cxfdependencies to version 4.1.0.jaxws-ridependencies to version 4.0.3.struts2-core,struts2-spring-plugin, andstruts2-convention-plugindependencies to version 6.7.0.Build:
Chores:
PHRmodule and update the relevant code.myoscar_client_utilslibrary and address any compilation errors resulting from its removal.