Fix role descriptor's hashCode, equals and isEmpty implementations

[slobodanadamovic]

Added missing remoteClusterPermissions to hashCode, equals and isEmpty implementations.

Resolves #108253, #108285

Note: Marking as >non-issue since the PR that introduced remoteClusterPermissions is not released yet.

[elasticsearchmachine]

Pinging @elastic/es-security (Team:Security)

[slobodanadamovic]

Marking it as draft until I figure out why FileRolesStoreTests is failing now 🤨

[slobodanadamovic]

I had to remove remoteClusterAliasMatcher from both hashCode and equals implementation as it is always wrong due to the nature of Predicate. I'm thinking of not overriding them in StringMatcher to avoid confusion.

var first = StringMatcher.of("remote-*");
var second = StringMatcher.of("remote-*");
first.equals(second); // this will never be true because `Predicate` in `StringMatcher` is different object

[jakelandis]

weird ... the unit tests extend AbstractXContentSerializingTestCase which in turn tests org.elasticsearch.test.AbstractWireTestCase#testConcurrentEquals.. which should catch things like that.

Taking a deeper look and adding this test:

   assertTrue(StringMatcher.of("*").equals(StringMatcher.of("*"))); //passes
   assertTrue(StringMatcher.of("foo").equals(StringMatcher.of("foo")));//fails

So those equality tests passed since i guess '*' is a special case.

I explicitly added the equals/hash to StringMatcher for this purpose (but only relied on tests from AbstractXContentSerializingTestCase of RemoteClusterPermissionGroup). Can you also remove the equals and hash code, or better update them to always return false with a comment to why (that is super trappy behavior).

Also, can you add a comment we explicitly omit the matcher ? It doesn't need be there and the only reason I included it in the equality was to avoid messages like this since most devs will just let the IDE generate these.

Also thanks for fixing the missing RoleDescriptor equality. I just ran the failing RoleDescriptor test a 1000 times and it only failed 3/1000 of the times (and got it to pass with 0/1000 failures) . We really shouldn't use that much randomization :/

[slobodanadamovic]

Can you also remove the equals and hash code, or better update them to always return false with a comment to why (that is super trappy behavior).

Returning false would be wrong when the same object is compared. So I decided to remove them since this is the correct behaviour.

[slobodanadamovic]

Also thanks for fixing the missing RoleDescriptor equality. I just ran the failing RoleDescriptor test a 1000 times and it only failed 3/1000 of the times (and got it to pass with 0/1000 failures) . We really shouldn't use that much randomization :/

This was accidental. I only caught it because isEmpty was failing and I saw that equals was not correct. This then uncovered issue when reloading FileRolesStore.

[jakelandis]

nit: thanks for the comment. however we probably don't need a full explanation, just a mention that it is omitted intentionally. I know it is really pedantic but long comments like this draw your attention as something important to read where all we really need to know is that we intentionally omitted those fields to avoid appearing as a bug and/or accidentally adding those fields here at some point.

[jakelandis]

LGTM

Thanks for fixing this.