Djaytan / mc-jobs-reborn-patch-place-break

A place-break patch extension of JobsReborn plugin for Bukkit servers (Minecraft).
MIT License
6 stars 2 forks source link

fix(spigot-adapter): #570 - logs noises when performing unsupported job action types #571

Closed Djaytan closed 9 months ago

Djaytan commented 9 months ago

This bug was due to a missing check after receiving the JobsReborn event when listening pre-payments (cf. fr.djaytan.mc.jrppb.spigot.listener.jobs.JobsPrePaymentListener class).

In fact, the action type check was only performed when converting a JobsReborn ActionType to a JRPPB BlockActionType (cf. ActionTypeConverter). Detecting the invalid action type there is too late and thus lead to an exception stack trace being printed in the server logs. This bug is pretty serious since it can generate too much noises (it's easy to see the stack trace being printed several times under a second). Generating too much exception stack traces without real justification can be enough to encourage a user uninstalling the plugin.

This fix aims to perform the check sooner when checking place-and-break exploit so that it can be handled the right way. The "right way" means never considering unsupported job action types as exploit. For that, the place to implement it must belongs at the Spigot adapter level of the patch (i.e. the spigot-patch-adapter module). This is where the data extracted from the JobsReborn events are interpreted and thus where we will be able to check the job action type value (cf. PatchPlaceBreakSpigotAdapterApi#isPlaceAndBreakExploit()).

Since the job action type support check is now required at two different places (i.e. by the PatchPlaceBreakSpigotAdapterApi & ActionTypeConverter classes), then it's the opportunity to extract the associated methods into a dedicated class: JobActionTypeSupportChecker. The methods getSupportedJobActionTypes() and getUnsupportedJobActionTypes() have been made public since they can be considered as utilities in other parts of the production code or when writing tests (this is typically the case in the ActionTypeConverter & PatchPlaceBreakSpigotAdapterApiBaseTest classes).

We are no longer referring to "validating a job action type" but instead we "check if a job action type is supported". In fact, both names are nearly equivalent but the later is more accurate (what would it means to consider a job action type as "invalid"? Doesn't it mean that the action is unsupported by the patch? Having a "supported" or "unsupported" job action type sounds better and more explicit, especially since we know that not all actions types are supported by the patch: only the BREAK, TNTBREAK and PLACE ones are).

Improvements have been made in the test classes: tests related to a given production method (e.g. isPlaceAndBreakExploit() from the PatchPlaceBreakSpigotAdapterApi class) have been grouped together under an internal class so that it's easier to find them when updating the associated production method.

Closes #570

sonarcloud[bot] commented 9 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions[bot] commented 9 months ago

:tada: This PR is included in version 3.0.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 9 months ago

:tada: This PR is included in version 3.0.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 9 months ago

:tada: This PR is included in version 3.0.3 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: