UserConverter.decode NPE when deserializing default constructed UserImpl
jpenglert opened this issue · 8 comments
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
- Run above unit test
Extra
Related to #637
I will submit a PR
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.
@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?
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.
@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?
@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?
@tsegismont no worries...one of the committers responded and merged my PR. Thanks again for your help on how to resolve this.