Make auths classes in Authorizations

[milleruntime]

It looks like all the serialization done for Authorizations is done with custom methods that we have created over the years. I am not sure why we need the interface on the class anymore.

[dlmarion]

I believe that this is part of the public API. I'm not sure if this violates our policy, but I think it might.

[milleruntime]

This change is causing a failure in FateIT.testTransactionStatus().

[milleruntime]

I believe that this is part of the public API.

Yeah, its definitely in the public API. I am not sure what the use case would be for using the Java serialization of the Auths though.

I'm not sure if this violates our policy, but I think it might.

The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?

[dlmarion]

The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?

Is there a security flaw?

I believe the solution for #2776 in regards to this class is to mark these fields as transient

[milleruntime]

The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?

Is there a security flaw?

I don't know.

I believe the solution for #2776 in regards to this class is to mark these fields as transient

That may be a better fix. But I think this would still break the serialization if a user extended Authorizations. I am going to write a test to experiment.

My point being, if we are going to break the serialization to fix the warning, lets just get rid of it.

[dlmarion]

If you remove the Serializable interface from the class, then I believe that breaks binary compatibility, which violates our public API policy. If the serailization/deserialization of Authorizations is done by other means, then adding transient may not break anything. If this is just a warning put out by the Java 18 JVM, then we might not need to do anything at this time.

[milleruntime]

I wrote a simple test that writes and reads the Authorizations object and it passes on main. I think changing the serialization breaks the API either way:

fields marked transient:
Exception in toString(): java.lang.NullPointerException: Cannot invoke "java.util.Set.iterator()" because "this.auths" is null
Dropped serializable:
java.io.NotSerializableException: org.apache.accumulo.core.security.Authorizations

Here is the test I added to AuthorizationsTest:

@Test
  public void testSerializable() throws Exception {
    ByteArrayOutputStream bytesOut = new ByteArrayOutputStream();
    ObjectOutputStream outputStream = new ObjectOutputStream(bytesOut);
    Authorizations auths = new Authorizations("foo");
    outputStream.writeObject(auths);
    outputStream.flush();

    ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesOut.toByteArray());
    ObjectInputStream inputStream = new ObjectInputStream(bytesIn);
    Authorizations readObject = (Authorizations) inputStream.readObject();
    assertEquals(auths, readObject);
  }

[dlmarion]

in the case where you marked the fields transient, then you just need to protect against NPE in equals() ?

EDIT: I think your serialization / deserialization worked with transient, the assertEquals caused it to fail

[milleruntime]

in the case where you marked the fields transient, then you just need to protect against NPE in equals() ?

EDIT: I think your serialization / deserialization worked with transient, the assertEquals caused it to fail

The equals() is a problem but its more than that. The toString() also needs rewritten. Probably the hashCode() method as well.

[ctubbsii]

Can't you just make ByteSequence serializable? ArrayList and HashSet already are.

[keith-turner]

Its kinda neat that Java 18 found this. @milleruntime would you happen to have the error/warn message? I am curious what it said.

Trying to understand the implications of removing the interface on binary compat I was looking at :

https://wiki.eclipse.org/Evolving_Java-based_APIs_2

Reading that doc it seems like removing it may only break binary compat in the case where code is casting to Serializable. Does that interpretation seem correct? Since serialization did not work, maybe no one was doing that and its ok to remove if code that was not casting to Serializable is ok.

[milleruntime]

Its kinda neat that Java 18 found this. @milleruntime would you happen to have the error/warn message? I am curious what it said.

I currently only have Java 18 in my IDE and this is all it is saying:

workspace/accumulo/core/src/main/java/org/apache/accumulo/core/security/Authorizations.java:47:29
java: non-transient instance field of a serializable class declared with a non-serializable type

Reading that doc it seems like removing it may only break binary compat in the case where code is casting to Serializable. Does that interpretation seem correct?

I will take a look at that link, thanks.

Since serialization did not work, maybe no one was doing that and its ok to remove if code that was not casting to Serializable is ok.

So the serialization of Authorizations does work as it is currently written. See #2777 (comment). I am going to create a test for all the Serialization warnings so we can figure out a way forward for #2776.

[milleruntime]

Can't you just make ByteSequence serializable? ArrayList and HashSet already are.

It looks like we may not have to. By changing the private types from interfaces to classes, the warnings went away:

-  private Set<ByteSequence> auths = new HashSet<>();
-  private List<byte[]> authsList = new ArrayList<>(); // sorted order
+  private final HashSet<ByteSequence> auths = new HashSet<>();
+  private final ArrayList<byte[]> authsList = new ArrayList<>(); // sorted order

[dlmarion]

If a user is doing the following:

Serializable auths = new Authorizations();

but never serializes or deserializes the object, then according to the JLS they will get a VerifyError.