changeset 330:82d1259b3e36

backend, pgsql storage: better items/notification handling, various fixes: - replaced const.VAL_AMODEL_ROSTER by const.VAL_AMODEL_PUBLISHER_ROSTER to follow change in pgsql schema - implemented whitelist access model - fixed bad access check during items retrieval (access was checked on recipient instead of requestor/sender) - getItemsData and notification filtering now use inline callbacks: this make these complexe workflows far mor easy to read, and clarity is imperative in these security critical sections. - publisher-roster access model now need to have only one owner, else it will fail. The idea is to use this model only when owner=publisher, else there is ambiguity on the roster to use to check access - replaced getNodeOwner by node.getOwners, as a node can have several owners - notifications filtering has been fixed in a similar way - psql: simplified withPEP method, pep_table argument is actually not needed - removed error.NotInRoster: error.Forbidden is used instead - notifications now notify all the owners, not only the first one
author Goffi <goffi@goffi.org>
date Sun, 26 Mar 2017 20:52:32 +0200
parents 98409ef42c94
children e93a9fd329d9
files sat_pubsub/backend.py sat_pubsub/const.py sat_pubsub/pgsql_storage.py
diffstat 3 files changed, 179 insertions(+), 191 deletions(-) [+]
line wrap: on
line diff
--- a/sat_pubsub/backend.py	Sun Mar 26 20:33:18 2017 +0200
+++ b/sat_pubsub/backend.py	Sun Mar 26 20:52:32 2017 +0200
@@ -124,8 +124,8 @@
                  "label": "Who can subscribe to this node",
                  "options": {
                      const.VAL_AMODEL_OPEN: "Public node",
-                     const.VAL_AMODEL_ROSTER: "Node restricted to some roster groups",
-                     const.VAL_AMODEL_JID: "Node restricted to some jids",
+                     const.VAL_AMODEL_PUBLISHER_ROSTER: "Node restricted to some groups of publisher's roster",
+                     const.VAL_AMODEL_WHITELIST: "Node restricted to some jids",
                      }
                 },
             const.OPT_ROSTER_GROUPS_ALLOWED:
@@ -559,120 +559,123 @@
         return self.storage.getAffiliations(entity)
 
 
-    def getItems(self, nodeIdentifier, recipient, maxItems=None,
+    def getItems(self, nodeIdentifier, requestor, recipient, maxItems=None,
                        itemIdentifiers=None, ext_data=None):
-        d = self.getItemsData(nodeIdentifier, recipient, maxItems, itemIdentifiers, ext_data)
+        d = self.getItemsData(nodeIdentifier, requestor, recipient, maxItems, itemIdentifiers, ext_data)
         d.addCallback(lambda items_data: [item_data.item for item_data in items_data])
         return d
 
-    def getItemsData(self, nodeIdentifier, recipient, maxItems=None,
+    @defer.inlineCallbacks
+    def getOwnerRoster(self, node, owners=None):
+        if owners is None:
+            owners = yield node.getOwners()
+
+        if len(owners) != 1:
+            log.msg('publisher-roster access is not allowed with more than 1 owner')
+            return
+
+        owner_jid = owners[0]
+
+        try:
+            roster = yield self.privilege.getRoster(owner_jid)
+        except Exception as e:
+            log.msg("Error while getting roster of {owner_jid}: {msg}".format(
+                owner_jid = owner_jid.full(),
+                msg = e))
+            return
+        defer.returnValue(roster)
+
+    @defer.inlineCallbacks
+    def getItemsData(self, nodeIdentifier, requestor, recipient, maxItems=None,
                        itemIdentifiers=None, ext_data=None):
         """like getItems but return the whole ItemData"""
+        if maxItems == 0:
+            log.msg("WARNING: maxItems=0 on items retrieval")
+            defer.returnValue([])
+
         if ext_data is None:
             ext_data = {}
-        d = self.storage.getNode(nodeIdentifier, ext_data.get('pep', False), recipient)
-        d.addCallback(_getAffiliation, recipient)
-        d.addCallback(self._doGetItems, recipient, maxItems, itemIdentifiers,
-                      ext_data)
-        return d
-
-    def checkGroup(self, roster_groups, entity):
-        """Check that entity is authorized and in roster
-        @param roster_group: tuple which 2 items:
-                        - roster: mapping of jid to RosterItem as given by self.privilege.getRoster
-                        - groups: list of authorized groups
-        @param entity: entity which must be in group
-        @return: (True, roster) if entity is in roster and authorized
-                 (False, roster) if entity is in roster but not authorized
-        @raise: error.NotInRoster if entity is not in roster"""
-        roster, authorized_groups = roster_groups
-        _entity = entity.userhostJID()
+        node = yield self.storage.getNode(nodeIdentifier, ext_data.get('pep', False), recipient)
+        node, affiliation = yield _getAffiliation(node, requestor)
 
-        if not _entity in roster:
-            raise error.NotInRoster
-        return (roster[_entity].groups.intersection(authorized_groups), roster)
-
-    def _getNodeGroups(self, roster, nodeIdentifier, pep):
-        d = self.storage.getNodeGroups(nodeIdentifier, pep)
-        d.addCallback(lambda groups: (roster, groups))
-        return d
+        if not iidavoll.ILeafNode.providedBy(node):
+            defer.returnValue([])
 
-    def _rosterEb(self, failure, owner_jid):
-        log.msg("Error while getting roster of {}: {}".format(unicode(owner_jid), failure.value))
-        return {}
+        if affiliation == 'outcast':
+            raise error.Forbidden()
 
-    def _doGetItems(self, result, requestor, maxItems, itemIdentifiers,
-                    ext_data):
-        node, affiliation = result
-        if maxItems == 0:
-            log.msg("WARNING: maxItems=0 on items retrieval")
-            return []
 
-        def append_item_config(items_data):
-            """Add item config data form to items with roster access model"""
+        # node access check
+        owner = affiliation == 'owner'
+        access_model = node.getAccessModel()
+        roster = None
+
+        if access_model == const.VAL_AMODEL_OPEN or owner:
+            pass
+        elif access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
+            roster = yield self.getOwnerRoster(node)
+
+            if roster is None:
+                raise error.Forbidden()
+
+            if requestor not in roster:
+                raise error.Forbidden()
+
+            authorized_groups = yield node.getAuthorizedGroups()
+
+            if not roster[requestor].groups.intersection(authorized_groups):
+                # requestor is in roster but not in one of the allowed groups
+                raise error.Forbidden()
+        elif access_model == const.VAL_AMODEL_WHITELIST:
+            affiliations = yield node.getAffiliations()
+            try:
+                affiliation = affiliations[requestor.userhostJID()]
+            except KeyError:
+                raise error.Forbidden()
+            else:
+                if affiliation not in ('owner', 'publisher', 'member'):
+                    raise error.Forbidden()
+        else:
+            raise Exception(u"Unknown access_model")
+
+        # at this point node access is checked
+
+        if owner:
+            # requestor_groups is only used in restricted access
+            requestor_groups = None
+        else:
+            if roster is None:
+                roster = yield self.getOwnerRoster(node)
+                if roster is None:
+                    roster = {}
+            roster_item = roster.get(requestor.userhostJID())
+            requestor_groups = tuple(roster_item.groups) if roster_item else tuple()
+
+        if itemIdentifiers:
+            items_data = yield node.getItemsById(requestor_groups, owner, itemIdentifiers)
+        else:
+            items_data = yield node.getItems(requestor_groups, owner, maxItems, ext_data)
+
+        if owner:
+            # Add item config data form to items with roster access model
             for item_data in items_data:
                 if item_data.access_model == const.VAL_AMODEL_OPEN:
                     pass
-                elif item_data.access_model == const.VAL_AMODEL_ROSTER:
+                elif item_data.access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
                     form = data_form.Form('submit', formNamespace=const.NS_ITEM_CONFIG)
-                    access = data_form.Field(None, const.OPT_ACCESS_MODEL, value=const.VAL_AMODEL_ROSTER)
+                    access = data_form.Field(None, const.OPT_ACCESS_MODEL, value=const.VAL_AMODEL_PUBLISHER_ROSTER)
                     allowed = data_form.Field(None, const.OPT_ROSTER_GROUPS_ALLOWED, values=item_data.config[const.OPT_ROSTER_GROUPS_ALLOWED])
                     form.addField(access)
                     form.addField(allowed)
                     item_data.item.addChild(form.toElement())
-                elif access_model == const.VAL_AMODEL_JID:
-                    #FIXME: manage jid
+                elif access_model == const.VAL_AMODEL_WHITELIST:
+                    #FIXME
                     raise NotImplementedError
                 else:
                     raise error.BadAccessTypeError(access_model)
-            return items_data
-
-        def access_checked(access_data):
-            authorized, roster = access_data
-            if not authorized:
-                raise error.NotAuthorized()
-
-            roster_item = roster.get(requestor.userhostJID())
-            authorized_groups = tuple(roster_item.groups) if roster_item else tuple()
-            owner = affiliation == 'owner'
-
-            if itemIdentifiers:
-                d = node.getItemsById(authorized_groups, owner, itemIdentifiers)
-            else:
-                d = node.getItems(authorized_groups, owner, maxItems, ext_data)
-                if owner:
-                    d.addCallback(append_item_config)
-
-            d.addCallback(self._items_rsm, node, authorized_groups,
-                          owner, itemIdentifiers,
-                          ext_data)
-            return d
 
-        if not iidavoll.ILeafNode.providedBy(node):
-            return []
-
-        if affiliation == 'outcast':
-            raise error.Forbidden()
-
-        access_model = node.getAccessModel()
-        d = node.getNodeOwner()
-
-        def gotOwner(owner_jid):
-            d_roster = self.privilege.getRoster(owner_jid)
-            d_roster.addErrback(self._rosterEb, owner_jid)
-            return d_roster
-
-        d.addCallback(gotOwner)
-
-        if access_model == const.VAL_AMODEL_OPEN or affiliation == 'owner':
-            d.addCallback(lambda roster: (True, roster))
-            d.addCallback(access_checked)
-        elif access_model == const.VAL_AMODEL_ROSTER:
-            d.addCallback(self._getNodeGroups, node.nodeIdentifier, ext_data.get('pep', False))
-            d.addCallback(self.checkGroup, requestor)
-            d.addCallback(access_checked)
-
-        return d
+        yield self._items_rsm(items_data, node, requestor_groups, owner, itemIdentifiers, ext_data)
+        defer.returnValue(items_data)
 
     def _setCount(self, value, response):
         response.count = value
@@ -906,7 +909,6 @@
         error.NodeExists: ('conflict', None, None),
         error.Forbidden: ('forbidden', None, None),
         error.NotAuthorized: ('not-authorized', None, None),
-        error.NotInRoster: ('not-authorized', 'not-in-roster-group', None),
         error.ItemNotFound: ('item-not-found', None, None),
         error.ItemForbidden: ('bad-request', 'item-forbidden', None),
         error.ItemRequired: ('bad-request', 'item-required', None),
@@ -971,8 +973,8 @@
         recipient = data['recipient']
 
         def afterPrepare(result):
-            owner_jid, notifications_filtered = result
-            #we notify the owner
+            owners, notifications_filtered = result
+            #we notify the owners
             #FIXME: check if this comply with XEP-0060 (option needed ?)
             #TODO: item's access model have to be sent back to owner
             #TODO: same thing for getItems
@@ -989,11 +991,13 @@
                     new_item.addChild(item_config.toElement())
                 return new_item
 
-            notifications_filtered.append((owner_jid,
-                                           set([pubsub.Subscription(node.nodeIdentifier,
-                                                            owner_jid,
-                                                            'subscribed')]),
-                                           [getFullItem(item_data) for item_data in items_data]))
+            for owner_jid in owners:
+                notifications_filtered.append(
+                    (owner_jid,
+                     set([pubsub.Subscription(node.nodeIdentifier,
+                                              owner_jid,
+                                              'subscribed')]),
+                     [getFullItem(item_data) for item_data in items_data]))
 
             if pep:
                 return self.backend.privilege.notifyPublish(
@@ -1018,14 +1022,16 @@
         recipient = data['recipient']
 
         def afterPrepare(result):
-            owner_jid, notifications_filtered = result
-            #we add the owner
+            owners, notifications_filtered = result
+            #we add the owners
 
-            notifications_filtered.append((owner_jid,
-                                           set([pubsub.Subscription(node.nodeIdentifier,
-                                                            owner_jid,
-                                                            'subscribed')]),
-                                           [item_data.item for item_data in items_data]))
+            for owner_jid in owners:
+                notifications_filtered.append(
+                    (owner_jid,
+                     set([pubsub.Subscription(node.nodeIdentifier,
+                                              owner_jid,
+                                              'subscribed')]),
+                     [item_data.item for item_data in items_data]))
 
             if pep:
                 return self.backend.privilege.notifyRetract(
@@ -1044,6 +1050,7 @@
         return d
 
 
+    @defer.inlineCallbacks
     def _prepareNotify(self, items_data, node, subscription=None):
         """Do a bunch of permissions check and filter notifications
 
@@ -1062,64 +1069,52 @@
             - items_data
         """
 
-        def filterNotifications(result):
-            """Check access of subscriber for each item, and keep only allowed ones"""
-            notifications, (owner_jid,roster) = result
-
-            #we filter items not allowed for the subscribers
-            notifications_filtered = []
-
-            for subscriber, subscriptions, items_data in notifications:
-                if subscriber == owner_jid:
-                    # as notification is always sent to owner,
-                    # we ignore owner if he is here
-                    continue
-                allowed_items = [] #we keep only item which subscriber can access
-
-                for item_data in items_data:
-                    item, access_model = item_data.item, item_data.access_model
-                    access_list = item_data.config
-                    if access_model == const.VAL_AMODEL_OPEN:
-                        allowed_items.append(item)
-                    elif access_model == const.VAL_AMODEL_ROSTER:
-                        _subscriber = subscriber.userhostJID()
-                        if not _subscriber in roster:
-                            continue
-                        #the subscriber is known, is he in the right group ?
-                        authorized_groups = access_list[const.OPT_ROSTER_GROUPS_ALLOWED]
-                        if roster[_subscriber].groups.intersection(authorized_groups):
-                            allowed_items.append(item)
-
-                    else: #unknown access_model
-                        raise NotImplementedError
-
-                if allowed_items:
-                    notifications_filtered.append((subscriber, subscriptions, allowed_items))
-            return (owner_jid, notifications_filtered)
-
 
         if subscription is None:
-            d1 = self.backend.getNotifications(node.nodeDbId, items_data)
+            notifications = yield self.backend.getNotifications(node.nodeDbId, items_data)
         else:
-            d1 = defer.succeed([(subscription.subscriber, [subscription],
-                                items_data)])
+            notifications = [(subscription.subscriber, [subscription], items_data)]
 
-        def _got_owner(owner_jid):
-            #return a tuple with owner_jid and roster
-            def rosterEb(failure):
-                log.msg("Error while getting roster of {}: {}".format(unicode(owner_jid), failure.value))
-                return (owner_jid, {})
+        owners = node.getOwners()
+        owner_roster = None
 
-            d = self.backend.privilege.getRoster(owner_jid)
-            d.addErrback(rosterEb)
-            d.addCallback(lambda roster: (owner_jid,roster))
-            return d
+        # now we check access of subscriber for each item, and keep only allowed ones
 
-        d2 = node.getNodeOwner()
-        d2.addCallback(_got_owner)
-        d = defer.gatherResults([d1, d2])
-        d.addCallback(filterNotifications)
-        return d
+        #we filter items not allowed for the subscribers
+        notifications_filtered = []
+
+        for subscriber, subscriptions, items_data in notifications:
+            subscriber_bare = subscriber.userhostJID()
+            if subscriber_bare in owners:
+                # as notification is always sent to owner,
+                # we ignore owner if he is here
+                continue
+            allowed_items = [] #we keep only item which subscriber can access
+
+            for item_data in items_data:
+                item, access_model = item_data.item, item_data.access_model
+                access_list = item_data.config
+                if access_model == const.VAL_AMODEL_OPEN:
+                    allowed_items.append(item)
+                elif access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
+                    if owner_roster is None:
+                        owner_roster= yield self.getOwnerRoster(node, owners)
+                    if owner_roster is None:
+                        owner_roster = {}
+                    if not subscriber_bare in owner_roster:
+                        continue
+                    #the subscriber is known, is he in the right group ?
+                    authorized_groups = access_list[const.OPT_ROSTER_GROUPS_ALLOWED]
+                    if owner_roster[subscriber_bare].groups.intersection(authorized_groups):
+                        allowed_items.append(item)
+                else: #unknown access_model
+                    # TODO: white list access
+                    raise NotImplementedError
+
+            if allowed_items:
+                notifications_filtered.append((subscriber, subscriptions, allowed_items))
+
+        defer.returnValue((owners, notifications_filtered))
 
     def _preDelete(self, data, pep, recipient):
         nodeIdentifier = data['node'].nodeIdentifier
@@ -1132,7 +1127,6 @@
                                                 redirectURI))
         return d
 
-
     def _mapErrors(self, failure):
         e = failure.trap(*self._errorMap.keys())
 
@@ -1291,6 +1285,7 @@
         except AttributeError:
             pass
         d = self.backend.getItems(request.nodeIdentifier,
+                                  request.sender,
                                   request.recipient,
                                   request.maxItems,
                                   request.itemIdentifiers,
--- a/sat_pubsub/const.py	Sun Mar 26 20:33:18 2017 +0200
+++ b/sat_pubsub/const.py	Sun Mar 26 20:52:32 2017 +0200
@@ -63,9 +63,12 @@
 OPT_SEND_LAST_PUBLISHED_ITEM = "pubsub#send_last_published_item"
 OPT_PUBLISH_MODEL = 'pubsub#publish_model'
 VAL_AMODEL_OPEN = 'open'
-VAL_AMODEL_ROSTER = 'roster'
-VAL_AMODEL_JID = 'jid'
+VAL_AMODEL_PUBLISHER_ROSTER = 'publisher-roster'
+VAL_AMODEL_WHITELIST = 'whitelist'
+VAL_AMODEL_PUBLISH_ONLY = 'publish-only'
+VAL_AMODEL_SELF_PUBLISHER = 'self-publisher'
 VAL_AMODEL_DEFAULT = VAL_AMODEL_OPEN
+VAL_AMODEL_ALL = (VAL_AMODEL_OPEN, VAL_AMODEL_PUBLISHER_ROSTER, VAL_AMODEL_WHITELIST, VAL_AMODEL_PUBLISH_ONLY, VAL_AMODEL_SELF_PUBLISHER)
 VAL_PMODEL_PUBLISHERS = 'publishers'
 VAL_PMODEL_SUBSCRIBERS = 'subscribers'
 VAL_PMODEL_OPEN = 'open'
--- a/sat_pubsub/pgsql_storage.py	Sun Mar 26 20:33:18 2017 +0200
+++ b/sat_pubsub/pgsql_storage.py	Sun Mar 26 20:52:32 2017 +0200
@@ -55,7 +55,6 @@
 
 from zope.interface import implements
 
-from twisted.internet import defer
 from twisted.words.protocols.jabber import jid
 from twisted.python import log
 
@@ -74,27 +73,24 @@
 
 # parseXml manage str, but we get unicode
 parseXml = lambda unicode_data: generic.parseXml(unicode_data.encode('utf-8'))
+PEP_COL_NAME = 'pep'
 
 
-def withPEP(query, values, pep, recipient, pep_table=None):
+def withPEP(query, values, pep, recipient):
     """Helper method to facilitate PEP management
 
     @param query: SQL query basis
     @param values: current values to replace in query
     @param pep: True if we are in PEP mode
     @param recipient: jid of the recipient
-    @param pep_table: added before pep if table need to be specified
     @return: query + PEP AND check,
         recipient's bare jid is added to value if needed
     """
-    pep_col_name = "{}pep".format(
-                   '' if pep_table is None
-                   else ".{}".format(pep_table))
     if pep:
-        pep_check="AND {}=%s".format(pep_col_name)
+        pep_check="AND {}=%s".format(PEP_COL_NAME)
         values=list(values) + [recipient.userhost()]
     else:
-        pep_check="AND {} IS NULL".format(pep_col_name)
+        pep_check="AND {} IS NULL".format(PEP_COL_NAME)
     return "{} {}".format(query, pep_check), values
 
 
@@ -261,8 +257,7 @@
                                             WHERE jid=%s) as e""",
                        (node_id, owner))
 
-        #TODO: manage JID access
-        if config[const.OPT_ACCESS_MODEL] == const.VAL_AMODEL_ROSTER:
+        if config[const.OPT_ACCESS_MODEL] == const.VAL_AMODEL_PUBLISHER_ROSTER:
             if const.OPT_ROSTER_GROUPS_ALLOWED in config:
                 allowed_groups = config[const.OPT_ROSTER_GROUPS_ALLOWED]
             else:
@@ -271,6 +266,8 @@
                 #TODO: check that group are actually in roster
                 cursor.execute("""INSERT INTO node_groups_authorized (node_id, groupname)
                                   VALUES (%s,%s)""" , (node_id, group))
+        # XXX: affiliations can't be set on during node creation (at least not with XEP-0060 alone)
+        #      so whitelist affiliations need to be done afterward
 
     def deleteNodeByDbId(self, db_id):
         """Delete a node using directly its database id"""
@@ -294,15 +291,7 @@
         if cursor.rowcount != 1:
             raise error.NodeNotFound()
 
-    def getNodeGroups(self, nodeIdentifier, pep, recipient=None):
-        return self.dbpool.runInteraction(self._getNodeGroups, nodeIdentifier, pep, recipient)
 
-    def _getNodeGroups(self, cursor, nodeIdentifier, pep, recipient):
-        cursor.execute(*withPEP("SELECT groupname FROM node_groups_authorized NATURAL JOIN nodes WHERE node=%s",
-                                (nodeIdentifier,), pep, recipient))
-        rows = cursor.fetchall()
-
-        return [row[0] for row in rows]
 
     def getAffiliations(self, entity, pep, recipient=None):
         d = self.dbpool.runQuery(*withPEP("""SELECT node, affiliation FROM entities
@@ -347,7 +336,6 @@
         self.nodeDbId = nodeDbId
         self.nodeIdentifier = nodeIdentifier
         self._config = config
-        self.owner = None;
 
 
     def _checkNodeExists(self, cursor):
@@ -360,11 +348,9 @@
     def getType(self):
         return self.nodeType
 
-    def getNodeOwner(self):
-        if self.owner:
-            return defer.succeed(self.owner)
-        d = self.dbpool.runQuery("""SELECT jid FROM nodes NATURAL JOIN affiliations NATURAL JOIN entities WHERE node_id=%s""", (self.nodeDbId,))
-        d.addCallback(lambda result: jid.JID(result[0][0]))
+    def getOwners(self):
+        d = self.dbpool.runQuery("""SELECT jid FROM nodes NATURAL JOIN affiliations NATURAL JOIN entities WHERE node_id=%s and affiliation='owner'""", (self.nodeDbId,))
+        d.addCallback(lambda rows: [jid.JID(r[0]) for r in rows])
         return d
 
 
@@ -590,7 +576,7 @@
                        (self.nodeDbId,))
         result = cursor.fetchall()
 
-        return [(jid.internJID(r[0]), r[1]) for r in result]
+        return {jid.internJID(r[0]): r[1] for r in result}
 
 
 
@@ -640,7 +626,7 @@
         item_id = cursor.fetchone()[0];
         self._storeCategories(cursor, item_id, item_data.categories)
 
-        if access_model == const.VAL_AMODEL_ROSTER:
+        if access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
             if const.OPT_ROSTER_GROUPS_ALLOWED in item_config:
                 item_config.fields[const.OPT_ROSTER_GROUPS_ALLOWED].fieldType='list-multi' #XXX: needed to force list if there is only one value
                 allowed_groups = item_config[const.OPT_ROSTER_GROUPS_ALLOWED]
@@ -650,6 +636,7 @@
                 #TODO: check that group are actually in roster
                 cursor.execute("""INSERT INTO item_groups_authorized (item_id, groupname)
                                   VALUES (%s,%s)""" , (item_id, group))
+        # TODO: whitelist access model
 
     def _storeCategories(self, cursor, item_id, categories, update=False):
         # TODO: handle canonical form
@@ -809,18 +796,20 @@
                 item_id = data[2]
                 date = data[3]
                 access_list = {}
-                if access_model == const.VAL_AMODEL_ROSTER: #TODO: jid access_model
+                if access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
                     cursor.execute('SELECT groupname FROM item_groups_authorized WHERE item_id=%s', (item_id,))
                     access_list[const.OPT_ROSTER_GROUPS_ALLOWED] = [r[0] for r in cursor.fetchall()]
 
                 ret.append(container.ItemData(item, access_model, access_list, date=date))
+                # TODO: whitelist item access model
             return ret
 
         items_data = [container.ItemData(generic.stripNamespace(parseXml(r[0])), r[1], r[2], date=r[3]) for r in result]
         return items_data
 
     def getItemsById(self, authorized_groups, unrestricted, itemIdentifiers):
-        """ Get items which are in the given list
+        """Get items which are in the given list
+
         @param authorized_groups: we want to get items that these groups can access
         @param unrestricted: if true, don't check permissions
         @param itemIdentifiers: list of ids of the items we want to get
@@ -849,9 +838,10 @@
                 item_id = result[2]
                 date= result[3]
                 access_list = {}
-                if access_model == const.VAL_AMODEL_ROSTER: #TODO: jid access_model
+                if access_model == const.VAL_AMODEL_PUBLISHER_ROSTER:
                     cursor.execute('SELECT groupname FROM item_groups_authorized WHERE item_id=%s', (item_id,))
                     access_list[const.OPT_ROSTER_GROUPS_ALLOWED] = [r[0] for r in cursor.fetchall()]
+                 #TODO: WHITELIST access_model
 
                 ret.append(container.ItemData(item, access_model, access_list, date=date))
         else: #we check permission before returning items