Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix role descriptor's hashCode, equals and isEmpty implementations #108255

Merged
merged 10 commits into from
May 6, 2024
Merged

Fix role descriptor's hashCode, equals and isEmpty implementations #108255

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1d99618
Fix role descriptor's hashCode, equals and isEmpty implementations
slobodanadamovic May 3, 2024
326e80b
Remove `remoteClusterAliasMatcher` from equals and hashCode
slobodanadamovic May 3, 2024
9304367
Merge branch 'main' of github.com:elastic/elasticsearch into sa-fix-r…
slobodanadamovic May 3, 2024
e643374
Remove equals and hashCode from StringMatcher
slobodanadamovic May 6, 2024
6c0dd74
Explain why we omit remoteClusterAliasMatcher
slobodanadamovic May 6, 2024
6768967
Merge branch 'main' of github.com:elastic/elasticsearch into sa-fix-r…
slobodanadamovic May 6, 2024
bc6f77a
Merge branch 'main' of github.com:elastic/elasticsearch into sa-fix-r…
slobodanadamovic May 6, 2024
a75639f
Unmute GetUserPrivilegesResponseTests.testSerializationForCurrentVersion
slobodanadamovic May 6, 2024
0e2892d
drop the long comment
slobodanadamovic May 6, 2024
a682b89
Merge branch 'main' of github.com:elastic/elasticsearch into sa-fix-r…
slobodanadamovic May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ public boolean equals(Object o) {
if (metadata.equals(that.getMetadata()) == false) return false;
if (Arrays.equals(runAs, that.runAs) == false) return false;
if (Arrays.equals(remoteIndicesPrivileges, that.remoteIndicesPrivileges) == false) return false;
if (remoteClusterPermissions.equals(that.remoteClusterPermissions) == false) return false;
return restriction.equals(that.restriction);
}

Expand All @@ -370,6 +371,7 @@ public int hashCode() {
result = 31 * result + Arrays.hashCode(runAs);
result = 31 * result + metadata.hashCode();
result = 31 * result + Arrays.hashCode(remoteIndicesPrivileges);
result = 31 * result + remoteClusterPermissions.hashCode();
result = 31 * result + restriction.hashCode();
return result;
}
Expand All @@ -382,6 +384,7 @@ public boolean isEmpty() {
&& runAs.length == 0
&& metadata.size() == 0
&& remoteIndicesPrivileges.length == 0
&& remoteClusterPermissions.groups().isEmpty()
&& restriction.isEmpty();
}

Expand Down
8 changes: 2 additions & 6 deletions .../org/elasticsearch/xpack/core/security/authz/permission/RemoteClusterPermissionGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.io.IOException;
import java.util.Arrays;
import java.util.Objects;

/**
* Represents a group of permissions for a remote cluster. For example:
Expand Down Expand Up @@ -107,15 +106,12 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
RemoteClusterPermissionGroup that = (RemoteClusterPermissionGroup) o;
return Arrays.equals(clusterPrivileges, that.clusterPrivileges)
&& Arrays.equals(remoteClusterAliases, that.remoteClusterAliases)
&& Objects.equals(remoteClusterAliasMatcher, that.remoteClusterAliasMatcher);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jakelandis jakelandis May 3, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Arrays.equals(clusterPrivileges, that.clusterPrivileges) && Arrays.equals(remoteClusterAliases, that.remoteClusterAliases);
}

@Override
public int hashCode() {
int result = Objects.hash(remoteClusterAliasMatcher);
result = 31 * result + Arrays.hashCode(clusterPrivileges);
int result = Arrays.hashCode(clusterPrivileges);
result = 31 * result + Arrays.hashCode(remoteClusterAliases);
return result;
}
Expand Down