144285 - checksetup.pl fails to set data dir (and other dir) permissions properly

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Michael S. Fischer]

On a clean installation (tar xzvf) of 2.61rc1, I do the following (Debian woody):

# checksetup.pl
(localconfig created)
# vi localconfig
(edit bug db password, change $webservergroup to www-data)
# checksetup.pl 
(finish setup)

Now, when I access index.cgi, I get the following error:

Software error:
Can't create data/params.22949 at defparams.pl line 59.

Looking at the data directory, it's owned by root:root, mode 771.  This probably explains the problem.

Comment 1

[Bradley Baetz (:bbaetz)]

Doh. We need to fixPerms data, not just chmod it. jp: does this affect that
other patch, too?

Comment 2

[J. Paul Reed [:preed]]

No idea if this helps, but this is what I found:

When I tried to reproduce with 2.14.1 BRANCH code, checksetup chgrp()ed
everything in the directory, including data to the user I specified in
webservergroup, although it did NOT do so to all (any?) of the files *in* data/

But, justdave commented on IRC that the patch we're backporting to the
2.14.1-BRANCH might have been what broke it... so...

Comment 3

[Bradley Baetz (:bbaetz)]

2.14 didn't change naything inside data, IIRC.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: v1

fixPerms 'data' and 'graphs'

Comment 5

[Bradley Baetz (:bbaetz)]

-> me

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 84155 [details] [diff] [review]
v1

r= justdave

Comment 7

[Michael S. Fischer]

Attachment: v2

The previous patch didn't change the permissions of the data directory itself;
only the contents.  This patch fixes that.

Comment 8

[Bradley Baetz (:bbaetz)]

Err. That patch is then recursive in the case where we do have a webservergroup.
We don't want to change the contents of the data directory, do we?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: v3

I don't think we want to change permissions/ownership on the contents of the
directory, at least not at this point (we may want to reconsider this for
2.17), but we do want to change the ownership of the directory itself.	The
problem is that fixPerms doesn't do that, but the fix is simple.  Here's a
combination of bbaetz' patch (v1) and a fix to make fixPerms always change the
permissions of the file(s) it gets handed whether they are actual files or
directories.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: v4

Same as v3 but with a redundant chown removed.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 84155 [details] [diff] [review]
v1

This is the right fix, although incomplete since a bug in fixPerms prevents it
from working right.  r=myk on this part of the fix.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 84821 [details] [diff] [review]
v4

r= justdave

looks good to me.  What was the bug in fixPerms?  Looks like it was set to only
chown a directory if it was going to be recursed (which we weren't doing with
data).	You fixed that, by the look of it, so it always chowns a directory
whether we're planning to recurse it or not.

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 84821 [details] [diff] [review]
v4

Theres the comment:

# do not change permissions on directories here unless $do_dirs is set

which you seem to be ignoring, now. Any idea why the comment was added? I don't
have a problem with changing it (in which case you should remove the comment,
too), but someone must have had a reason at some point...

Comment 14

[Matthew Tuck [:CodeMachine]]

What is do_dirs supposed to do?

Comment 15

[Dave Miller [:justdave]]

$do_dirs tells it to recurse a directory.  If directories are found inside the
list of files matched, it recurses them and does everything inside them, too.

I think the intent was that if you did fixPerms(glob "*") or whatever that it
wouldn't change permissions on directories that were found inside the loop.
(Which is correctly what it should be doing so we don't muck with the CVS
directory for example).

So what actually needs to happen here is it needs to keep track of whether it
was called only with that directory or if the directory was obtained as part of
a glob.  If it was referenced explicitly, then change permissions on it, if it
was part of a glob, ignore it.

Comment 16

[Matthew Tuck [:CodeMachine]]

OK, well, with this patch, it appers to chown non recursed directories, but not
chmod them ...  I don't see why this would be the case - you might need an extra
branch of the if.

If you're going to change the behaviour of do_dirs to do the dir but not recurse
it you should check all call sites.  But personally, I'm not sure why we would
want to change the behaviour to make it possible to not recurse.  Or even why we
need to have an option at all.

CVS shouldn't be a problem, it is already handled especially in that code.

Also, why doesn't data have do_dirs, in either case?

Comment 17

[Bradley Baetz (:bbaetz)]

data shouldn't have do-dirs because we set the permissions of stuff under there
specially.

Comment 18

[Matthew Tuck [:CodeMachine]]

Then please rename do_dirs to recurse.

Comment 19

[Bradley Baetz (:bbaetz)]

Bleh.

Maybe we should just chown data directly, rather than fuss with all this at this
stage?

Comment 20

[Dave Miller [:justdave]]

Sounds like a good plan to me, Brad.

Comment 21

[Bradley Baetz (:bbaetz)]

Attachment: v5

OK, take 5!

Comment 22

[Dave Miller [:justdave]]

Comment on attachment 86000 [details] [diff] [review]
v5

r= justdave

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 86000 [details] [diff] [review]
v5

r=myk

Moving right along.

Comment 24

[Bradley Baetz (:bbaetz)]

Checked in, trunk + branch:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.154; previous revision: 1.153
done

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.5; previous revision: 1.149.2.4
done