diff --git a/controllers/registry/module.js b/controllers/registry/module.js index d5303915b..63553270a 100644 --- a/controllers/registry/module.js +++ b/controllers/registry/module.js @@ -366,7 +366,7 @@ exports.upload = function *(next) { var isMaintainer = yield* packageService.isMaintainer(name, username); - if (!isMaintainer && !this.user.isAdmin) { + if (!isMaintainer) { this.status = 403; this.body = { error: 'forbidden user', @@ -474,10 +474,9 @@ exports.updateLatest = function *(next) { debug('can not get nextMod'); return yield* next; } - - var isMaintainer = yield* packageService.isMaintainer(name, username); - - if (!isMaintainer && !this.user.isAdmin) { + // TODO: old publish flow, we will delete it in the next version + // so dont change the old logic + if (!common.isMaintainer(this.user, nextMod.package.maintainers)) { this.status = 403; this.body = { error: 'forbidden user', @@ -561,22 +560,23 @@ exports.addPackageAndDist = function *(next) { tags.push([t, distTags[t]]); } - debug('addPackageAndDist %s:%s, attachment size: %s', name, version, attachment.length); + debug('%s addPackageAndDist %s:%s, attachment size: %s, maintainers: %j', + username, name, version, attachment.length, versionPackage.maintainers); var exists = yield Module.get(name, version); var shasum; if (exists) { - this.status = 409; + this.status = 403; this.body = { - error: 'conflict', - reason: 'Document update conflict.' + error: 'forbidden', + reason: 'cannot modify pre-existing version: ' + version }; return; } // check maintainers var isMaintainer = yield* packageService.isMaintainer(name, username); - if (!isMaintainer && !this.user.isAdmin) { + if (!isMaintainer) { this.status = 403; this.body = { error: 'forbidden user', @@ -796,6 +796,7 @@ exports.removeWithVersions = function* (next) { // step2: check permission var isMaintainer = yield* packageService.isMaintainer(name, username); + // admin can delete the module if (!isMaintainer && !this.user.isAdmin) { this.status = 403; this.body = { @@ -908,7 +909,7 @@ exports.removeAll = function *(next) { } var isMaintainer = yield* packageService.isMaintainer(name, username); - + // admin can delete the module if (!isMaintainer && !this.user.isAdmin) { this.status = 403; this.body = { @@ -1004,10 +1005,12 @@ exports.listAllModuleNames = function *() { }); }; -exports.updateTag = function *() { +// PUT /:name/:tag +exports.updateTag = function* () { var version = this.request.body; var tag = this.params.tag; var name = this.params.name; + debug('updateTag: %s %s to %s', name, version, tag); if (!version) { this.status = 400; @@ -1042,11 +1045,12 @@ exports.updateTag = function *() { } // check permission - if (!common.isMaintainer(this.user, mod.package.maintainers)) { + var isMaintainer = yield* packageService.isMaintainer(name, this.user.name); + if (!isMaintainer) { this.status = 403; this.body = { - error: 'forbidden', - reason: 'no permission to modify ' + name + error: 'forbidden user', + reason: this.user.name + ' not authorized to modify ' + name }; return; } diff --git a/proxy/module_maintainer.js b/proxy/module_maintainer.js index f2eb8bd7f..3be902136 100644 --- a/proxy/module_maintainer.js +++ b/proxy/module_maintainer.js @@ -29,10 +29,14 @@ exports.get = function* (name) { if (maintainers && maintainers.length > 0) { users = maintainers; } + return users.map(function (user) { + return user.name; + }); + } else { + return users.map(function (row) { + return row.user; + }); } - return users.map(function (row) { - return row.user; - }); }; var ADD_SQL = 'INSERT INTO module_maintainer(name, user, gmt_create) \ diff --git a/services/package.js b/services/package.js index b589a73dc..e4112229b 100644 --- a/services/package.js +++ b/services/package.js @@ -28,7 +28,8 @@ exports.updateMaintainers = function* (name, usernames) { exports.isMaintainer = function* (name, username) { var maintainers = yield* ModuleMaintainer.get(name); if (maintainers.length === 0) { - return false; + // no maintainers, meaning this module is free for everyone + return true; } return maintainers.indexOf(username) >= 0; }; diff --git a/test/controllers/registry/module.test.js b/test/controllers/registry/module.test.js index d1d6ea75b..c1e803abb 100644 --- a/test/controllers/registry/module.test.js +++ b/test/controllers/registry/module.test.js @@ -38,6 +38,8 @@ describe('controllers/registry/module.test.js', function () { before(function (done) { app.listen(0, function () { var pkg = require(path.join(fixtures, 'package_and_tgz.json')); + pkg.maintainers[0].name = 'cnpmjstest10'; + pkg.versions['0.0.1'].maintainers[0].name = 'cnpmjstest10'; request(app) .put('/' + pkg.name) .set('authorization', baseauth) @@ -378,7 +380,7 @@ describe('controllers/registry/module.test.js', function () { }); }); - describe('PUT /:name', function () { + describe('PUT /:name old publish flow', function () { var pkg = { name: 'testputmodule', description: 'test put module', @@ -626,16 +628,16 @@ describe('controllers/registry/module.test.js', function () { res.body.should.have.keys('ok', 'rev'); res.body.ok.should.equal(true); - // upload again should 409 + // upload again should 403 request(app) .put('/' + pkg.name) .set('authorization', baseauth) .send(pkg) - .expect(409, function (err, res) { + .expect(403, function (err, res) { should.not.exist(err); res.body.should.eql({ - error: 'conflict', - reason: 'Document update conflict.' + error: 'forbidden', + reason: 'cannot modify pre-existing version: 0.0.1' }); done(); }); @@ -811,61 +813,62 @@ describe('controllers/registry/module.test.js', function () { }); }); - describe('PUT /:name/:tag', function () { + describe('PUT /:name/:tag updateTag()', function () { it('should create new tag ok', function (done) { request(app) - .put('/testputmodule/newtag') + .put('/mk2testmodule/newtag') .set('content-type', 'application/json') .set('authorization', baseauth) - .send('"0.1.9"') - .expect(201, done); + .send('"0.0.1"') + .expect(201) + .expect({"ok":true}, done); }); it('should override exist tag ok', function (done) { request(app) - .put('/testputmodule/newtag') + .put('/mk2testmodule/newtag') .set('content-type', 'application/json') .set('authorization', baseauth) - .send('"0.1.9"') + .send('"0.0.1"') .expect(201, done); }); it('should tag invalid version 403', function (done) { request(app) - .put('/testputmodule/newtag') + .put('/mk2testmodule/newtag') .set('content-type', 'application/json') .set('authorization', baseauth) .send('"hello"') .expect(403) .expect({ error: 'forbidden', - reason: 'setting tag newtag to invalid version: hello: testputmodule/newtag' + reason: 'setting tag newtag to invalid version: hello: mk2testmodule/newtag' }, done); }); it('should tag not eixst version 403', function (done) { request(app) - .put('/testputmodule/newtag') + .put('/mk2testmodule/newtag') .set('content-type', 'application/json') .set('authorization', baseauth) .send('"5.0.0"') .expect(403) .expect({ error: 'forbidden', - reason: 'setting tag newtag to unknown version: 5.0.0: testputmodule/newtag' + reason: 'setting tag newtag to unknown version: 5.0.0: mk2testmodule/newtag' }, done); }); - it('should tag permission 403', function (done) { + it('should not maintainer tag return no permission 403', function (done) { request(app) - .put('/testputmodule/newtag') + .put('/mk2testmodule/newtag') .set('content-type', 'application/json') .set('authorization', baseauthOther) - .send('"0.1.9"') + .send('"0.0.1"') .expect(403) .expect({ - error: 'forbidden', - reason: 'no permission to modify testputmodule' + error: 'forbidden user', + reason: 'cnpmjstest101 not authorized to modify mk2testmodule' }, done); }); });