Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protected chests lack ability to forge keys #378

Open
mazes-80 opened this issue Sep 6, 2024 · 10 comments
Open

Protected chests lack ability to forge keys #378

mazes-80 opened this issue Sep 6, 2024 · 10 comments
Labels
Enhancement New feature or request

Comments

@mazes-80
Copy link

mazes-80 commented Sep 6, 2024

Use case: allow a "guest" to use a protected chest even he is not member of one area.
Notice: creating an area limited to the specific chest also works, but it consumes one area protection "slot", by default you don't own that many slot as a classical user.

Adding a on_skeleton_key_use() and defining ways to open chest seems quite straightforward.

May also require to think of who can emit keys: only chest owner ? all people owning the area ? Only people listed able to make keys by the owner ? Another checkbox to define who can per chest ?

@BuckarooBanzay BuckarooBanzay added Enhancement New feature or request Controversial labels Sep 6, 2024
@BuckarooBanzay
Copy link
Member

Sounds good in theory but i'm not sure if this will over-complicate things on the usage- and maybe code-side too 🤔

@S-S-X
Copy link
Member

S-S-X commented Sep 7, 2024

not sure if this will over-complicate things on the usage- and maybe code-side too

Didn't really check but I don't think this will complicate code that much really and probably can be fairly easily implemented, pretty much completely isolated and optional feature without sprinkling config or mod checks everywhere.

Pretty sure it is just extra functions added for nodes and nothing else so should be able to be pretty much completely enclosed in its own lua files easily.

But I also remember looking at something like this with another mod and somehow remember there might have been other issues with keys, just can't remember what it was... I think, if implemented, these can however be up to the server owner assumin it is configurable instead of always available thing.

@S-S-X
Copy link
Member

S-S-X commented Sep 7, 2024

Possibly starting to remember something I think it might have been that implementation I was talking about was a bit hacky involving additional external state tracking and protection overrides for key usage.

@OgelGames
Copy link
Contributor

OgelGames commented Sep 7, 2024

Should be fairly easy to add, just need to copy the code here and adjust it for protected chests:

if data.locked then
def.on_skeleton_key_use = function(pos, player, newsecret)
-- Copied from default chests.lua
local meta = minetest.get_meta(pos)
local owner = meta:get_string("owner")
local player_name = player:get_player_name()
if owner ~= player_name then
minetest.record_protection_violation(pos, player_name)
minetest.chat_send_player(player_name, "You do not own this chest.")
return nil
end
local secret = meta:get_string("key_lock_secret")
if secret == "" then
secret = newsecret
meta:set_string("key_lock_secret", secret)
end
return secret, "a locked chest", owner
end
end

To decide who can make a key, it would make sense to use minetest.is_protected.

@S-S-X
Copy link
Member

S-S-X commented Sep 7, 2024

To decide who can make a key, it would make sense to use minetest.is_protected.

On the other hand this can cause issues with revoking access if owner used both keys and protection, basically owner can't be sure who has a key. Sure there's both upsides and downsides on this but I think likely for those who don't know exact underlying logic it would be safer and still good enough to require actual owner to make keys, I mean it isn't immediately obvious who can make keys and for other implementations I think it is always restricted only to owner.

I'd say best to carefully weight upsides and downsides from average players perspective rather than from mod developer or power users perspective.

@mazes-80
Copy link
Author

mazes-80 commented Sep 7, 2024

i'm not sure if this will over-complicate things on the usage-

It may, quite everything I proposed for the doors redo mod last year was merged except this exact same concept: protection levels for protected areas
The reason it was not merged: quite complex usage.
a case of protected things:

  • 0: toggle door
  • 1: toggle+share keys
  • 2: toggle+share keys+dig door

and maybe code-side too 🤔

Code side is not really that complex.
Apart using whitelist for people able to share keys ! In the upper sample I do not manage whitelists as it is just a thought I had yesterday while typing this enhancement proposal.

@mazes-80
Copy link
Author

mazes-80 commented Sep 7, 2024

On the other hand this can cause issues with revoking access if owner used both keys and protection, basically owner can't be sure who has a key. Sure there's both upsides and downsides on this but I think likely for those who don't know exact underlying logic it would be safer and still good enough to require actual owner to make keys, I mean it isn't immediately obvious who can make keys and for other implementations I think it is always restricted only to owner.

I looked up a bit and saw there is no owner meta for protected chests. For this mod, this approach would require to set owner meta when placing a protected chest

@mazes-80
Copy link
Author

mazes-80 commented Sep 8, 2024

This branch kind of works but without owner meta, protected chests are not able to use the default.can_interact_with_node function from MTG.
I will try to see if adding owner meta to protected chests does not interfere with other parts.
With this extra field it would work seamlessly with: keyring; as it overrides MTG function.

May PR be good to retrieve feedback and comments ? Or is it better to work separate until more advanced work ?

@S-S-X
Copy link
Member

S-S-X commented Sep 8, 2024

I will try to see if adding owner meta to protected chests does not interfere with other parts.

Automating it probably wont end well as we have no idea who's the owner.

This fact that there's no owner meta and that key crafting authorization would work completely different actually gives feeling that this probably should not be done just like that.

Technically on lua code everything is simple and easy to do but end result for delivered functionality is going to be both different from similar functionality and rather complicated. Trying out and comparing different approaches beforehand, even if just on paper, would be best thing to do here.

May PR be good to retrieve feedback and comments ? Or is it better to work separate until more advanced work ?

Yes @mazes-80 PR or even multiple PRs for different approaches (if you happen to have alternatives) would be good.

@mazes-80
Copy link
Author

mazes-80 commented Sep 8, 2024

I added an owner meta branch.
I still need to do some extra tests on the new behaviour before PR, but I'm now quite sure adding owner meta for protected chest does not have side effects on existing things from the code perspective.

The branch allows:

  • use with matching keys or keyrings in hand
  • share keys for the placer/owner
  • share keys for area members when setting enabled
  • set owner field when using key on chests without owner field

From an user perspective: owner field for protected chest is:

  • good if someone tries to "steal" your chest by claiming a free area, you can get back stuff but you lost your chest, but stuff shared like in a normal chest shouldn't be that rare.
  • bad if an area "admin" tries to exclude you from a zone, even any zone member can dig the chest, to place it back, it's easy to forget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants