Merge lp:~beuno/loggerhead/serve-config into lp:loggerhead

[Martin Albisetti]

[Martin Albisetti (beuno)]

This branch does the following:
- Allows hiding branches with serve-branches
- Tests it
- Adds some mising copyright headers
- Renames README.txt to README

[Matt Nordhoff (mnordhoff)]

Without doing a real review, I spotted a couple trivial whitespace issues:

* The copyright headers tend to have 2 spaces between "2009" and "Canonical".

* TestEmptyBranch's docstring starts with a space.

Aside from that, one question: What about using 403 Forbidden for hidden branches instead of 404 Not Found? Obviously it'd reveal their existence, but it's also more true to a setting named "serve_http = False". Sort of. Maybe. :D

[Adrian Wilkins (adrian-wilkins)]

> What about using 403 Forbidden for hidden
> branches instead of 404 Not Found? Obviously it'd reveal their existence, but
> it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> :D

I've noted that some servers go the other way, and pitch a 403 for requests to non-existent branches as well. This is either "more secure" or "of dubious helpfulness", depending on your point of view.

[Matt Nordhoff (mnordhoff)]

> > What about using 403 Forbidden for hidden
> > branches instead of 404 Not Found? Obviously it'd reveal their existence,
> but
> > it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> > :D
>
> I've noted that some servers go the other way, and pitch a 403 for requests to
> non-existent branches as well. This is either "more secure" or "of dubious
> helpfulness", depending on your point of view.

For truly non-existent branches, I vote 404.

For hidden branches, IMO 403 is correct, but I can accept 404 for the privacy aspect.

Anyway, what do you want to do here? I'm okay either way. :)

[Matt Nordhoff (mnordhoff)]

I approve, pending the 403 vs. 404 bikeshed I started, but someone more knowledgeable than me should verify that you didn't miss any entry points.

Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts between them and this branch, and I'd prefer that you fix them. I could probably handle it myself, but it'd mostly be guesswork, trying random things until it stopped spewing 500 Internal Server Errors.

review: Needs Fixing

[lp:~beuno/loggerhead/serve-config]

361. By Martin Albisetti on 2009-05-27

Fixed whitespace and other trivial bits from the review

[Martin Albisetti (beuno)]

> * The copyright headers tend to have 2 spaces between "2009" and "Canonical".
>
> * TestEmptyBranch's docstring starts with a space.

Done.

> Aside from that, one question: What about using 403 Forbidden for hidden
> branches instead of 404 Not Found? Obviously it'd reveal their existence, but
> it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> :D

I chose 404 because it gives out less about what's available. I feel that if we spit out a 403, there has to be some sort of ACL or way of viewing them, when there actually isn't.

[Matt Nordhoff (mnordhoff)]

> I approve, pending the 403 vs. 404 bikeshed I started, but someone more
> knowledgeable than me should verify that you didn't miss any entry points.
>
> Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or
> lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts
> between them and this branch, and I'd prefer that you fix them. I could
> probably handle it myself, but it'd mostly be guesswork, trying random things
> until it stopped spewing 500 Internal Server Errors.

Also: I think you should use ConfigObj's as_bool method instead of "== 'False'".

[Matt Nordhoff (mnordhoff)]

> Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or
> lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts
> between them and this branch, and I'd prefer that you fix them. I could
> probably handle it myself, but it'd mostly be guesswork, trying random things
> until it stopped spewing 500 Internal Server Errors.

Those branches have now landed, FYI.

[lp:~beuno/loggerhead/serve-config]

362. By Martin Albisetti on 2009-05-28

Merge from trunk and fix conflict by refactoring the code

[Martin Albisetti (beuno)]

Ok, merged with trunk and resolved conflicts by refactoring.
All tests pass :)

[Martin Albisetti (beuno)]

> Also: I think you should use ConfigObj's as_bool method instead of "==
> 'False'".

Would you be so kind as to point me to an example of this?

[Matt Nordhoff (mnordhoff)]

I just noticed that a few tabs got introduced into the code:

$ ack-grep --python '\t'
loggerhead/apps/transport.py
111: bzrdir = BzrDir.open_containing_from_transport(
112: self.transport.clone(path_info))
113: branch = bzrdir.open_branch()
114: if branch.get_config().get_user_option('http_serve') == 'False':
115: raise httpexceptions.HTTPNotFound()
117: return

loggerhead/controllers/directory_ui.py
75: if b.get_config().get_user_option('http_serve') == 'False':

[Matt Nordhoff (mnordhoff)]

Martin Albisetti wrote:
> Ok, merged with trunk and resolved conflicts by refactoring.
> All tests pass :)

I fixed the tabs and a couple other trivial things in
lp:~mnordhoff/loggerhead/serve-config.

However, now there's a different issue: When serving branches on a
remote transport, trying to access .bzr/ results in a
TooManyConcurrentRequests traceback from the
BzrDir.open_containing_from_transport() call in check_is_a_branch().

E.g.:

serve-config$ bzr serve --directory .. &
serve-config$ ./serve-branches bzr://localhost/ &
serve-config$ bzr log -r -1 http://localhost:8080/serve-config/

[Matt Nordhoff (mnordhoff)]

Martin Albisetti wrote:
>> Also: I think you should use ConfigObj's as_bool method instead of "==
>> 'False'".
>
> Would you be so kind as to point me to an example of this?

Sorry, I dunno. When going through Bazaar's configuration system, I'm
not sure if it's possible to use it.

[lp:~beuno/loggerhead/serve-config]

363. By Martin Albisetti on 2009-06-01

Merge in Peng's tab fixes

[Michael Hudson-Doyle (mwhudson)]

I guess this is bb:comment... two points:

1) I'm not sure what checking in branch.py and in transport.py really gains you -- I think just checking in branch.py would be OK?

2) '== "False"' as a condition check is pretty icky, I think at least 'false' should be accepted too. It's unfortunate that bzrlib doesn't let you at the as_bool method of configobj. Perhaps we can just paste in a version of that as a standalone function? And file a bug on bzrlib.

Let's chat about these points on IRC sometime.

review: Needs Information

[Martin Albisetti (beuno)]

 > 1) I'm not sure what checking in branch.py and in transport.py really gains
> you -- I think just checking in branch.py would be OK?

Checking in transport.py blocks serving branches as well I think.

> 2) '== "False"' as a condition check is pretty icky, I think at least 'false'
> should be accepted too. It's unfortunate that bzrlib doesn't let you at the
> as_bool method of configobj. Perhaps we can just paste in a version of that
> as a standalone function? And file a bug on bzrlib.
>
> Let's chat about these points on IRC sometime.

Sure, ping me.

[lp:~beuno/loggerhead/serve-config]

364. By Martin Albisetti on 2009-06-02

Merge from trunk

365. By Martin Albisetti on 2009-06-02

Add deprecation warning to start-loggerhead

[Michael Hudson-Doyle (mwhudson)]

Land it, let's deal with the fallout later :)

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-05-17 11:10:38 +0000
+++ NEWS	2009-05-27 12:51:51 +0000
@@ -115,6 +115,10 @@
 
     - Fix 'bzr serve --http' errors. (Matt Nordhoff, #377551)
 
+    - Added the option to hide branches by setting http_serve = False
+      in locations.conf (Martin Albisetti)
+
+
 1.10 [22Dec2008]
 ---------------
 
 
=== renamed file 'README.txt' => 'README'
--- README.txt	2009-03-17 22:31:42 +0000
+++ README	2009-05-27 12:52:49 +0000
@@ -1,7 +1,7 @@
 LOGGERHEAD
 ==========
 
-[ Version 1.6 for Bazaar 1.6 ]
+[ Version 1.10 for Bazaar 1.6 ]
 
 Loggerhead is a web viewer for Bazaar branches.  It can be used to
 navigate a branch history, annotate files, perform searches... all the
@@ -63,31 +63,14 @@
 USING A CONFIG FILE
 -------------------
 
-Previous versions of Loggerhead read their configuration from a config
-file.  This mode of operation is still supported by the
-'start-loggerhead' script.  A 'loggerhead.conf.example' file is
-included in the source which has comments explaining the various
-options.
-
-Loggerhead can then be started by running::
-
-    $ ./start-loggerhead
-
-This will run loggerhead in the background, listening on port 8080 by
-default.
-
-To stop Loggerhead, run::
-
-    $ ./stop-loggerhead
-
-In the configuration file you can configure projects, and branches per
-project.  The idea is that you could be publishing several (possibly
-unrelated) projects through the same loggerhead instance, and several
-branches for the same project.  See the "loggerhead.conf.example" file
-included with the source.
-
-A debug and access log are stored in the logs/ folder, relative to
-the location of the start-loggerhead script.
+To hide branches from being displayed, add to ``~/.bazaar/locations.conf``,
+under the branch's section:
+
+    [/path/to/branch]
+    http_serve = False
+
+
+More configuration options to come soon.
 
 
 SERVING LOGGERHEAD FROM BEHIND APACHE
@@ -107,13 +90,14 @@
 run behind a proxy at the root of a site, but if you're running it at
 some path into the site, you'll need to specify is using '--prefix=/some_path'.
 
-FILES CHANGED CACHE
--------------------
-
-To speed up the display of the changelog view for large trees,
-loggerhead can be configured to cache the files changes between
-revisions.  Set the 'cachepath' value in the config file.
-
+
+SEARCH
+------
+
+Search is currently supported by using the bzr-search plugin (available
+at: ``https://launchpad.net/bzr-search``
+You need to have the plugin installed and each branch indexed to allow
+searching on branches.
 
 SUPPORT
 -------
 
=== modified file 'loggerhead/apps/branch.py'
--- loggerhead/apps/branch.py	2009-05-13 11:34:09 +0000
+++ loggerhead/apps/branch.py	2009-05-27 09:55:41 +0000
@@ -1,3 +1,19 @@
+# Copyright (C) 2008, 2009  Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
 """The WSGI application for serving a Bazaar branch."""
 
 import logging
@@ -113,6 +129,10 @@
         return self.branch.get_config().get_user_option('public_branch')
 
     def app(self, environ, start_response):
+        # Check again if the branch is blocked from being served, this is
+        # mostly for tests. It's already checked in apps/transport.py
+        if self.branch.get_config().get_user_option('http_serve') == 'False':
+            raise httpexceptions.HTTPNotFound()
         self._url_base = environ['SCRIPT_NAME']
         self._static_url_base = environ.get('loggerhead.static.url')
         if self._static_url_base is None:
 
=== modified file 'loggerhead/apps/transport.py'
--- loggerhead/apps/transport.py	2009-05-04 20:28:50 +0000
+++ loggerhead/apps/transport.py	2009-05-27 09:55:41 +0000
@@ -1,6 +1,23 @@
+# Copyright (C) 2008, 2009  Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
 """Serve branches at urls that mimic a transport's file system layout."""
 
 from bzrlib import branch, errors, lru_cache, urlutils
+from bzrlib.bzrdir import BzrDir
 
 from paste.request import path_info_pop
 from paste import httpexceptions
@@ -63,7 +80,10 @@
                 raise httpexceptions.HTTPNotFound()
             return self.app_for_non_branch(environ)(environ, start_response)
         else:
-            return self.app_for_branch(b)(environ, start_response)
+            if b.get_config().get_user_option('http_serve') == 'False':
+                raise httpexceptions.HTTPNotFound()
+            else:
+                return self.app_for_branch(b)(environ, start_response)
 
 
 class BranchesFromTransportRoot(object):
@@ -82,6 +102,14 @@
         elif environ['PATH_INFO'] == '/favicon.ico':
             return favicon_app(environ, start_response)
         elif '/.bzr/' in environ['PATH_INFO']:
+            try:
+                bzrdir = BzrDir.open_containing_from_transport(
+                         self.transport.clone(environ['PATH_INFO']))
+                branch = bzrdir.open_branch()
+                if branch.get_config().get_user_option('http_serve') == 'False':
+                    raise httpexceptions.HTTPNotFound()
+            except errors.NotBranchError:
+                pass
             app = urlparser.make_static(None, self.transport)
             return app(environ, start_response)
         else:
 
=== modified file 'loggerhead/controllers/directory_ui.py'
--- loggerhead/controllers/directory_ui.py	2009-05-05 18:37:26 +0000
+++ loggerhead/controllers/directory_ui.py	2009-05-26 16:21:53 +0000
@@ -72,6 +72,8 @@
         for d in listing:
             try:
                 b = branch.Branch.open_from_transport(self.transport.clone(d))
+            	if b.get_config().get_user_option('http_serve') == 'False':
+                    continue
             except:
                 if not stat.S_ISDIR(self.transport.stat(d).st_mode):
                     continue
 
=== modified file 'loggerhead/tests/test_simple.py'
--- loggerhead/tests/test_simple.py	2009-02-18 10:40:22 +0000
+++ loggerhead/tests/test_simple.py	2009-05-27 12:44:28 +0000
@@ -1,8 +1,26 @@
+# Copyright (C) 2008, 2009  Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
+
 import cgi
 import logging
 
 from bzrlib.tests import TestCaseWithTransport
 from bzrlib.util.configobj.configobj import ConfigObj
+from bzrlib import config
 
 from loggerhead.apps.branch import BranchWSGIApp
 from paste.fixture import TestApp
@@ -103,6 +121,7 @@
 
 
 class TestEmptyBranch(BasicTests):
+    """ Test that an empty branch doesn't break"""
 
     def setUp(self):
         BasicTests.setUp(self)
@@ -118,3 +137,22 @@
         res = app.get('/files')
         res.mustcontain('No revisions!')
 
+
+class TestHiddenBranch(BasicTests):
+    """
+    Test that hidden branches aren't shown
+    FIXME: not tested that it doesn't show up on listings
+    """
+
+    def setUp(self):
+        BasicTests.setUp(self)
+        self.createBranch()
+        locations = config.locations_config_filename()
+        config.ensure_config_dir_exists()
+        open(locations, 'wb').write('[%s]\nhttp_serve = False'
+                                    % (self.tree.branch.base,))
+
+    def test_no_access(self):
+        app = self.setUpLoggerhead()
+        res = app.get('/changes', status=404)
+