Remove Invalid Tokens from cache when found #1333

saquino0827 · 2024-09-13T18:53:29Z

Description

Uncache our TI token if an InvalidToken is caught and found.

Issue

Checklist

I have added tests to cover my changes
I have added logging where useful (with appropriate log level)
I have added JavaDocs where required
I have updated the documentation accordingly

Note: You may remove items that are not applicable

Co-authored-by: jcrichlake <[email protected]> Co-authored-by: pluckyswan <[email protected]> Co-authored-by: halprin <[email protected]>

Co-authored-by: Sylvie <[email protected]>

jcrichlake · 2024-09-16T14:16:35Z

app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java

    @Inject private Secrets secrets;
    @Inject private Logger logger;

+    String ourPublicKey = "trusted-intermediary-public-key-" + ApplicationContext.getEnvironment();


Do we want this public key name hardcoded here? And if so do we need a #pragma allow secret

It previously existed, but I moved it to a higher level so it's accessible in tests. I can add the #pragma allow secrets if we want but I also don't think it got flagged before as a secret.

luis-pabon-tf · 2024-09-16T14:39:57Z

app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java

@@ -21,10 +21,12 @@ public class AuthRequestValidator {
    private static final AuthRequestValidator INSTANCE = new AuthRequestValidator();

    @Inject private AuthEngine jwtEngine;
-    @Inject private Cache keyCache;
+    @Inject Cache keyCache;


Seems this is the only inject we actually use in this class. Would a small doc be good?

Are you sure? I see jwtEngine, secrets, and logger being used below.

I added some extra comments on the class. I think all the injects are used unless I'm misunderstanding what you mean.

…ov/trusted-intermediary into story/1326/no-cache-bad-key

sonarqubecloud · 2024-09-16T15:26:32Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

pluckyswan

Checklist needs to be updated (remove items or check them off)

somesylvie · 2024-10-15T21:50:32Z

/review

github-actions · 2024-10-15T21:51:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 1326 - Fully compliant Fully compliant requirements: Update TI service's caching system to remove cached token when an Invalid Token is caught.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Token Removal Logic Ensure that the logic for removing tokens from cache upon catching an InvalidTokenException is correctly implemented and tested.

github-actions · 2024-10-15T21:51:20Z

app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java

@@ -49,12 +52,12 @@ public boolean isValidAuthenticatedRequest(DomainRequest request)
            return true;
        } catch (InvalidTokenException e) {
            logger.logError("Invalid bearer token!", e);
+            this.keyCache.remove(ourPublicKey);


Consider adding a log statement before removing the key from the cache to aid in debugging and operational monitoring. [important]

somesylvie · 2024-10-15T21:53:54Z

/describe

github-actions · 2024-10-15T21:54:41Z

Title

Remove Invalid Tokens from cache when found

User description

Description

Uncache our TI token if an InvalidToken is caught and found.

Issue

#1326

Checklist

I have added tests to cover my changes
I have added logging where useful (with appropriate log level)
I have added JavaDocs where required
I have updated the documentation accordingly

Note: You may remove items that are not applicable

PR Type

bug_fix, enhancement

Description

Implemented functionality to remove invalid tokens from the cache in AuthRequestValidator.
Added unit tests to verify that invalid tokens are correctly removed from the cache.
Introduced a remove method in KeyCache and updated the Cache interface to support this functionality.

Changes walkthrough 📝

Relevant files

Tests

AuthRequestValidatorTest.groovy `Update Unit Tests for AuthRequestValidator to Reflect Cache Changes` app/src/test/groovy/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidatorTest.groovy Modified unit test to use singleton instance of `KeyCache` instead of a mock. Updated test to verify that invalid tokens are removed from the cache.	+2/-7
KeyCacheTest.groovy `Add Unit Test for Key Removal in KeyCache` shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/external/inmemory/KeyCacheTest.groovy Added a new test to verify the functionality of the `remove` method in `KeyCache`.	+14/-0

Enhancement

AuthRequestValidator.java `Implement Removal of Invalid Tokens from Cache in AuthRequestValidator` app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java Added removal of invalid tokens from cache in `isValidAuthenticatedRequest` method. Made `ourPublicKey` a class variable for better accessibility.	+6/-3
KeyCache.java `Implement Key Removal Method in KeyCache` shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/inmemory/KeyCache.java Implemented the `remove` method to support deletion of keys from the cache.	+5/-0
Cache.java `Extend Cache Interface with Remove Method` shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/Cache.java Added the `remove` method to the `Cache` interface.	+2/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

somesylvie · 2024-10-15T21:55:47Z

/improve

github-actions · 2024-10-15T21:56:21Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add validation for the `ourPublicKey` variable before cache operations Validate the `ourPublicKey` variable to ensure it is not null or empty before using it to access or remove keys from the cache. app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java [55] -this.keyCache.remove(ourPublicKey); +if (ourPublicKey != null && !ourPublicKey.isEmpty()) { + this.keyCache.remove(ourPublicKey); +} Suggestion importance[1-10]: 8 Why: Validating the `ourPublicKey` before using it in cache operations prevents potential runtime errors or security issues, making this a critical enhancement.	8
Enhancement	Add logging for the removal of public keys from the cache Ensure that the removal of the public key from the cache is logged for better traceability and debugging. app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java [55] this.keyCache.remove(ourPublicKey); +logger.logDebug("Public key removed from cache."); Suggestion importance[1-10]: 7 Why: Adding logging for key removal operations enhances traceability and debugging, which is crucial for maintaining security-related features.	7
Best practice	Implement exception handling for cache key removal Consider implementing a mechanism to handle potential exceptions when removing keys from the cache to avoid application crashes. app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java [55] -this.keyCache.remove(ourPublicKey); +try { + this.keyCache.remove(ourPublicKey); +} catch (Exception e) { + logger.logError("Failed to remove public key from cache", e); +} Suggestion importance[1-10]: 6 Why: Implementing exception handling for cache operations is a best practice that ensures the application's robustness, although the current context does not indicate frequent issues with this operation.	6
Maintainability	Refactor the construction of `ourPublicKey` into a separate method Refactor the retrieval of the environment-specific public key into a separate method to enhance modularity and readability. app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthRequestValidator.java [29] -String ourPublicKey = "trusted-intermediary-public-key-" + ApplicationContext.getEnvironment(); +String ourPublicKey = getEnvironmentSpecificPublicKey(); +private String getEnvironmentSpecificPublicKey() { + return "trusted-intermediary-public-key-" + ApplicationContext.getEnvironment(); +} + Suggestion importance[1-10]: 5 Why: Refactoring the public key retrieval into a separate method improves modularity and readability, although it's a relatively minor maintainability improvement.	5

saquino0827 and others added 4 commits September 13, 2024 13:46

Remove Invalid Tokens from cache when found

3e064e4

Co-authored-by: jcrichlake <[email protected]> Co-authored-by: pluckyswan <[email protected]> Co-authored-by: halprin <[email protected]>

Updated the public key into a class variable and fixed unit tests

c6ffbb0

Co-authored-by: Sylvie <[email protected]>

Remove excess blank newline

813353e

Co-authored-by: Sylvie <[email protected]>

Merge branch 'main' into story/1326/no-cache-bad-key

d6af118

jcrichlake reviewed Sep 16, 2024

View reviewed changes

jcrichlake approved these changes Sep 16, 2024

View reviewed changes

luis-pabon-tf reviewed Sep 16, 2024

View reviewed changes

saquino0827 and others added 3 commits September 16, 2024 10:21

Add comments for caching

d7abdd9

Merge branch 'story/1326/no-cache-bad-key' of https://github.com/CDCg…

8fba3bc

…ov/trusted-intermediary into story/1326/no-cache-bad-key

Merge branch 'main' into story/1326/no-cache-bad-key

1e26b25

halprin approved these changes Sep 16, 2024

View reviewed changes

pluckyswan reviewed Sep 16, 2024

View reviewed changes

saquino0827 merged commit 2ef6e94 into main Sep 16, 2024
16 checks passed

saquino0827 deleted the story/1326/no-cache-bad-key branch September 16, 2024 15:53

github-actions bot reviewed Oct 15, 2024

View reviewed changes

somesylvie mentioned this pull request Oct 15, 2024

Adding AI to our PR review process #1357

Closed

15 tasks

saquino0827 mentioned this pull request Oct 16, 2024

SFTP Ingestion Service Setup with PR Agent #1431

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Invalid Tokens from cache when found #1333

Remove Invalid Tokens from cache when found #1333

saquino0827 commented Sep 13, 2024 •

edited by luis-pabon-tf

Loading

jcrichlake Sep 16, 2024

saquino0827 Sep 16, 2024

jcrichlake Sep 16, 2024

luis-pabon-tf Sep 16, 2024

halprin Sep 16, 2024

saquino0827 Sep 16, 2024

sonarqubecloud bot commented Sep 16, 2024

pluckyswan left a comment

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

github-actions bot Oct 15, 2024

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

Remove Invalid Tokens from cache when found #1333

Remove Invalid Tokens from cache when found #1333

Conversation

saquino0827 commented Sep 13, 2024 • edited by luis-pabon-tf Loading

Description

Issue

Checklist

jcrichlake Sep 16, 2024

Choose a reason for hiding this comment

saquino0827 Sep 16, 2024

Choose a reason for hiding this comment

jcrichlake Sep 16, 2024

Choose a reason for hiding this comment

luis-pabon-tf Sep 16, 2024

Choose a reason for hiding this comment

halprin Sep 16, 2024

Choose a reason for hiding this comment

saquino0827 Sep 16, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 16, 2024

Quality Gate passed

pluckyswan left a comment

Choose a reason for hiding this comment

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

PR Reviewer Guide 🔍

github-actions bot Oct 15, 2024

Choose a reason for hiding this comment

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

Title

User description

Description

Issue

Checklist

PR Type

Description

Changes walkthrough 📝

somesylvie commented Oct 15, 2024

github-actions bot commented Oct 15, 2024 • edited Loading

PR Code Suggestions ✨

saquino0827 commented Sep 13, 2024 •

edited by luis-pabon-tf

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading