os: add userInfo() method

[cjihrig]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

os

Description of change

os.getpw() calls libuv's uv_os_get_passwd() function. It returns an object containing the current effective user's username, uid, gid, shell, and homedir. On Windows, the uid and gid are -1, and the shell is null.

[addaleax]

Might it be a good idea to name this differently? os.getpw sounds a lot like it returns some actual password, and I think users who are not so familiar with unices will be thrown off by this; and even for those who are it might take a bit to get from getpw to /etc/passwd.

passwd has been a misnomer for a long time now, but it has the advantage of being an identifier that many people recognize. I think libuv made a good call by naming it uv_os_get_passwd, and I’d suggest using either os.getPasswd or os.getUserInfo (or similar) – the latter having the advantage of describing what the method actually does, the former having the advantage of describing where the returned data comes from.

[mscdex]

I agree that it should probably be named something else. I don't have any suggestions at the moment though. I suppose os.getUserInfo() as @addaleax suggested would be alright.

[bnoordhuis]

os.userinfo() or os.userInfo() then; the os module has a tradition of dropping the 'get' prefix.

Alternatively, you could expose the individual properties as functions. uid and gid aren't relevant on Windows and arguably neither on Unices (because process.uid and process.gid exist), homedir duplicates os.homedir() to some extent, leaving username and shell -although the latter should probably be named loginShell or something like that.

If you stick with the object-with-properties approach, then the documentation should explain the difference between os.homedir() and os.getpw().homedir.

[Fishrock123]

+1 to renaming, or separating out username and default shell.

[jasnell]

+1 to renaming. The change LGTM with that change made.

[cjihrig]

Updated. I decided to rename to userInfo(), mostly to give a direct OS alternative to homedir(), as there has already been at least one request to bypass the environment variable checks.

[jasnell]

Quick nit: given the likelihood that this is running on linux, there's a possibility that this may not actually be a OneByteString. It complicates things just a bit, but it might be worthwhile allowing an optional options (e.g. {encoding:'utf8'} to be passed in to GetUserInfo so that the encoding of the path can be specified. This would make it consistent with the new Buffer support in the fs module.

[jasnell]

LGTM. Left a comment that could be deferred to a separate PR if necessary.

[silverwind]

shell and homedir should be swapped here because that's the way they appear in passwd.

[silverwind]

I'm also worried about the duplication and the fact that it's not Windows compatible.

Are there use cases where process.env.USER and process.env.SHELL are not suitable?

[Fishrock123]

What about loginShell or defaultShell vs shell? I don't super mind one way or other but wanted to make sure we discussed it too.

[jasnell]

@silverwind ... as I understand it, yes. I cannot recall the exact issue number but there is an open request to be able to retrieve the actual user information separately from the environment variables in certain use cases. The reason is that the env var could actually be incorrect in those edge cases.

The fact that this is not Windows compatible is not too concerning to me. We already have plenty of similar cases.

[jasnell]

I'd prefer keeping it to just shell simply because it's more concise.

[silverwind]

How about adding gecos and password (yeah, I know it's mostly unused today) fields? Would give this method at least some value.

[sindresorhus]

How about adding gecos

👍 On OS X at least it contains the user's fullname, which I could use for my fullname module.

[cjihrig]

@jasnell I changed to UTF8 for consistency with os.homedir(). How do you see an encoding argument working? Hypothetical situation, but what if the shell, home directory, and username all had different encodings?

[cjihrig]

re: gecos, support would need to land in libuv first.

[jasnell]

In that edge case, the dev could pass encoding: 'buffer' and get back
Buffer instances for those instead of strings.
On Apr 10, 2016 6:38 AM, "Colin Ihrig" notifications@github.com wrote:

@jasnell https://github.com/jasnell I changed to UTF8 for consistency
with os.homedir(). How do you see an encoding argument working?
Hypothetical situation, but what if the shell, home directory, and username
all had different encodings?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6104 (comment)

[jasnell]

Refs: #5582

[cjihrig]

@jasnell updated to include an encoding option.

[jasnell]

nit: perhaps add a short note that if encoding is passed as 'buffer' the username, homedir, and shell will be returned as Buffer instances.

[jasnell]

LGTM with a nit.

[cjihrig]

nit addressed. CI is green: https://ci.nodejs.org/job/node-test-pull-request/2248/

[sindresorhus]

I made a polyfill for anyone needing this for existing Node.js versions (all the way back to 0.10): https://github.com/sindresorhus/user-info

[rvagg]

Can someone justify this please? Most of this doesn't make sense cross-platform and as @bnoordhuis has pointed out we already expose most of these properties. Can we please not just implement things here because libuv does, we need to have more discipline than that. Too many tiny new features are getting in just because we can without much attention paid to is it worth it.

[rvagg]

Adding a temporary dont-land-on-v5.x so we can discuss further.

• username - can be fetched from environment variable, is there a compelling case for providing direct access to this?
• uid - available at process.uid
• gid - available at process.gid
• shell - available via environment variable (usually), what is the use-case here? have there been calls for this? If there even prior art for use of this?
• homedir - available via os.homedir() with justification for providing a different mechanism given in Issue about os.homedir() #5582 which boils down to a criticism of the way we implemented homedir() yet we skipped over the part where we have a discussion about the API design decisions we made and whether we should have allowed $HOME to override what the system says. I'd actually argue that we change homedir() to not use $HOME since that's a decision that can be made at the application layer as process.env.HOME is already available. So now we have two ways to access homedir and they can be different!

I'm not a fan of this and would personally prefer a revert.

[silverwind]

I too feel that the added functionality seems rather unnecessary and that it was landed too fast without enough consensus.

[jasnell]

Can't really agree with the "landed too fast" part. The PR was opened 8 days ago and landed 2 days ago. The standard rule in the collaborator's guide is 48/72 hours (weekday/weekend). We've had plenty of stuff land in far less time. While I can certainly understand regarding the not enough consensus part, given that I'm the only one who gave it an LGTM, it cannot be said that there wasn't enough time to review it. On the consensus part, this was looked at by six people, four of whom are CTC, none of whom objected. Again, we've had stuff land with less.

That said, @rvagg makes a fair request with regards to use case. For my part, I consider this an ok fix for #5582 because it does not change the existing behavior of homedir(). I'd much rather have the existing code continue to work as it has and expose the new functionality as a semver-minor addition.

bash-3.2$ sudo -u root ./node -pe "require('os').userInfo()"
{ uid: 0,
  gid: 0,
  username: 'root',
  homedir: '/var/root',
  shell: '/bin/sh' }
bash-3.2$ sudo -u root ./node -pe "require('os').homedir()"
/Users/james
bash-3.2$

I agree that the uid and gid are of significantly less value here given process.getgid() and process.getuid() work perfectly well.

[jbergstroem]

(Sorry for joining post-merge as well)

I feel that the new homedir is confusing, being named after os.homedir. I personally don't have a strong opinion about reading $HOME or not (if sudo is acting up I don't feel Node.js is to blame) but the naming is unfortunate since we now have two very similar names returning different things. The other attributes available might fit well into userInfo but is already available elsewhere. I would prefer changing how os.homedir works if that indeed is a problem.

[jasnell]

One thing to note is that, as it is currently implemented, os.userInfo() is slower than the existing ways of getting this information.

Given the benchmark:

'use strict';

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

const bench = common.createBenchmark(main, {
  method: ['old', 'new'],
  millions: [2]
});

function main(conf) {
  const n = +conf.millions * 1e6;

  var i = 0;
  switch (conf.method) {
    case 'old':
      bench.start();
      for (; i < n; i++) {
        var m = {
          user: process.env.USER,
          shell: process.env.SHELL,
          homedir: os.homedir(),
          gid: process.getgid(),
          uid: process.getuid()
        }
      }
      bench.end(n / 1e6);
      break;
    case 'new':
      bench.start();
      for (; i < n; i++) {
        os.userInfo();
      }
      bench.end(n / 1e6);
      break;
    default:
      throw new Error('Unexpected');
  }
}

We see:

bash-3.2$ ./node benchmark/process/userinfo.js 
process/userinfo.js method=old millions=2: 1.11115
process/userinfo.js method=new millions=2: 0.50853

If I modify the os.userInfo() implementation to use some basic caching keyed off the encoding option:

const _userInfo = {};
exports.userInfo = function(options = {encoding: 'utf'}) {
  if (typeof options === 'string')
    options = {encoding: options};
  if (!_userInfo[options.encoding]) {
    _userInfo[options.encoding] = binding.getUserInfo(options);
  }
  return _userInfo[options.encoding];
}

The benchmark changes to:

bash-3.2$ ./node benchmark/process/userinfo.js 
process/userinfo.js method=old millions=2: 1.12694
process/userinfo.js method=new millions=2: 59.45691

Given that it's not likely that this function will be called multiple times within a single process, however, the benchmark itself (and adding caching) may be of little value.

[Fishrock123]

This landed in v6, and as far as I can tell should not have. Should we issue a revert? (Getting in was a bug of our release process, by the sounds of it.)

[ChALkeR]

@Fishrock123 Strictly speaking, reverting a semver-minor feature would be a semver-major… Or is this feature broken?

[addaleax]

This feature is not broken, and I agree, fully removing this in v6 is probably not a good idea.

[Fishrock123]

We should deprecate it in the docs then.

[Fishrock123]

No-one is significantly using it yet, so we can do that immediately.

[jasnell]

It should go through the normal deprecation dance if anything. I certainly disagree with the sentiment that it shouldn't have landed in v6. It was part of master, it landed through the normal processes, there was no revert PR before v6.