test: enable setuid/setgid test #12403

Trott · 2017-04-13T19:49:32Z

Refactor test to be skipped in situations where it is expected to fail.
Move from disabled directory to parallel.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test process

Trott · 2017-04-13T19:50:08Z

Will be interested to see how this goes on CI....

CI: https://ci.nodejs.org/job/node-test-pull-request/7382/

Trott · 2017-04-13T20:50:57Z

Fixed up a bit. Once again, CI: https://ci.nodejs.org/job/node-test-pull-request/7385/

Trott · 2017-04-13T23:15:44Z

Fixed it up so it isn't skipped anymore.

Would be nice if there was a way to specify tests that need superuser, but I guess we shouldn't expect devs to have local superuser necessarily. (Would be nice for CI, though!)

New CI: https://ci.nodejs.org/job/node-test-pull-request/7387/

Trott · 2017-04-13T23:34:09Z

Fixed up more to be forgiving if nobody is not a valid group id.

CI: https://ci.nodejs.org/job/node-test-pull-request/7388/

Trott · 2017-04-13T23:56:24Z

SmartOS is letting me know that different OS's can have somewhat different error messages. Made the check a little more lenient.

CI: https://ci.nodejs.org/job/node-test-pull-request/7389/

cjihrig

LGTM once the CI is straightened out.

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel.

Trott · 2017-04-14T03:33:02Z

Yet more lenient.

CI: https://ci.nodejs.org/job/node-test-pull-request/7391/

Trott · 2017-04-14T05:46:06Z

Single ARM failure is infrastructure-related. (That instance that failed doesn't even run this test.) CI is effectively green.

thefourtheye · 2017-04-15T11:08:36Z

test/parallel/test-process-setuid-setgid.js

+
+  assert.throws(
+    () => { process.setgid('nobody'); },
+    /^Error: (EPERM, .+|setgid group id does not exist)$/


I am actually getting Error: EPERM, Operation not permitted for both the cases. Also, the capture group doesn't look correct to me.

I am actually getting Error: EPERM, Operation not permitted for both the cases.

@thefourtheye Error message depends on your operating system. SmartOS gives Error: EPERM, Not owner here.

The regexp allows any error that starts with Error: EPERM, and also allows Error: setgid group id does not exist for the situation where nobody is not included on the operating system (which happens on ubuntu1404-64 in FIPS mode on our CI).

TL;DR: Test passes if you have group nobody and get EPERM back along with some text from the OS, and it will also pass if group nobody doesn't exist at all.

Also, the capture group doesn't look correct to me.

@thefourtheye There's room for improvement. For one thing, it could be a non-capturing group. But the regexp seems to work to me. What situation are you concerned about?

> /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: setgid group id does not exist') true > /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: setgid group id does not exist adding some text so it will be false') false > /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: EPERM, setgid group id does not exist adding some text so it will be false except now it as EPERM at the start so it should be true again') true >

Oh I thought EPERM is always going to be there. As "setgid group id does not exist" is present in all the cases, can we make EPERM, optional and check for the presence of "setgid group id does not exist"?

The only thing that's there in all cases is Error: .

> /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: EPERM, arbitrary text.') true >

The regexp can be thought of as: String must start with Error: and then there are two options that will match after that. One is EPERM, followed by one or more characters of anything. The other is the string setgid group id does not exist precisely.

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Trott · 2017-04-16T20:44:12Z

Landed in 189afc8

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

gibfahn · 2017-05-15T20:52:32Z

@Trott should this be backported to v6.x?

Trott · 2017-05-16T03:07:46Z

@Trott should this be backported to v6.x?

@gibfahn I think so.

gibfahn · 2017-05-16T16:29:34Z

Didn't backport cleanly, could you raise a PR?

Trott · 2017-05-16T19:16:09Z

@gibfahn #13060

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Backport-PR-URL: nodejs#13060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Backport-PR-URL: #13060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Trott · 2017-07-18T06:28:27Z

#13060 landed. Removed backport-requested-v6.x and adding backported-to-v6.x.

Trott added process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. labels Apr 13, 2017

Trott force-pushed the fix-setuid-test branch from 8f74ea0 to 4653295 Compare April 13, 2017 20:41

jasnell approved these changes Apr 13, 2017

View reviewed changes

Trott changed the title ~~test: enable setuid/setgit test~~ test: enable setuid/setgid test Apr 13, 2017

Trott force-pushed the fix-setuid-test branch from 4653295 to c0baf52 Compare April 13, 2017 23:14

Trott force-pushed the fix-setuid-test branch from c0baf52 to 6db736f Compare April 13, 2017 23:33

Trott force-pushed the fix-setuid-test branch from 6db736f to 93a2ec9 Compare April 13, 2017 23:55

cjihrig approved these changes Apr 14, 2017

View reviewed changes

test: enable setuid/setgid test

7e46be1

Refactor test for situations where it was expected to fail. Move from disabled directory to parallel.

Trott force-pushed the fix-setuid-test branch from 93a2ec9 to 7e46be1 Compare April 14, 2017 03:31

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

hiroppy approved these changes Apr 14, 2017

View reviewed changes

thefourtheye reviewed Apr 15, 2017

View reviewed changes

thefourtheye approved these changes Apr 16, 2017

View reviewed changes

santigimeno approved these changes Apr 16, 2017

View reviewed changes

Trott closed this Apr 16, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the lts-watch-v6.x label May 15, 2017

gibfahn added the backport-requested-v6.x label May 16, 2017

Trott mentioned this pull request May 16, 2017

(v6.x backport) test: enable setuid/setgid test #13060

Closed

4 tasks

gibfahn mentioned this pull request May 17, 2017

parallel/test-process-setuid-setgid fails if "nobody" user does not exist #13071

Closed

Trott added backported-to-v6.x and removed backport-requested-v6.x labels Jul 18, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

Trott deleted the fix-setuid-test branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: enable setuid/setgid test #12403

test: enable setuid/setgid test #12403

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

cjihrig left a comment

Trott commented Apr 14, 2017

Trott commented Apr 14, 2017

thefourtheye Apr 15, 2017

Trott Apr 16, 2017

Trott Apr 16, 2017

thefourtheye Apr 16, 2017

Trott Apr 16, 2017

thefourtheye Apr 16, 2017

Trott commented Apr 16, 2017

gibfahn commented May 15, 2017

Trott commented May 16, 2017

gibfahn commented May 16, 2017

Trott commented May 16, 2017

Trott commented Jul 18, 2017

test: enable setuid/setgid test #12403

test: enable setuid/setgid test #12403

Conversation

Trott commented Apr 13, 2017

Checklist

Affected core subsystem(s)

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

Trott commented Apr 13, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Trott commented Apr 14, 2017

Trott commented Apr 14, 2017

thefourtheye Apr 15, 2017

Choose a reason for hiding this comment

Trott Apr 16, 2017

Choose a reason for hiding this comment

Trott Apr 16, 2017

Choose a reason for hiding this comment

thefourtheye Apr 16, 2017

Choose a reason for hiding this comment

Trott Apr 16, 2017

Choose a reason for hiding this comment

thefourtheye Apr 16, 2017

Choose a reason for hiding this comment

Trott commented Apr 16, 2017

gibfahn commented May 15, 2017

Trott commented May 16, 2017

gibfahn commented May 16, 2017

Trott commented May 16, 2017

Trott commented Jul 18, 2017