search

mt-mods / technic

Technic mod for Minetest
18 stars 25 forks source link

Protected chests lack ability to forge keys #378

Open mazes-80 opened 2 months ago

mazes-80 commented 2 months ago

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 commented 2 months ago

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

S-S-X commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Should be fairly easy to add, just need to copy the code here and adjust it for protected chests: https://github.com/mt-mods/technic/blob/bb70986b2ed64dba5d1285d348762cc20c0862bf/technic_chests/register.lua#L201-L219 To decide who can make a key, it would make sense to use minetest.is_protected.

S-S-X commented 2 months ago

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 commented 2 months ago

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:

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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