Remove equals & hashcode based on JWT, KafkaPrincipal ones will be in effect

[uwburn]

No description provided.

[scholzj]

Can you add some tests that the equals(...) method still works after your change?

[scholzj]

You should also look at the spotbugs - it looks like you should have some equals imeplementation there.

[ERROR] io.strimzi.kafka.oauth.server.authorizer.JwtKafkaPrincipal doesn't override org.apache.kafka.common.security.auth.KafkaPrincipal.equals(Object) [io.strimzi.kafka.oauth.server.authorizer.JwtKafkaPrincipal] At JwtKafkaPrincipal.java:[line 1] EQ_DOESNT_OVERRIDE_EQUALS

[uwburn]

Thanks for the headup about spotbug, i will also try to arrange a the test.

There's a thing i would like to point out, in any case: connections.max.reauth.ms is VERY important from what i've understood, because without it, the client does refresh the token, but the reauth does not take place against the broker, meaning it will keep the old auth data, and i'm quite sure that when also dealing with authz this is incovenient. I have verified this by looking at the net traffic with wireshark, without the option the token is sent to Kafka just when the connection is estabilished, with the option the token is actually sent periodically. I went through this because i'm trying to add the support for the Node.js client.

If you have time, i suggest you to take a look at it, the behaviour of the oauth plugin, while seemingly fine from what i have experienced so far (apart the equality stuff), probably needs some checks and it should also be pointed out in the documentation the relevancy of specifying connections.max.reauth.ms.

[uwburn]

I'm a bit lost on the test. I see that there are in place test with various providers as integration/system tests.

I'm afraid testing the effects of the equals implementation is really entangled with Kafka internals that are beyond my possibility of digging out.

Also i'm not familiar with spotbugs, i tried mvn spotbugs:check to replicate Travis error, but to no avail. Is it the correct command?

I changed the equals/hashcode to:

    @Override
    public boolean equals(Object o) {
        return super.equals(o);
    }

    @Override
    public int hashCode() {
        return super.hashCode();
    }

But that's really silly, and any other meaningful check on the JWT itself seems redundant with what's already in place on the KafkaPrincipal implementation, for how JwtKafkaPrincipal is constructed.

[scholzj]

Also i'm not familiar with spotbugs, i tried mvn spotbugs:check to replicate Travis error, but to no avail. Is it the correct command?

mvn spotbugs:check seems to be what the Travis is running. So that should be it. I think Spotbugs needs to to build it first, so maybe you should run mvn clean package spotbugs:check or something similar?

I'm a bit lost on the test. I see that there are in place test with various providers as integration/system tests.

I'm not sure if @mstruk has any ideas. But I think you should add. simple Unit test which would show that twoprincipals which should equal return true and two principals which should not equal should return false.

[mstruk]

@uwburn Thank you for a very valuable analysis.

We had some discussion around equals() / hashCode() on initial PR of authz support with @tombentley and the intuitively safe no-op implementation like you suggested as silly was deemed as such back then. It wasn't quite clear to me at the time how this is used and where it might cause problems - e.g. is it or will it be one day used in a HashMap in what should it mean exactly ...

It's clear now that the extra payload of jwt field should not be included in the equals() evaluation, and thus the safest approach really is for super.equals() to be used in case they add some relevant extra information. Another approach for the future is to store JWT completely separately from the principal object - into a cache.

The override with just delegation to super has a benefit of telling everyone that we didn't forget about thinking about equals() and hashCode(), and that's the intent of the spotbugs - although it's very easy to brainlessly add it as a routine, and it does look a bit silly by itself. On the other hand if we just suppress the warning that also means that we had to think about it at least a little bit, so it's the same.

I'd say we just remove equals() and hashCode(), add the warning suppresion annotation on the class, and add a comment that we delegate equals() / and hashCode() to super on purpose.

We don't really have unit tests, just the integration tests. I'd have to think some more where exactly to put the test, let me look at that some more.

[mstruk]

I added the test. See: uwburn#1

[mstruk]

@uwburn If you merge my uwburn#1 PR to your branch, we can then merge this PR.

[scholzj]

@tombentley Could you have a look at this please if you have a minute?

[tombentley]

Suggested change

	* This class uses the KafkaPrincipal object to store additional info obtained at sesion authentication time,
	* This class uses the KafkaPrincipal object to store additional info obtained at session authentication time,

[tombentley]

We don't ever expect this class to be subclassed, but I guess if it were the subclass could override equals()/hashCode(), breaking this contract. So I suppose we should either make this class final, or do it as originally with overridden (but final) equals()/hashCode() delegating to super.

[mstruk]

Seems like an overkill to me, but sure, we can make the class final.
Not that it should be treated like a contract with third party devs that might get an impression that it's a good idea to extend our classes, and think that we won't one day completely change them. Everything in terms of classes, methods, and configuration that we don't explicitly document should be treated as an implementation detail that can change at any time.
This final should only be considered a failsafe for a contributor to this project, to save them from inadvertently re-introducing the bug.

[mstruk]

@uwburn If you can merge uwburn#2, and then we're done here.

[uwburn]

Sorry for the delay, i was away from home.

[mstruk]

Continuing this in #64.

@uwburn Thanks again for the contribution.