Previous versions allowed null address in getApplicablePunishment

AuroraLS3 commented 1 year ago

LibertyBans Version

1.1.0-M2

I have confirmed that ...

[X] LibertyBans is up to date
[X] No similar issue has been reported

Platform

Spigot/Paper

Description

I got reported an NPE https://github.com/plan-player-analytics/Plan/issues/2948

I have possibly fixed this issue on my end by updating the use of API to use the builder, but I think it might be an issue in the API as well so I'm reporting it.

Code that produces NPE (Valid API call in Libertybans 0.8.3)

   private LibertyBans api() {
        if (api != null) {
            return api;
        }
        Omnibus omnibus = OmnibusProvider.getOmnibus();
        this.api = omnibus.getRegistry().getProvider(LibertyBans.class).orElseThrow(NotReadyException::new);
        if (api == null) throw new NotReadyException();
        return api;
    }

    private PunishmentSelector selector() {
        return api().getSelector();
    }

    private Optional<Punishment> punishment(UUID playerUUID, PunishmentType type) {
        return selector().getApplicablePunishment(playerUUID, null, type) // NPE here
                .toCompletableFuture().join();

Exception

java.lang.NullPointerException: address
   java.base/java.util.Objects.requireNonNull(Objects.java:259)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionByApplicabilityImpl.<init>(SelectionByApplicabilityImpl.java:53)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionByApplicabilityBuilderImpl.buildWith(SelectionByApplicabilityBuilderImpl.java:69)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionByApplicabilityBuilderImpl.buildWith(SelectionByApplicabilityBuilderImpl.java:31)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionBuilderBaseImpl.build(SelectionBuilderBaseImpl.java:108)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionByApplicabilityBuilderImpl.build(SelectionByApplicabilityBuilderImpl.java:82)
   LibertyBans-ClassLoader//space.arim.libertybans.core.selector.SelectionByApplicabilityBuilderImpl.build(SelectionByApplicabilityBuilderImpl.java:31)
   space.arim.libertybans.api.select.PunishmentSelector.getApplicablePunishment(PunishmentSelector.java:135)

A248 commented 1 year ago

Passing a null address to applicable punishment selection has never been formally allowed. However, I noticed the implementation prior to 1.1.0 was missing a null check. As a result, the null address value was simply passed through to the implementation, where it was swallowed by the database abstraction layer, resulting in some behavior which is neither consistent across configurations nor reasonable per the method contract.

Selecting an applicable punishment only makes sense where you have a UUID and address combination. Since this breakage is partially my fault for not having checked the null parameter in the past, I decided to take a look at your commit and give you some pointers how to adjust.

https://github.com/plan-player-analytics/Extension-LibertyBans/commit/64631550fd0b31bf962ec1f3a7faf5fbc5e9ffca

Using the builder won't solve the NPE you receive due to specifying a null address. I don't know how Plan's internal framework is designed, but if you really want applicable punishments, you will need to find a way to pass the player's address. If, however, you don't want applicable punishments, then you should select by PlayerVictim.of(uuid) -- which will find all player bans but exclude IP-based punishments. Thus, you need to decide whether you want applicable punishments or simply user punishments; if the former, you need to pass the player's address.

Moreover, you are using getAllSpecificPunishments plus stream().findFirst(). Why not getFirstSpecificPunishment which returns only a single punishment? Then your API call will avoid retrieving unnecessary punishments from the database.

EDIT: Also, by specifying the address strictness in the builder call, you are in fact overriding the user's configuration with AddressStrictness.LENIENT. You are able to simply omit the addressStrictness call, which will automatically pick up on the user's configuration.

AuroraLS3 commented 1 year ago

Alright, thanks for the tips.

I made some assumptions since I didn't find the javadocs for the api I assumed that

giving null address would get rows which have the user uuid specified as the one being punished
That the LENIENT would skip address check altogether if one was not provided.

Plan calls this method asynchronously so the player may have already left the server and address may no longer be available. I think I can deal with that though, by simply not calling the api when that is the case.

Thanks!

A248 commented 1 year ago

For your convenience, the javadocs are attached to the API artifact, as noted on the docs page: https://docs.libertybans.org/#/Developer-API.

AuroraLS3 commented 1 year ago

I guess idea doesn't load them by default 😅

A248 / LibertyBans