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

feat: infer uid/gid instead of accepting as options #1

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Jul 15, 2019

BREAKING CHANGE: the uid gid options are no longer respected or
necessary. As of this change, cacache will always match the cache
contents to the ownership of the cache directory (or its parent
directory), regardless of what the caller passes in.

Reasoning:

The number one reason to use a uid or gid option was to keep root-owned
files from causing problems in the cache. In npm's case, this meant
that CLI's ./lib/command.js had to work out the appropriate uid and gid,
then pass it to the libnpmcommand module, which had to in turn pass the
uid and gid to npm-registry-fetch, which then passed it to
make-fetch-happen, which passed it to cacache. (For package fetching,
pacote would be in that mix as well.)

Added to that, cacache.rm() will actually write a file into the
cache index, but has no way to accept an option so that its call to
entry-index.js will write the index with the appropriate uid/gid.
Little ownership bugs were all over the place, and tricky to trace
through. (Why should make-fetch-happen even care about accepting or
passing uids and gids? It's an http library.)

This change allows us to keep the cache from having mixed ownership in
any situation.

Of course, this does mean that if you have a root-owned but
user-writable folder (for example, /tmp), then the cache will try to
chown everything to root.

The solution is for the user to create a folder, make it user-owned, and
use that, rather than relying on cacache to create the root cache folder.

If we decide to restore the uid/gid opts, and use ownership inferrence
only when uid/gid are unset, then take care to also make rm take an
option object, and pass it through to entry-index.js.

@isaacs isaacs self-assigned this Jul 15, 2019
BREAKING CHANGE: the uid gid options are no longer respected or
necessary.  As of this change, cacache will always match the cache
contents to the ownership of the cache directory (or its parent
directory), regardless of what the caller passes in.

Reasoning:

The number one reason to use a uid or gid option was to keep root-owned
files from causing problems in the cache.  In npm's case, this meant
that CLI's ./lib/command.js had to work out the appropriate uid and gid,
then pass it to the libnpmcommand module, which had to in turn pass the
uid and gid to npm-registry-fetch, which then passed it to
make-fetch-happen, which passed it to cacache.  (For package fetching,
pacote would be in that mix as well.)

Added to that, `cacache.rm()` will actually _write_ a file into the
cache index, but has no way to accept an option so that its call to
entry-index.js will write the index with the appropriate uid/gid.
Little ownership bugs were all over the place, and tricky to trace
through.  (Why should make-fetch-happen even care about accepting or
passing uids and gids?  It's an http library.)

This change allows us to keep the cache from having mixed ownership in
any situation.

Of course, this _does_ mean that if you have a root-owned but
user-writable folder (for example, `/tmp`), then the cache will try to
chown everything to root.

The solution is for the user to create a folder, make it user-owned, and
use that, rather than relying on cacache to create the root cache folder.

If we decide to restore the uid/gid opts, and use ownership inferrence
only when uid/gid are unset, then take care to also make rm take an
option object, and pass it through to entry-index.js.
@isaacs isaacs merged commit ac84d14 into latest Jul 15, 2019
@isaacs isaacs deleted the infer-uid-gid branch September 17, 2019 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant