TehBrian / RestrictionHelper

A small library that eases the pain of checking the build restrictions of various plugins.
MIT License
2 stars 1 forks source link

create event and check if it's canceled #106

Open TehBrian opened 8 months ago

TehBrian commented 8 months ago

Could we toss all API-specific logic simply by faking a Bukkit place/destroy event, checking if it was canceled, and denying/accepting based on that?

RedstoneFuture commented 4 months ago

I've tested it on another plugin: It seems to work e.g. for PlotSqured and WorldGuard.

/src/main/java/io/github/rypofalem/armorstandeditor/PlayerEditorManager.java#L341-L360

    static boolean canEdit(Player player, Location location) {
        Block block = location.getBlock();

        // Creating test place-event for the target location:
        BlockPlaceEvent placeEvent = new BlockPlaceEvent(block, block.getState(), location.getBlock(), 
                player.getActiveItem(), player, false, player.getActiveItemHand());

        // Checking build permission by test-event:
        Bukkit.getServer().getPluginManager().callEvent(placeEvent);
        boolean canBuild = !placeEvent.isCancelled();
        placeEvent.setCancelled(true);

        return canBuild;
    }
TehBrian commented 4 months ago

One issue I'm worried about is the event messing with other plugins.

For example, if the event isn't canceled by WG/PS, CoreProtect could log it, creating records for actions that never happened.

Is there a way to pass events to specific plugins?

RedstoneFuture commented 4 months ago

If it was not canceled, it was allowed. And if it was forbidden, it must be canceled.

The method just doesn't tell you by which plugin / why it was forbidden.

TehBrian commented 4 months ago

If it was not canceled, it was allowed. And if it was forbidden, it must be canceled.

The method just doesn't tell you by which plugin / why it was forbidden.

Right. That's fine.

I'm worried about plugins that do more than just allow/deny actions. Plugins that log and rollback events, or track player statistics.

If the event is canceled, no worries. But if the event is not canceled, then those plugins would think that the player performed behavior that they didn't actually perform.

RedstoneFuture commented 4 months ago

I understand that, but who restriction system terminates the actions without the event cancel? That just seems very dirty, doesn't it?

How is the protection plugin able to terminate, especially block break/place/interact events, without event-cancelling? Block replacement? I don't know why it should.

TehBrian commented 4 months ago

I understand that, but who restriction system terminates the actions without the event cancel? That just seems very dirty, doesn't it?

I'm not worried about protection plugins preventing an action but not canceling it. I doubt there are any protection plugins like that.

I'm worried about protection plugins that allow an action, mixed with plugins (not protection plugins) that listen to those actions.

For example, say you want to check whether a player could break a block. RestrictionHelper fires a break event, and no protection plugins cancel it. Great; now you know that the player can break that block.

But they player didn't actually break that block. Meanwhile, a plugin that "levels up" players based on how many blocks they've broken (e.g., mcMMO) falsely tracks that as a block broken.

The only solution I can think of is to pass the break event to only protection plugins.

Do you see what I mean?

RedstoneFuture commented 4 months ago

Ah, plugins that log this demo event, yes. That would have to be checked again. That's a good point.

RedstoneFuture commented 4 months ago

Yes, the test-event is unfortunately logged e.g. by CoreProtect on location without AIR. I don't see any good way to get around this.

/src/main/java/io/github/rypofalem/armorstandeditor/PlayerEditorManager.java#L341-L360

    static boolean canEdit(Player player, Location location) {
        Block block = location.getBlock();

        // Creating test place-event for the target location. (Works also for Fine Adjustment.)
        // Used 'BlockPlaceEvent', because e.g. 'EntityPlaceEvent' is not handled the same for every restriction system.
        BlockPlaceEvent placeEvent = new BlockPlaceEvent(block, block.getState(), location.getBlock(), 
                player.getActiveItem(), player, false, player.getActiveItemHand());

        // Checking build permission by test-event. (The block is generally not placed via 'callEvent()' method.)
        Bukkit.getServer().getPluginManager().callEvent(placeEvent);
        // An event-cancel afterward would have no effect.

        return !placeEvent.isCancelled();
    }
TehBrian commented 4 months ago

Good news! Turns out that the solution is super simple.

The implementation of #callEvent (see PaperEventManager) boils down to calling each listener of each handler registered to the event, plus some fancy exception logging (which we don't need).

I haven't tested it, but I think that RestrictionHelper could be simplified to this single statement.

final boolean canceled = !Stream.of(event.getHandlers().getRegisteredListeners())
    .filter(l -> l.getPlugin().getName().matches("(PlotSquared|WorldGuard)"))
    .filter(l -> l.getPlugin().isEnabled())
    .filter(l -> {
      try {
        l.callEvent(event);
        return event.isCancelled();
      } catch (final Throwable ex) {
        return false;
      }
    }).toList().isEmpty();

My only worry is that PlotSquared, WorldGuard, or other protection plugins react poorly to fake events. For example, would they send a permission message to the player? If so, it would be better to stick with our current approach.