Neos-Metaverse / NeosPublic

A public issue/wiki only repository for the NeosVR project
193 stars 9 forks source link

Avatar protection deleting slot #2897

Closed ghost closed 3 years ago

ghost commented 3 years ago

Is your tweak request related to a problem? Please describe.

I was working on something and forgot about the x button on avatar protection causing the slot to be deleted now. I ended up losing a lot of work.

Describe what would you like tweaked

Change the component to work the way it did before and allow protection owner to remove one instance of it with the x. Maybe add the same context menu confirmation for the x button that would appear when clicking remove all.

Describe alternatives you've considered

Alternatively add a second button next to the remove instances button that only removes that exact instance of protection. Although I don't know what exactly is different about this compared to the x.

Additional context

No response

Khosumi commented 3 years ago

this is a very strange change as it removes a lot of flexibility with the protection component.

Removing a single instance now requires workarounds. Considering it is a widely used component, changing its behaviour suddenly without previous communication seems jarring.

mralext20 commented 3 years ago

a simple reminder that the SimpleAvatarProtection component is

from this discord message

I don't plan on making any additions to SimpleAvatarProtection sorry! The things I added are already spilling out and causing mess in a bunch of places and I don't want to make it worse.

It needs a properly designed system, the object ID / license system will take care of that.

Khosumi commented 3 years ago

we have currently no other methods of protecting assets such as models, textures, etc. As long as this is the case, it will require some flexibility. I understand the reasoning behind putting it on the backburner until object ID comes around but we still need to protect our assets.

We also have no idea what this security concern is, so as far as I am concerned we are left in the dark about this change.

kazu0617 commented 3 years ago

we have currently no other methods of protecting assets such as models, textures, etc. As long as this is the case, it will require some flexibility. I understand the reasoning behind putting it on the backburner until object ID comes around but we still need to protect our assets.

We also have no idea what this security concern is, so as far as I am concerned we are left in the dark about this change.

But In neos-updates, you can check this message.


Security:
- SimpleAvatarProtection removal confirmation is now spawned in the userspace (reported by @art0007i, Ticket #892900)
- SimpleAvatarProtection will now self-destruct if removed by the X button in the inspector by the owner. Use the dedicated button for removal of all instances (reported by @art0007i, Ticket #892900)
``
Khosumi commented 3 years ago

Changing how security works should be announced ahead of time, dedicating a single line in a patch note can very easily be missed, as evidenced by this thread and the general confusion I heard when I brought this topic up with all of my friends.

Moreover it doesn't address the concerns about flexibility. This is fundamentally different as to how it worked previously. Having a button to remove all instances at once is great, but making it the only option is not so great. For example I have an avatar with multiple meshes, and instead of simply removing the 1 component, I either need to remove ALL and reattach security components on every other assets, or reparent before removing instances.

Frooxius commented 3 years ago

I'm sorry that you lost work, but unfortunately this is a tricky situation. We changed the behavior due to an exploit report we have received from the community.

This component was never meant to be for a strong protection, it's a very simple protection. We could restore the old behavior, but that would also reopen the security hole. We can do that, but you'll have to accept the fact that it'll have weakened protection.

If you want a system that will provide more robust protection, while also having a lot more flexibility, please vote for the https://github.com/Neos-Metaverse/NeosPublic/issues/21. Such system takes significant amount of time to design and implement, which means we'll have to prioritize it over other larger features that you're also asking for.

Electronus commented 3 years ago

perhaps having a remove instance button that also has the confirmation and removing the "X" button would help with this component?

Frooxius commented 3 years ago

We can add another button to remove a single instance, but the X button is added automatically for all components, regardless of type, the component doesn't have control over that.

Electronus commented 3 years ago

I feared that would be the case. Sad times.

Zyzyl commented 3 years ago

A warning message was recently added to explain caveats around the behaviour of the CharacterController component. Is that feasible here as well (i.e. indicate not to use the X)?

Frooxius commented 3 years ago

Yes that can be added too.

kazu0617 commented 3 years ago

Along with this, would adding a button that says "Delete only this component" in addition to "Delete all components under the hierarchy" leave a security hole open? (If it does, then so be it, but if not, I think that would be a good idea.

ProbablePrime commented 3 years ago

Would probably be the appropriate terminology.

Frooxius commented 3 years ago

Made the changes in 2021.9.1.643, thanks for the feedback!