Permission updates authorization #702
Conversation
keianhzo
force-pushed
the
permissions-check
branch
from
8d1067f
to
3dc2795
Compare
lib/ret_web/channels/hub_channel.ex
Outdated
| |> can?(spawn_camera(hub)) | ||
| |> Kernel.not()) or | ||
| (type == "permission" and hub |> Ret.Hub.is_owner?(account.account_id) |> Kernel.not()) do | ||
| {:noreply, socket} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If I'm right type == "photo" and type == "video" will be always false
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Fixes https://mozilla-hub.atlassian.net/browse/HUBSTRKY-773