Managed Groups First Cut #558

Open
sarah wants to merge 2 commits from managed-groups into master
Owner

Ok, this is part 2; the main change is this is rebased on some of the smaller changes we reviewed and merged in separately (like offline delivery and overlay checking)

This implements the most basic version of hybrid groups where a group manager can proxy messages between peers with some very basic filtering.

What is done:

  • Fix a bug in Event Manager that was resulting in duplicate queues / events
  • Get/Set Conversation Number Vars (used a lot in hybrid groups, cuts down on code)
  • Initial Cut of functionality.Hybrid for both Clients and the Manager
  • New Cwtch level APIs for Internal Conversation Management (NewConversation, InternalInsertMessage and SignMessage - these are needed because Extensions now need to manage inserting messages on behalf of a conversation.
  • Hybrid Groups integration test that simple sets up a group and has alice post a message and check that 1) bob receives the message and 2) alice acks the message
  • Channel routing based on Overlay Number (see code comments)
  • Moving some Test Utils around to make writing integration tests cleaner. There will probably be more of these.

Changes from part 1 noted below:

  • ACL / Membership Checking
  • Offline Delivery
  • Queue for Offline Delivery (essential for above)

Things not yet implemented:

  • Group Level Encryption / Key Management
  • Group Metadata Management (add/edit/rotate key etc.)
  • Any checking that clients are following the protocol defined by the formal model

However, even without all these things, I think now is the right time to review this PR and get a feel for this code. All those other things are nuanced and will require detailed review and I don't want them lost while reviewing all the stuff in this PR - which is mostly a structural sketch.


This is also the first version that can be nicely integrated into the UI and works. See screenshots in Cwtch Group...

Ok, this is part 2; the main change is this is rebased on some of the smaller changes we reviewed and merged in separately (like offline delivery and overlay checking) This implements the most basic version of hybrid groups where a group manager can proxy messages between peers with some very basic filtering. What is done: - [X] Fix a bug in Event Manager that was resulting in duplicate queues / events - [X] Get/Set Conversation Number Vars (used a lot in hybrid groups, cuts down on code) - [X] Initial Cut of functionality.Hybrid for both Clients and the Manager - [X] New Cwtch level APIs for Internal Conversation Management (`NewConversation`, `InternalInsertMessage` and `SignMessage` - these are needed because Extensions now need to manage inserting messages on behalf of a conversation. - [X] Hybrid Groups integration test that simple sets up a group and has alice post a message and check that 1) bob receives the message and 2) alice acks the message - [X] Channel routing based on Overlay Number (see code comments) - [X] Moving some Test Utils around to make writing integration tests cleaner. There will probably be more of these. Changes from part 1 noted below: - [X] ACL / Membership Checking - [X] Offline Delivery - [X] Queue for Offline Delivery (essential for above) Things not yet implemented: - [ ] Group Level Encryption / Key Management - [ ] Group Metadata Management (add/edit/rotate key etc.) - [ ] Any checking that clients are following the protocol defined by the formal model However, even without all these things, I think now is the right time to review this PR and get a feel for this code. All those other things are nuanced and will require detailed review and I don't want them lost while reviewing all the stuff in this PR - which is mostly a structural sketch. --- This is also the first version that can be nicely integrated into the UI and works. See screenshots in Cwtch Group...
dan was assigned by sarah 2024-05-06 22:09:54 +00:00
sarah added 1 commit 2024-05-06 22:09:55 +00:00
continuous-integration/drone/pr Build is pending Details
9e5cb8b403
Managed Groups First Cut
sarah force-pushed managed-groups from 9e5cb8b403 to e8f11cc329 2024-05-06 22:10:04 +00:00 Compare
dan reviewed 2024-05-07 20:04:13 +00:00
@ -44,0 +62,4 @@
profile.PublishEvent(ev)
}
}
messsages, _ = profile.GetMostRecentMessages(ci.ID, 2, 0, uint(100))
Owner

constant 0 and 2

constant 0 and 2
dan reviewed 2024-05-07 20:06:26 +00:00
@ -44,0 +57,4 @@
body := message.Body
ev := event.NewEvent(event.SendMessageToPeer, map[event.Field]string{event.ConversationID: strconv.Itoa(ci.ID), event.RemotePeer: ci.Handle, event.Data: body})
ev.EventID = message.Signature // we need this ensure that we correctly ack this in the db when it comes back
// TODO: The EventBus is becoming very noisy...we may want to consider a one-way shortcut to Engine i.e. profile.Engine.SendMessageToPeer
Owner

that would move work into this thread potentially which could introduce even more race conditions tho

that would move work into this thread potentially which could introduce even more race conditions tho
dan requested changes 2024-05-12 18:12:14 +00:00
dan left a comment
Owner

Ok I've read

Ok I've read
@ -0,0 +62,4 @@
}
// This file contains code for the Hybrid Group / Managed Group types..
type HybridGroupMessage struct {
Owner

do we want json aliases for these too?

do we want json aliases for these too?
@ -0,0 +34,4 @@
// OnEvent handles File Sharing Hooks like Manifest Received and FileDownloaded
func (f *GroupManagerFunctionality) OnEvent(ev event.Event, profile peer.CwtchPeer) {
switch ev.EventType {
case event.PeerStateChange:
Owner

EventsToRegister is ProtocolEngineCreated and NewMessageFromPeerEngine

but events handled are

PeerStateChange and NewMessageFromPeerEngine

EventsToRegister is ProtocolEngineCreated and NewMessageFromPeerEngine but events handled are PeerStateChange and NewMessageFromPeerEngine
@ -0,0 +38,4 @@
handle := ev.Data["RemotePeer"]
// check that we have authenticated with this peer
if connections.ConnectionStateToType()[ev.Data[event.ConnectionState]] == connections.AUTHENTICATED {
mg, err := f.GetManagedGroup(profile)
Owner

i'm a little fuzzy, would the group manager never be running on a cwtchpeer representing a person with p2p contacts and managed groups? it'll be a stand alone cwtchpeer managing a single group? (cus my next question is what if the profile is in multiple managed groups?)

if that's the case, it almost feels like it's some kind of new cwtch peer that an app can instantiate? a GMbot peer, which has some different restrictions and APIs almost? like a GM peer then will never have .SendMessage directly called on it from a ui etc?

does this functionality break down if it's called to manage more than 1 group? should it?

i'm a little fuzzy, would the group manager never be running on a cwtchpeer representing a person with p2p contacts and managed groups? it'll be a stand alone cwtchpeer managing a single group? (cus my next question is what if the profile is in multiple managed groups?) if that's the case, it almost feels like it's some kind of new cwtch peer that an app can instantiate? a GMbot peer, which has some different restrictions and APIs almost? like a GM peer then will never have .SendMessage directly called on it from a ui etc? does this functionality break down if it's called to manage more than 1 group? should it?
@ -0,0 +142,4 @@
// Establish a new Managed Group and return its conversation id
func (f *GroupManagerFunctionality) ManageNewGroup(profile peer.CwtchPeer) (int, error) {
ac := model.DefaultP2PAccessControl()
Owner

it feels like some where we need a check this groupmanager hasn't already set up its one fixed name group and throw an error when being called again

it feels like some where we need a check this groupmanager hasn't already set up its one fixed name group and throw an error when being called again
@ -0,0 +146,4 @@
// by setting the ManageGroup permission in this ACL we are allowing the manage to
// take control of how this group is structured, see OnEvent above...
ac.ManageGroup = true
// note: a manager can only manage one group. This will (probably) always be true and has a few benefits
Owner

👍

can we move some of these comments/docs up to the top?

👍 can we move some of these comments/docs up to the top?
@ -0,0 +148,4 @@
ac.ManageGroup = true
// note: a manager can only manage one group. This will (probably) always be true and has a few benefits
// and downsides.
// The main downside is that it requires a new manager per group (and thus an onion service per group)
Owner

(and thus an onion service per group) => (and thus a cwtch peer / onion service per group)

(and thus an onion service per group) => (and thus a cwtch peer / onion service per group)
@ -0,0 +152,4 @@
// However, it means that we can lean on p2p functionality like profile images / metadata / name
// etc. for group metadata and effectively get that for-free in the client.
// HOWEVER: hedging our bets here by giving this group a numeric handle...
handle := fmt.Sprintf("managed:%d", 000)
Owner

make it a const for now then :)

make it a const for now then :)
@ -0,0 +168,4 @@
// AddHybridContact is a wrapper arround NewContactConversation which sets the contact
// up for Hybrid Group channel messages...
// TODO this function assumes that authorization has been done at a higher level..
func (f *GroupManagerFunctionality) AddHybridContact(profile peer.CwtchPeer, handle string) error {
Owner

it's not currently wired in anywhere except tests? where's it envisioned to be called from, ah some of the other handle events not yet handled?

it's not currently wired in anywhere except tests? where's it envisioned to be called from, ah some of the other handle events not yet handled?
@ -0,0 +26,4 @@
}
func (f ManagedGroupFunctionality) EventsToRegister() []event.Type {
return []event.Type{event.ProtocolEngineCreated, event.NewMessageFromPeerEngine}
Owner

why listening to ProtocolEngineCreated?

why listening to ProtocolEngineCreated?
@ -0,0 +47,4 @@
}
// We reject managed group requests for groups not setup as managed groups...
if ci.ACL[handle].ManageGroup {
Owner

so this is the managed group check that says this peerMessage is from the groupManager peer so we trust it? if the future with hybrid groups we could get group messages from the group of members with some ACL saying trusted to relay?

so this is the managed group check that says this peerMessage is from the groupManager peer so we trust it? if the future with hybrid groups we could get group messages from the group of members with some ACL saying trusted to relay?
@ -0,0 +107,4 @@
// handleAddMemberEvent adds a group member to the conversation ACL
// assumes we are called after an event provided by an authorized peer (i.e. ManageGroup == true)
func (f *ManagedGroupFunctionality) handleAddMemberEvent(profile peer.CwtchPeer, conversation model.Conversation, ame AddMemberEvent) {
Owner

the AddMemberEvent doesn't come with ACL? maybe i have to read more but how do non default ACL's propagate?

the AddMemberEvent doesn't come with ACL? maybe i have to read more but how do non default ACL's propagate?
@ -0,0 +126,4 @@
func (f *ManagedGroupFunctionality) handleRotateKeyEvent(profile peer.CwtchPeer, conversation model.Conversation, rke RotateKeyEvent) {
keyScope := attr.LocalScope.ConstructScopedZonedPath(attr.ConversationZone.ConstructZonedPath("key"))
keyB64 := base64.StdEncoding.EncodeToString(rke.Key)
profile.SetConversationAttribute(conversation.ID, keyScope, keyB64)
Owner

for now since these are just managed groups with one manager replacing the key is fine because we are assuming an ordered message stream?

but in a hybrid situation where messages may be relayed not in order would we want to keep old keys associated with an older message id range?

for now since these are just managed groups with one manager replacing the key is fine because we are assuming an ordered message stream? but in a hybrid situation where messages may be relayed not in order would we want to keep old keys associated with an older message id range?
@ -0,0 +11,4 @@
// CwtchProfile function that combines some core-functionalities like Hybrid Groups so that
// they can be transparently exposed in autobindings.
// DEV NOTE: consider moving other cross-cutting interface functions here to simplfy CwtchPeer
type InterfaceFunctionality struct {
Owner

How is this exposed API wise? we have a composed CwtchPeer interface already, I would have throught we'd be composing a groupManager cwtch peer to inject these wrappers?

How is this exposed API wise? we have a composed CwtchPeer interface already, I would have throught we'd be composing a groupManager cwtch peer to inject these wrappers?
@ -0,0 +83,4 @@
// ***** Cwtch Server management *****
app := app2.NewApp(acn, "./storage", app2.LoadAppSettings("./storage"))
managerApp := app2.NewApp(acn, "./managerstorage", app2.LoadAppSettings("./managerstorage"))
Owner

we shouldn't need a seperate app, just two peers right? i'd actually be a little more comfortable running both of them out of the same app to make sure they cohabit nicely and peers are properly isolated etc

we shouldn't need a seperate app, just two peers right? i'd actually be a little more comfortable running both of them out of the same app to make sure they cohabit nicely and peers are properly isolated etc
@ -0,0 +90,4 @@
settings := app.ReadSettings()
settings.ExperimentsEnabled = true
settings.Experiments[constants.GroupsExperiment] = true
settings.Experiments[constants.GroupManagerExperiment] = false
Owner

is this not implicit? we don't want ppl setting up peers to have to disable all new and future experiments

is this not implicit? we don't want ppl setting up peers to have to disable all new and future experiments
@ -0,0 +97,4 @@
managerSettings := managerApp.ReadSettings()
managerSettings.ExperimentsEnabled = true
managerSettings.Experiments[constants.GroupsExperiment] = true
managerSettings.Experiments[constants.GroupManagerExperiment] = true
Owner

ah see this is what i mean, does this mean the way the experiment is compose right now it cannot be scoped to one cwtch peer? only an entire app? that's prolly a hint something in our design is a little off, and it will cause problems for clients cus our libcwtch api is bound to one app

i think the experiment can be enabled for the app, but we need a way of scoping the groupManager functionality to a peer, which relates to my previous comment, it feels like an excentension of CwtchPeer not a sub functionality

ah see this is what i mean, does this mean the way the experiment is compose right now it cannot be scoped to one cwtch peer? only an entire app? that's prolly a hint something in our design is a little off, and it will cause problems for clients cus our libcwtch api is bound to one app i think the experiment can be enabled for the app, but we need a way of scoping the groupManager functionality to a peer, which relates to my previous comment, it feels like an excentension of CwtchPeer not a sub functionality
@ -0,0 +129,4 @@
// Now we can allow alice, bob and carol to create a new hybrid group...
log.Infof("now we can allow alice bob and carol to join the hybrid group")
inter := inter.InterfaceFunctionality{}
err = inter.ImportBundle(alice, "managed:"+manager.GetOnion())
Owner

this isn't "wrapping" it in any way we can currently expose in the API, it's had to be manually done client side, which, isn't wrapping it then.

this isn't "wrapping" it in any way we can currently expose in the API, it's had to be manually done client side, which, isn't wrapping it then.
@ -0,0 +207,4 @@
pprof.Lookup("goroutine").WriteTo(os.Stdout, 1)
fmt.Println("")
if numGoRoutinesStart != numGoRoutinesPostAppShutdown {
Owner

is this test passing? i'm surprised since we do acn creation after capturing numGoRoutinesStart and defer it's close till this function ends?

is this test passing? i'm surprised since we do acn creation after capturing numGoRoutinesStart and defer it's close till this function ends?
testing/utils.go Outdated
@ -0,0 +11,4 @@
"time"
)
func WaitForConnection(t *testing.T, peer peer.CwtchPeer, addr string, target connections.ConnectionState) {
Owner

shouldn't this be cut from the test it came from and reused there as well?

shouldn't this be cut from the test it came from and reused there as well?
sarah force-pushed managed-groups from e8f11cc329 to fdec3302af 2024-05-13 19:24:36 +00:00 Compare
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/337
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/338
Some checks failed
continuous-integration/drone/pr Build is failing
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
Sign in to join this conversation.
No description provided.