Managed Groups First Cut #558
No reviewers
Labels
No Label
applications
BLOCKED
bug
design
duplicate
enhancement
fixed?
funding-needed
help wanted
infrastructure
invalid
payments
qubes
question
ready-for-implementation
refactor
spam
tapir-server
testing
tor
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cwtch.im/cwtch#558
Loading…
Reference in New Issue
No description provided.
Delete Branch "managed-groups"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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:
NewConversation
,InternalInsertMessage
andSignMessage
- these are needed because Extensions now need to manage inserting messages on behalf of a conversation.Changes from part 1 noted below:
Things not yet implemented:
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...
9e5cb8b403
toe8f11cc329
@ -44,0 +62,4 @@
profile.PublishEvent(ev)
}
}
messsages, _ = profile.GetMostRecentMessages(ci.ID, 2, 0, uint(100))
constant 0 and 2
@ -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
that would move work into this thread potentially which could introduce even more race conditions tho
Ok I've read
@ -0,0 +62,4 @@
}
// This file contains code for the Hybrid Group / Managed Group types..
type HybridGroupMessage struct {
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:
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)
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()
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
👍
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)
(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)
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 {
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}
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 {
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) {
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)
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 {
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"))
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
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
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())
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 {
is this test passing? i'm surprised since we do acn creation after capturing numGoRoutinesStart and defer it's close till this function ends?
@ -0,0 +11,4 @@
"time"
)
func WaitForConnection(t *testing.T, peer peer.CwtchPeer, addr string, target connections.ConnectionState) {
shouldn't this be cut from the test it came from and reused there as well?
e8f11cc329
tofdec3302af
Drone Build Status: failure
https://build.openprivacy.ca/cwtch.im/cwtch/337
Drone Build Status: failure
https://build.openprivacy.ca/cwtch.im/cwtch/338