Ensure user state is never conveyed for "appear offline" users #257
Conversation
| @@ -68,7 +68,6 @@ public override async Task OnConnectedAsync() | |||
| usage.Item = new MetadataClientState(Context.ConnectionId, Context.GetUserId(), versionHash); | |||
|
|
|||
| await logLogin(usage); | |||
| await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); | |||
There was a problem hiding this comment.
Just to confirm intent, this is deleted because it's basically conveying nothing, correct? I'm not really sure why I put that there in the first place but I guess that makes sense.
There was a problem hiding this comment.
Yeah, it should never fire anything.
|
|
||
| return Clients.Group(ONLINE_PRESENCE_WATCHERS_GROUP).UserPresenceUpdated(userId, userPresence); | ||
| } | ||
|
|
||
| private bool shouldBroadcastPresentToOtherUsers(MetadataClientState state) |
There was a problem hiding this comment.
Method name is typo'd:
| private bool shouldBroadcastPresentToOtherUsers(MetadataClientState state) | |
| private bool shouldBroadcastPresenceToOtherUsers(MetadataClientState state) |
(don't apply directly for obvious reasons)
| @@ -129,7 +129,14 @@ public async Task UpdateStatus(UserStatus? status) | |||
| if (usage.Item.UserStatus != status) | |||
| { | |||
| usage.Item.UserStatus = status; | |||
| await broadcastUserPresenceUpdate(usage.Item.UserId, usage.Item.ToUserPresence()); | |||
|
|
|||
| if (status == UserStatus.Offline) | |||
There was a problem hiding this comment.
Any reason why this is a local check that isn't using the helper method? (shouldBroadcastPresentToOtherUsers() I mean)
There was a problem hiding this comment.
It's actually a different condition – this case we explicitly want to sent the null status just once when a user transitions to "appear offline". See comment just below, hopefully.
i tried this and it didn't read as well as how i have it now. if we end up having this in more places, we could reconsider of course. |
Noticed during testing that "appear offline" doesn't actually work under the surface. If someone was to watch packets, they would be able to see invisible users and track their activity.
This change ensures that
Of important note, I think this is only the start of the issue, as spectator server is probably still revealing play state to anyone that subscribes for it.. Fixing this either requires merging hubs (has been discussed IRL for other reasons) or possibly having the client conveying its "online" state when beginning a play session.
(Not touching on that today, will open an issue)