Check for npmrc in home directory for linux root users #4047

kaylie-alexa · 2017-07-29T02:19:37Z

Summary
Fix for #3920.

In this commit, userHome for linux users running as root was changed to /usr/local/share, mainly to allow for other users to run globally added bins. However this introduced a bug where npmrc wasn't being looked in the root directory after checking /user/local/share. So this change pushes another location to check in case userHome is different from native home directory returned by os.homedir()

Test plan
I've added a test to generally test the getPossibleConfigLocations method, but I haven't been able to properly mock out running it as a root user on linux. Suggestions welcome!

BYK

LGTM. Just one question about Linux whitelist vs. Windows blacklist

BYK · 2017-07-29T11:47:33Z

src/util/user-home-dir.js

-  process.platform === 'linux' && ROOT_USER ? path.resolve('/usr/local/share') : require('os').homedir();
+export const home = require('os').homedir();
+
+const userHomeDir = process.platform === 'linux' && ROOT_USER ? path.resolve('/usr/local/share') : home;


Why limit to linux instead of blacklisting Windows only?

Hmm, I'm not sure I fully understand the question. Would you mind explaining a bit? Windows is already filtered out in the ROOT_USER here

yarn/src/util/root-user.js

Line 4 in 7a053e2

if (process.platform !== 'win32' && process.getuid) {

What about other operating systems like BSD? I wonder if we should use this root logic for those, too.

It seems safe to add 'freebsd' to the list, are there any other platforms we should add from here? https://nodejs.org/api/process.html#process_process_platform

Ah actually @BYK I see what you're saying. Yeah I don't think the platform check is even necessary given that line of code in root-user.js

Daniel15 · 2017-07-29T18:12:02Z

src/registries/npm-registry.js

@@ -14,7 +14,7 @@ import {addSuffix} from '../util/misc';
 import {getPosixPath, resolveWithHome} from '../util/path';

 const normalizeUrl = require('normalize-url');
-const userHome = require('../util/user-home-dir').default;
+const {default: userHome, home} = require('../util/user-home-dir');


While you're here, could you please change this to an import statement? I've been meaning to revisit one of my old pull requests where I updated all require statements to use import, but just haven't had time.

sure thing!

BYK

Pushing back for the test describe comment. I'll push a fix myself and merge it for expediency.

Thanks a lot for the fix!

BYK · 2017-07-31T10:42:15Z

src/registries/npm-registry.js

-const ini = require('ini');
+import normalizeUrl from 'normalize-url';
+import {default as userHome, home} from '../util/user-home-dir';
+import path from 'path';


Nice! I think the convention is to list stdlib imports at the top, right?

BYK · 2017-07-31T10:42:47Z

__tests__/registries/npm-registry.js

-  const mockReporter = jest.fn();
+  const mockLog = [];
+  const mockReporter = {
+    lang: (...args) => mockLog.push(...args),


I think we can also use the BufferReporter for tests?

BYK · 2017-07-31T10:43:13Z

__tests__/registries/npm-registry.js

@@ -292,3 +297,16 @@ describe('getScope functional test', () => {
    });
  });
 });
+
+describe('getPossibleConfigLocations', () => {
+  describe('searches recursively to home directory', async () => {


This should be a test or it call not describe.

BYK · 2017-07-31T10:45:00Z

src/registries/npm-registry.js

+    // When home directory for global install is different from where $HOME/npmrc is stored,
+    // E.g. /usr/local/share vs /root on linux machines, check the additional location
+    if (home !== userHome) {
+      possibles.push([true, path.join(home, localfile)]);


I think we should change this data structure to be a normal object instead of an array sometime in the future: {isHomeDir: true, path: path.join(home, localfile)}

kaylie-alexa · 2017-07-31T17:56:12Z

@BYK thanks for being awesome and making those changes!

felixfbecker · 2017-08-26T03:28:14Z

Any ETA for a release containing this fix?

Daniel15 · 2017-08-26T03:31:33Z

@felixfbecker You can use a nightly build if you like. https://nightly.yarnpkg.com/

The Yarn team are working on 1.0 at the moment.

felixfbecker · 2017-08-26T03:36:54Z

Tried to, but unfortunately I need to add it to an Alpine Docker image and running the install script just results in

Step 8/16 : RUN curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --nightly
 ---> Running in dc6404f17920
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6746    0  6746    0     0  33679      0 --:--:-- --:--:-- --:--:-- 54845
Installing Yarn!
> Downloading tarball...

[1/2]: https://nightly.yarnpkg.com/latest.tar.gz --> /tmp/yarn.tar.gz.XXXXLPmEmH
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  821k  100  821k    0     0  1764k      0 --:--:-- --:--:-- --:--:-- 1764k

[2/2]: https://nightly.yarnpkg.com/latest.tar.gz.asc --> /tmp/yarn.tar.gz.XXXXLPmEmH.asc
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   900  100   900    0     0   7945      0 --:--:-- --:--:-- --:--:--  7945
> WARNING: GPG is not installed, integrity can not be verified!
> Extracting to ~/.yarn...
tar: unrecognized option: strip
BusyBox v1.24.2 (2016-03-29 19:24:15 GMT) multi-call binary.

Usage: tar -[cxtZzJjahmvO] [-X FILE] [-T FILE] [-f TARFILE] [-C DIR] [FILE]...

Create, extract, or list files from a tar file

Operation:
        c       Create
        x       Extract
        t       List
        f       Name of TARFILE ('-' for stdin/out)
        C       Change to DIR before operation
        v       Verbose
        Z       (De)compress using compress
        z       (De)compress using gzip
        J       (De)compress using xz
        j       (De)compress using bzip2
        a       (De)compress using lzma
        O       Extract to stdout
        h       Follow symlinks
        m       Don't restore mtime
        exclude File to exclude
        X       File with names to exclude
        T       File with names to include
The command '/bin/sh -c curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --nightly' returned a non-zero code: 1

Really don't want to change our whole image just for a hotfix. I guess the alternative is switching to npm 5

BYK · 2017-08-29T09:06:56Z

@felixfbecker that's unfortunate :( I'll file an issue to see why the script is not working on Alpine. Guessing some mismatch between GNU tar and others.

In the meantime, you can use the single-file bundle from https://nightly.yarnpkg.com/yarn-1.0.0-20170825.2124.js (just curl this wherever and use it directly run it.

Daniel15 · 2017-08-31T05:03:06Z

In the meantime, you can use the single-file bundle from nightly.yarnpkg.com/yarn-1.0.0-20170825.2124.js

https://nightly.yarnpkg.com/latest.js will also give you the latest version.

You could also manually grab the tarball from https://nightly.yarnpkg.com/latest.tar.gz, check the GPG signature, and extract it. That's what the install script does :)

Check for npmrc

134c88f

kaylie-alexa mentioned this pull request Jul 29, 2017

yarn doesn't use .npmrc from $HOME when run as root #3920

Closed

kaylie-alexa changed the title ~~Check for npmrc~~ Check for npmrc in home directory for linux root users Jul 29, 2017

add correct export

7ab2495

BYK self-assigned this Jul 29, 2017

BYK reviewed Jul 29, 2017

View reviewed changes

Daniel15 reviewed Jul 29, 2017

View reviewed changes

feedback from @BYK and @arcanis

8c93001

BYK requested changes Jul 31, 2017

View reviewed changes

Burak Yigit Kaya added 3 commits July 31, 2017 11:51

Use test instead of describe

8dc1ca6

Merge branch 'master' into check-npmrc

d5ff169

Use BufferReporter and expect.*Containing in tests

e01a46f

BYK approved these changes Jul 31, 2017

View reviewed changes

Burak Yigit Kaya added 2 commits July 31, 2017 12:43

Fix Windows tests

c51a71d

Double the backslashes, double the fun

754b215

BYK merged commit 0ef3bf1 into yarnpkg:master Jul 31, 2017

BYK mentioned this pull request Aug 29, 2017

Bug: install script broken on Alpine Linux #4280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for npmrc in home directory for linux root users #4047

Check for npmrc in home directory for linux root users #4047

kaylie-alexa commented Jul 29, 2017 •

edited by BYK

Loading

BYK left a comment

BYK Jul 29, 2017

kaylie-alexa Jul 29, 2017

Daniel15 Jul 29, 2017

kaylie-alexa Jul 29, 2017

kaylie-alexa Jul 29, 2017

Daniel15 Jul 29, 2017

kaylie-alexa Jul 29, 2017

BYK left a comment

BYK Jul 31, 2017

BYK Jul 31, 2017

BYK Jul 31, 2017

BYK Jul 31, 2017

kaylie-alexa commented Jul 31, 2017 •

edited

Loading

felixfbecker commented Aug 26, 2017

Daniel15 commented Aug 26, 2017 •

edited

Loading

felixfbecker commented Aug 26, 2017

BYK commented Aug 29, 2017

Daniel15 commented Aug 31, 2017

Check for npmrc in home directory for linux root users #4047

Check for npmrc in home directory for linux root users #4047

Conversation

kaylie-alexa commented Jul 29, 2017 • edited by BYK Loading

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaylie-alexa commented Jul 31, 2017 • edited Loading

felixfbecker commented Aug 26, 2017

Daniel15 commented Aug 26, 2017 • edited Loading

felixfbecker commented Aug 26, 2017

BYK commented Aug 29, 2017

Daniel15 commented Aug 31, 2017

kaylie-alexa commented Jul 29, 2017 •

edited by BYK

Loading

kaylie-alexa commented Jul 31, 2017 •

edited

Loading

Daniel15 commented Aug 26, 2017 •

edited

Loading