Invalid equals() / hashCode() on JwtKafkaPrincipal breaks re-authenti…

…cation (#64)

* Remove equals & hashcode based on JWT, KafkaPrincipal ones will apply

Signed-off-by: Michele Tibaldi <[email protected]>

* Add test for the JwtKafkaPrincipal equals() and hashCode()

Signed-off-by: Marko Strukelj <[email protected]>

* Make JwtKafkaPrincipal final + fix typo as suggested in #60

Signed-off-by: Marko Strukelj <[email protected]>

Co-authored-by: Michele Tibaldi <[email protected]>

oauth-keycloak-authorizer/pom.xml

@@ -44,6 +44,17 @@
             <artifactId>scala-library</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.github.spotbugs</groupId>
+            <artifactId>spotbugs-annotations</artifactId>
+            <scope>provided</scope>
+        </dependency>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>${junit.version}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>
@@ -56,6 +67,10 @@
                     <target>${maven.compiler.target}</target>
                 </configuration>
             </plugin>
+            <plugin>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <version>${maven.surefire.version}</version>
+            </plugin>
         </plugins>
     </build>
 </project>

oauth-keycloak-authorizer/src/main/java/io/strimzi/kafka/oauth/server/authorizer/JwtKafkaPrincipal.java

@@ -7,9 +7,19 @@
 import io.strimzi.kafka.oauth.common.BearerTokenWithPayload;
 import org.apache.kafka.common.security.auth.KafkaPrincipal;
 
-import java.util.Objects;
-
-public class JwtKafkaPrincipal extends KafkaPrincipal {
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+/**
+ * This class uses the KafkaPrincipal object to store additional info obtained at session authentication time,
+ * and required later by a custom authorizer.
+ *
+ * This class is the only notion of client session that we can get. Kafka code holds on to it for as long as the session is alive,
+ * and then the object can be garbage collected.
+ *
+ * Any additional fields should not be included in equals / hashcode check. If they are, that will break re-authentication.
+ */
+@SuppressFBWarnings("EQ_DOESNT_OVERRIDE_EQUALS")
+public final class JwtKafkaPrincipal extends KafkaPrincipal {
 
     private final BearerTokenWithPayload jwt;
 
@@ -25,20 +35,4 @@ public JwtKafkaPrincipal(String principalType, String name, BearerTokenWithPaylo
     public BearerTokenWithPayload getJwt() {
         return jwt;
     }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-
-        if (!super.equals(o)) return false;
-
-        JwtKafkaPrincipal that = (JwtKafkaPrincipal) o;
-        return Objects.equals(jwt, that.jwt);
-    }
-
-    @Override
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), jwt);
-    }
 }

oauth-keycloak-authorizer/src/test/java/io/strimzi/kafka/oauth/server/authorizer/JwtKafkaPrincipalTest.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright 2017-2019, Strimzi authors.
+ * License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
+ */
+package io.strimzi.kafka.oauth.server.authorizer;
+
+import io.strimzi.kafka.oauth.common.BearerTokenWithPayload;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class JwtKafkaPrincipalTest {
+
+    @Test
+    public void testEquals() {
+
+        BearerTokenWithPayload token = new MockBearerTokenWithPayload("service-account-my-client",
+                System.currentTimeMillis(), System.currentTimeMillis() + 60000, null, "BEARER-TOKEN-9823eh982u", "Whatever");
+        JwtKafkaPrincipal principal = new JwtKafkaPrincipal("User", "service-account-my-client", token);
+
+
+        BearerTokenWithPayload token2 = new MockBearerTokenWithPayload("bob",
+                System.currentTimeMillis(), System.currentTimeMillis() + 60000, null, "BEARER-TOKEN-0000dd0000", null);
+        JwtKafkaPrincipal principal2 = new JwtKafkaPrincipal("User", "service-account-my-client", token2);
+
+
+        JwtKafkaPrincipal principal3 = new JwtKafkaPrincipal("User", "service-account-my-client");
+
+        JwtKafkaPrincipal principal4 = new JwtKafkaPrincipal("User", "bob");
+
+
+        Assert.assertTrue("principal should be equal to principal2", principal.equals(principal2));
+        Assert.assertTrue("principal2 should be equal to principal", principal2.equals(principal));
+
+        Assert.assertTrue("principal should be equal to principal3", principal.equals(principal3));
+        Assert.assertTrue("principal3 should be equal to principal", principal3.equals(principal));
+
+        Assert.assertTrue("principal2 should be equal to principal3", principal2.equals(principal3));
+        Assert.assertTrue("principal3 should be equal to principal2", principal3.equals(principal2));
+
+        Assert.assertTrue("principal should be equal to itself", principal.equals(principal));
+        Assert.assertTrue("principal2 should be equal to itself", principal2.equals(principal2));
+        Assert.assertTrue("principal3 should be equal to itself", principal3.equals(principal3));
+        Assert.assertTrue("principal4 should be equal to itself", principal4.equals(principal4));
+
+        Assert.assertFalse("principal should not be equal to principal4", principal.equals(principal4));
+        Assert.assertFalse("principal4 should not be equal to principal", principal4.equals(principal));
+        Assert.assertFalse("principal3 should not be equal to principal4", principal3.equals(principal4));
+        Assert.assertFalse("principal4 should not be equal to principal3", principal4.equals(principal3));
+
+        long hash1 = principal.hashCode();
+        long hash2 = principal2.hashCode();
+        long hash3 = principal3.hashCode();
+        long hash4 = principal4.hashCode();
+
+        Assert.assertTrue("Hashcode1 should be equal to hashcode2", hash1 == hash2);
+        Assert.assertTrue("Hashcode1 should be equal to hashcode3", hash1 == hash3);
+        Assert.assertFalse("Hashcode1 should not be equal to hashcode4", hash1 == hash4);
+    }
+}

oauth-keycloak-authorizer/src/test/java/io/strimzi/kafka/oauth/server/authorizer/MockBearerTokenWithPayload.java

@@ -0,0 +1,73 @@
+/*
+ * Copyright 2017-2019, Strimzi authors.
+ * License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
+ */
+package io.strimzi.kafka.oauth.server.authorizer;
+
+import io.strimzi.kafka.oauth.common.BearerTokenWithPayload;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+public class MockBearerTokenWithPayload implements BearerTokenWithPayload {
+
+
+    private final String principalName;
+    private final long createTime;
+    private final long lifetime;
+    private final Set<String> scopes;
+    private final String token;
+    private Object payload;
+
+    MockBearerTokenWithPayload(String principalName, long createTime, long lifetime, String scope, String token, Object payload) {
+        this.principalName = principalName;
+        this.createTime = createTime;
+        this.lifetime = lifetime;
+
+        Set<String> scopesSet = new HashSet<>();
+        String[] parsedScopes = scope != null ? scope.split(" ") : new String[0];
+        for (String s: parsedScopes) {
+            scopesSet.add(s);
+        }
+        scopes = Collections.unmodifiableSet(scopesSet);
+
+        this.token = token;
+        this.payload = payload;
+    }
+
+    @Override
+    public Object getPayload() {
+        return payload;
+    }
+
+    @Override
+    public void setPayload(Object payload) {
+        this.payload = payload;
+    }
+
+    @Override
+    public String value() {
+        return token;
+    }
+
+    @Override
+    public Set<String> scope() {
+        return scopes;
+    }
+
+    @Override
+    public long lifetimeMs() {
+        return lifetime;
+    }
+
+    @Override
+    public String principalName() {
+        return principalName;
+    }
+
+    @Override
+    public Long startTimeMs() {
+        return createTime;
+    }
+}

pom.xml

@@ -71,6 +71,7 @@
         <maven.compiler.target>1.8</maven.compiler.target>
         <maven.compiler.version>3.8.1</maven.compiler.version>
         <maven.dependency.version>3.1.1</maven.dependency.version>
+        <maven.surefire.version>2.22.1</maven.surefire.version>
         <maven.javadoc.version>3.1.0</maven.javadoc.version>
         <maven.source.version>3.0.1</maven.source.version>
         <maven.gpg.version>1.6</maven.gpg.version>
@@ -222,6 +223,10 @@
                         </execution>
                     </executions>
                 </plugin>
+                <plugin>
+                    <artifactId>maven-surefire-plugin</artifactId>
+                    <version>${maven.surefire.version}</version>
+                </plugin>
             </plugins>
         </pluginManagement>
         <plugins>