Support setting setuid/setgid/sticky in updateMetadata and adapt tests

[tmcgee123]

Ref #156

Let me know if you need me to get rid of the merge commit, I can open a new branch and apply the changes there if need be.

Thanks everyone for the help!!

• write tests to set special bits for files
• write tests to set special bits for directories
• fix test discrepancy of file mode on CI vs. local

[phated]

@erikkemperman wouldn't there need to be tests added for those actually getting set on the files created?

[phated]

This is failing on appveyor (windows)

[erikkemperman]

Yes, maybe I should've mentioned that in the pr to @tmcgee123 's fork -- it was just quicker to fix the extant tests that now failed than describing it in words.

I don't have a Windows box, and am not familiar with appveyor, can we see those errors online somewhere?

[tmcgee123]

If you post which tests are failing I could run these on a Windows box (I have Windows 7 and 10) and debug them. I can't seem to find the stacktrace or error log either on AppVeyor 😿

[erikkemperman]

@tmcgee123 I found them, here. Seems like the mobile version of GH doesn't link these clearly.

getModeDiff now always returns 0 on Windows, which it didn't before, and this now fails since this is a general purpose function which is tested even on Windows.

Looking at your patch more closely (I just checked the failing tests yesterday) I am wondering why it would need to be anything more than simply changing the mask to 7777? The isOwner call will still return false on Windows so it won't attempt to chmod, with or without setuid/setgid/sticky bits.

Am I missing something obvious?

[tmcgee123]

@erikkemperman I found the broken tests, sorry I didn't know I needed to click on the node environments it was running in (derp).

In regards to the other changes than the mask, I made them because of the inner functionality of updateMetaData (the inner onStat):

I was trying to return correctly here, since Windows doesn't have file modes.

   if (!modeDiff && !timesDiff) {
      return callback(null, fd);
    }

I was also concerned that if for some reason if the timesDiff was true and made it to this point, that mode would be invoked and didn't know if that would effect anything else.

    if (modeDiff) {
      return mode();
    }

[erikkemperman]

I think mode() will never be invoked if isOwner() returns false, which it does on Windows. So that would be all right, it seems to me, without any further guards.

[tmcgee123]

Gotcha, reverted my commits and squashed everything, hopefully the build passes this time!

[tmcgee123]

After looking back at the code, if what you are saying is true can we combine the conditionals? For example:

    if (!modeDiff && !timesDiff) {
      return callback(null, fd);
    }

    // Check access, `futimes` and `fchmod` only work if we own the file,
    // or if we are effectively root.
    if (!isOwner(stat)) {
      return callback(null, fd);
    }

to this:

    /* Nothing to do.
       Check access, `futimes` and `fchmod` only work if we own the file,
       or if we are effectively root.
    */ 
    if ((!modeDiff && !timesDiff) || !isOwner(stat)) {
      return callback(null, fd);
    }

[erikkemperman]

Yay, getting there! All tests pass!

Now, as @phated mentioned, tests ought to be added that actually set these special bits, both for files and directories...

[erikkemperman]

That would be functionally identical but, imho, less legible.

[tmcgee123]

@erikkemperman that makes sense. I've went ahead and added tests to the destModes to cover the areas you've said (I went ahead an added on to fileOperations test as well).

So, there's something interesting happening with the tests that I need help with. On the CI, the tests pass. But locally (Mac OS X 10.11.1) the test fails with. Quite strange actually, if you look at the failed build: https://travis-ci.org/gulpjs/vinyl-fs/builds/124775406 and then compare to what you are seeing locally with the update test, their results are flopped.

Sorry to take so much time with this small change, but I am very glad this repo enforces TDD standards. Thanks for the help!

[erikkemperman]

@tmcgee123 Sorry, I don't have the time for this that it deserves... Still buried in a project at work.

But just a guess: OS-specific differences could be due to mkdirp (which is used to create directories) using process.umask, see here.

Not sure if we should adapt the tests and/or fileOperations.js itself to account for this, though, if that is in fact the issue here.

[phated]

It looks like both platforms are passing with that last commit. I'm not sure when I'll have a chance to review. Hopefully tomorrow but it might be next week. Thanks for the work on this.

[erikkemperman]

Yes, but now it fails npm test on macosx :-(

[tmcgee123]

I can look into this more today and tomorrow. I was able to figure out the "Linux" file mode by using http://permissions-calculator.org/decode/ which is why the test is passing on CI now and not locally. I think @erikkemperman is on the right track with the umask.

In terms of accounting for this in fileOperations.js, if the modes that are being set are invalid then I would say we should definitely account for this. Either way, I think it would be smart to have a unit test for this either way to show that directories get assigned a different file mode for Mac vs Linux.

[tmcgee123]

@erikkemperman do you think this could be the cause of what we are seeing? https://github.com/substack/node-mkdirp/issues/80

[erikkemperman]

Could be, it seems that issue is about new directories vs calling mkdirp on existing directories. It looks like @contra would like mkdirp to just do a chmod in the latter case?

Just had a casual glance at it though, I might be way off... Busy busy busy, sorry about that.

[phated]

It looks like we don't handle that correctly anyway? Maybe we should write a test for it and skip it with a comment saying that it doesn't work for directories that already exist and point at that issue? I haven't run this locally to test yet.

[tmcgee123]

I wrote a simple catch like there was for Windows to see if it was Darwin. Let me know if this is acceptable for the time being or if you'd like me to add anything else.

Thanks!

[erikkemperman]

I am now thinking it's not even an OS issue, per se. They might have different defaults for umask but users or admins can override it.

So my feeling now is that vinyl-fs should account for it much like mkdirp does. That would mean a more fundamental change than the scope of this issue/pr though...

https://en.m.wikipedia.org/wiki/Umask

About the issue in the mkdirp repo, contra's suggestion makes sense to me. There's been no response though, maybe it would help if there was a concrete PR for it.

[phated]

@erikkemperman the node.js platform's builtin methods already account for umask, so I don't think we need to.

[erikkemperman]

@phated Yes but the tests seem to be assuming the umask to be 022 (or a submask of that like 002), which is apparently the usual default but users/admins might have changed that. See the literals 755 and 655 in the mode tests, which conveniently pre-exclude the default umask.

But if the umask includes any of the other bits, the tests will fail:

$ umask
0022
$ npm test
[pass]
$ umask 0033
$ umask
0033
$ npm test
[FAIL]
$ umask 0022 # don't forget to restore to whatever the first one returned

[erikkemperman]

And also, if the effective umask restricts what folks pass into dest when writing to disk, should the vinyl object not reflect this?

EDIT Sorry, never mind, the above is already handled correctly since updateMetadata always updates file.stat to reflect the attributes on disk. (Should've known, I think I introduced this myself at one point during the earlier metadata efforts :-))

The question about literals in the tests stands, though.

[phated]

@erikkemperman I think process.umask and some weirdness around mkdir modes on OSX are out of scope for this PR/issue combination. I'll open some other issues.

[phated]

I'm happy where this is at. Merging.

[phated]

@tmcgee123 thanks for this!

[tmcgee123]

@phated glad to help! Sorry for the delayed response, work has been busy the last week. Hopefully I'll have time to contribute some more, cheers! 🍻