UserConverter.decode NPE when deserializing default constructed UserImpl

[jpenglert]

Version

4.4.4

UserImpl has a default constructor which does not initialize its authorizations, attributes, or principal fields. The UserConverter class expects User to return a non-null value for those fields. If an instance of UserImpl is constructed using the default constructor and then later on serialized (since it implements ClusterSerializable) and then deserialized it will result in a NPE when it delegates serialization to UserConverter because UserConverter does not perform a null check on the fields it's decoding from the given JsonObject.

I encountered this with the Pac4jUser class from org.pac4j:vertx-pac4j which extends UserImpl and implements the default constructor which leaves all the fields null. The VertxProfileManager from org.pac4j:vertx-pac4j uses the Pac4jUser default constructor.

Seems like UserConverter should check for null on all fields before attempting to deserialize it.

Do you have a reproducer?

  @Test
  public void decode_UserImpl_defaultCtor() {
    UserImpl user = new UserImpl();
    JsonObject json = UserConverter.encode(user);
    User decoded = UserConverter.decode(json);
    assertNotNull(decoded);
  }

Steps to reproduce

1. Run above unit test

Extra

Related to https://github.com/eclipse-vertx/vertx-auth/issues/637

[jpenglert]

I will submit a PR

[jpenglert]

Submitted https://github.com/eclipse-vertx/vertx-auth/pull/658

[tsegismont]

If an instance of UserImpl is constructed using the default constructor

@jpenglert this should not be done by user code. The default constructor is only present because it is required for ClusterSerializable implementations.

[jpenglert]

@tsegismont understood that an instance of UserImpl should not be constructed using the default constructor by user code. However, is there any harm in updating the deserializing code to be more robust by handling null fields like the serializing code does?

[tsegismont]

The problem is UserImpl makes checks (like create a User without providing a principal) that the change allows to circumvent.

My opinion is VertxProfileManager in Pac4j shouldn't invoke an internal constructor in UserImpl. Instead, it should one of the creation methods in the User interface.

[jpenglert]

@tsegismont I made a PR for vertx-pac4j but that project looks pretty dead. If you know anyone over there maybe you could ping them?

Seems like vertx-pac4j may need to do a more in-depth re-work such that instead of extending UserImpl with Pac4jUser they use the io.vertx.ext.auth.authorization.Authorization interface instead?

[tsegismont]

@jpenglert sorry, I don't know any maintainers personally and I'm not familiar with vertx-pac4j

Have you tried to look at the GH profile of the committers? Maybe someone shares an email address?

[jpenglert]

@tsegismont no worries...one of the committers responded and merged my PR. Thanks again for your help on how to resolve this.