Tapir Server Refactor #317

Merged
dan merged 12 commits from tapir_server into master 2020-09-28 21:56:20 +00:00
4 changed files with 55 additions and 39 deletions
Showing only changes of commit f74e8647ef - Show all commits

View File

@ -2,15 +2,18 @@ package model
import "errors"
// KeyType provides a wrapper for a generic public key type identifier (could be an onion address, a zcash address etc.)
type KeyType string
Outdated
Review

These are all string, do we want to make a
type KeyType string
that these can be?

These are all string, do we want to make a ```type KeyType string``` that these can be?
const (
// KeyTypeOnion - a cwtch address
KeyTypeOnion = "onion" // bulletin board
// KeyTypeServerOnion - a cwtch address
KeyTypeServerOnion = KeyType("bulletin_board_onion") // bulletin board
// KeyTypeTokenOnion - a cwtch peer with a PoW based token protocol
KeyTypeTokenOnion = "token_onion"
KeyTypeTokenOnion = KeyType("token_service_onion")
//KeyTypePrivacyPass - a privacy pass based token server
KeyTypePrivacyPass = "pp_key"
KeyTypePrivacyPass = KeyType("privacy_pass_public_key")
)
// Key provides a wrapper for a generic public key identifier (could be an onion address, a zcash address etc.)
@ -18,18 +21,18 @@ type Key string
// KeyBundle manages a collection of related keys for various different services.
type KeyBundle struct {
Keys map[string]Key
Keys map[KeyType]Key
}
// HasKey returns true if the bundle has a public key of a given type.
func (kb *KeyBundle) HasKey(name string) bool {
_, exists := kb.Keys[name]
// HasKeyType returns true if the bundle has a public key of a given type.
func (kb *KeyBundle) HasKeyType(keytype KeyType) bool {
_, exists := kb.Keys[keytype]
return exists
}
dan marked this conversation as resolved Outdated
Outdated
Review

for clarity arg named 'name' or type string made me think this was gettting a key for an onion, renaming it keytype or making it keytype clarifies the api a bunch :)

for clarity arg named 'name' or type string made me think this was gettting a key for an onion, renaming it keytype or making it keytype clarifies the api a bunch :)
// GetKey retrieves a key with a given type from the bundle
func (kb *KeyBundle) GetKey(name string) (Key, error) {
key, exists := kb.Keys[name]
func (kb *KeyBundle) GetKey(keytype KeyType) (Key, error) {
key, exists := kb.Keys[keytype]
if exists {
return key, nil
}
@ -40,7 +43,7 @@ func (kb *KeyBundle) GetKey(name string) (Key, error) {
func (kb *KeyBundle) AttributeBundle() map[string]string {
ab := make(map[string]string)
for k, v := range kb.Keys {
ab[k] = string(v)
ab[string(k)] = string(v)
}
return ab
}

View File

@ -78,6 +78,14 @@ func (p *PublicProfile) SetAttribute(name string, value string) {
p.Attributes[name] = value
}
// IsServer returns true if the onion address is associated with a server.
func (p *PublicProfile) IsServer(onion string) bool {
Outdated
Review

I think if you wanted to be fancy you can do:

func (p *PublicProfile) IsServer(onion string) (isServer bool) {
	_, isServer = p.GetAttribute(string(KeyTypeServerOnion));
}
I think if you wanted to be fancy you can do: ``` func (p *PublicProfile) IsServer(onion string) (isServer bool) { _, isServer = p.GetAttribute(string(KeyTypeServerOnion)); } ```
if _, isServer := p.GetAttribute(string(KeyTypeServerOnion)); isServer == true {
return true
}
return false
}
// GetAttribute returns the value of a value set with SetCustomAttribute. If no such value has been set exists is set to false.
func (p *PublicProfile) GetAttribute(name string) (value string, exists bool) {
p.lock.Lock()

View File

@ -48,7 +48,7 @@ type CwtchPeer interface {
DeleteContact(string)
DeleteGroup(string)
AddServer(string)
AddServer(string) error
JoinServer(string)
SendMessageToGroup(string, string) error
SendMessageToGroupTracked(string, string) (string, error)
@ -205,36 +205,41 @@ func (cp *cwtchPeer) AddContact(nick, onion string, authorization model.Authoriz
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, event.SaveHistoryKey, event.DeleteHistoryDefault))
}
sarah marked this conversation as resolved
Review

Comment (quality.sh misses public methods on structs filling in Interfaces some times wierdly...)

Comment (quality.sh misses public methods on structs filling in Interfaces some times wierdly...)
func (cp *cwtchPeer) AddServer(serverSpecification string) {
func (cp *cwtchPeer) AddServer(serverSpecification string) error {
keyBundle := new(model.KeyBundle)
json.Unmarshal([]byte(serverSpecification), &keyBundle)
err := json.Unmarshal([]byte(serverSpecification), &keyBundle)
Outdated
Review

why is it called serverSpecification if it deserializes to a keybundle?

why is it called serverSpecification if it deserializes to a keybundle?
log.Debugf("Got new key bundle %v", keyBundle)
if keyBundle.HasKey(model.KeyTypeOnion) {
onionKey, _ := keyBundle.GetKey(model.KeyTypeOnion)
if keyBundle.HasKeyType(model.KeyTypeServerOnion) {
onionKey, _ := keyBundle.GetKey(model.KeyTypeServerOnion)
onion := string(onionKey)
decodedPub, _ := base32.StdEncoding.DecodeString(strings.ToUpper(onion))
ab := keyBundle.AttributeBundle()
ab["nick"] = onion
pp := &model.PublicProfile{Name: onion, Ed25519PublicKey: decodedPub, Authorization: model.AuthUnknown, Onion: onion, Attributes: ab}
cp.Profile.AddContact(onion, pp)
pd, _ := json.Marshal(pp)
cp.eventBus.Publish(event.NewEvent(event.PeerCreated, map[event.Field]string{
event.Data: string(pd),
event.RemotePeer: onion,
}))
if cp.GetContact(onion) == nil {
decodedPub, _ := base32.StdEncoding.DecodeString(strings.ToUpper(onion))
ab := keyBundle.AttributeBundle()
ab["nick"] = onion
Outdated
Review

ab[attr.GetLocalScope("name")] = onion

is what ui uses

```ab[attr.GetLocalScope("name")] = onion``` is what ui uses
Outdated
Review

attr.GetPublicScope("name")

is what ui is using. i dont think any code is using attributes["nick"] anymore. should be safe to just drop this line tho as the ui falls back on onion if no "settings.name" attribute is found

attr.GetPublicScope("name") is what ui is using. i dont think any code is using attributes["nick"] anymore. should be safe to just drop this line tho as the ui falls back on onion if no "settings.name" attribute is found
pp := &model.PublicProfile{Name: onion, Ed25519PublicKey: decodedPub, Authorization: model.AuthUnknown, Onion: onion, Attributes: ab}
// Publish every key as an attribute
for k, v := range ab {
log.Debugf("Server (%v) has %v key %v", onion, k, v)
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, k, v))
cp.Profile.AddContact(onion, pp)
Outdated
Review

this seems to be exposing a bit of a bug (i think ui makes sure we don't double do it but nothing in cwtchPeer or profile do...), no where do we make a check if we already have this contact. with this current behaviour we would be wiping out existing profile along with its message histories and settings and attributes and authentication level.

I assume we need to import a contact for the owner of the group or something? we need something more robust here that can added these keys to an existing contact?

this seems to be exposing a bit of a bug (i think ui makes sure we don't double do it but nothing in cwtchPeer or profile do...), no where do we make a check if we already have this contact. with this current behaviour we would be wiping out existing profile along with its message histories and settings and attributes and authentication level. I assume we need to import a contact for the owner of the group or something? we need something more robust here that can added these keys to an existing contact?
Outdated
Review

Will add a check and an error for this case

Will add a check and an error for this case
pd, _ := json.Marshal(pp)
cp.eventBus.Publish(event.NewEvent(event.PeerCreated, map[event.Field]string{
event.Data: string(pd),
event.RemotePeer: onion,
}))
// Publish every key as an attribute
for k, v := range ab {
log.Debugf("Server (%v) has %v key %v", onion, k, v)
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, k, v))
}
// Default to Deleting Peer History
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, event.SaveHistoryKey, event.DeleteHistoryDefault))
return nil
}
// Default to Deleting Peer History
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, event.SaveHistoryKey, event.DeleteHistoryDefault))
// Server Already Exists
return errors.New("a contact already exists with the given onion address")
Outdated
Review

do we need typed errors here? or should this not be an error? because if a client app is using this is getting an error on AddServer expected if the server exists? they'll have to filter it out from all the other "couldnt add server so we dont have it errors" and ignore this specigic one, cus they can go ahead and still use the server

do we need typed errors here? or should this not be an error? because if a client app is using this is getting an error on AddServer expected if the server exists? they'll have to filter it out from all the other "couldnt add server so we dont have it errors" and ignore this specigic one, cus they can go ahead and still use the server
}
return err
}
// GetContacts returns an unordered list of onions
@ -332,8 +337,8 @@ func (cp *cwtchPeer) InviteOnionToGroup(onion string, groupid string) error {
// JoinServer manages a new server connection with the given onion address
func (cp *cwtchPeer) JoinServer(onion string) {
if cp.GetContact(onion) != nil {
tokenY, yExists := cp.GetContact(onion).GetAttribute(model.KeyTypePrivacyPass)
tokenOnion, onionExists := cp.GetContact(onion).GetAttribute(model.KeyTypeTokenOnion)
tokenY, yExists := cp.GetContact(onion).GetAttribute(string(model.KeyTypePrivacyPass))
tokenOnion, onionExists := cp.GetContact(onion).GetAttribute(string(model.KeyTypeTokenOnion))
if yExists && onionExists {
cp.eventBus.Publish(event.NewEvent(event.JoinServer, map[event.Field]string{event.GroupServer: onion, event.ServerTokenY: tokenY, event.ServerTokenOnion: tokenOnion}))
}

View File

@ -84,9 +84,9 @@ func (s *Server) Run(acn connectivity.ACN) {
// KeyBundle provides the keybundle of the server (mostly used for testing)
func (s *Server) KeyBundle() model.KeyBundle {
kb := model.KeyBundle{Keys: make(map[string]model.Key)}
kb := model.KeyBundle{Keys: make(map[model.KeyType]model.Key)}
identity := s.config.Identity()
kb.Keys[model.KeyTypeOnion] = model.Key(identity.Hostname())
kb.Keys[model.KeyTypeServerOnion] = model.Key(identity.Hostname())
kb.Keys[model.KeyTypeTokenOnion] = model.Key(tor.GetTorV3Hostname(s.tokenService.PublicKey()))
kb.Keys[model.KeyTypePrivacyPass] = model.Key(s.tokenServer.Y.String())
return kb