Update after release of Private Bookmarks

- Fix two issues with private bookmarks exposing data.
- Update query for with_content to work properly.
- Update tests to make bookmarks public by default.
- Updates from production during release process.

bookie/bcelery/celery.py

@@ -55,7 +55,7 @@ def load_ini():
         },
         'fetch_unfetched': {
             'task': 'bookie.bcelery.tasks.fetch_unfetched_bmark_content',
-            'schedule': timedelta(seconds=60),
+            'schedule': timedelta(seconds=60*60),
         },
     }
 )

bookie/models/__init__.py

@@ -395,7 +395,7 @@ def get_by_url(url, username=None):
         if username:
             qry = qry.filter(Bmark.username == username)
 
-        return qry.one()
+        return qry.first()
 
     @staticmethod
     def get_by_hash(hash_id, username=None):
@@ -415,20 +415,18 @@ def find(limit=50, order_by=None, page=0, tags=None, username=None,
              with_content=False, with_tags=True, requested_by=None):
         """Search for specific sets of bookmarks"""
         qry = Bmark.query
-        # qry = qry.join(Bmark.hashed).\
-        #     options(contains_eager(Bmark.hashed))
+        qry = qry.join(Bmark.hashed).\
+            options(contains_eager(Bmark.hashed))
 
         offset = limit * page
 
-        if requested_by != username:
+        # If noqa is not used here the below error occurs with make lint.
+        # comparison to False should be 'if cond is False:'
+        # or 'if not cond:'
+        if not requested_by:
+            qry = qry.filter(Bmark.is_private == False)    # noqa
+        elif requested_by != username:
             qry = qry.filter(Bmark.is_private == False)    # noqa
-            # If noqa is not used here the below error occurs with make lint.
-            # comparison to False should be 'if cond is False:'
-            # or 'if not cond:'
-
-        if with_content:
-            qry = qry.outerjoin(Bmark.readable).\
-                options(contains_eager(Bmark.readable))
 
         if username:
             qry = qry.filter(Bmark.username == username)
@@ -453,6 +451,14 @@ def find(limit=50, order_by=None, page=0, tags=None, username=None,
                     offset(offset).\
                     from_self()
             else:
+                if username:
+                    good_filter = and_(
+                        Bmark.bid == bmarks_tags.c.bmark_id,
+                        Bmark.username == username
+                    )
+                else:
+                    good_filter = (Bmark.bid == bmarks_tags.c.bmark_id)
+
                 bids_we_want = select(
                     [bmarks_tags.c.bmark_id.label('good_bmark_id')],
                     from_obj=[
@@ -463,7 +469,7 @@ def find(limit=50, order_by=None, page=0, tags=None, username=None,
                                 bmarks_tags.c.tag_id == Tag.tid
                             )
                         ).
-                        join('bmarks', Bmark.bid == bmarks_tags.c.bmark_id)
+                        join('bmarks', good_filter)
                     ]).\
                     group_by(bmarks_tags.c.bmark_id, Bmark.stored).\
                     having(
@@ -478,14 +484,17 @@ def find(limit=50, order_by=None, page=0, tags=None, username=None,
                 )
 
         # now outer join with the tags again so that we have the
-        # full list of tags for each bmark we filterd down to
+        # full list of tags for each bmark we filtered down to
         if with_tags:
             qry = qry.outerjoin(Bmark.tags).\
                 options(contains_eager(Bmark.tags))
 
-        qry = qry.options(joinedload('hashed'))
+        if with_content:
+            qry = qry.outerjoin(Bmark.readable).\
+                options(contains_eager(Bmark.readable))
 
-        return qry.all()
+        qry = qry.options(joinedload('hashed'))
+        return qry.order_by(order_by).all()
 
     @staticmethod
     def user_dump(username):
@@ -528,7 +537,7 @@ def popular(limit=50, page=0, with_tags=False):
 
     @staticmethod
     def store(url, username, desc, ext, tags, dt=None, inserted_by=None,
-              is_private=True):
+              is_private=False):
         """Store a bookmark
 
         :param url: bookmarked url
@@ -636,7 +645,7 @@ class Bmark(Base):
     stored = Column(DateTime, default=datetime.utcnow)
     updated = Column(DateTime, onupdate=datetime.utcnow)
     clicks = Column(Integer, default=0)
-    is_private = Column(Boolean, nullable=False, default=True)
+    is_private = Column(Boolean, nullable=False, default=False)
 
     # this could be chrome_extension, firefox_extension, website, browser XX,
     # import, etc
@@ -668,7 +677,7 @@ class Bmark(Base):
                         uselist=False)
 
     def __init__(self, url, username, desc=None, ext=None, tags=None,
-                 is_private=True):
+                 is_private=False):
         """Create a new bmark instance
 
         :param url: string of the url to be added as a bookmark

bookie/tests/factory.py

@@ -79,12 +79,13 @@ def make_twitter_connection(username='admin'):
     return tconnection
 
 
-def make_bookmark(user=None):
+def make_bookmark(user=None, is_private=False):
     """Generate a fake bookmark for testing use."""
     bmark = Bmark(random_url(),
                   username=u"admin",
                   desc=random_string(),
                   ext=random_string(),
+                  is_private=is_private,
                   tags=u"bookmarks")
 
     if user:

bookie/tests/test_api/test_base_api.py

@@ -232,6 +232,7 @@ def test_add_private_bookmark(self):
             'tags': u'bookmarks',
             'api_key': key,
             'username': u'admin',
+            'is_private': 'true',
         }
 
         res = self.testapp.post('/api/v1/admin/bmark',
@@ -624,7 +625,7 @@ def test_bookmark_recent_diff_user(self):
                                status=200)
 
         # Make sure we can decode the body.
-        bmark = json.loads(res.body)['bmarks'][0]
+        bmark = json.loads(res.body)['bmarks'][1]
         self.assertEqual(
             GOOGLE_HASH,
             bmark[u'hash_id'],

bookie/tests/test_models/test_bmarkmgr.py

@@ -50,6 +50,7 @@ def test_total_ct_accounts_for_privacy(self):
             b = Bmark(
                 url=gen_random_word(12),
                 username=user.username,
+                is_private=True
             )
             b.hash_id = gen_random_word(3)
             DBSession.add(b)
@@ -113,20 +114,23 @@ def test_unique_ct_accounts_for_privacy(self):
             b = Bmark(
                 url=gen_random_word(12),
                 username=users[i].username,
+                is_private=True
             )
             DBSession.add(b)
 
         # Add in our dupes
         c = Bmark(
             url=common,
             username=users[3].username,
+            is_private=True
         )
         DBSession.add(c)
         DBSession.flush()
 
         d = Bmark(
             url=common,
             username=users[4].username,
+            is_private=True,
         )
         DBSession.add(d)
         DBSession.flush()
@@ -192,19 +196,22 @@ def test_per_user_accounts_for_privacy(self):
             b = Bmark(
                 url=gen_random_word(12),
                 username=user.username,
+                is_private=True,
             )
             DBSession.add(b)
 
         c = Bmark(
             url=gen_random_word(10),
             username=usercommon.username,
+            is_private=True,
         )
         DBSession.add(c)
         DBSession.flush()
 
         d = Bmark(
             url=gen_random_word(10),
             username=usercommon.username,
+            is_private=True,
         )
         DBSession.add(d)
         DBSession.flush()
@@ -335,6 +342,7 @@ def test_find_bookmarks_diff_user(self):
             b = Bmark(
                 url=gen_random_word(12),
                 username=user.username,
+                is_private=True,
             )
             DBSession.add(b)
 

bookie/tests/test_models/test_privatebmark.py

@@ -20,7 +20,8 @@ def test_is_private_default(self):
         user.username = gen_random_word(10)
         bmark = Bmark(
             url=gen_random_word(12),
-            username=user.username
+            username=user.username,
+            is_private=True,
         )
         self.assertEqual(
             True,
@@ -66,7 +67,8 @@ def test_storing_private_true(self):
             username=user.username,
             desc=bmark_desc,
             ext=bmark_ext,
-            tags=bmark_tags
+            is_private=True,
+            tags=bmark_tags,
         )
         self.assertEqual(
             True,

bookie/tests/test_models/test_tagmgr.py

@@ -47,7 +47,7 @@ def test_basic_complete_same_user(self):
         bmark_ext = u'Some extended notes'
 
         # Store the bookmark.
-        bmark = make_bookmark()
+        bmark = make_bookmark(is_private=True)
         bmark.url = bmark_url
         bmark.username = user.username
         bmark.description = bmark_desc
@@ -82,7 +82,7 @@ def test_basic_complete_same_user_accounts_for_privacy(self):
         bmark_ext = u'Some extended notes'
 
         # Store the bookmark.
-        bmark = make_bookmark()
+        bmark = make_bookmark(is_private=True)
         bmark.url = bmark_url
         bmark.username = user.username
         bmark.description = bmark_desc
@@ -116,7 +116,7 @@ def test_basic_complete_diff_user(self):
         bmark_ext = u'Some extended notes'
 
         # Store the bookmark.
-        bmark = make_bookmark()
+        bmark = make_bookmark(is_private=True)
         bmark.url = bmark_url
         bmark.username = user.username
         bmark.description = bmark_desc
@@ -157,7 +157,7 @@ def test_basic_complete_diff_user_accounts_for_privacy(self):
         bmark_ext = u'Some extended notes'
 
         # Store the bookmark.
-        bmark = make_bookmark()
+        bmark = make_bookmark(is_private=True)
         bmark.url = bmark_url
         bmark.username = user.username
         bmark.description = bmark_desc
@@ -197,7 +197,7 @@ def test_case_insensitive(self):
         bmark_ext = u'Some extended notes'
 
         # Store the bookmark.
-        bmark = make_bookmark()
+        bmark = make_bookmark(is_private=True)
         bmark.url = bmark_url
         bmark.username = user.username
         bmark.description = bmark_desc

bookie/views/api.py

@@ -193,8 +193,13 @@ def bmark_get(request):
 
 def _update_mark(mark, params):
     """Update the bookmark found with settings passed in"""
-    mark.description = params.get('description', mark.description)
+    description = params.get('description', None)
+    if not description and mark:
+        description = mark.description
+
+    mark.description = description
     mark.extended = params.get('extended', mark.extended)
+    mark.is_private = asbool(params.get('is_private', False))
 
     new_tag_str = params.get('tags', None)
 
@@ -250,23 +255,31 @@ def bmark_add(request):
                 rdict['hash_id'],
                 username=user.username
             )
-            mark = _update_mark(mark, params)
 
         except NoResultFound:
+            mark = None
+
+        if mark:
+            mark = _update_mark(mark, params)
+        else:
             request.response.status_code = 404
             return _api_response(request, {
                 'error': 'Bookmark with hash id {0} not found.'.format(
                          rdict['hash_id'])
             })
 
     else:
-        # check if we already have this
+        # Check if we already have this bookmark.
         try:
             mark = BmarkMgr.get_by_url(params['url'],
                                        username=user.username)
-            mark = _update_mark(mark, params)
 
         except NoResultFound:
+            mark = None
+
+        if mark:
+            mark = _update_mark(mark, params)
+        else:
             # then let's store this thing
             # if we have a dt param then set the date to be that manual
             # date
@@ -289,7 +302,7 @@ def bmark_add(request):
                 params.get('tags', u''),
                 dt=stored_time,
                 inserted_by=inserted_by,
-                is_private=params.get('is_private', True),
+                is_private=asbool(params.get('is_private', False)),
             )
 
         # we need to process any commands associated as well
@@ -360,6 +373,8 @@ def bmark_recent(request, with_content=False):
     # check if we have a page count submitted
     page = int(params.get('page', '0'))
     count = int(params.get('count', RESULTS_MAX))
+    if not with_content:
+        with_content = asbool(params.get('with_content', False))
 
     # we only want to do the username if the username is in the url
     username = rdict.get('username', None)
@@ -403,14 +418,14 @@ def bmark_recent(request, with_content=False):
     # if we allow showing of content the query hangs and fails on the
     # postgres side. Need to check the query and figure out what's up.
     # see bug #142
-    # We don't allow with_content by default because of this bug.
     recent_list = BmarkMgr.find(
         limit=count,
         order_by=order_by,
         page=page,
         tags=tags,
         username=username,
         with_tags=True,
+        with_content=with_content,
         requested_by=requested_by,
     )