Yep but even this unfocus is only called from the client. So both safeguards (move away or lost focus) are in clients hand.
The only safety that really works is on OnActorUse where the server checks (once) itself via a linetrace if that actor is true or some dream of a hacked client. However the server never checks this again (for all those container use events). If it’s open then it’s open forever until the client is in the mood to tell the server he should close it.
I hope everybody would make this hack proof. Cheating sucks.
I think this have to be included into Server_UseContainerItem, Server_MoveContainerItem, Server_TakeContainerItem, Server_DepositContainerItem, Server_EquipFromContainer, Server_UnequipToContainer, Server_SplitContainerItem and Server_SplitItemFromContainer.
No just call the check before the function after the event, this inventory is only a framework so of course there will be some holes and features you will have to fill in on your own.
Of course after the event. But all those names I wrote (Server_UseContainerItem, Server_MoveContainerItem, Server_TakeContainerItem, Server_DepositContainerItem, Server_EquipFromContainer, Server_UnequipToContainer, Server_SplitContainerItem and Server_SplitItemFromContainer) are exactly such events. So I’m not sure why you say no. I’m even not sure why you placed this in an inventory event (instead of the container events) as the character can never run away from it’s controller that keeps the inventory component (that’s modified by server events) but I think I have to take a look to the inventory events as well afterwards. Probably some “IsDeadAndWouldTinkerAtInventory” prevention?
My inventory is setup similar to the ARPG inventory but its not the same asset, anyways my “UseInventoryItem” is used for all types of inventory’s ranging from Containers to Sub-Inventory’s which is why i have more variables on mine, and i was just giving you an example, there’s more than one way to add extra security find the best way for your project!
Ok. But it’s not the case for the ARPG inventory. Its ok if you give an example for something different… thanks. However if you take a look to that what I wrote then you should see that I suggested the same as you did… just at a place where it actually protects your containers in the original ARPG inventory. Placing that check with the same macro on the original ARPG at the event you posted in your screenshot would not protect your containers in the original ARPG inventory. So its a bit confusing if you say it have to be done different if you are talking about differnt things.
Well I did and I still do… but you said it was wrong and you did not say why. We are talking of a marketplace framework. So we should talk about the same thing. We don’t talk about extra security but basic security for multiplayer. I tend to replace those linetraces anyway as in my opinion this is great for FP but not optimal for TP but I would not suggest this as a solution for everybody who uses this framework. However… that said. You could create a framework cheat aware like you could create a framework threadsafe or not. Nobody said we would not add something ourself. Nobody expects a full game. Nobody wrotes that he gives up. But if you create something with multiplayer in mind such checks have to be implemented from scratch. Usually I’m not going to create my own events and come again later and think about some “Switch has authority” at a second run (where I have to rethink again what I thought last time when I created that function, event, actor, …). At least I try to avoid that as much as possible. So I was asking if this was just a mistake of the creator or if it’s expected and I oversee some further checks somewhere else probably. It seems there are some checks … but called from the wrong side (client).
What happen when a client is trying to move a item from there inventory to a container? what event is fired? asking me why i added it to UseInventoryItem when the answer is pretty clear? you also need to cover both ends not just moving items from a container i don’t like your tone so ill leave it here good luck.
Ether way its not really easily hacked a client cant move away from the open container when the movement is handled on the server so your really overlooking the point.
If you do this via drag and drop then it’s not the Server_UseInventoryItem so it’s not that pretty clear at the first look. Fixing something after somebody else work is not that intuitive and clear as you might think. However I found out now that it is fired if you move via right-click instead. So there might be more surprises.
The question was… why you said “no” as I wrote that this check have to be put to all those Server Container events. I don’t expect an answer anymore.
I never said I would stop at that point. I asked why you said Server container events are the wrong place. It seems there is missing cheat protection on various places. So of course this have to be checked further.
That was my main question. - Ok again… a open container does not tell the server to block its movement component. The client may move and the server is fine with that. The only reason why the container gets closed is because the Client tells the server that it should close (if it’s a nice Client). If you put a “Switch Has Authority” before the “ServerCloseContainer” in the original inventory player controller before that check at the movement part then the event never gets called. And it’s the same when you put it behind a “Is Local Controller” -> False?-> ServerCloseContainer branch. I’m not sure if it’s that hard to cheat.
@Neutronux I said no becuase i miss read your qustion, and if you can move your character with the container UI open then there is a problem somewhere as this shouldn’t happen.
Ok. Thanks. But my problem with that “if you can move character” is that the ServerCloseContainer is a Server event. As far my understanding of a Server event goes is that this is an event-type that’s created for the client to get something done one the server … with a “please”.
Like:
Client: Please mighty server spawn me a projectile.
Server: Ok you got enough ammo … or no way you are out of ammo bad client.
Client: Please mighty server open me that container.
Server: Ok you are close enough … or no way you are too far away bad client.
So far it’s implemented in the Action RPG… at least regarding the container.
However I’m in doubt that the Server even is even created for the other way around that sets the server in the position to ask with some please.
Server: But please tell me when I have to despawn that projectile. Ok?
Server: And please even tell me when I have to close that container if you move.
Client: Smiles. I swear by the flowerpot of my grandma that I would tell you if it’s the time for you todo so.
You could follow me? I don’t think that the close event could work that way if it’s a Server event … ever. So there is nothing that prevents the client to use the Container if he chooses to cheat and just says: Shame on you server if you don’t know yourself where I am if I tinker with your container then you deserve it that I still loot it from somewhere else.
Edit: Or in other words. Its the job of the client to call a server event and the job of the server to do something or reject something in the response to the request (triggered on the client) afterwards. But as far my understanding of the eventype Server goes is that it’s not the job of the Server to tell the client if he should call this or not (or if this is ever verified if its a event-type that’s created for the Clients wishes). Its like the server could tell me if I have to push a mouse button for a further projectile… just not manually via mouse but in hope the code in the client is unmodified (and a ServerCloseContainer comes because the Client sends it). The event that protects the container should not be an eventtype thats created to be triggered from the client but that’s called from the server no matter what (if the client ever sends something to the server or not). The event might stay but only for cosmetic stuff … like: Hey server let me know when I should close that window … as you would refuse to let me do something with it any longer. The point is… the server does not refuses it … if the client don’t brings him to that idea. At least as far my understanding of that used Eventtype goes.
@Neutronux Yes that is correct there is nothing preventing a client from moving away from the container once its open, the inventory reference to the container has already been set, so ether add that extra node to check or force close it if the client moves away.
@Neutronux
A server only solution would just be to move the close container call away from being input based and have a check run on the server to see if the characters position has changed(or velocity, etc) and if there is a valid container reference then call the close container logic and set the container reference to null. Easy peasy.
The ARPGIS demo content is just showing how to use the modular Inventory System. Please feel free to make changes to the existing InventoryPlayerController as the controller just sets up the inventory system as has some basic extras in there. It was never meant to be a fully finished game template that requires no changes, it’s purpose is just to demo one way to use the component based modular Inventory System.
To weigh in on @Neutronux and @OverRated_AU convo about security - typically in a multiplayer game you want to reduce as much info as possible being sent between Client and Server.
No checks need to be done on the Server side for what the Client is doing in terms of making sure Client-only things like UI close off or particle effects activate, etc. Leaving the Client to determine how it closes is fine and if someone hacks their Client to have weird UI behaviour then that isn’t going to be a security issue.
The security issue comes in to play if the client is still getting updates of container changes (new items placed in there, etc) when it shouldn’t be or if they can still drag items out of it and into their inventory and have it replicated to the Server. The way this is generally handled is by having the Server do a simple check on whether or not the interaction is still valid on the Server-side (whether it be based on character position or other parameters) and ensuring it does not send out container updates to character’s that cannot interact with it.
Having the Client tell the Server when it has moved or having the Server actively checking this is unnecessary, the Server should act independently in this scenario and when it runs any logic for the container it determines who it is updating or whether an interaction is valid when one is requested by a Client.
In regards to Neutronux’s valid query, he was wondering if there are possible security checks done somewhere else that isn’t directly apparent. While he doesn’t directly answer the question, the developer seems to imply that there isn’t with saying that it was not designed as a solution but as an example; though, I’m not sure why it is stated having multiplayer-support when he is saying the multiplayer is actually implemented as a demo example (although likely the multiplayer-support just implies replication of inventory and nothing further, though that leaves it as an incomplete multiplayer system).
If you’re developing a serious game that you don’t want hackers to exploit and want more perspective on it - I’d advise scrapping any downloaded multiplayer logic and implementing your own, rather than relying on a Marketplace developer to make it hack-proof; especially as they can’t always cover everyone’s use-cases. It is best to save using implementations provided by others as a reference, rather than as a solution; however, a valid use-case would be for a prototype where flaws and design-oversights are not of any concern.
Pirate the developer has always stated the UI is for demoing the inventory only, if people want to use this as a starting point there free to but it was never intended to be used out of the box, adding a server check on a server function isn’t going to add anymore bandwidth all the server is doing is looking to see if the client is valid before using these functions, your statement is pointless and has no meaning, the client role is to call these server events and if these server event has no way of knowing if the client is valid to use them is just plan dumb which is what you pretty much just said.
100% multiplayer support does not mean only replication… at least not for everybody. The setup for the player controller might be demo … but all those Server do something with your container events are part of the framework and don’t verify (as some other Server events do). So in my opinion they deserved a bit more love regarding cheat prevention. Anyway it is like it is… and I think it’s not all bad. I still don’t regret my purchase at all. However adding cheat prevention is a bit like using a non-threadsafe library in a multithreading environment. It’s not always just that easy like to put a mutex on the top afterwards.
I think he don’t means the server check but how the gui gets closed. As far I remember actually the clients sends the server that he should close the inventory and (only) then the server sends back the client that he should close the GUI. This “close GUI” could be a predicted client only implemention as well as the “may use container” should be a server only implemention. But I don’t care that much regarding this one more event as it does not happen every frame.
100% multiplayer supported means the inventory system was designed with optimizations for MP in mind and following good design patterns. I’m not really sure what the problem is on your end.
But saying it doesn’t support multiplayer and it’s bad is your opinion and I respect that. If you would like a refund let me know and contact epic.
There is a lot of inaccurate assumptions being made on this topic and unfortunately I’m at the airport and not able to call out and respond to every point.
When you send the client command to the server to close the inventory it invalidates the reference so the problem you seem to really have is with the server just telling the client it’s finished processing the clients request then the client closes the UI.
If you want to trigger the client UI close first then call the server logic that’s your choice. But going out of your way to say it’s completely hackable because the UI closes last not first is simply untrue. The ARPGIS was also built with the assumption most people buying it aren’t programmers or understand networking in bluepeints I made the choice to flow the logic in a way to help people understand sending commands and recieving a response from the server if the command is validated. This design is consistant in all of the ARPGIS and allows simply following the flow of logic making it easy to understand and practically self documents what is going on. This design choice does not add any real networking bandwidth overhead for the benefits of readability and understanding of logic gained.
I think you misunderstood that. Nobody said that. The gui does not matter regarding security. Its hackable if the server still allows the client to do things when he should not be able to do it anymore or depends on the client to make this decision. This is the case in the player controller of the demo that sends an event after movement (good) but a (ClientTo)Server event (bad). Anyway this was discussed already and you are right you could fix this via invalidating the container reference (triggered from the server - instead of the client as it’s the case right now). I’ve fixed that already. So far I just gave an answer to the post of OverRated_AU as an answer to Furinyx… because I don’t think his plans was dumb as he wrote.
Yes it looks clean. Thanks. This was even one reason why I choosed to continue without recreating it from scratch. Its easy to oversee some paths if you did not create something yourself regarding security.
Hmm, for some reason I am able to add an item to my character’s inventory but when I check for it, it isn’t returning as there. I duplicated the Misc_Human_Skull item and customized its inventory item properties and then made it so that when I use a certain actor it will give the player that new item. I am able to see the item in my inventory, drop it, pick it back up, etc. Problem is that when I call CheckInventoryForItemAmount with the new item’s ItemID and an amount of 1, HasItemAmount returns false. Any ideas?
is the check being done as the server or the client? It has to be done by the server. A good way to debug is with the print string node as it will output Server: first if the command is run on the server.
Cheating sucks.