[sugar] review: Guillaume's 'roster' branch of Gadget

[Dafydd Harries]

> diff --git a/gadget/component.py b/gadget/component.py
> index 6e2ea07..9600c7c 100644
> --- a/gadget/component.py
> +++ b/gadget/component.py
> @@ -98,8 +98,9 @@ class GadgetService(component.Service):
>      features = [ns.BUDDY, ns.ACTIVITY, "%s+notify" % ns.BUDDY_PROP]
>      identities = [('collaboration', 'gadget', 'OLPC Gadget')]
>  
> -    def __init__(self, model):
> +    def __init__(self, model, roster):
>          self.model = model
> +        self.roster = roster
>          self.mucs = {}
>          self.iq_dispatch = {
>              (ns.BUDDY, 'query'): self.iq_buddy_query,
> @@ -121,10 +122,13 @@ class GadgetService(component.Service):
>          xmlstream.addObserver('//message', self.message)
>          xmlstream.addObserver('//presence', self.presence)
>  
> -        # FIXME: we should send presence to buddies subscribed to gadget. How?
>          if self.debug:
>              self.xmlstream.setDispatchFn(self.onElement)
>  
> +        for jid in self.roster.jids:
> +            # FIXME: How can I find the right 'from' ?
> +            self.send_presence("gadget.localhost", jid)
> +

Isn't the right from address the same address as we authenticated to the
jabber server with? This is the same as config['domain'], which can be
accessed here as self.parent.thisEntity.full().

Should we send presence probes to all our contacts here too?

>      def onElement(self, e):
>          log.msg('receive:', e.toXml())
>          self.xmlstream.onElement(e)
> @@ -389,13 +393,14 @@ class GadgetService(component.Service):
>          elif type == 'subscribed':
>              log.msg('subscribed to %s' % jid)
>  
> +            self.roster.add(jid)
>              self.send_presence(to, from_)
> -            # FIXME: add buddy to model
>  
>          elif type == 'unsubscribed':
>              log.msg('unsubscribed from %s' % jid)
>              # the buddy will be removed from the model when we'll receive
>              # the 'unavailable' presence
> +            self.roster.remove(jid)
>  
>          elif type == 'probe':
>              log.msg('%s is online' % jid)
> diff --git a/gadget/config.py b/gadget/config.py
> index 1984767..8802771 100644
> --- a/gadget/config.py
> +++ b/gadget/config.py
> @@ -21,6 +21,7 @@ def parse_config(fh):
>          'server_port': int,
>          'domain': str,
>          'password': str,
> +        'roster_path': str,
>          }
>  
>      for line in fh:
> diff --git a/gadget/roster.py b/gadget/roster.py
> new file mode 100644
> index 0000000..f14eed2
> --- /dev/null
> +++ b/gadget/roster.py
> @@ -0,0 +1,28 @@
> +class GadgetRoster:
> +    def __init__(self, path):
> +        self.path = path
> +
> +        try:
> +            lines = file(path, 'r').readlines()
> +        except IOError:
> +            self.jids = set()

I'm a bit worried that we might lose errors here. Not all IO errors are "file
not found".

> +        else:
> +            # JID's have to contain one and only one '@'
> +            jids = filter(lambda x: x.count('@') == 1, lines)

Are we expecting that the roster might contain invalid JIDs somehow? This
seems a bit strange for me.

At any rate, if we are, we should use twisted.jabber.words.jid.parse() to
validate JIDs.

> +            # remove ending '\n'
> +            self.jids = set(map(lambda x: x.strip(), jids))
> +
> +    def save(self):

Perhaps this method should be prefixed with an underscore.

> +        file(self.path, 'w').write(reduce(lambda x,y: "%s\n%s" % (x,y), self.jids))

There's a problem here: opening the file for writing might succeed, in which
case it will be truncated, but writing to the file might fail, in which case
we would lose the roster data. The correct approach is to write the new roster
to a temporary file and rename it over the old one.
twisted.python.filepath.FilePath implements this behaviour; we could use that.

reduce() can be inefficient for large lists. Perhaps this would be better:

  ''.join('%s\n' % x for x in jids)

(By using a generator expression, the temporary strings can be garbage
collected as they are used.)

In terms of IO, writing the entire roster out on every change might be
expensive. Another option would be to append JIDs to the file. A third option
would be to use SQLite, which would also take care of the data loss problem.

> +
> +    def add(self, jid):
> +        self.jids.add(jid)
> +        self.save()
> +
> +    def remove(self, jid):
> +        try:
> +            self.jids.remove(jid)
> +        except KeyError:
> +            pass
> +        else:
> +            self.save()
> diff --git a/gadget/test_component.py b/gadget/test_component.py
> index 3335b09..f59fbae 100644
> --- a/gadget/test_component.py
> +++ b/gadget/test_component.py
> @@ -1,4 +1,5 @@
>  
> +import os
>  import unittest
>  from twisted.internet import defer, reactor
>  from twisted.protocols import loopback
> @@ -10,9 +11,13 @@ from twisted.words.protocols.jabber.client import IQ
>  from component import GadgetService, parse_properties, properties_to_xml,\
>          PropertyTypeError
>  from model import GadgetModel, Activity
> +from roster import GadgetRoster
>  
>  import ns
>  
> +# FIXME: can we use this path?
> +ROSTER_PATH = '/tmp/roster'
> +

No we can't! This is vulnerable to a symlink attack bug where an attacker
makes /tmp/roster point to one of your files. Unlinking the file first won't
help you because there is always a race. Writing to fixed locations in shared
directories is never a good idea. Always use the tempfile module for such
things.

This is moot, however, because I'm going to recommend that we use an in-memory
roster object for the tests. In fact, the roster object can just be a set,
because the API is just the add() and remove() functions.

>  def sort_activity(a, b):
>      """"activities are sorted according their id"""
>      return cmp(a['id'], b['id'])
> @@ -43,7 +48,6 @@ def parse_activities_result(stanza):
>  
>      return sorted(activities)
>  
> -
>  class TestAuthenticator(xmlstream.Authenticator):
>      def streamStarted(self, root=None):
>          self.xmlstream.sendHeader()
> @@ -74,7 +78,14 @@ class TestCase(TrialTestCase):
>          auth = component.ConnectComponentAuthenticator('foo', 'bar')
>          self.client = xmlstream.XmlStream(auth)
>          self.model = GadgetModel()
> -        self.service = GadgetService(self.model)
> +
> +        try:
> +            os.unlink(ROSTER_PATH)
> +        except OSError:
> +            pass
> +
> +        self.roster = GadgetRoster('/tmp/roster')

  self.roster = set()

> +        self.service = GadgetService(self.model, self.roster)
>          self.service.debug = True
>          self.service.parent = self.client
>          loopback.loopbackAsync(self.server, self.client)
> @@ -613,6 +624,19 @@ class SubscriptionTest(TestCase):
>  
>      def presence2(self, stanza):
>          assert stanza['type'] == 'subscribe'
> +
> +        #presence = domish.Element((ns.CLIENT, 'presence'))
> +        #presence['from'] = 'bob at foo.org'
> +        #presence['to'] = 'gadget.foo.org'
> +        #presence['type'] = 'subscribed'
> +        #self.server.send(presence)
> +        #reactor.callLater(0, self.later)
> +
> +        # FIXME: presence2 is called twice if we send this stanza
> +        self.done.callback(None)
> +

This is surprising and should be investigated. This is not how
addOnetimeObserver() is supposed to behave.

> +    def later(self):
> +        assert self.roster.jids == set(['bob at foo.org'])
>          self.done.callback(None)
>  
>  class ProbePresenceTest(TestCase):
> diff --git a/gadget/test_roster.py b/gadget/test_roster.py
> new file mode 100644
> index 0000000..e4af0ff
> --- /dev/null
> +++ b/gadget/test_roster.py
> @@ -0,0 +1,44 @@
> +import os
> +import unittest
> +
> +from gadget.roster import GadgetRoster
> +
> +# FIXME: can we use this path?
> +ROSTER_PATH = '/tmp/roster'
> +

Again, we should change this.

> +class RosterTest(unittest.TestCase):
> +    def write_roster_file(self, jids):
> +        file(ROSTER_PATH, 'w').write(reduce(lambda x,y: "%s\n%s" % (x,y), jids))
> +
> +    def test_roster(self):
> +        # first try with an unexistent file
> +        try:
> +            os.unlink(ROSTER_PATH)
> +        except OSError:
> +            pass
> +        assert GadgetRoster(ROSTER_PATH).jids == set()
> +
> +        self.write_roster_file(['alice at foo.org', 'bob at baz.net'])
> +
> +        # roster contains good jid ?
> +        assert GadgetRoster(ROSTER_PATH).jids == set(['alice at foo.org', 'bob at baz.net'])
> +
> +        # add a jid
> +        GadgetRoster(ROSTER_PATH).add('charles at localhost')
> +        assert GadgetRoster(ROSTER_PATH).jids == set(['alice at foo.org', 'bob at baz.net',
> +            'charles at localhost'])
> +
> +        # re-add the same jid
> +        GadgetRoster(ROSTER_PATH).add('charles at localhost')
> +        assert GadgetRoster(ROSTER_PATH).jids == set(['alice at foo.org', 'bob at baz.net',
> +            'charles at localhost'])
> +
> +        # remove alice
> +        GadgetRoster(ROSTER_PATH).remove('alice at foo.org')
> +        assert GadgetRoster(ROSTER_PATH).jids == set(['bob at baz.net',
> +            'charles at localhost'])
> +
> +        # remove an unexisting jid
> +        GadgetRoster(ROSTER_PATH).remove('lucien at foo.org')
> +        assert GadgetRoster(ROSTER_PATH).jids == set(['bob at baz.net',
> +            'charles at localhost'])

-- 
Dafydd