status-im / status-console-client

Status messaging console user interface
Mozilla Public License 2.0
10 stars 2 forks source link

Feature/add pair installation message #107

Closed cammellos closed 5 years ago

cammellos commented 5 years ago

This PR does a few things:

1) Use StatusProtocol message instead of Message as we receive different types of messages 2) Enable PFS by default, as that's what we will have for v1, this means that datadir and installationID needs to be set if you expect to correctly communicate, as it's a stateful process 3) Adds PairInstallationMessages 4) Use filter service, which means negotiated and partitioned topic works now

There still a lot to do but it's a start. Currently mailserver logic is disconnected from filters and needs to be revisited (it works only for public chats & discovery topic).

I tried to keep datasync up to date, but probably we should integrate it in a different way as it quite cumbersome to keep both up to date.

The original codebase made the assumption that only a single type of messages was sent protocol.Message, while it is only one of the types of messages that we exchange:

https://github.com/status-im/status-react/blob/develop/src/status_im/transport/message/transit.cljs for a list of messages.

I have introduced a new type PairInstallationMessage, and changed the return type for subscriptions etc to:

type StatusMessage struct {
Message interface{}
ID []byte
SigPubKey ...
Flags Flags
}

where interface{} can be a Message or SyncInstallationMessage.

On the subscription we match the type and act accordingly.

As a side note I feel Flags is a bit misplaced in Message as seems to be dealing with stuff that is handled on a different level (whether the message has been seen by the user etc, which is application/db , and not relevant to transit or protocol).

Let me know how you feel is the best way to structure this, I have just changed the code so that it (should) work.

I had to remove the sorting of messages as that only applies to chat messages, but other types of messages do not have a clock value, @decanus let me know if it's necessary or we can remove it.

decanus commented 5 years ago

@cammellos those shouldn't be protocol messages I believe. We decode it to be a status message. Or is my understanding here wrong?

cammellos commented 5 years ago

I am not sure on the terminology, it's all a bit confusing, so just to clarify what I mean:

First of all, this is all application level logic, as some messages are encrypted and a wrapper around it (group chats messages, pairing messages).

StatusMessage -> Any type of message the status app exchanges (text messages, contact updates, pairing etc)

Message -> A subset of StatusMessage, those are only the messages that are exchanged as chat messages, the application layer will know how to handle it (as of now)

I have added SyncInstallationMessage -> A subset of StatusMessage, a sibling of Message.

Before this PR a subscription returned only Message, but that's only dealing with a subset of all the messages exchanged on status.

As I see, we either change subscriptions to deal with interface{} and then do the type casting on the receiving end (as in this PR), or we create multiple subscriptions each with a different type (don't have a strong opinion either ways), and probably there are a few more options.

Not sure it answer the question, I also have had a look at the datasync code, still not familiar with it, but we can have a chat tomorrow.

decanus commented 5 years ago

@cammellos lets discuss on our call tomorrow.

adambabik commented 5 years ago

As a side note I feel Flags is a bit misplaced in Message as seems to be dealing with stuff that is handled on a different level (whether the message has been seen by the user etc, which is application/db , and not relevant to transit or protocol).

That's correct. The idea was to have a Message struct in protocol/client, use composition and put all non-protocol relevant things into it.

Regarding how to handle PairInstallationMessage, I think we need to decide whether this and other meta-messages like contacts exchange is a part of the protocol or not. Even if the answer is yes, the another question is whether client.Messenger needs to know about them in order to do some actions or it can be hidden from it. If it can be hidden, then protocol.Protocol interface should not change.

Currently, in protocol_whisper.go adapter we have a ProtocolService instance which handles PFS messages. My idea with Publisher refactor in status-go was that we can replace ProtocolService with Publisher instance and let it handle all that logic connected to negotiated topics etc. Question, is whether it is possible for Publisher to handle all these meta-messages or there are more cases? And even more general one, can we encapsulate all these meta-messages in ProtocolWhisperAdapter without exposing them to client.Messenger?

cammellos commented 5 years ago

Regarding how to handle PairInstallationMessage, I think we need to decide whether this and other meta-messages like contacts exchange is a part of the protocol or not.

I would consider them part of the protocol, as some of them are essential for the functioning of the app, and some of them might be used to actually allow exchanging of messages (i.e Message )(contact request in tribute to talk for example).

Even if the answer is yes, the another question is whether client.Messenger needs to know about them in order to do some actions or it can be hidden from it.

I guess it's up to us, I would think so (I am assuming client.Messenger handles application logic).

At first sight ProtocolWhisperAdapter does not seem to be the best place to handle those messages, as they are not whisper specific (similarly pfs messages don't seem to belong on that layer, but a layer above).

The way I see it:

I would expect that the whisper layer returns to the above layer a structure like:

type whatever struct {
  payload []byte
  signature ...
  timestamp ...
}

(Here goes the data sync layer, which needs access to the raw payload, but nothing else)

Which is then passed to the encryption layer (PFS), which will unencrypt if necessary:

type whatever struct {
  unencryptedPayload []byte
  payload []byte
  signature
  timestamp
}

Which is then passed to the layer above, application level, which will do the transit stuff, and pass to the client code:

type whatever struct {
   Message/PairInstallationMessage/ContactUpdate whatever 
  signature 
   timestamp
}

at which point an action is taken, some layers might enrich the message with informations that are passed to the above layer, such for example the PFS might add information about the new devices discovered.

Of course any of these "layers" can be collapsed if it makes sense to do so, and they can be horizontal rather than vertical.

If so layered, replacing whisper with pss means only changing a small bit of it, and respecting the interface of the messages passed to it (a signature/timestamp/ and payload), so instinctively the protocol_whisper.go does not seem to be the best place to handle client specific application (Message, ContactUpdate ...)

cammellos commented 5 years ago

And even more general one, can we encapsulate all these meta-messages in ProtocolWhisperAdapter without exposing them to client.Messenger?

This I am not sure, what do you see as the responsibility for client.Messenger? (If it's only handling messages, than it makes sense, but if it's handling contacts as well , blocked etc, then it needs to have knowledge of multiple types of messages, ContactUpdate, SyncInstallation etc)

adambabik commented 5 years ago

@cammellos that sounds good. I wrote protocol_whisper.go as a place where we can put it now but of course it can be split for better visibility. Also, currently it's an adapter between a transport and protocol layers. So, it looks like we want to have something like this:

+---------------------------------------------+
|                                             |
|  App Layer (client.Messenger)               |  Might handle only Protocol
|                                             |
+---------------------------------------------+
+---------------------------------------------+
|            ||            ||                 |
|  Protocol  ||  Contacts  ||  Installations  |  Each component has own struct
|            ||            ||                 |  Has access to unencrypted raw payload
+---------------------------------------------+
+---------------------------------------------+
|                                             |
|  Encryption                                 |
|                                             |
+---------------------------------------------+
+---------------------------------------------+
|                     ||                      |
|  Whisper Adapter    ||  Data Sync           |  Return a struct
|                     ||                      |  {
+---------------------------------------------+    payload []byte
+---------------------------------------------+    signature
|                                             |    timestamp
|  Transport Layer (shh, PSS)                 |  }
|                                             |
+---------------------------------------------+

I used ascii flow. You can import this drawing into it and modify.

Whisper adapter and Data Sync would implement some interface that we need to define (some modification of protocol.Protocol that would pass enough data for the encryption layer).

This I am not sure, what do you see as the responsibility for client.Messenger? (If it's only handling messages, than it makes sense, but if it's handling contacts as well , blocked etc, then it needs to have knowledge of multiple types of messages, ContactUpdate, SyncInstallation etc)

It should handle contacts as well. The initial Igor's idea was to keep contacts management in status-react but I think eventually we want client.Messenger to handle them as well. Otherwise some syncing is required anyway.

Is that correct or we should change something?

cammellos commented 5 years ago

sounds good to me, we can always adjust things as we go along if qe change our minds. thanks!

On Fri, Jun 28, 2019, 11:08 Adam Babik notifications@github.com wrote:

@cammellos https://github.com/cammellos that sounds good. I wrote protocol_whisper.go as a place where we can put it now but of course it can be split for better visibility. Also, currently it's an adapter between a transport and protocol layers. So, it looks like we want to have something like this:

+---------------------------------------------+ App Layer (client.Messenger) Might handle only Protocol
+---------------------------------------------+ +---------------------------------------------+ Protocol Contacts Installations Each component has own struct Has access to unencrypted raw payload +---------------------------------------------+ +---------------------------------------------+
Encryption
+---------------------------------------------+ +---------------------------------------------+ Whisper Adapter Data Sync Return a struct { +---------------------------------------------+ payload []byte +---------------------------------------------+ signature timestamp Transport Layer (shh, PSS) }

+---------------------------------------------+

I used ascii flow http://asciiflow.com/. You can import this drawing into it and modify.

Whisper adapter and Data Sync would implement some interface that we need to define (some modification of protocol.Protocol that would pass enough data for the encryption layer).

This I am not sure, what do you see as the responsibility for client.Messenger? (If it's only handling messages, than it makes sense, but if it's handling contacts as well , blocked etc, then it needs to have knowledge of multiple types of messages, ContactUpdate, SyncInstallation etc)

It should handle contacts as well. The initial Igor's idea was to keep contacts management in status-react but I think eventually we want client.Messenger to handle them as well. Otherwise some syncing is required anyway.

Is that correct or we should change something?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-console-client/pull/107?email_source=notifications&email_token=AAHYJMF5XTNUMVREOHIQDE3P4XIJLA5CNFSM4H342XM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYZRCDI#issuecomment-506663181, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHYJMFM4ZJXP4IN6CFKHFLP4XIJLANCNFSM4H342XMQ .

adambabik commented 5 years ago

@cammellos I am not sure if we can layer horizontally Protocol, Contacts and Installations. Please confirm if that can work.

Let's talk about a flow of sending a message. (1) I need to create a message and encode it -- this is Protocol responsibility. (2) I need to pick a contact I want to send a message to -- Contacts can be used. What's not clear to me is Installations. Do we want to have a single contact with multiple installations? (3) Now, I can pass the message to the encryption layer. This needs to know about installation obviously. (4) What's the most tricky for me is how can we pass it down. Whisper adapter needs to know the payload to send but also somehow calculate topic, signature, use a proper public key to encrypt the whisper envelope.

Maye we should have Encryption in the horizontal layer next to Protocol, Contacts and Installations? That sounds easier to implement but it does sound worse in terms of "describing" the protocol.

Another option is to just pass some metadata down from upper layers and let the lower layers pick whatever they need.

cammellos commented 5 years ago

The way I see it is:

Clients wants to send a 1 to 1 message(public messages are simpler):

oversimplified:


func SendMessage(contactId, text) {

  contact := Contacts.Get(contactId) // not sure is even needed
 clockValue  = Chat.GetClockValue()
 payload = Transit.EncodeMessage(text, clockValue)

 encryptionSpec = Encryption.BuildMessage(contactId, payload) // This
returns: the encrypted payload, the devices targeted, any shared secret,
and the lowest version of the protocol running, device management is done
at this level already

topic = WhisperTopic.GetTopic(contactId, encryptionSpec.sharedSecret,
encryptionSpec.MinimumProtocolVersion) // Get a topic based on version,
shard secret etc

whisperMessage := Whisper.BuildMessage(contactID, topic, payload)

Whisper.Dispatch(whisperMessage)

// Chat.AddMessage(contactId, message) goes somewhere
}

Basically something on these lines, similarly for sending ContactRequest, other then the Chat specific steps

Naming I just used the most obvious to try to get the idea through, not necessarily the one we have in the code

adambabik commented 5 years ago

Ok, that sounds good. It looks like Installations is actually a part of the encryption layer and does not need to be exposed to client.Messenger. Installations, shared secret, multidevice support will all be encapsulated what currently is a Publisher in status-go.

The troublesome part is a topic and topic negotiation. This is Whisper specific so it probably should live in the Whisper adapter. We need to specify what information exactly the Whisper adapter needs as it looks like contact, shared secret and MinimumProtocolVersion are required.

client.Messenger then will need to have access to:

// protocol package will also expose also CreatePublicTextMessage etc.
type ProtocolEncoder interface {
    Encode(protocol.Message)
    Decode([]byte) protocol.Message
}

type Messenger struct {
    db Database
    contact *ContactsManager
    protocolEncoder ProtocolEncoder
    encryptor *Encryptor // currently most of its logic is in Publisher in status-go
    transporter Transporter // in our case it will be Whisper adapter but can be PSS in the future; currently the interface is defined as `protocol.Protocol`
}

func (m *Messenger) Send(contact Contact, data []byte) {
    clockValue := calculateNextClock(m.db.LastMessageClock(contact))
    message := buildMessage(data, clockValue, contact) // it will use contact to figure out ChatID and message type
    payload := m.protocolEncoder.Encode(message)

    // This returns: the encrypted payload, the devices targeted, any shared secret,
    // and the lowest version of the protocol running, device management is done
    // at this level already
    encryptionSpec := m.encryptor.BuildMessage(contact, payload) 

    messageID, err := m.transporter.Send(encryptionSpec)

    m.db.AddMessage(messageID, message)
}

func (m *Messenger) Join(contact Contact) {
    // currently it is encapsulated in Stream
    // Depending on contact, we can subscribe to various types of messages
    channel := m.transporter.Subscribe(contact, ProtocolMessage)
    go m.handleProtocolMessages(channel)

    if contact.Type == ContactPrivate {
        channel := m.transporter.Subscribe(contact, PairInstallationMessage)
        go m.handlePairInstallationMessage(channel)

        channel := m.transporter.Subscribe(contact, ContactCodeMessage)
        go m.handleContactCodeMessage(channel)
    }

    // ...
}
cammellos commented 5 years ago

Ok, that sounds good. It looks like Installations is actually a part of the encryption layer and does not need to be exposed to client.Messenger

Not sure about a client.Messenger but some clients (status-react for example), will need some information about Installations, namely:

1) What are our installations 2) The ability to disable/enable an installation 3) The ability to set some metadata (name, the platform, the fcmtoken) 4) Anytime one of our installations is updated (a new installation, a change in name)

that's basically 4 endpoints:

GetInstallations() SetMetadata(installation-id, name, token, platform) EnableInstallation() DisableInstallation()

and some way to signal that a installations have been updated (and a client can call GetInstallations() to refresh) That's basically what status-react uses, we can change things of course, but the idea is that our own installations needs to be managed by the user.

adambabik commented 5 years ago

Ok, then we can encapsulate that in client.Messenger for now as we need a single interface. client.Messenger is what the UI implementation (status-react) would use.

cammellos commented 5 years ago

@adambabik so given the discussion we had, what should we change (if anything) in this PR?