You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/02/10 15:28:39 UTC
[GitHub] [knox] pzampino opened a new pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
pzampino opened a new pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260
…n state service
## What changes were proposed in this pull request?
Modified handling of "Unknown token" exception in JWTFilters, such that it no longer results in a HTTP 500 response. Also, improved some token-related log messages.
## How was this patch tested?
Augmented CommonJWTFilterTest.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377179358
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -41,6 +41,9 @@
@Message( level = MessageLevel.INFO, text = "Unable to verify token: {0}" )
void unableToVerifyToken(@StackTrace( level = MessageLevel.ERROR) Exception e);
+ @Message( level = MessageLevel.ERROR, text = "Unable to verify token expiration: {0}" )
Review comment:
INFO or WARN should be good I think. I just saw the message above about unable to verify being INFO and was like hmmm that seems worse than not being able to verify the expiration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377178183
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -41,6 +41,9 @@
@Message( level = MessageLevel.INFO, text = "Unable to verify token: {0}" )
void unableToVerifyToken(@StackTrace( level = MessageLevel.ERROR) Exception e);
+ @Message( level = MessageLevel.ERROR, text = "Unable to verify token expiration: {0}" )
Review comment:
I went back and forth with this, and could go either way really. Do you think INFO is sufficient?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377135229
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -170,7 +170,13 @@ protected void configureExpectedParameters(FilterConfig filterConfig) {
protected boolean tokenIsStillValid(JWT jwtToken) {
Date expires;
if (tokenStateService != null) {
- expires = new Date(tokenStateService.getTokenExpiration(jwtToken.toString()));
+ long timestamp = 0;
+ try {
+ timestamp = tokenStateService.getTokenExpiration(jwtToken.toString());
+ } catch (Exception e) {
Review comment:
Do we not have a specific exception coming from getTokenExpiration? or tokenStateService?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377135461
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -41,6 +41,9 @@
@Message( level = MessageLevel.INFO, text = "Unable to verify token: {0}" )
void unableToVerifyToken(@StackTrace( level = MessageLevel.ERROR) Exception e);
+ @Message( level = MessageLevel.ERROR, text = "Unable to verify token expiration: {0}" )
Review comment:
Do we need this at an error level?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377272413
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -170,7 +170,13 @@ protected void configureExpectedParameters(FilterConfig filterConfig) {
protected boolean tokenIsStillValid(JWT jwtToken) {
Date expires;
if (tokenStateService != null) {
- expires = new Date(tokenStateService.getTokenExpiration(jwtToken.toString()));
+ long timestamp = 0;
+ try {
+ timestamp = tokenStateService.getTokenExpiration(jwtToken.toString());
+ } catch (Exception e) {
Review comment:
I would rather change the method signatures in a separate commit, so I've created https://issues.apache.org/jira/browse/KNOX-2230 to address this concern.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino merged pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #260: KNOX-2228 - JWTFilter should handle unknown token exception from toke…
URL: https://github.com/apache/knox/pull/260#discussion_r377177657
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -170,7 +170,13 @@ protected void configureExpectedParameters(FilterConfig filterConfig) {
protected boolean tokenIsStillValid(JWT jwtToken) {
Date expires;
if (tokenStateService != null) {
- expires = new Date(tokenStateService.getTokenExpiration(jwtToken.toString()));
+ long timestamp = 0;
+ try {
+ timestamp = tokenStateService.getTokenExpiration(jwtToken.toString());
+ } catch (Exception e) {
Review comment:
I've been debating the need for an UnknownTokenException, and your comment may have just convinced me to implement it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services