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.

Sign up for GitHub

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

Permission updates authorization #702

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Permission updates authorization #702

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3dc2795
Auth permission updates
keianhzo Jun 23, 2023
38306cd
Correctly handle message permissions
keianhzo Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions lib/ret_web/channels/hub_channel.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,17 +297,23 @@ defmodule RetWeb.HubChannel do
account = Guardian.Phoenix.Socket.current_resource(socket)
hub = socket |> hub_for_socket

if (type != "photo" and type != "video") or account |> can?(spawn_camera(hub)) do
broadcast!(
socket,
event,
payload
|> Map.delete("session_id")
|> Map.put(:session_id, socket.assigns.session_id)
|> payload_with_from(socket)
)
if (type == "photo" and type == "video" and
account
|> can?(spawn_camera(hub))
|> Kernel.not()) or
(type == "permission" and hub |> Ret.Hub.is_owner?(account.account_id) |> Kernel.not()) do
{:noreply, socket}
Copy link
Contributor

@johnshaughnessy johnshaughnessy Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This tuple does not appear to be returned from the function, which means the checks in this if have effectively been removed, since the code execution will always fall through to the broadcast! call below.

Copy link
Contributor

Choose a reason for hiding this comment

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

This check should not be for whether the account is_owner, but whether the account can?(update_hub(hub)).

Ideally, though, a client would not need to broadcast this change to its peers in a separate "message" -- reticulum would broadcast any changes after handling them in the handle_in("update_hub" function (which also means that the authorization is moved to only one spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm right type == "photo" and type == "video" will be always false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Actually I realized that I was looking at this the wrong way.

The actual permission updates are handled by handle_in("update_hub") as @johnshaughnessy mentioned and that is working fine as we authorize there. What this handler wasn't doing is authorizing the permission and chat messages so a malicious party could:

  • Send a fake permission message making everyone in the room believe that the voice/chat has been disabled (independently of the actual permission value)
  • Send a chat message as we are not authorizing those here at all.

I think my latest commit correctly addressed these issues.

Copy link
Contributor

@takahirox takahirox Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking where to check the permissions (that don't cause any side effect in server side)...

Ideally in the clients? With the future Add-on APIs addon creators may add their own permission types and most of them might not likt to edit server side code.

So, checking update_hubs permission in Reticulum may be good because it seems to have a side effect in Reticulum database. As @johnshaughnessy describes, perhaps broadcasting should happen in update_hubs handler after authorization.) But not sure about chat.

I don't mean to block this PR but want to clarify the future direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that if an add-on needs to add a new permissions, that needs to happen on reticulum first. Checking permissions can happen in the client's both ends as we are currently doing.

This PR addresses the chat messages authorization not the permission updates. Are you suggesting that we should authorize the chat messages also in the client ends instead of here? That can also be an option, not sure what would be the pros/cons but as we are already doing that for some other permissions might make sense. What do you think @johnshaughnessy?

Copy link
Contributor

@takahirox takahirox Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, different pros/cons depending on where to check the permissions. I think what we need to do is considernig balance and deciding a rule or policy. If the places where the permissions are checked are scattered, it causes harder maintainability.

We may merge this PR and discuss it in another issue separately.

Random thoughts before digging into the current implementation and performance profile: Perhaps it may be good to basically check the permissions in the clients end to avoid the performance bottleneck in Reticulum? And it may fit to add-on APIs because of no need to edit Reticulum code? Checking some permissions in the Reticulum may not be avoidable if it has any side effect in Reticulum database tho.

end

broadcast!(
socket,
event,
payload
|> Map.delete("session_id")
|> Map.put(:session_id, socket.assigns.session_id)
|> payload_with_from(socket)
)

{:noreply, socket}
end

Expand Down