loggerhead

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

  1. serve-config
  2. Merge into trunk-rich
Proposed by Martin Albisetti
Status: Merged
Merged at revision: not available
Proposed branch: lp:~beuno/loggerhead/serve-config
Merge into: lp:loggerhead
Diff against target: None lines
To merge this branch: bzr merge lp:~beuno/loggerhead/serve-config
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Matt Nordhoff Needs Fixing
Review via email: mp+6821@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Albisetti (beuno) wrote :

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

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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

Revision history for this message
Adrian Wilkins (adrian-wilkins) wrote :

> 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.

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

> > 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. :)

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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 updated
361. By Martin Albisetti

Fixed whitespace and other trivial bits from the review

Revision history for this message
Martin Albisetti (beuno) wrote :

> * 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.

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

> 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'".

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

> 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 updated
362. By Martin Albisetti

Merge from trunk and fix conflict by refactoring the code

Revision history for this message
Martin Albisetti (beuno) wrote :

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

Revision history for this message
Martin Albisetti (beuno) 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?

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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':

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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/

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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 updated
363. By Martin Albisetti

Merge in Peng's tab fixes

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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
Revision history for this message
Martin Albisetti (beuno) wrote :

 > 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 updated
364. By Martin Albisetti

Merge from trunk

365. By Martin Albisetti

Add deprecation warning to start-loggerhead

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-05-17 11:10:38 +0000
3+++ NEWS 2009-05-27 12:51:51 +0000
4@@ -115,6 +115,10 @@
5
6 - Fix 'bzr serve --http' errors. (Matt Nordhoff, #377551)
7
8+ - Added the option to hide branches by setting http_serve = False
9+ in locations.conf (Martin Albisetti)
10+
11+
12 1.10 [22Dec2008]
13 ---------------
14
15
16=== renamed file 'README.txt' => 'README'
17--- README.txt 2009-03-17 22:31:42 +0000
18+++ README 2009-05-27 12:52:49 +0000
19@@ -1,7 +1,7 @@
20 LOGGERHEAD
21 ==========
22
23-[ Version 1.6 for Bazaar 1.6 ]
24+[ Version 1.10 for Bazaar 1.6 ]
25
26 Loggerhead is a web viewer for Bazaar branches. It can be used to
27 navigate a branch history, annotate files, perform searches... all the
28@@ -63,31 +63,14 @@
29 USING A CONFIG FILE
30 -------------------
31
32-Previous versions of Loggerhead read their configuration from a config
33-file. This mode of operation is still supported by the
34-'start-loggerhead' script. A 'loggerhead.conf.example' file is
35-included in the source which has comments explaining the various
36-options.
37-
38-Loggerhead can then be started by running::
39-
40- $ ./start-loggerhead
41-
42-This will run loggerhead in the background, listening on port 8080 by
43-default.
44-
45-To stop Loggerhead, run::
46-
47- $ ./stop-loggerhead
48-
49-In the configuration file you can configure projects, and branches per
50-project. The idea is that you could be publishing several (possibly
51-unrelated) projects through the same loggerhead instance, and several
52-branches for the same project. See the "loggerhead.conf.example" file
53-included with the source.
54-
55-A debug and access log are stored in the logs/ folder, relative to
56-the location of the start-loggerhead script.
57+To hide branches from being displayed, add to ``~/.bazaar/locations.conf``,
58+under the branch's section:
59+
60+ [/path/to/branch]
61+ http_serve = False
62+
63+
64+More configuration options to come soon.
65
66
67 SERVING LOGGERHEAD FROM BEHIND APACHE
68@@ -107,13 +90,14 @@
69 run behind a proxy at the root of a site, but if you're running it at
70 some path into the site, you'll need to specify is using '--prefix=/some_path'.
71
72-FILES CHANGED CACHE
73--------------------
74-
75-To speed up the display of the changelog view for large trees,
76-loggerhead can be configured to cache the files changes between
77-revisions. Set the 'cachepath' value in the config file.
78-
79+
80+SEARCH
81+------
82+
83+Search is currently supported by using the bzr-search plugin (available
84+at: ``https://launchpad.net/bzr-search``
85+You need to have the plugin installed and each branch indexed to allow
86+searching on branches.
87
88 SUPPORT
89 -------
90
91=== modified file 'loggerhead/apps/branch.py'
92--- loggerhead/apps/branch.py 2009-05-13 11:34:09 +0000
93+++ loggerhead/apps/branch.py 2009-05-27 09:55:41 +0000
94@@ -1,3 +1,19 @@
95+# Copyright (C) 2008, 2009 Canonical Ltd.
96+#
97+# This program is free software; you can redistribute it and/or modify
98+# it under the terms of the GNU General Public License as published by
99+# the Free Software Foundation; either version 2 of the License, or
100+# (at your option) any later version.
101+#
102+# This program is distributed in the hope that it will be useful,
103+# but WITHOUT ANY WARRANTY; without even the implied warranty of
104+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
105+# GNU General Public License for more details.
106+#
107+# You should have received a copy of the GNU General Public License
108+# along with this program; if not, write to the Free Software
109+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
110+#
111 """The WSGI application for serving a Bazaar branch."""
112
113 import logging
114@@ -113,6 +129,10 @@
115 return self.branch.get_config().get_user_option('public_branch')
116
117 def app(self, environ, start_response):
118+ # Check again if the branch is blocked from being served, this is
119+ # mostly for tests. It's already checked in apps/transport.py
120+ if self.branch.get_config().get_user_option('http_serve') == 'False':
121+ raise httpexceptions.HTTPNotFound()
122 self._url_base = environ['SCRIPT_NAME']
123 self._static_url_base = environ.get('loggerhead.static.url')
124 if self._static_url_base is None:
125
126=== modified file 'loggerhead/apps/transport.py'
127--- loggerhead/apps/transport.py 2009-05-04 20:28:50 +0000
128+++ loggerhead/apps/transport.py 2009-05-27 09:55:41 +0000
129@@ -1,6 +1,23 @@
130+# Copyright (C) 2008, 2009 Canonical Ltd.
131+#
132+# This program is free software; you can redistribute it and/or modify
133+# it under the terms of the GNU General Public License as published by
134+# the Free Software Foundation; either version 2 of the License, or
135+# (at your option) any later version.
136+#
137+# This program is distributed in the hope that it will be useful,
138+# but WITHOUT ANY WARRANTY; without even the implied warranty of
139+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
140+# GNU General Public License for more details.
141+#
142+# You should have received a copy of the GNU General Public License
143+# along with this program; if not, write to the Free Software
144+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
145+#
146 """Serve branches at urls that mimic a transport's file system layout."""
147
148 from bzrlib import branch, errors, lru_cache, urlutils
149+from bzrlib.bzrdir import BzrDir
150
151 from paste.request import path_info_pop
152 from paste import httpexceptions
153@@ -63,7 +80,10 @@
154 raise httpexceptions.HTTPNotFound()
155 return self.app_for_non_branch(environ)(environ, start_response)
156 else:
157- return self.app_for_branch(b)(environ, start_response)
158+ if b.get_config().get_user_option('http_serve') == 'False':
159+ raise httpexceptions.HTTPNotFound()
160+ else:
161+ return self.app_for_branch(b)(environ, start_response)
162
163
164 class BranchesFromTransportRoot(object):
165@@ -82,6 +102,14 @@
166 elif environ['PATH_INFO'] == '/favicon.ico':
167 return favicon_app(environ, start_response)
168 elif '/.bzr/' in environ['PATH_INFO']:
169+ try:
170+ bzrdir = BzrDir.open_containing_from_transport(
171+ self.transport.clone(environ['PATH_INFO']))
172+ branch = bzrdir.open_branch()
173+ if branch.get_config().get_user_option('http_serve') == 'False':
174+ raise httpexceptions.HTTPNotFound()
175+ except errors.NotBranchError:
176+ pass
177 app = urlparser.make_static(None, self.transport)
178 return app(environ, start_response)
179 else:
180
181=== modified file 'loggerhead/controllers/directory_ui.py'
182--- loggerhead/controllers/directory_ui.py 2009-05-05 18:37:26 +0000
183+++ loggerhead/controllers/directory_ui.py 2009-05-26 16:21:53 +0000
184@@ -72,6 +72,8 @@
185 for d in listing:
186 try:
187 b = branch.Branch.open_from_transport(self.transport.clone(d))
188+ if b.get_config().get_user_option('http_serve') == 'False':
189+ continue
190 except:
191 if not stat.S_ISDIR(self.transport.stat(d).st_mode):
192 continue
193
194=== modified file 'loggerhead/tests/test_simple.py'
195--- loggerhead/tests/test_simple.py 2009-02-18 10:40:22 +0000
196+++ loggerhead/tests/test_simple.py 2009-05-27 12:44:28 +0000
197@@ -1,8 +1,26 @@
198+# Copyright (C) 2008, 2009 Canonical Ltd.
199+#
200+# This program is free software; you can redistribute it and/or modify
201+# it under the terms of the GNU General Public License as published by
202+# the Free Software Foundation; either version 2 of the License, or
203+# (at your option) any later version.
204+#
205+# This program is distributed in the hope that it will be useful,
206+# but WITHOUT ANY WARRANTY; without even the implied warranty of
207+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
208+# GNU General Public License for more details.
209+#
210+# You should have received a copy of the GNU General Public License
211+# along with this program; if not, write to the Free Software
212+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
213+#
214+
215 import cgi
216 import logging
217
218 from bzrlib.tests import TestCaseWithTransport
219 from bzrlib.util.configobj.configobj import ConfigObj
220+from bzrlib import config
221
222 from loggerhead.apps.branch import BranchWSGIApp
223 from paste.fixture import TestApp
224@@ -103,6 +121,7 @@
225
226
227 class TestEmptyBranch(BasicTests):
228+ """ Test that an empty branch doesn't break"""
229
230 def setUp(self):
231 BasicTests.setUp(self)
232@@ -118,3 +137,22 @@
233 res = app.get('/files')
234 res.mustcontain('No revisions!')
235
236+
237+class TestHiddenBranch(BasicTests):
238+ """
239+ Test that hidden branches aren't shown
240+ FIXME: not tested that it doesn't show up on listings
241+ """
242+
243+ def setUp(self):
244+ BasicTests.setUp(self)
245+ self.createBranch()
246+ locations = config.locations_config_filename()
247+ config.ensure_config_dir_exists()
248+ open(locations, 'wb').write('[%s]\nhttp_serve = False'
249+ % (self.tree.branch.base,))
250+
251+ def test_no_access(self):
252+ app = self.setUpLoggerhead()
253+ res = app.get('/changes', status=404)
254+

Subscribers

People subscribed via source and target branches