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
KNOX-2207 - TokenStateService revocation should remove persisted token state #252
KNOX-2207 - TokenStateService revocation should remove persisted token state #252
Conversation
...r/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
@@ -208,6 +205,18 @@ protected void updateExpiration(final String token, long expiration) { | |||
} | |||
} | |||
|
|||
protected void removeRevokedExpiredToken(final String token) { | |||
if (!isValidIdentifier(token)) { |
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.
Could just call validateToken() here?
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
...ver/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java
Outdated
Show resolved
Hide resolved
@pzampino addressed the review comments in new PR, let me know what you think. |
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.
The isExpired(String) implementation is confusing.
It could probably be as simple as:
validateToken(token);
return (getTokenExpiration(token) <= System.currentTimeMillis());
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token); | ||
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max"); | ||
} catch (AliasServiceException e) { | ||
log.failedToUpdateTokenExpiration(e); |
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.
This seems a misleading log message
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.
Right, I'll get it fixed, thanks for letting me know @smolnar82
What changes were proposed in this pull request?
Do not maintain state for revoked tokens.
How was this patch tested?
This patch was manually tested.
Please review Knox Contributing Process before opening a pull request.