Check token content before handling them

This adds plausibility checks before handling tokens as for example jwt
or api keys. Doing so we generate less error logs and therefore we cause
less confusion.

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Unreleased
 ### Fixed
 - Null Pointer Exception on anonymous migration with deleted repositories ([#1371](https://github.com/scm-manager/scm-manager/pull/1371))
+### Changed
+- Reduced logging for invalid JWT or api keys ([#1374](https://github.com/scm-manager/scm-manager/pull/1374))
 
 ## [2.7.0] - 2020-10-12
 ### Added

scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java

@@ -30,6 +30,8 @@ import org.apache.shiro.authc.UsernamePasswordToken;
 import org.apache.shiro.authc.credential.AllowAllCredentialsMatcher;
 import org.apache.shiro.authz.AuthorizationException;
 import org.apache.shiro.realm.AuthenticatingRealm;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import sonia.scm.plugin.Extension;
 import sonia.scm.repository.RepositoryRole;
 import sonia.scm.repository.RepositoryRoleManager;
@@ -43,6 +45,8 @@ import static com.google.common.base.Preconditions.checkArgument;
 @Extension
 public class ApiKeyRealm extends AuthenticatingRealm {
 
+  private static final Logger LOG = LoggerFactory.getLogger(ApiKeyRealm.class);
+
   private final ApiKeyService apiKeyService;
   private final DAORealmHelper helper;
   private final RepositoryRoleManager repositoryRoleManager;
@@ -58,7 +62,14 @@ public class ApiKeyRealm extends AuthenticatingRealm {
 
   @Override
   public boolean supports(AuthenticationToken token) {
-    return token instanceof UsernamePasswordToken || token instanceof BearerToken;
+    if (token instanceof UsernamePasswordToken || token instanceof BearerToken) {
+      boolean containsDot = getPassword(token).contains(".");
+      if (containsDot) {
+        LOG.debug("Ignoring token with at least one dot ('.'); this is probably a JWT token");
+      }
+      return !containsDot;
+    }
+    return false;
   }
 
   @Override

scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java

@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
+
 package sonia.scm.security;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -29,6 +29,8 @@ import org.apache.shiro.authc.AuthenticationInfo;
 import org.apache.shiro.authc.AuthenticationToken;
 import org.apache.shiro.authc.credential.AllowAllCredentialsMatcher;
 import org.apache.shiro.realm.AuthenticatingRealm;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import sonia.scm.group.GroupDAO;
 import sonia.scm.plugin.Extension;
 import sonia.scm.user.UserDAO;
@@ -54,6 +56,7 @@ public class BearerRealm extends AuthenticatingRealm
   @VisibleForTesting
   static final String REALM = "BearerRealm";
 
+  private static final Logger LOG = LoggerFactory.getLogger(BearerRealm.class);
 
   /** dao realm helper */
   private final DAORealmHelper helper;
@@ -76,7 +79,17 @@ public class BearerRealm extends AuthenticatingRealm
     setAuthenticationTokenClass(BearerToken.class);
   }
 
-  //~--- methods --------------------------------------------------------------
+  @Override
+  public boolean supports(AuthenticationToken token) {
+    if (token instanceof BearerToken) {
+      boolean containsDot = ((BearerToken) token).getCredentials().contains(".");
+      if (!containsDot) {
+        LOG.debug("Ignoring token without a dot ('.'); this probably is an API key");
+      }
+      return containsDot;
+    }
+    return false;
+  }
 
   /**
    * Validates the given bearer token and retrieves authentication data from

scm-webapp/src/main/java/sonia/scm/web/security/TokenRefreshFilter.java

@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
+
 package sonia.scm.web.security;
 
 import org.apache.shiro.authc.AuthenticationException;
@@ -90,6 +90,10 @@ public class TokenRefreshFilter extends HttpFilter {
 
   private void examineToken(HttpServletRequest request, HttpServletResponse response, BearerToken token) {
     AccessToken accessToken;
+    if (!token.getCredentials().contains(".")) {
+      LOG.trace("Ignoring token without dot. This probably is an API key, no JWT");
+      return;
+    }
     try {
       accessToken = resolver.resolve(token);
     } catch (AuthenticationException e) {

scm-webapp/src/test/java/sonia/scm/security/ApiKeyRealmTest.java

@@ -96,6 +96,15 @@ class ApiKeyRealmTest {
     assertThrows(AuthorizationException.class, () -> realm.doGetAuthenticationInfo(token));
   }
 
+  @Test
+  void shouldIgnoreTokensWithDots() {
+    BearerToken token = valueOf("this.is.no.api.token");
+
+    boolean supports = realm.supports(token);
+
+    assertThat(supports).isFalse();
+  }
+
   void verifyScopeSet(String... permissions) {
     verify(authenticationInfoBuilder).withScope(argThat(scope -> {
       assertThat(scope).containsExactly(permissions);

scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java

@@ -61,4 +61,11 @@ class ApiKeyTokenHandlerTest {
 
     assertThat(token).isEmpty();
   }
+
+  @Test
+  void shouldParseRealWorldExample() {
+    Optional<ApiKeyTokenHandler.Token> token = handler.readToken("JhcGlLZXlJZCI6IkE2U0ROWmV0MjEiLCJ1c2VyIjoiaG9yc3QiLCJwYXNzcGhyYXNlIjoiWGNKQ01PMnZuZ1JaOEhVU21BSVoifQ");
+
+    assertThat(token).get().extracting("user").isEqualTo("horst");
+  }
 }

scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java

@@ -40,6 +40,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static sonia.scm.security.BearerToken.valueOf;
 
 /**
  * Unit tests for {@link BearerRealm}.
@@ -96,4 +97,13 @@ class BearerRealmTest {
   void shouldThrowIllegalArgumentExceptionForWrongTypeOfToken() {
     assertThrows(IllegalArgumentException.class, () -> realm.doGetAuthenticationInfo(new UsernamePasswordToken()));
   }
+
+  @Test
+  void shouldIgnoreTokensWithoutDot() {
+    BearerToken token = valueOf("this-is-no-jwt-token");
+
+    boolean supports = realm.supports(token);
+
+    assertThat(supports).isFalse();
+  }
 }

scm-webapp/src/test/java/sonia/scm/web/security/TokenRefreshFilterTest.java

@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
+
 package sonia.scm.web.security;
 
 import org.apache.shiro.authc.AuthenticationToken;
@@ -52,6 +52,7 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static sonia.scm.security.BearerToken.valueOf;
 
 @ExtendWith({MockitoExtension.class})
 class TokenRefreshFilterTest {
@@ -103,7 +104,7 @@ class TokenRefreshFilterTest {
 
   @Test
   void shouldNotRefreshNonJwtToken() throws IOException, ServletException {
-    BearerToken token = mock(BearerToken.class);
+    BearerToken token = createValidToken();
     JwtAccessToken jwtToken = mock(JwtAccessToken.class);
     when(tokenGenerator.createToken(request)).thenReturn(token);
     when(resolver.resolve(token)).thenReturn(jwtToken);
@@ -116,7 +117,7 @@ class TokenRefreshFilterTest {
 
   @Test
   void shouldRefreshIfRefreshable() throws IOException, ServletException {
-    BearerToken token = mock(BearerToken.class);
+    BearerToken token = createValidToken();
     JwtAccessToken jwtToken = mock(JwtAccessToken.class);
     JwtAccessToken newJwtToken = mock(JwtAccessToken.class);
     when(tokenGenerator.createToken(request)).thenReturn(token);
@@ -128,4 +129,8 @@ class TokenRefreshFilterTest {
     verify(issuer).authenticate(request, response, newJwtToken);
     verify(filterChain).doFilter(request, response);
   }
+
+  BearerToken createValidToken() {
+    return valueOf("some.jwt.token");
+  }
 }