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

Fix for flaky test AuthenticationTokenTest.equals #1338

Closed
wants to merge 3 commits into from

Conversation

219sansim
Copy link

Overview
The AuthenticationTokenTest.Equals fails because the token generated is not the same everytime. The JSONObject.toString() gives a non-deterministic output because because JSONObject values are unordered. The solution is to sort the key-value pairs of jsonHeader and jsonPayload before converting it to their string representation. I had to modify XsuaaTokenCompTest.java as it was affected by this change and had a failing test.
Command to reproduce the failure:
mvn -pl spring-security edu.illinois:nondex-maven-plugin:2.1.7-SNAPSHOT:nondex -Dtest=com.sap.cloud.security.spring.token.authentication.AuthenticationTokenTest#equals
Error Output:
The test fails on 3/5 iterations of the NonDex tool

 Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   AuthenticationTokenTest.equals:40 expected: <AuthenticationToken [Principal=Jwt header
        {"alg":"RS256","kid":"default-kid-ias"}
Jwt payload
        {"exp":6974056800,"aud":"theClientId","scim_id":"the-user-id","app_tid":"the-app-tid","zone_uuid":"the-zone-id","user_uuid":"the-user-id","azp":"theClientId","cid":"theClientId"}
, Credentials=[PROTECTED], Authenticated=true, Details=null, Granted Authorities=[]]> but was: <AuthenticationToken [Principal=Jwt header
        {"kid":"default-kid-ias","alg":"RS256"}
Jwt payload
        {"aud":"theClientId","zone_uuid":"the-zone-id","exp":6974056800,"azp":"theClientId","cid":"theClientId","app_tid":"the-app-tid","scim_id":"the-user-id","user_uuid":"the-user-id"}
, Credentials=[PROTECTED], Authenticated=true, Details=null, Granted Authorities=[]]>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

Reason:
The org.json.JSONObject class uses Maps as the data structure for the JSONObject. And, according to HashMap documentation,
"This class makes no guarantees as to the order of the map; in particular, it does not guarantee that the order will remain constant over time."
This causes the jsonHeader.toString() and jsonPayload.toString() to give a non-deterministic output.
About Flaky Tests:
Flaky tests are tests in software development that produce inconsistent or unreliable results, which can lead to non-deterministic outcomes of the test case, fixing them is important to both reliable testing and fixing vulnerabilities in the code.

Copy link

cla-assistant bot commented Nov 6, 2023

CLA assistant check
All committers have signed the CLA.

//Ensure keys and values are correctly formatted as JSON strings
JSONObject jsonObject = new JSONObject();
List<String> headerlist = new ArrayList<>();
jsonHeader.keySet().forEach(key -> headerlist.add("\"" + key.toString() + "\":" + jsonObject.valueToString(jsonHeader.get(key))));

Check notice

Code scanning / CodeQL

Useless toString on String Note

Redundant call to 'toString' on a String object.
Collections.sort(headerlist);
String header = "{" + String.join(", ", headerlist) + "}";
List<String> payloadlist = new ArrayList<>();
jsonPayload.keySet().forEach(key -> payloadlist.add("\"" + key.toString() + "\":" + jsonObject.valueToString(jsonPayload.get(key))));

Check notice

Code scanning / CodeQL

Useless toString on String Note

Redundant call to 'toString' on a String object.
@liga-oz
Copy link
Contributor

liga-oz commented Jan 5, 2024

follow up PR was merged #1403

@liga-oz liga-oz closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants