SpongeAPI icon indicating copy to clipboard operation
SpongeAPI copied to clipboard

Vanished players visible in command elements and tab completion

Open WillBAnders opened this issue 5 years ago • 2 comments

The GenericArguments.player() element does not take into account players which are vanished when parsing, especially with tab completing. Players can thus use tab completion to determine if a vanished administrator is online.

I would consider this a change for API 8 due to the potential consequences of changing behavior here. In the case of vanish, this would also require an API method to determine whether a player can see others who are vanished (like an operator).

Vanish may also not be the only criteria here worth considering as well, and perhaps a more general is visible or is known type of behavior for specifying what things a source is aware of during the execution of a command. For example, if a player does not have permission for a certain world that should be hidden as well (though I believe a custom element handles this better, elaborated below).

Ultimately, any user-facing element should probably be controlled by the plugin to handle this type of contextual information. Things that are more admin/operator oriented could use the more 'generic' arguments as they should show everything anyways. I'm not up to date on the current status of arguments in API 8, but I think there's a lot of contextual restrictions that need to be considered that tend to be ignored in plugins resulting in this type of behavior.

WillBAnders avatar Aug 20 '19 18:08 WillBAnders

This is a far bigger issue than you think this is - and it's a lot of words basically asking for the following:

  • Sponge to implement contextual data, at least as far as vanish goes
  • The command system to recognise this

You're far better off asking about Contextual Data first, or at least asking to have per-source-target-combo vanish states. Making the command system recognise this will be simple once that's done.

Now, this won't happen until at least API 8, considering we're going to be focussing efforts on that very soon. However, I'll give answers based on API 7.

The GenericArguments.player() element does not take into account players which are vanished when parsing, especially with tab completing

And it never will. There might be an additional element if this comes to pass, but the players element should be exactly that, the players that are online.

perhaps a more general is visible or is known type of behavior for specifying what things a source is aware of during the execution of a command

Surely having contextual vanish would be enough?

For example, if a player does not have permission for a certain world that should be hidden as well (though I believe a custom element handles this better, elaborated below)

Yes, because the world element might not be used for just warping to a world. It might be used for an element to find the requirements for accessing the world, or you might be able to just view stats about it, or it generates an object based on the world - I don't know.

Ultimately, any user-facing element should probably be controlled by the plugin to handle this type of contextual information.

Correct - a custom element. I am not about to go make a complicated argument system that everyone will shun because I've tried to make it so every plugin can control every other plugin's interaction.

I think there's a lot of contextual restrictions that need to be considered that tend to be ignored in plugins resulting in this type of behavior.

What restrictions - apart from vanish this isn't really that clear to me? The only one I can think of is permissions based - select players with(out) a permission. I suppose something like this is possible with a complex callback system where plugins supply callbacks (like context calculators) that allow for the player list to be filtered when the element is called. However, and this is really a big however, plugins do not need to use our parameter systems. Many might use ACF, or Scammander, or their own system. API 8 may even make it easier for those to be used - in which case they will ignore it anyway.

I don't really think we should put in the effort to create a web of callbacks for elements, only for the bigger plugins to not use them. I can just about see doing it for contextual vanish, especailly as that will be 95% of the use case, I'm sure, but I'd rather keep the complexity down as much as possible.

dualspiral avatar Aug 20 '19 19:08 dualspiral

This came around as an issue where players were able to determine if a user was vanished by using tab completion - kind of similar to website logins that say the username was correct but the password wasn't; it gives information to them they're not supposed to know.

This is a large issue, so I'll separate the two main things I have in my mind here. The first, as you've said, is contextual data - specifically for this case the ability to determine if a player is viewable to another (which might go beyond vanish?). I don't have too much to add here that hasn't been discussed elsewhere on this.

The second is how to integrate these types of contextual checks into commands. Put simply, if any player has access to a command using GenericArguments.player(), they can determine who is online. I don't consider to be an issue with the element, but it is something that needs to be considered when it is used. Adding a new element for it is also what I'd recommend, and moving forward having GenericArguments.allPlayers() and GenericArguments.viewablePlayers() to further differentiate them I think would be a good idea.

I completely agree that any type of crazy callback system is a bad idea (both in general and within Sponge), so the natural solution seems to be conveying to plugin developers that they should incorporate context into their commands. The issue is that sharing this information is difficult, and perhaps something that could be incorporated in the PermissionService further down the line.

To summarize:

  • ContextualData is a plus, hope to see that in API 8.
  • I do not advocate for changing GenericArguments.player(). I do think the addition of one for viewable players and a rename for clarity is a good idea moving forward.
  • Plugin developers need to be aware that things like GenericArguments.player() can give players information they shouldn't have, and more commonly tab complete things that they can't use. A greater notice of this issue and encouraging custom elements for these cases will help developers make well-designed commands.
  • The addition of arbitrary contextual data for things like 'can view world' or 'can interact with block in this claim' type of stuff would be helpful for plugin interoperability.

WillBAnders avatar Aug 20 '19 21:08 WillBAnders