Merge lp:~wallyworld/launchpad/no-private-bug-rss into lp:launchpad

[Ian Booth]

[r=jtv][bug=302097] Do not allow private rss feeds and provide a better error message

I was going to land this branch in two parts -

1. prevent the rss feed from being enabled when a private bug/branch pages is loaded
2. handle the case where inline ajax editing is used to change the bug/branch status

Part one was already put up as a mp and approved.
However, turns out you can't disable the rss icon dynamically so I've updated the original branch with some code to better handle the case where a user url hacks or attempts to get to the feed page for a private bug/branch.

== Implementation ==

There was already code in BugFeed and BranchFeed to raise an Unauthorised exception if an attempt was made to get to the feed page for a private bug/branch. This lead the user to a login page and it all went south from there as described in the bug report.

The new implementation doesn't raise an exception - it simply adds an error notification to the request response and redirects back to the page for the bug/branch. In the case of branches, the user may not have permission to see the branch so the lp code index page is used instead. For bugs, users who cannot see the bug are directed to the lp bugs index page with the error notification. So they are taken somewhere sensible and given an error message. This last bit is more of an edge case.

== Demo ==

Try and enter an rss feed url directly for a private bug or branch eg
http://feeds.launchpad.dev/bugs/14/bug.atom
or
Log in as a user (eg daf) and navigate to bug 14. The rss feed icon in the browser should be disabled.

== Tests ==

I added extra lines to the existing xx-links.txt doc test to check the two items listed at the beginning of this mp.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/feeds.py
  lib/canonical/launchpad/pagetests/feeds/xx-links.txt
  lib/lp/bugs/feed/bug.py
  lib/lp/code/feed/branch.py

./lib/canonical/launchpad/pagetests/feeds/xx-links.txt
       1: narrative uses a moin header.
       9: narrative uses a moin header.
      33: narrative uses a moin header.
      77: narrative uses a moin header.
     107: narrative uses a moin header.
     141: narrative uses a moin header.
     168: want exceeds 78 characters.
     171: want exceeds 78 characters.
     174: want exceeds 78 characters.
     177: want exceeds 78 characters.
     179: narrative uses a moin header.
     227: narrative uses a moin header.
     254: narrative uses a moin header.
     267: narrative uses a moin header.
     280: narrative uses a moin header.
     289: want exceeds 78 characters.
     293: narrative uses a moin header.
     309: narrative uses a moin header.
     325: narrative uses a moin header.
     341: narrative uses a moin header.
     351: want exceeds 78 characters.
     352: want exceeds 78 characters.
     369: source exceeds 78 characters.
./lib/lp/bugs/feed/bug.py
      80: E251 no spaces around keyword / parameter equals

[Gavin Panella (allenap)]

review: Approve

[Jeroen T. Vermeulen (jtv)]

Hi Ian,

It generally looks good but as always I have some notes. And while I'm here, thanks for the little drive-by cleanups!

=== modified file 'lib/canonical/launchpad/browser/feeds.py'
--- lib/canonical/launchpad/browser/feeds.py 2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/browser/feeds.py 2011-05-20 09:01:26 +0000
@@ -177,6 +177,14 @@
             "Context %r does not provide interface %r"
             % (context, self.usedfor))

+ @classmethod
+ def feed_allowed(cls, context):
+ """Return True if a feed is allowed for the given context.

We normally use dromedaryCase for methods, where the first word is expected to be a verb. Skimming over this change at first I thought it was a property!

The "verb" part has a corollary: "feed allowed" can be misread as "feed those that are allowed." It's not hard to figure out that that's not what you mean, but it's a small distraction that can help slow down and confuse the reader.

@@ -346,4 +368,5 @@
     def feed_links(self):
         return [feed_type(self.context)
                 for feed_type in self.feed_types
- if feed_type.usedfor.providedBy(self.context)]
+ if feed_type.usedfor.providedBy(self.context) and
+ feed_type.feed_allowed(self.context)]

The indentation of that last clause threw me off a bit. Is there some more legible way to write this?

=== modified file 'lib/canonical/launchpad/pagetests/feeds/xx-links.txt'
--- lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-05 05:46:41 +0000
+++ lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-20 09:01:26 +0000
@@ -42,6 +42,37 @@
         href="http://feeds.launchpad.dev/bugs/1/bug.atom"
         title="Bug 1 Feed" />]

+But if the bug is private, there should be no link.
+
+ >>> from canonical.launchpad.testing.pages import setupBrowserForUser
+ >>> from zope.component import getUtility
+ >>> from lp.registry.interfaces.person import IPersonSet
+ >>> login(ANONYMOUS)
+ >>> user = getUtility(IPersonSet).getByEmail('<email address hidden>')
+ >>> logout()
+ >>> auth_browser = setupBrowserForUser(user, 'daf')
+ >>> auth_browser.open('http://bugs.launchpad.dev/jokosher/+bug/14')
+ >>> soup = BeautifulSoup(auth_browser.contents)
+ >>> soup.head.findAll('link', type='application/atom+xml')
+ []

What exactly is auth_browser for? If you merely need a brower for a logged-in user, there's no need to construct one; use user_browser which is set up implicitly for this usage.

+But if they somehow manage to hack the url or use inline ajax editing of the
+bug status and attempt to subscribe, they are redirected to the bug page:

You're "But"-ing your own "But"!

+ >>> def print_notifications(browser):
+ ... tags = find_tags_by_class(
+ ... browser.contents, 'error message')
+ ... if len(tags) == 0:
+ ... print 'No messages.'
+ ... else:
+ ... for tag in tags:
+ ... print extract_text(tag)

We have a helper for this: get_feedback_messages.

@@ -319,3 +350,24 @@
     [<link rel="alternate" type="application/atom+xml"
         href="http://feeds.launch...

review: Approve (code)

[Ian Booth (wallyworld)]

Yello

>
> It generally looks good but as always I have some notes. And while I'm here, thanks for the little drive-by cleanups!
>

I always appreciate the thoroughness of your reviews.

>
>
> We normally use dromedaryCase for methods, where the first word is expected to be a verb. Skimming over this change at first I thought it was a property!
>
> The "verb" part has a corollary: "feed allowed" can be misread as "feed those that are allowed." It's not hard to figure out that that's not what you mean, but it's a small distraction that can help slow down and confuse the reader.
>

Good point. Will fix.

>
>
> @@ -346,4 +368,5 @@
> def feed_links(self):
> return [feed_type(self.context)
> for feed_type in self.feed_types
> - if feed_type.usedfor.providedBy(self.context)]
> + if feed_type.usedfor.providedBy(self.context) and
> + feed_type.feed_allowed(self.context)]
>
>
> The indentation of that last clause threw me off a bit. Is there some more legible way to write this?
>

I'll find something.

>
> === modified file 'lib/canonical/launchpad/pagetests/feeds/xx-links.txt'
> --- lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-05 05:46:41 +0000
> +++ lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-20 09:01:26 +0000
> @@ -42,6 +42,37 @@
> href="http://feeds.launchpad.dev/bugs/1/bug.atom"
> title="Bug 1 Feed" />]
>
> +But if the bug is private, there should be no link.
> +
> + >>> from canonical.launchpad.testing.pages import setupBrowserForUser
> + >>> from zope.component import getUtility
> + >>> from lp.registry.interfaces.person import IPersonSet
> + >>> login(ANONYMOUS)
> + >>> user = getUtility(IPersonSet).getByEmail('<email address hidden>')
> + >>> logout()
> + >>> auth_browser = setupBrowserForUser(user, 'daf')
> + >>> auth_browser.open('http://bugs.launchpad.dev/jokosher/+bug/14')
> + >>> soup = BeautifulSoup(auth_browser.contents)
> + >>> soup.head.findAll('link', type='application/atom+xml')
> + []
>
> What exactly is auth_browser for? If you merely need a brower for a logged-in user, there's no need to construct one; use user_browser which is set up implicitly for this usage.
>

I'll take another look. There's already a "browser" instance which is
used for (I think) non-authenticated access but I didn't realise there
was already a "user_browser" instance.

>
> +But if they somehow manage to hack the url or use inline ajax editing of the
> +bug status and attempt to subscribe, they are redirected to the bug page:
>
>
> You're "But"-ing your own "But"!
>
>
> + >>> def print_notifications(browser):
> + ... tags = find_tags_by_class(
> + ... browser.contents, 'error message')
> + ... if len(tags) == 0:
> + ... print 'No messages.'
> + ... else:
> + ... for tag in tags:
> + ... print extract_text(tag)
>
> We have a helper for this: get_feedback_messages.
>

Really? I copied the above code from elsewhere :-)
I'll fix it.

>
> === modified file 'lib/lp/bugs/feed/bug.py'
> --- lib/lp/bugs/feed/bug.py 2...

[Ian Booth (wallyworld)]

>
> What exactly is auth_browser for? If you merely need a brower for a logged-in user, there's no need to construct one; use user_browser which is set up implicitly for this usage.
>

I looked at the code again in more detail. I need more than just "a"
logged in user. I need a browser logged in for a specific user so that a
private bug and a private branch can be accessed. Hence I get the user I
need:

user = getUtility(IPersonSet).getByEmail('<email address hidden>')

and then get the logged in browser for that user, using the password =
'daf':

auth_browser = setupBrowserForUser(user, 'daf')

So I think the use of auth_browser is ok unless there is something I am
missing.

=== modified file 'lib/canonical/launchpad/browser/feeds.py'
--- lib/canonical/launchpad/browser/feeds.py	2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/browser/feeds.py	2011-05-24 03:31:14 +0000
@@ -177,6 +177,14 @@
             "Context %r does not provide interface %r"
             % (context, self.usedfor))
 
+    @classmethod
+    def allowFeed(cls, context):
+        """Return True if a feed is allowed for the given context.
+
+        Subclasses should override this method as necessary.
+        """
+        return True
+
 
 class BugFeedLink(FeedLinkBase):
     usedfor = IBugTask
@@ -190,6 +198,12 @@
         return urlappend(self.rooturl,
                          'bugs/' + str(self.context.bug.id) + '/bug.atom')
 
+    @classmethod
+    def allowFeed(cls, context):
+        """See `FeedLinkBase`"""
+        # No feeds for private bugs.
+        return not context.bug.private
+
 
 class BugTargetLatestBugsFeedLink(FeedLinkBase):
     usedfor = IHasBugs
@@ -222,6 +236,7 @@
             return urlappend(canonical_url(self.context, rootsite='feeds'),
                              'announcements.atom')
 
+
 class RootAnnouncementsFeedLink(AnnouncementsFeedLink):
     usedfor = ILaunchpadRoot
 
@@ -296,11 +311,18 @@
     @property
     def title(self):
         return 'Latest Revisions for Branch %s' % self.context.displayname
+
     @property
     def href(self):
         return urlappend(canonical_url(self.context, rootsite="feeds"),
                          'branch.atom')
 
+    @classmethod
+    def allowFeed(cls, context):
+        """See `FeedLinkBase`"""
+        # No feeds for private branches.
+        return not context.private
+
 
 class PersonRevisionsFeedLink(FeedLinkBase):
     """Feed links for revisions created by a person."""
@@ -344,6 +366,11 @@
 
     @property
     def feed_links(self):
+
+        def allowFeed(feed_type, context):
+            return (feed_type.usedfor.providedBy(context) and
+                feed_type.allowFeed(context))
+
         return [feed_type(self.context)
-                for feed_type in self.feed_types
-                if feed_type.usedfor.providedBy(self.context)]
+            for feed_type in self.feed_types
+            if allowFeed(feed_type, self.context)]

=== removed file 'lib/canonical/launchpad/pagetests/feeds/xx-authentication.txt'
--- lib/canonical/launchpad/pagetests/feeds/xx-authentication.txt	2009-10-22 13:02:12 +0000
+++ lib/canonical/launchpad/pagetests/feeds/xx-authentication.txt	1970-01-01 00:00:00 +0000
@@ -1,26 +0,0 @@
-= Authentication for feeds =
-
-Requests made to feeds.launchpad.dev are not challenged for
-authentication, so most access is done anonymously.  In the event a
-request provides basic authentication, even though not requested by
-the server, the web application will nonetheless use the
-unauthenticated anonymous user for all interaction.
-
-    # Setup a Foo Bar user and let Zope handle the errors.
-    >>> browser = setupBrowser(auth='Basic [e-mail]:test')
-    >>> browser.handleErrors = True
-
-    # First, get the bug through the web site, to ensure the
-    # bug exists and is private.
-    >>> browser.open('http://launchpad.dev/bugs/14')
-    >>> print browser.url
-    http://bugs.launchpad.dev/jokosher/+bug/14
-
-    # Traversing to the private bug will trigger an Unauthorized error.
-    # For feeds, this will result in a Forbidden error.  This test
-    # ensures FeedsUnauthorizedError works and that the unauthorized
-    # user is the current user.
-    >>> browser.open('http://feeds.launchpad.dev/bugs/14/bug.atom')
-    Traceback (most recent call last):
-      ...
-    HTTPError: HTTP Error 403: Forbidden

=== modified file 'lib/canonical/launchpad/pagetests/feeds/xx-links.txt'
--- lib/canonical/launchpad/pagetests/feeds/xx-links.txt	2011-05-05 05:46:41 +0000
+++ lib/canonical/launchpad/pagetests/feeds/xx-links.txt	2011-05-24 03:31:14 +0000
@@ -42,6 +42,34 @@
         href="http://feeds.launchpad.dev/bugs/1/bug.atom"
         title="Bug 1 Feed" />]
 
+But if the bug is private, there should be no link.
+
+    # Set up an authenticated browser.
+    >>> from canonical.launchpad.testing.pages import setupBrowserForUser
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> login(ANONYMOUS)
+    >>> user = getUtility(IPersonSet).getByEmail('[e-mail]')
+    >>> logout()
+    >>> auth_browser = setupBrowserForUser(user, 'daf')
+
+    # First check that the bug exists.
+    >>> auth_browser.open('http://launchpad.dev/bugs/14')
+    >>> print auth_browser.url
+    http://bugs.launchpad.dev/jokosher/+bug/14
+
+    >>> soup = BeautifulSoup(auth_browser.contents)
+    >>> soup.head.findAll('link', type='application/atom+xml')
+    []
+
+Even so, if they somehow manage to hack the url or use inline ajax editing of
+the bug status and attempt to subscribe, they are redirected to the bug page:
+
+    >>> auth_browser.open('http://feeds.launchpad.dev/bugs/14/bug.atom')
+    >>> print auth_browser.url
+    http://bugs.launchpad.dev/
+    >>> print get_feedback_messages(auth_browser.contents)
+    [u'The requested bug is private. Feeds do not serve private bugs.']
 
 == Latest Bugs and Branches for a Person ==
 
@@ -319,3 +347,24 @@
     [<link rel="alternate" type="application/atom+xml"
         href="http://feeds.launchpad.dev/~mark/firefox/release--0.9.1/branch.atom"
 	title="Latest Revisions for Branch lp://dev/~mark/firefox/release--0.9.1" />]
+
+But if the branch is private, there should be no link.
+
+    >>> login(ANONYMOUS)
+    >>> user = getUtility(IPersonSet).getByEmail('[e-mail]')
+    >>> logout()
+    >>> auth_browser = setupBrowserForUser(user)
+    >>> auth_browser.open(
+    ... 'https://code.launchpad.dev/~name12/landscape/feature-x')
+    >>> soup = BeautifulSoup(auth_browser.contents)
+    >>> soup.head.findAll('link', type='application/atom+xml')
+    []
+
+Even so, if they somehow manage to hack the url, they are redirected to a page
+with an error notification:
+
+    >>> browser.open('http://feeds.launchpad.dev/~name12/landscape/feature-x/branch.atom')
+    >>> print browser.url
+    http://code.launchpad.dev/
+    >>> print get_feedback_messages(browser.contents)
+    [u'The requested branch is private. Feeds do not serve private branches.']

=== modified file 'lib/canonical/launchpad/pagetests/feeds/xx-security.txt'
--- lib/canonical/launchpad/pagetests/feeds/xx-security.txt	2011-05-05 05:46:41 +0000
+++ lib/canonical/launchpad/pagetests/feeds/xx-security.txt	2011-05-24 03:31:14 +0000
@@ -42,13 +42,6 @@
     >>> BSS(browser.contents)('entry')
     []
 
-Looking up a single bug raises an Unauthorized error if it is private.
-
-    >>> browser.open('http://feeds.launchpad.dev/bugs/1/bug.atom')
-    Traceback (most recent call last):
-      ...
-    Unauthorized: ...
-
 There should be just one <tr> elements for the table header in
 these HTML feeds, since all the bugs are private.
 
@@ -89,9 +82,9 @@
     >>> try:
     ...      browser.open('http://feeds.launchpad.dev/bugs/1/bug.html')
     ... except Unauthorized:
-    ...     pass
-    ... else:
-    ...     print "Should raise Unauthorized exception"
+    ...     print "Shouldn't  raise Unauthorized exception"
+    >>> BSS(browser.contents)('entry')
+    []
 
 Revert configuration change after tests are finished.
 

=== modified file 'lib/lp/bugs/feed/bug.py'
--- lib/lp/bugs/feed/bug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/feed/bug.py	2011-05-24 03:31:14 +0000
@@ -14,13 +14,14 @@
 
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.component import getUtility
-from zope.security.interfaces import Unauthorized
 
 from canonical.config import config
 from canonical.launchpad.webapp import (
     canonical_url,
     urlparse,
     )
+from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
 from canonical.launchpad.webapp.publisher import LaunchpadView
 from canonical.lazr.feed import (
     FeedBase,
@@ -141,8 +142,8 @@
     def getBugsFromBugTasks(self, tasks):
         """Given a list of BugTasks return the list of associated bugs.
 
-        Since a Bug can have multiple BugTasks, we only select bugs that have not
-        yet been seen.
+        Since a Bug can have multiple BugTasks, we only select bugs that have
+        not yet been seen.
         """
         bug_ids = []
         for task in tasks:
@@ -171,7 +172,18 @@
         # For a `BugFeed` we must ensure that the bug is not private.
         super(BugFeed, self).initialize()
         if self.context.private:
-            raise Unauthorized("Feeds do not serve private bugs")
+            if check_permission("launchpad.View", self.context):
+                message_prefix = "This bug is private."
+                redirect_url = canonical_url(self.context)
+            else:
+                # Bug cannot be seen so redirect to the bugs index page.
+                message_prefix = "The requested bug is private."
+                root = getUtility(ILaunchpadRoot)
+                redirect_url = canonical_url(root, rootsite='bugs')
+
+            self.request.response.addErrorNotification(
+                message_prefix + " Feeds do not serve private bugs.")
+            self.request.response.redirect(redirect_url)
 
     @property
     def title(self):

=== modified file 'lib/lp/code/feed/branch.py'
--- lib/lp/code/feed/branch.py	2011-03-03 01:13:47 +0000
+++ lib/lp/code/feed/branch.py	2011-05-24 03:31:14 +0000
@@ -30,6 +30,7 @@
     LaunchpadView,
     urlparse,
     )
+from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
 from canonical.lazr.feed import (
     FeedBase,
     FeedEntry,
@@ -74,6 +75,7 @@
         super(BranchFeedContentView, self).__init__(context, request)
         self.feed = feed
         self.template_ = template
+
     def render(self):
         """Render the view."""
         return ViewPageTemplateFile(self.template_)(self)
@@ -375,7 +377,7 @@
 
     def __init__(self, person, rootsite):
 
-        no_email =  person.name_without_email
+        no_email = person.name_without_email
         if no_email:
             self.name = no_email
         else:
@@ -400,8 +402,25 @@
         """See `IFeed`."""
         # For a `BranchFeed` we must ensure that the branch is not private.
         super(BranchFeed, self).initialize()
-        if self.context.private:
-            raise Unauthorized("Feeds do not serve private branches")
+        try:
+            feed_allowed = not self.context.private
+            if not feed_allowed:
+                # We are logged in and can see the branch so redirect to the
+                # branch index page.
+                message_prefix = "This branch is private."
+                redirect_url = canonical_url(self.context)
+        except Unauthorized:
+            # Branch cannot be seen so redirect to the code index page.
+            feed_allowed = False
+            message_prefix = "The requested branch is private."
+            root = getUtility(ILaunchpadRoot)
+            redirect_url = canonical_url(root, rootsite='code')
+
+        if not feed_allowed:
+            self.request.response.addErrorNotification(
+                message_prefix +
+                " Feeds do not serve private branches.")
+            self.request.response.redirect(redirect_url)
 
     @property
     def title(self):