Repository: gulpjs/vinyl-fs
Pull request #162 (2016-04-20 20:02:45)

Support setting setuid/setgid/sticky in updateMetadata and adapt tests
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!!

- [x] write tests to set special bits for files
- [x] 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](https://ci.appveyor.com/project/gulpjs/vinyl-fs/build/137). Seems like the mobile version of GH doesn't like 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 `getOwner` 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, reverting my commit and going to push. Thanks again!
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). Go ahead and check off those tasks if it is satisfactory. Let men know if I need to change anything.
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](https://github.com/substack/node-mkdirp/blob/master/index.js#L20).

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:

```sh
$ 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?
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.

Patch:
diff --git a/lib/fileOperations.js b/lib/fileOperations.js
index e323fb7..cec9efb 100644
--- a/lib/fileOperations.js
+++ b/lib/fileOperations.js
@@ -6,8 +6,7 @@ var isEqual = require('lodash.isequal');
 var isValidDate = require('vali-date');
 
 // TODO shared module
-// TODO include sticky/setuid/setgid, i.e. 7777?
-var MASK_MODE = parseInt('0777', 8);
+var MASK_MODE = parseInt('7777', 8);
 var DEFAULT_FILE_MODE = parseInt('0666', 8);
 var APPEND_MODE_REGEXP = /a/;
 


Test patch:
diff --git a/test/destModes.js b/test/destModes.js
index 84a0525..edefae1 100644
--- a/test/destModes.js
+++ b/test/destModes.js
@@ -23,13 +23,14 @@ function wipeOut() {
     });
 }
 
-var MASK_MODE = parseInt('777', 8);
+var MASK_MODE = parseInt('7777', 8);
 
 function masked(mode) {
   return mode & MASK_MODE;
 }
 
 var isWindows = (os.platform() === 'win32');
+var isDarwin = (os.platform() === 'darwin');
 
 describe('.dest() with custom modes', function() {
   beforeEach(wipeOut);
@@ -70,6 +71,46 @@ describe('.dest() with custom modes', function() {
     stream.end();
   });
 
+
+
+  it('should set the sticky bit on the mode of a written stream file if set on the vinyl object', function(done) {
+    if (isWindows) {
+      this.skip();
+      return;
+    }
+
+    var inputPath = path.join(__dirname, './fixtures/test.coffee');
+    var inputBase = path.join(__dirname, './fixtures/');
+    var expectedPath = path.join(__dirname, './out-fixtures/test.coffee');
+    var expectedContents = fs.readFileSync(inputPath);
+    var expectedMode = parseInt('1655', 8);
+
+    var contentStream = through.obj();
+    var expectedFile = new File({
+      base: inputBase,
+      cwd: __dirname,
+      path: inputPath,
+      contents: contentStream,
+      stat: {
+        mode: expectedMode,
+      },
+    });
+
+    var onEnd = function() {
+      expect(masked(fs.lstatSync(expectedPath).mode)).toEqual(expectedMode);
+      done();
+    };
+
+    var stream = vfs.dest('./out-fixtures/', { cwd: __dirname });
+    stream.on('end', onEnd);
+    stream.write(expectedFile);
+    setTimeout(function() {
+      contentStream.write(expectedContents);
+      contentStream.end();
+    }, 100);
+    stream.end();
+  });
+
   it('should set the mode of a written stream file if set on the vinyl object', function(done) {
     if (isWindows) {
       this.skip();
@@ -143,6 +184,41 @@ describe('.dest() with custom modes', function() {
     stream.end();
   });
 
+  it('should set sticky bit on the mode of a written directory if set on the vinyl object', function(done) {
+    if (isWindows) {
+      this.skip();
+      return;
+    }
+
+    var inputPath = path.join(__dirname, './fixtures/test');
+    var inputBase = path.join(__dirname, './fixtures/');
+    var expectedPath = path.join(__dirname, './out-fixtures/test');
+    var expectedMode = parseInt('1655', 8);
+
+    var expectedFile = new File({
+      base: inputBase,
+      cwd: __dirname,
+      path: inputPath,
+      contents: null,
+      stat: {
+        isDirectory: function() {
+          return true;
+        },
+        mode: expectedMode,
+      },
+    });
+
+    var onEnd = function() {
+      expect(masked(fs.lstatSync(expectedPath).mode)).toEqual(expectedMode);
+      done();
+    };
+
+    var stream = vfs.dest('./out-fixtures/', { cwd: __dirname });
+    stream.on('end', onEnd);
+    stream.write(expectedFile);
+    stream.end();
+  });
+
   it('should write new files with the mode specified in options', function(done) {
     if (isWindows) {
       this.skip();
@@ -255,11 +331,13 @@ describe('.dest() with custom modes', function() {
       return;
     }
 
+
     var inputBase = path.join(__dirname, './fixtures');
     var inputPath = path.join(__dirname, './fixtures/wow/suchempty');
     var expectedBase = path.join(__dirname, './out-fixtures/wow');
     var expectedPath = path.join(__dirname, './out-fixtures/wow/suchempty');
-    var expectedDirMode = parseInt('755', 8);
+    // NOTE: Darwin does not set setgid
+    var expectedDirMode = isDarwin ? parseInt('755', 8) : parseInt('2755', 8);
     var expectedFileMode = parseInt('655', 8);
 
     var firstFile = new File({
@@ -322,7 +400,7 @@ describe('.dest() with custom modes', function() {
     stream.end();
   });
 
-  it('should see a file with special chmod (setuid/setgid/sticky) as matching', function(done) {
+  it('should see a file with special chmod (setuid/setgid/sticky) as distinct', function(done) {
     if (isWindows) {
       this.skip();
       return;
@@ -349,7 +427,7 @@ describe('.dest() with custom modes', function() {
     });
 
     var onEnd = function() {
-      expect(fchmodSpy.calls.length).toEqual(0);
+      expect(fchmodSpy.calls.length).toEqual(1);
       done();
     };
 
diff --git a/test/fileOperations.js b/test/fileOperations.js
index c5ccaac..568263c 100644
--- a/test/fileOperations.js
+++ b/test/fileOperations.js
@@ -21,7 +21,7 @@ var updateMetadata = fo.updateMetadata;
 
 var resolution = defaultResolution();
 
-var MASK_MODE = parseInt('777', 8);
+var MASK_MODE = parseInt('7777', 8);
 
 function masked(mode) {
   return mode & MASK_MODE;
@@ -170,13 +170,13 @@ describe('getModeDiff', function() {
     done();
   });
 
-  it('ignores the sticky/setuid/setgid bits', function(done) {
+  it('includes the sticky/setuid/setgid bits', function(done) {
     var fsMode = parseInt('1777', 8);
     var vfsMode = parseInt('4777', 8);
 
     var result = getModeDiff(fsMode, vfsMode);
 
-    expect(result).toEqual(0);
+    expect(result).toEqual(fsMode ^ vfsMode);
 
     done();
   });
@@ -795,6 +795,29 @@ describe('updateMetadata', function() {
     });
   });
 
+
+  it('updates the sticky bit on mode on fs and vinyl object if there is a diff', function(done) {
+    if (isWindows) {
+      this.skip();
+      return;
+    }
+
+    var fchmodSpy = expect.spyOn(fs, 'fchmod').andCallThrough();
+
+    var mode = parseInt('1777', 8);
+    file.stat.mode = mode;
+
+    var fd = fs.openSync(inputPath, 'w+');
+
+    updateMetadata(fd, file, function(err, fd2) {
+      expect(fchmodSpy.calls.length).toEqual(1);
+      var stats = fs.fstatSync(fd);
+      expect(file.stat.mode).toEqual(stats.mode);
+
+      fs.close(fd2, done);
+    });
+  });
+
   it('forwards fchmod error and descriptor upon error', function(done) {
     if (isWindows) {
       this.skip();