Code review comment for lp:~mr-russ/bzr/add-doc-for-shared-ssh-server

Revision history for this message
Andrew Bennetts (spiv) wrote :

This looks basically good, thank you! I have some comments though:

In terms of markup, you should probably use ReST's “::” marker for preformatted blocks, like the example authorized_keys lines (although we should verify that it won't cause excessively wide pages; if it does there's probably some magic sphinx/ReST markup we can use to workaround that though…).

For me the most important part of this section about how to use a single user account on the server to host branches for multiple users, so it should be titled and introduced accordingly. A fair few users ask about how to set that up and it would be nice to make it easy to find. (The earlier sections have already described the case where the users already have their own accounts and home directories.) A title like “Using a restricted SSH account for multiple users and/or simpler paths” might be clearer, although it's certainly more verbose! (Thinking about this makes me think that the existing “Further Configuration” section probably needs retitling and/or rearranging too, but that's obviously out-of-scope for your patch.)

I think your example authorized_keys lines are missing the --inet option, which means they won't work. I'd in fact suggest using the contrib/bzr_ssh_path_limiter script as the command= in authorized_keys, because it will give a nicer error to users that attempt to use that SSH key for something other than bzr, and would avoid this particular problem.

If you don't have time to address these points soon let us know, I'd be happy to tackle them and get this landed in time for 2.4.

review: Needs Fixing

« Back to merge proposal