-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove Invalid Tokens from cache when found #1333
Changes from 4 commits
3e064e4
c6ffbb0
813353e
d6af118
d7abdd9
8fba3bc
1e26b25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
@Inject private Secrets secrets; | ||
@Inject private Logger logger; | ||
|
||
String ourPublicKey = "trusted-intermediary-public-key-" + ApplicationContext.getEnvironment(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want this public key name hardcoded here? And if so do we need a #pragma allow secret There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool |
||
|
||
private AuthRequestValidator() {} | ||
|
||
public static AuthRequestValidator getInstance() { | ||
|
@@ -49,12 +51,12 @@ public boolean isValidAuthenticatedRequest(DomainRequest request) | |
return true; | ||
} catch (InvalidTokenException e) { | ||
logger.logError("Invalid bearer token!", e); | ||
this.keyCache.remove(ourPublicKey); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a log statement before removing the key from the cache to aid in debugging and operational monitoring. [important] |
||
return false; | ||
} | ||
} | ||
|
||
protected String retrievePublicKey() throws SecretRetrievalException { | ||
var ourPublicKey = "trusted-intermediary-public-key-" + ApplicationContext.getEnvironment(); | ||
String key = this.keyCache.get(ourPublicKey); | ||
if (key != null) { | ||
return key; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,4 +6,6 @@ public interface Cache { | |
void put(String key, String value); | ||
|
||
String get(String key); | ||
|
||
void remove(String key); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this is the only inject we actually use in this class. Would a small doc be good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I see
jwtEngine
,secrets
, andlogger
being used below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some extra comments on the class. I think all the injects are used unless I'm misunderstanding what you mean.