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

Storage promotion/demotion and legacy object unpinning for entity state API #700

Merged
merged 13 commits into from
Jul 17, 2023
6 changes: 5 additions & 1 deletion lib/ret/storage.ex
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,10 @@ defmodule Ret.Storage do
end
end

def promote(_id, nil, nil, _account) do
{:error, :not_allowed}
end

def promote(nil, _key, _promotion_token, _account) do
{:error, :not_found}
end
Expand Down Expand Up @@ -329,7 +333,7 @@ defmodule Ret.Storage do
end
end

# If an owned file does not have a promotion token associated with it, it can be promoted with any given
# If an owned file does not have a promotion token associated with it, it can be promoted with any given
# promotion token, including nil.
defp check_promotion_token(nil, _token), do: {:ok}
defp check_promotion_token(actual_token, token) when actual_token == token, do: {:ok}
Expand Down
95 changes: 79 additions & 16 deletions lib/ret_web/channels/hub_channel.ex
Original file line number Diff line number Diff line change
Expand Up @@ -467,10 +467,17 @@ defmodule RetWeb.HubChannel do
def handle_in("save_entity_state", params, socket) do
params = parse(params)

with {:ok, hub} <- authorize(socket, :write_entity_state),
{:ok, %{entity: entity}} <- Ret.create_entity(hub, params) do
with {:ok, hub, account} <- authorize(socket, :write_entity_state),
{:ok, %{entity: entity}} <- Ret.create_entity(hub, params),
:ok <- maybe_promote_file(params, account, socket) do
entity = Repo.preload(entity, [:sub_entities])
broadcast!(socket, "entity_state_saved", EntityView.render("show.json", %{entity: entity}))

broadcast!(
socket,
"entity_state_saved",
EntityView.render("show.json", %{entity: entity})
)

{:reply, :ok, socket}
else
{:error, reason} ->
Expand All @@ -481,7 +488,7 @@ defmodule RetWeb.HubChannel do
def handle_in("update_entity_state", %{"update_message" => update_message} = params, socket) do
params = parse(params)

with {:ok, hub} <- authorize(socket, :write_entity_state),
with {:ok, hub, _account} <- authorize(socket, :write_entity_state),
{:ok, _} <- Ret.insert_or_update_sub_entity(hub, params) do
broadcast!(socket, "entity_state_updated", update_message)
{:reply, :ok, socket}
Expand All @@ -491,19 +498,46 @@ defmodule RetWeb.HubChannel do
end
end

def handle_in("delete_entity_state", %{"nid" => nid, "file_id" => file_id}, socket) do
{:ok, hub, account} = authorize(socket, :write_entity_state)

with {:ok, _} <- Ret.delete_entity(hub.hub_id, nid) do
OwnedFile.set_inactive(file_id, account.account_id)
:ok
else
_ ->
if account |> can?(pin_objects(hub)) do
RoomObject.perform_unpin(hub, nid)
OwnedFile.set_inactive(file_id, account.account_id)
end
end

broadcast!(socket, "entity_state_deleted", %{
"nid" => nid,
"creator" => socket.assigns.session_id
})

{:reply, :ok, socket}
end
Copy link
Contributor

@johnshaughnessy johnshaughnessy Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handler (and the following one) looks suspicious.

If for some reason the delete_entity_state request does not result in the entity state being deleted (and the associated OwnedFile becoming inactive), this function should reply_error with a reason. The reasons we should catch are:

  • If the socket is unauthorized,
  • If delete_entity fails (e.g. the entity doesn't exist),
  • (Maybe?) If the OwnedFile doesn't exist

Also, I think we need to handle the RoomObject records slightly differently.

On the client, when we load the legacy room objects, we synthesize messages for them so that the client can act as if they are entity create/update messages. (See loadLegacyRoomObjects and messageForLegacyRoomObject in the client.)

We would like to add to this flow a step where a client, when it synthesizes this message, also calls save_entity_state so that that data is migrated to the new EntityState storage (non destructively).

Therefore, when reticulum receives an delete_entity_state directive, there are two (valid) states we should be concerned with:

  1. There is both an EntityState and a RoomObject for this nid. We need to delete both.
  2. There is only an EntityState for this nid. We only need to delete the entity state.

Deleting a non-existant RoomObject is probably no trouble, so trying to delete both each time is probably a fine strategy (knowing that many times, the RoomObject won't exist).

And, as you have done here -- if there is a file_id in the payload, we need to mark the associated OwnedFile as inactive. To avoid duplicating code, might consider having a single handle_in("delete_entity_state", %{"nid" => nid } = payload ... , and if payload has a file_id, call set_inactive. But that's more a matter of style, not correctness.


def handle_in("delete_entity_state", %{"nid" => nid}, socket) do
with {:ok, hub} <- authorize(socket, :write_entity_state),
{:ok, _} <- Ret.delete_entity(hub.hub_id, nid) do
broadcast!(socket, "entity_state_deleted", %{
"nid" => nid,
"creator" => socket.assigns.session_id
})
{:ok, hub, account} = authorize(socket, :write_entity_state)

{:reply, :ok, socket}
with {:ok, _} <- Ret.delete_entity(hub.hub_id, nid) do
:ok
else
{:error, reason} ->
reply_error(socket, reason)
_ ->
if account |> can?(pin_objects(hub)) do
RoomObject.perform_unpin(hub, nid)
end
end

broadcast!(socket, "entity_state_deleted", %{
"nid" => nid,
"creator" => socket.assigns.session_id
})

{:reply, :ok, socket}
end

def handle_in("get_host", _args, socket) do
Expand Down Expand Up @@ -950,7 +984,7 @@ defmodule RetWeb.HubChannel do

with {:ok, account} <- account_for_socket(socket),
:ok <- auth(hub, account, :write_entity_state) do
{:ok, hub}
{:ok, hub, account}
end
end

Expand Down Expand Up @@ -1445,14 +1479,43 @@ defmodule RetWeb.HubChannel do
%{nid: nid, root_nid: root_nid, update_message: Jason.encode!(update_message)}
end

defp parse(%{"nid" => nid, "create_message" => create_message, "updates" => updates}) do
defp parse(
%{
"nid" => nid,
"create_message" => create_message,
"updates" => updates
} = params
) do
%{
nid: nid,
create_message: Jason.encode!(create_message),
updates: Enum.map(updates, &parse/1)
updates: Enum.map(updates, &parse/1),
promotion_token: Map.get(params, "promotion_token", nil),
file_id: Map.get(params, "file_id", nil),
file_access_token: Map.get(params, "file_access_token", nil)
}
end

defp maybe_promote_file(%{file_id: nil} = _params, _account, _socket) do
:ok
end

defp maybe_promote_file(params, account, _socket) do
with {:ok, _owned_file} <-
Storage.promote(
params.file_id,
params.file_access_token,
params.promotion_token,
account
) do
OwnedFile.set_active(params.file_id, account.account_id)
:ok
else
{:error, reason} ->
{:error, reason}
end
end

defp reply_error(socket, reason) do
{:reply, {:error, %{reason: reason}}, socket}
end
Expand Down
50 changes: 49 additions & 1 deletion test/ret_web/channels/entity_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ defmodule RetWeb.EntityTest do
import RetWeb.EntityTestUtils, only: [read_json: 1]

alias RetWeb.SessionSocket
alias Ret.{Repo, HubRoleMembership}
alias Ret.{Repo, HubRoleMembership, Storage}

@payload_save_entity_state read_json("save_entity_state_payload.json")
@payload_save_entity_state_2 read_json("save_entity_state_payload_2.json")
@payload_save_entity_state_promotable read_json("save_entity_state_payload_promotable.json")
@payload_save_entity_state_unpromotable read_json("save_entity_state_payload_unpromotable.json")
@payload_update_entity_state read_json("update_entity_state_payload.json")
@payload_delete_entity_state read_json("delete_entity_state_payload.json")
@default_join_params %{"profile" => %{}, "context" => %{}}
Expand Down Expand Up @@ -70,6 +72,52 @@ defmodule RetWeb.EntityTest do
reason: :unauthorized
}
end

test "save_entity_state succeeds if provided correct promotion keys", %{
socket: socket,
hub: hub,
account: account
} do
%HubRoleMembership{hub: hub, account: account} |> Repo.insert!()
temp_file = generate_temp_file("test")

{:ok, _, socket} =
subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(account))

{:ok, uuid} = Storage.store(%Plug.Upload{path: temp_file}, "text/plain", "secret")

updated_map =
@payload_save_entity_state_promotable
|> Map.put("file_id", uuid)
|> Map.put("file_access_token", "secret")

assert_reply push(socket, "save_entity_state", updated_map), :ok
end

test "save_entity_state fails if provided incorrect promotion parameters", %{
socket: socket,
hub: hub,
account: account
} do
%HubRoleMembership{hub: hub, account: account} |> Repo.insert!()
temp_file = generate_temp_file("test2")

{:ok, _, socket} =
subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(account))

{:ok, uuid} = Storage.store(%Plug.Upload{path: temp_file}, "text/plain", "secret")

updated_map =
@payload_save_entity_state_unpromotable
|> Map.put("file_id", uuid)
|> Map.put("file_access_token", " not_secret")

assert_reply push(socket, "save_entity_state", updated_map),
:error,
%{
reason: :not_allowed
}
end
end

test "update_entity_state overwrites previous entity state", %{
Expand Down
54 changes: 54 additions & 0 deletions test/support/utils/save_entity_state_payload_promotable.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
{
"nid": "hvpg97x",
"create_message": {
"version": 1,
"networkId": "hvpg97x",
"prefabName": "media",
"initialData": {
"src": "https://hubs.local:4000/files/101ac987-61f4-4645-b512-def3f0336fdf.jpg?token=2bbfa5ff552a159cff50b8bc212a477c",
"recenter": true,
"resize": true,
"animateLoad": true,
"fileId": "101ac987-61f4-4645-b512-def3f0336fdf",
"isObjectMenuTarget": true
}
},
"updates": [
{
"root_nid": "hvpg97x",
"nid": "hvpg97x",
"update_message": {
"nid": "hvpg97x",
"lastOwnerTime": 1157850816,
"timestamp": 1157852116,
"owner": "9eac3551-20d7-4b04-80fc-2009b5e8fe10",
"data": {
"networked-transform": {
"version": 1,
"data": {
"position": [
44.335304260253906,
1.4967195987701416,
-6.183996677398682
],
"rotation": [
-0.03446304053068161,
0.6161360144615173,
0.026999054476618767,
0.7864221334457397
],
"scale": [
1,
1,
1
]
}
}
}
}
}
],
"file_id": "101ac987-61f4-4645-b512-def3f0336fdf",
"file_access_token": "2bbfa5ff552a159cff50b8bc212a477c",
"promotion_token": "1701f2fc68081168c7b19ebfabc76dc6"
}
54 changes: 54 additions & 0 deletions test/support/utils/save_entity_state_payload_unpromotable.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
{
"nid": "hvpg97x",
"create_message": {
"version": 1,
"networkId": "hvpg97x",
"prefabName": "media",
"initialData": {
"src": "https://hubs.local:4000/files/101ac987-61f4-4645-b512-def3f0336fdf.jpg?token=2bbfa5ff552a159cff50b8bc212a477c",
"recenter": true,
"resize": true,
"animateLoad": true,
"fileId": "101ac987-61f4-4645-b512-def3f0336fdf",
"isObjectMenuTarget": true
}
},
"updates": [
{
"root_nid": "hvpg97x",
"nid": "hvpg97x",
"update_message": {
"nid": "hvpg97x",
"lastOwnerTime": 1157792473,
"timestamp": 1157845450,
"owner": "9eac3551-20d7-4b04-80fc-2009b5e8fe10",
"data": {
"networked-transform": {
"version": 1,
"data": {
"position": [
44.30113220214844,
1.4787851572036743,
-5.9799041748046875
],
"rotation": [
-0.032501693814992905,
0.6693336963653564,
0.029331713914871216,
0.7416709065437317
],
"scale": [
1,
1,
1
]
}
}
}
}
}
],
"file_id": "101ac987-61f4-4645-b512-def3f0336fdf",
"promotion_token": "1701f2fc68081168c7b19ebfabc76dc6",
"file_access_token": "dasfaisodfjoein0ewoINCORRECT"
}