From f5856ef40479250abf496e684d88c54f27ee8e73 Mon Sep 17 00:00:00 2001 From: Chris Hostetter Date: Sun, 18 Aug 2019 12:20:51 -0700 Subject: [PATCH] SOLR-13701: Fixed JWTAuthPlugin to update metrics prior to continuing w/other filters or returning error --- solr/CHANGES.txt | 2 ++ .../org/apache/solr/security/JWTAuthPlugin.java | 16 ++++++++-------- .../apache/solr/cloud/SolrCloudAuthTestCase.java | 15 ++++++--------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 3e8a74753ab..1fa18573c15 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -153,6 +153,8 @@ Bug Fixes * SOLR-13700: Fixed a race condition when initializing metrics for new security plugins on security.json change (hossman) +* SOLR-13701: Fixed JWTAuthPlugin to update metrics prior to continuing w/other filters or returning error (hossman) + Other Changes ---------------------- diff --git a/solr/core/src/java/org/apache/solr/security/JWTAuthPlugin.java b/solr/core/src/java/org/apache/solr/security/JWTAuthPlugin.java index 4c8c28a57df..c5ba67c6bc2 100644 --- a/solr/core/src/java/org/apache/solr/security/JWTAuthPlugin.java +++ b/solr/core/src/java/org/apache/solr/security/JWTAuthPlugin.java @@ -302,8 +302,8 @@ public class JWTAuthPlugin extends AuthenticationPlugin implements SpecProvider, if (jwtConsumer == null) { if (header == null && !blockUnknown) { log.info("JWTAuth not configured, but allowing anonymous access since {}==false", PARAM_BLOCK_UNKNOWN); - filterChain.doFilter(request, response); numPassThrough.inc(); + filterChain.doFilter(request, response); return true; } // Retry config @@ -342,22 +342,22 @@ public class JWTAuthPlugin extends AuthenticationPlugin implements SpecProvider, } if (log.isDebugEnabled()) log.debug("Authentication SUCCESS"); - filterChain.doFilter(wrapper, response); numAuthenticated.inc(); + filterChain.doFilter(wrapper, response); return true; case PASS_THROUGH: if (log.isDebugEnabled()) log.debug("Unknown user, but allow due to {}=false", PARAM_BLOCK_UNKNOWN); - filterChain.doFilter(request, response); numPassThrough.inc(); + filterChain.doFilter(request, response); return true; case AUTZ_HEADER_PROBLEM: case JWT_PARSE_ERROR: log.warn("Authentication failed. {}, {}", authResponse.getAuthCode(), authResponse.getAuthCode().getMsg()); - authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_BAD_REQUEST, BearerWwwAuthErrorCode.invalid_request); numErrors.mark(); + authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_BAD_REQUEST, BearerWwwAuthErrorCode.invalid_request); return false; case CLAIM_MISMATCH: @@ -365,25 +365,25 @@ public class JWTAuthPlugin extends AuthenticationPlugin implements SpecProvider, case JWT_VALIDATION_EXCEPTION: case PRINCIPAL_MISSING: log.warn("Authentication failed. {}, {}", authResponse.getAuthCode(), exceptionMessage); - authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.invalid_token); numWrongCredentials.inc(); + authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.invalid_token); return false; case SIGNATURE_INVALID: log.warn("Signature validation failed: {}", exceptionMessage); - authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.invalid_token); numWrongCredentials.inc(); + authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.invalid_token); return false; case SCOPE_MISSING: - authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.insufficient_scope); numWrongCredentials.inc(); + authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, BearerWwwAuthErrorCode.insufficient_scope); return false; case NO_AUTZ_HEADER: default: - authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, null); numMissingCredentials.inc(); + authenticationFailure(response, authResponse.getAuthCode().getMsg(), HttpServletResponse.SC_UNAUTHORIZED, null); return false; } } diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudAuthTestCase.java b/solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudAuthTestCase.java index 1c4adbfd9db..9485c80a438 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudAuthTestCase.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudAuthTestCase.java @@ -119,16 +119,13 @@ public class SolrCloudAuthTestCase extends SolrCloudTestCase { expectedCounts.put("failMissingCredentials", (long) failMissingCredentials); expectedCounts.put("errors", (long) errors); - Map counts = countSecurityMetrics(cluster, prefix, AUTH_METRICS_KEYS); - boolean success = isMetricsEqualOrLarger(AUTH_METRICS_TO_COMPARE, expectedCounts, counts); - if (!success) { - log.info("First metrics count assert failed, pausing 2s before re-attempt"); - Thread.sleep(2000); - counts = countSecurityMetrics(cluster, prefix, AUTH_METRICS_KEYS); - success = isMetricsEqualOrLarger(AUTH_METRICS_TO_COMPARE, expectedCounts, counts); - } + final Map counts = countSecurityMetrics(cluster, prefix, AUTH_METRICS_KEYS); + final boolean success = isMetricsEqualOrLarger(AUTH_METRICS_TO_COMPARE, expectedCounts, counts); - assertTrue("Expected metric minimums for prefix " + prefix + ": " + expectedCounts + ", but got: " + counts, success); + assertTrue("Expected metric minimums for prefix " + prefix + ": " + expectedCounts + + ", but got: " + counts + "(Possible cause is delay in loading modified " + + "security.json; see SOLR-13464 for test work around)", + success); if (counts.get("requests") > 0) { assertTrue("requestTimes count not > 1", counts.get("requestTimes") > 1);