Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test for loading read-only modules #20138

Closed
wants to merge 2 commits into from
Closed

test: add test for loading read-only modules #20138

wants to merge 2 commits into from

Conversation

billti
Copy link
Contributor

@billti billti commented Apr 18, 2018

Adds a test-case to cover loading modules the user does not have permission
to write to.

Covers issue logged in #20112

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

CC @Trott @cjihrig @richardlau

Adds a test-case to cover loading modules the user does not have permission
to write to.
Covers issue logged in #20112
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 18, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI passes once the necessary libuv patch lands on master. (Will label this "blocked" until that happens.)

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Apr 18, 2018
@richardlau
Copy link
Member

Let's start a CI anyway and check that it passes the linter, actually fails on Windows as expected (prior to the libuv fix) and is skipped everywhere else: https://ci.nodejs.org/job/node-test-pull-request/14368/

We'll obviously need to rerun the CI when the libuv fix lands and verify that the test then passes.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some ignorable nits)

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

if (common.isWindows) {
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be more optimal to add this check at the beginning, something like:

const common = require('../common');

// TODO: Similar checks on *nix-like systems (e.g using chmod or the like)
if (!common.isWindows)
  common.skip('test only runs on Windows');

const assert = require('assert');
// ...

This way the test will not waste the time and resources loading all the other modules. common.skip() makes the script exit, so no else is required after it and we can save one indentation level as well.

// Create readOnlyMod.js and set to read only
const readOnlyMod = path.join(tmpdir.path, 'readOnlyMod');
const readOnlyModRelative = path.relative(__dirname, readOnlyMod);
const readOnlyModFullPath = readOnlyMod + '.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually prefer template literals in such cases, so maybe:

const readOnlyModFullPath = `${readOnlyMod}.js`;

but feel free to ignore this and next stylistic notes)

fs.writeFileSync(readOnlyModFullPath, 'module.exports = 42;');
// Removed any inherited ACEs, and any explicitly granted ACEs for the
// current user
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(
  `icacls.exe "${readOnlyModFullPath}" /inheritance:r /remove "%USERNAME%"`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did originally have template literals, but a) I wasn't sure they were supported all the way back to all LTS releases (I see now they are), and b) One long string was exceeded the lint line limit (but I could wrap it as shown above). I can add them back if desired.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I find template literals in these cases more clear, they have less syntactic noise (operators, quotes), but this can be my personal taste, so you can decide freely) We did not set strict linting rule for this, but we had many PRs that have replaced concatenations with templates, so templates may be prevalent style in tests now.

'" /inheritance:r /remove "%USERNAME%"');

// Grant the current user read & execute only
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(`icacls.exe "${readOnlyModFullPath}" /grant "%USERNAME%":RX`);

}

// Remove the expliclty granted rights, and reenable inheritance
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(
  `icacls.exe "${readOnlyModFullPath}" /remove "%USERNAME%" /inheritance:e`);

@Trott
Copy link
Member

Trott commented Apr 18, 2018

CI seems to have come back exactly as expected. 🎉 (Everything is green except Windows because we haven't yet fixed the bug that this test is written for.)

@vsemozhetbyt
Copy link
Contributor

It seems only Windows job failed in previous CI, so we can wait till #20129 is landed and run this variant then.


// Grant the current user read & execute only
cp.execSync(`icacls.exe "${readOnlyModFullPath}" /grant "%USERNAME%":RX`);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we simply end the test here with require(readOnlyModRelative);? Is the cleanup required?

Copy link
Contributor Author

@billti billti Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as the test must explicitly remove any write access to the file, I figure clean up could potentially hit issues if I don't restore the default permissions afterwards (per line 42). Once that is done, removing the file isn't strictly necessary (line 46), but I see no harm in it (per the comment note above it).

Copy link
Member

@lpinca lpinca Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nothing wrong with the tear down, I just think it's not needed as tmpdir.refresh() purges everything.

@vsemozhetbyt vsemozhetbyt added this to the 10.0.0 milestone Apr 19, 2018
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

CI is green. Landing...

vsemozhetbyt pushed a commit that referenced this pull request Apr 22, 2018
Adds a test-case to cover loading modules
the user does not have permission to write to.

Covers issue logged in #20112

PR-URL: #20138
Refs: #20112
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Landed in 3ba81e3
Thank you, @billti!

jasnell pushed a commit that referenced this pull request Apr 23, 2018
Adds a test-case to cover loading modules
the user does not have permission to write to.

Covers issue logged in #20112

PR-URL: #20138
Refs: #20112
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bzoz added a commit to JaneaSystems/libuv that referenced this pull request Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants