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: do not alter file ownership #141

Merged
merged 1 commit into from
Oct 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/content/read.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict'

const fs = require('@npmcli/fs')
const fs = require('fs/promises')
const fsm = require('fs-minipass')
const ssri = require('ssri')
const contentPath = require('./path')
Expand Down
6 changes: 2 additions & 4 deletions lib/content/rm.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
'use strict'

const util = require('util')

const fs = require('fs/promises')
const contentPath = require('./path')
const { hasContent } = require('./read')
const rimraf = util.promisify(require('rimraf'))

module.exports = rm

async function rm (cache, integrity) {
const content = await hasContent(cache, integrity)
// ~pretty~ sure we can't end up with a content lacking sri, but be safe
if (content && content.sri) {
await rimraf(contentPath(cache, content.sri))
await fs.rm(contentPath(cache, content.sri), { recursive: true, force: true })
return true
} else {
return false
Expand Down
14 changes: 5 additions & 9 deletions lib/content/write.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
'use strict'

const events = require('events')
const util = require('util')

const contentPath = require('./path')
const fixOwner = require('../util/fix-owner')
const fs = require('@npmcli/fs')
const fs = require('fs/promises')
const moveFile = require('../util/move-file')
const Minipass = require('minipass')
const Pipeline = require('minipass-pipeline')
const Flush = require('minipass-flush')
const path = require('path')
const rimraf = util.promisify(require('rimraf'))
const ssri = require('ssri')
const uniqueFilename = require('unique-filename')
const fsm = require('fs-minipass')
Expand Down Expand Up @@ -40,7 +37,7 @@ async function write (cache, data, opts = {}) {
return { integrity: sri, size: data.length }
} finally {
if (!tmp.moved) {
await rimraf(tmp.target)
await fs.rm(tmp.target, { recursive: true, force: true })
}
}
}
Expand Down Expand Up @@ -111,7 +108,7 @@ async function handleContent (inputStream, cache, opts) {
return res
} finally {
if (!tmp.moved) {
await rimraf(tmp.target)
await fs.rm(tmp.target, { recursive: true, force: true })
}
}
}
Expand Down Expand Up @@ -152,7 +149,7 @@ async function pipeToTmp (inputStream, cache, tmpTarget, opts) {

async function makeTmp (cache, opts) {
const tmpTarget = uniqueFilename(path.join(cache, 'tmp'), opts.tmpPrefix)
await fixOwner.mkdirfix(cache, path.dirname(tmpTarget))
await fs.mkdir(path.dirname(tmpTarget), { recursive: true })
return {
target: tmpTarget,
moved: false,
Expand All @@ -163,10 +160,9 @@ async function moveToDestination (tmp, cache, sri, opts) {
const destination = contentPath(cache, sri)
const destDir = path.dirname(destination)

await fixOwner.mkdirfix(cache, destDir)
await fs.mkdir(destDir, { recursive: true })
await moveFile(tmp.target, destination)
tmp.moved = true
await fixOwner.chownr(cache, destination)
}

function sizeError (expected, found) {
Expand Down
49 changes: 19 additions & 30 deletions lib/entry-index.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
'use strict'

const util = require('util')
const crypto = require('crypto')
const fs = require('@npmcli/fs')
const {
appendFile,
mkdir,
readFile,
readdir,
rm,
writeFile,
} = require('fs/promises')
const Minipass = require('minipass')
const path = require('path')
const ssri = require('ssri')
const uniqueFilename = require('unique-filename')

const contentPath = require('./content/path')
const fixOwner = require('./util/fix-owner')
const hashToSegments = require('./util/hash-to-segments')
const indexV = require('../package.json')['cache-version'].index
const moveFile = require('@npmcli/move-file')
const _rimraf = require('rimraf')
const rimraf = util.promisify(_rimraf)

module.exports.NotFoundError = class NotFoundError extends Error {
constructor (cache, key) {
Expand Down Expand Up @@ -65,7 +68,7 @@ async function compact (cache, key, matchFn, opts = {}) {

const setup = async () => {
const target = uniqueFilename(path.join(cache, 'tmp'), opts.tmpPrefix)
await fixOwner.mkdirfix(cache, path.dirname(target))
await mkdir(path.dirname(target), { recursive: true })
return {
target,
moved: false,
Expand All @@ -74,24 +77,17 @@ async function compact (cache, key, matchFn, opts = {}) {

const teardown = async (tmp) => {
if (!tmp.moved) {
return rimraf(tmp.target)
return rm(tmp.target, { recursive: true, force: true })
}
}

const write = async (tmp) => {
await fs.writeFile(tmp.target, newIndex, { flag: 'wx' })
await fixOwner.mkdirfix(cache, path.dirname(bucket))
await writeFile(tmp.target, newIndex, { flag: 'wx' })
await mkdir(path.dirname(bucket), { recursive: true })
// we use @npmcli/move-file directly here because we
// want to overwrite the existing file
await moveFile(tmp.target, bucket)
tmp.moved = true
try {
await fixOwner.chownr(cache, bucket)
} catch (err) {
if (err.code !== 'ENOENT') {
throw err
}
}
}

// write the file atomically
Expand Down Expand Up @@ -123,7 +119,7 @@ async function insert (cache, key, integrity, opts = {}) {
metadata,
}
try {
await fixOwner.mkdirfix(cache, path.dirname(bucket))
await mkdir(path.dirname(bucket), { recursive: true })
const stringified = JSON.stringify(entry)
// NOTE - Cleverness ahoy!
//
Expand All @@ -133,19 +129,13 @@ async function insert (cache, key, integrity, opts = {}) {
//
// Thanks to @isaacs for the whiteboarding session that ended up with
// this.
await fs.appendFile(bucket, `\n${hashEntry(stringified)}\t${stringified}`)
await fixOwner.chownr(cache, bucket)
await appendFile(bucket, `\n${hashEntry(stringified)}\t${stringified}`)
} catch (err) {
if (err.code === 'ENOENT') {
return undefined
}

throw err
// There's a class of race conditions that happen when things get deleted
// during fixOwner, or between the two mkdirfix/chownr calls.
//
// It's perfectly fine to just not bother in those cases and lie
// that the index entry was written. Because it's a cache.
}
return formatEntry(cache, entry)
}
Expand Down Expand Up @@ -180,7 +170,7 @@ function del (cache, key, opts = {}) {
}

const bucket = bucketPath(cache, key)
return rimraf(bucket)
return rm(bucket, { recursive: true, force: true })
}

module.exports.lsStream = lsStream
Expand Down Expand Up @@ -246,7 +236,7 @@ async function ls (cache) {
module.exports.bucketEntries = bucketEntries

async function bucketEntries (bucket, filter) {
const data = await fs.readFile(bucket, 'utf8')
const data = await readFile(bucket, 'utf8')
return _bucketEntries(data, filter)
}

Expand All @@ -266,9 +256,8 @@ function _bucketEntries (data, filter) {
let obj
try {
obj = JSON.parse(pieces[1])
} catch (e) {
// Entry is corrupted!
return
} catch (_) {
// eslint-ignore-next-line no-empty-block
nlf marked this conversation as resolved.
Show resolved Hide resolved
}
// coverage disabled here, no need to test with an entry that parses to something falsey
// istanbul ignore else
Expand Down Expand Up @@ -331,7 +320,7 @@ function formatEntry (cache, entry, keepAll) {
}

function readdirOrEmpty (dir) {
return fs.readdir(dir).catch((err) => {
return readdir(dir).catch((err) => {
if (err.code === 'ENOENT' || err.code === 'ENOTDIR') {
return []
}
Expand Down
10 changes: 5 additions & 5 deletions lib/rm.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
'use strict'

const util = require('util')

const { rm } = require('fs/promises')
const glob = require('./util/glob.js')
const index = require('./entry-index')
const memo = require('./memoization')
const path = require('path')
const rimraf = util.promisify(require('rimraf'))
const rmContent = require('./content/rm')

module.exports = entry
Expand All @@ -25,7 +24,8 @@ function content (cache, integrity) {

module.exports.all = all

function all (cache) {
async function all (cache) {
memo.clearMemoized()
return rimraf(path.join(cache, '*(content-*|index-*)'))
const paths = await glob(path.join(cache, '*(content-*|index-*)'), { silent: true, nosort: true })
nlf marked this conversation as resolved.
Show resolved Hide resolved
return Promise.all(paths.map((p) => rm(p, { recursive: true, force: true })))
}
91 changes: 0 additions & 91 deletions lib/util/fix-owner.js

This file was deleted.

7 changes: 7 additions & 0 deletions lib/util/glob.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict'

const { promisify } = require('util')
const glob = promisify(require('glob'))

const globify = (pattern) => pattern.split('//').join('/')
module.exports = (path, options) => glob(globify(path), options)
2 changes: 1 addition & 1 deletion lib/util/move-file.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict'

const fs = require('@npmcli/fs')
const fs = require('fs/promises')
const move = require('@npmcli/move-file')
const pinflight = require('promise-inflight')

Expand Down
13 changes: 3 additions & 10 deletions lib/util/tmp.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
'use strict'

const fs = require('@npmcli/fs')

const fixOwner = require('./fix-owner')
const { withTempDir } = require('@npmcli/fs')
const fs = require('fs/promises')
const path = require('path')

module.exports.mkdir = mktmpdir
Expand All @@ -23,11 +22,5 @@ function withTmp (cache, opts, cb) {
cb = opts
opts = {}
}
return fs.withTempDir(path.join(cache, 'tmp'), cb, opts)
}

module.exports.fix = fixtmpdir

function fixtmpdir (cache) {
return fixOwner(cache, path.join(cache, 'tmp'))
return withTempDir(path.join(cache, 'tmp'), cb, opts)
}
Loading