admin user should never publish to other user's packages. fix #363

but admin user need have the permission to delete other's packages.

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;
   }

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) \

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;
 };

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);
     });
   });