Enhancement: Option to disable offline inventory viewing #171

New Issue

2020-12-02T20:06:37-05:00

Combustible commented

2020-12-02 20:06:37 -05:00

(Migrated from github.com)

We've got a relatively complicated data storage system on my server which redirects the player data storage to a database. So there are no files to store locally - this allows us to share player data across multiple worlds (the plugin we developed for this is https://github.com/TeamMonumenta/monumenta-redis-sync)

It took us a long time to track down an obscure inventory corruption bug, and eventually tracked it back to the way OpenInv switches a player to offline mode. What would happen is that a moderator would open a player's inventory on server 1, then they would transfer to server 2, change their inventory, and go back to server 1. From server 1's perspective, the player logs out, then logs back in - so openinv helpfully overwrites their inventory with the state from before (and thus losing any changes the player made on server 2).

We've been working around this by building our own OpenInv version with the following patch. It'd be nice if we had some config file option so we wouldn't need to build our own version for this. Totally understand if it's not worth your time - this is obviously a special case that will probably never effect many people. Just thought I'd ask.

+++ b/plugin/src/main/java/com/lishid/openinv/OpenInv.java
@@ -447,12 +447,23 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
         // Replace stored player with our own version
         this.playerCache.put(key, this.accessor.getPlayerDataManager().inject(player));

+        /* Remove the cached entry if it exists */
+        this.playerCache.invalidate(key);
+
         if (this.inventories.containsKey(key)) {
-            this.inventories.get(key).setPlayerOffline();
+            Iterator<HumanEntity> iterator = this.inventories.remove(key).getBukkitInventory().getViewers().iterator();
+            while (iterator.hasNext()) {
+                HumanEntity human = iterator.next();
+                human.closeInventory();
+            }
         }

         if (this.enderChests.containsKey(key)) {
-            this.enderChests.get(key).setPlayerOffline();
+            Iterator<HumanEntity> iterator = this.enderChests.remove(key).getBukkitInventory().getViewers().iterator();
+            while (iterator.hasNext()) {
+                HumanEntity human = iterator.next();
+                human.closeInventory();
+            }
         }
     }

We've got a relatively complicated data storage system on my server which redirects the player data storage to a database. So there are no files to store locally - this allows us to share player data across multiple worlds (the plugin we developed for this is https://github.com/TeamMonumenta/monumenta-redis-sync) It took us a long time to track down an obscure inventory corruption bug, and eventually tracked it back to the way OpenInv switches a player to offline mode. What would happen is that a moderator would open a player's inventory on server 1, then they would transfer to server 2, change their inventory, and go back to server 1. From server 1's perspective, the player logs out, then logs back in - so openinv helpfully overwrites their inventory with the state from before (and thus losing any changes the player made on server 2). We've been working around this by building our own OpenInv version with the following patch. It'd be nice if we had some config file option so we wouldn't need to build our own version for this. Totally understand if it's not worth your time - this is obviously a special case that will probably never effect many people. Just thought I'd ask. ```diff +++ b/plugin/src/main/java/com/lishid/openinv/OpenInv.java @@ -447,12 +447,23 @@ public class OpenInv extends JavaPlugin implements IOpenInv { // Replace stored player with our own version this.playerCache.put(key, this.accessor.getPlayerDataManager().inject(player)); + /* Remove the cached entry if it exists */ + this.playerCache.invalidate(key); + if (this.inventories.containsKey(key)) { - this.inventories.get(key).setPlayerOffline(); + Iterator<HumanEntity> iterator = this.inventories.remove(key).getBukkitInventory().getViewers().iterator(); + while (iterator.hasNext()) { + HumanEntity human = iterator.next(); + human.closeInventory(); + } } if (this.enderChests.containsKey(key)) { - this.enderChests.get(key).setPlayerOffline(); + Iterator<HumanEntity> iterator = this.enderChests.remove(key).getBukkitInventory().getViewers().iterator(); + while (iterator.hasNext()) { + HumanEntity human = iterator.next(); + human.closeInventory(); + } } } ```

Jikoo commented

2020-12-02 20:32:54 -05:00

(Migrated from github.com)

I'm definitely interested in helping work around this case, but isn't it possible to work around already without patching OpenInv? I.E.:

@EventHandler
public void onPlayerQuit(PlayerQuitEvent event) {
  closeAll(event.getPlayer().getInventory().getViewers());
  closeAll(event.getPlayer().getEnderChest().getViewers());
  OpenInv.getPlugin(OpenInv.class).unload(event.getPlayer());
}

private void closeAll(Iterable<HumanEntity> viewers) {
  Iterator<HumanEntity> iterator = viewers.iterator();
  while (iterator.hasNext()) {
    iterator.next().closeInventory();
  }
}

Deny moderators OpenInv.openoffline and voila, no offline access, right?

I'm definitely interested in helping work around this case, but isn't it possible to work around already without patching OpenInv? I.E.: ```Java @EventHandler public void onPlayerQuit(PlayerQuitEvent event) { closeAll(event.getPlayer().getInventory().getViewers()); closeAll(event.getPlayer().getEnderChest().getViewers()); OpenInv.getPlugin(OpenInv.class).unload(event.getPlayer()); } private void closeAll(Iterable<HumanEntity> viewers) { Iterator<HumanEntity> iterator = viewers.iterator(); while (iterator.hasNext()) { iterator.next().closeInventory(); } } ``` Deny moderators `OpenInv.openoffline` and voila, no offline access, right?

Combustible commented

2020-12-12 00:43:03 -05:00

(Migrated from github.com)

Hadn't thought of that - yeah, that does seem like it would work. I wish the permission wasn't needed though - because if anyone did want to use my redis sync plugin and openinv, that'd be something they'd have to do manually and if they forget they'd randomly lose data randomly for a long time before figuring out why. This was our problem - it wasn't at all clear that this was the interaction causing this, since it's quite rare to have someone's inventory opened when they log out. If I could solve it purely by "integrating" my plugin with the OpenInv API (the code snippet) then that would be perfect.

Could be as simple as an API method that says "disableOfflineViewing()"... but not sure if you think that's too ugly. Would only be a couple lines of code & no config file change though...

Actually, on second thought... I might not need to deny the permission. There's no player data for openinv to open - so I assume if the player file is missing, it just fails and doesn't open anything. Cool. I'll give this a try.

Hadn't thought of that - yeah, that does seem like it would work. I wish the permission wasn't needed though - because if anyone did want to use my redis sync plugin and openinv, that'd be something they'd have to do manually and if they forget they'd randomly lose data randomly for a long time before figuring out why. This was our problem - it wasn't at all clear that this was the interaction causing this, since it's quite rare to have someone's inventory opened when they log out. If I could solve it purely by "integrating" my plugin with the OpenInv API (the code snippet) then that would be perfect. Could be as simple as an API method that says "disableOfflineViewing()"... but not sure if you think that's too ugly. Would only be a couple lines of code & no config file change though... Actually, on second thought... I might not need to deny the permission. There's no player data for openinv to open - so I assume if the player file is missing, it just fails and doesn't open anything. Cool. I'll give this a try.

Jikoo commented

2020-12-12 09:49:57 -05:00

(Migrated from github.com)

Yeah, OpenInv shouldn't open a nonexistent player. Alternately, it should be possible to force users to not have permission to view offline via a PermissionAttachment if you're after maximum portability in a hurry - on login, just slap a new attachment with the node set to false on everyone.

I'm definitely still interested in implementing a cleaner solution on the OpenInv end, but I've been focused elsewhere and haven't really been working on this project much.

Yeah, OpenInv shouldn't open a nonexistent player. Alternately, it should be possible to force users to not have permission to view offline via a PermissionAttachment if you're after maximum portability in a hurry - on login, just slap a new attachment with the node set to false on everyone. I'm definitely still interested in implementing a cleaner solution on the OpenInv end, but I've been focused elsewhere and haven't really been working on this project much.

Combustible commented

2020-12-12 13:59:04 -05:00

(Migrated from github.com)

Hmm... No, this doesn't quite work, even though it seems like it should. Apparently getPlayer().getInventory().getViewers() is empty when I have an OpenInv viewer attached to it.

It looks like OpenInv's .getBukkitInventory().getViewers() is returning more things somehow. I don't really understand why this is happening, but it definitely is.

Hmm... No, this doesn't quite work, even though it seems like it should. Apparently ```getPlayer().getInventory().getViewers()``` is empty when I have an OpenInv viewer attached to it. It looks like OpenInv's ```.getBukkitInventory().getViewers()``` is returning more things somehow. I don't really understand why this is happening, but it definitely is.

Jikoo commented

2020-12-14 11:56:49 -05:00

(Migrated from github.com)

Hm. I know Bukkit and NMS actually keep separate lists, probably has to do with it. I think we used to manage the Bukkit list but it ended up causing duplicate entries. Will definitely look into that Eventually(TM).

Jikoo commented

2022-06-07 20:05:42 -04:00

(Migrated from github.com)

https://github.com/Jikoo/OpenInv/releases/tag/4.2.0

Combustible commented

2022-08-28 15:39:27 -04:00

(Migrated from github.com)

Just discovered this as I'm trying to rebase my ancient disabling patch onto OpenInv so I can update my server to 1.18.2. Hooray, thank you very much for implementing this! My one-patch fork can finally die :)

🎉 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: minster586/OpenInv#171