John Peacock <jpeacock@rowman.com> writes:
> http://subversion.tigris.org/issues/show_bug.cgi?id=1541
>
> This may seem like a minor issue, but this patch does the right thing
> the first time when creating a repository under *nix-like O/S's. It's
> a common complaint from new [to *nix] admins; the book still assumes
> that the administrator knows how permissions and ownership operate
> together.
>
> The patch includes both code and documentation. I thought it was
> appropriate to leave the patch attached to the issue, rather than
> attaching it here as well.
That's fine, yep. Review follows:
> Log
> [[[
> Implement two new options to the 'svnadmin create' command to explicitly
> create a new repository with rights appropriate for either
>
> 1) exclusive access by a single user/process
> 2) shared access by all members of an existing system group
>
> This is likely to only be relevant to Unix-like operating systems,
> where chown(), chmod(), setgid(), and setuid() are meaningful.
>
> * configure.in
> Add tests to check for existance of chown, chmod, setgid, and setuid
>
> * subversion/svnadmin/main.c
> (#include) - need svn_private_config.h in order to get autoconf #define's
>
> (enum),(options_table),(cmd_table): add owner_id, group_id to option tables
>
> (subcommand_create): Add logic to get system UID and GID and switch
> current process to those values before calling svn_repos_create()
> Create empty top level repository directory first and change ownership
>
> * doc/book/book/ch05.xml
> (svn-ch-5-sect-2): Add paragraph and link to discussion of --owner and
> --group options in svn-ch-5-sect-5.
>
> (svn-ch-5-sect-5): Document the use of --owner and --group to create
> the repository with specific rights for limited repository access.
> ]]]
This is only a minor nit, just mentioning it first since the log
message comes first: your log style is slightly different from most
other log messages. I rewrote the message like this:
Implement new --owner and --group options to 'svnadmin create', to
explicitly create a new repository with rights appropriate for either
1. exclusive access by a single user/process
2. shared access by all members of an existing system group
Patch from John Peacock <jpeacock@rowman.com>.
This is likely to only be relevant to Unix-like operating systems,
where chown(), chmod(), setgid(), and setuid() are meaningful.
* configure.in
Add tests to check for existence of chown, chmod, setgid, and setuid
* subversion/svnadmin/main.c: Include svn_private_config.h for
autoconf #defines.
(options_table): Add --owner and --group options.
(cmd_table): Describe behavior of new options for 'create'.
(subcommand_create): Add logic to get system UID and GID and switch
current process to those values before calling svn_repos_create().
Create empty top level repository directory first and change
ownership.
* doc/book/book/ch05.xml
(svn-ch-5-sect-2): Add paragraph and link to discussion of --owner
and --group options in svn-ch-5-sect-5.
(svn-ch-5-sect-5): Document the use of --owner and --group to create
the repository with specific rights for limited repository access.
On to the patch... Basically looks good to me, but I have a lot of
detail questions:
+ /* If the user requested the repository be created as a specific
+ * owner and/or group, we need to lookup the system UID/GID and
+ * switch the current process to that user and group before
+ * creating the repository. In this case, svnadmin must be run
+ * as root, in order to be able to switch to an arbitrary user
+ * and group. This feature does not work (currently) under Win32.
+ */
+ if (opt_state->owner_id &&
+#if !(HAVE_UMASK && HAVE_GETUID && HAVE_SETUID && HAVE_SETGID)
+ 0)
+#else
+ getuid() == 0)
+#endif
+ {
+ apr_uid_t retrieved_uid;
+ apr_gid_t retrieved_gid;
+ apr_status_t apr_err;
+ mode_t mode;
You put all of the code inside a conditional based on
opt_state->owner_id. If someone just wants gid, then won't this fail?
I.e., should it really be
+ if ((opt_state->owner_id || opt_state->group_id) &&
with appropriate adjustments in the code, or something like that?
Also, in this comment
+ mode = 0770; /* mode is roughly !umask */
did you really mean "~umask" not "!umask"? (A similar comment appears
later, btw.)
Next:
+ /* Need to pre-create the top level directory to ensure that
+ * even if the user/group would not ordinarily be able to
+ * write to the parent directory, the repository will be created
+ */
+
+ if (mkdir(opt_state->repository_path,mode) != 0)
+ return svn_error_create (SVN_ERR_ILLEGAL_TARGET, NULL,
+ "problem creating directory for repository (must not already exist)");
+
A few comments here:
Don't use raw 'mkdir()', use apr_mkdir() or some svn_io_* function.
But there's a larger question. How does svn_repos_create() behave if
the new directory already exists? Tracing it down to
create_repos_dir(), it appears that it is okay as long as the new
directory is empty. The doc string for svn_repos_create() should say
this, however. Making that doc fix as part of your change is fine,
since yours is the first code to depend explicitly on that aspect of
the API. The doc change should specify what error will be returned if
the directory is not empty.
Also, please keep lines under 80 columns -- even without all the extra
indentation of the patch format and my quoting, that line above
exceeds 80.
+ if (chown(opt_state->repository_path,retrieved_uid,retrieved_gid) != 0)
+ return svn_error_create (SVN_ERR_ILLEGAL_TARGET, NULL,
+ "problem setting owner/group for repository");
+ if (setgid(retrieved_gid) != 0)
+ return svn_error_create (SVN_ERR_INCORRECT_PARAMS, NULL,
+ "problem setting gid");
+ if (setuid(retrieved_uid) != 0)
+ return svn_error_createf (SVN_ERR_INCORRECT_PARAMS, NULL,
+ "problem setting uid");
Too bad we don't have APR functions for this, but whatever.
Does this always result in the correct ownership and permissions being
inherited by the subdirectories and files of the repository,
especially the db/ subdir? Not asking because I'm suspicious, just
ignorant. I assume you did testing and confirmed that it works as it
should. Will it do so consistently across all Unix-like platforms,
though? I'm not enough of an expert in this to say for sure.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 24 18:05:11 2003