diff --git a/src/apps.js b/src/apps.js index 467e68987..2fb7072aa 100644 --- a/src/apps.js +++ b/src/apps.js @@ -68,7 +68,6 @@ var appdb = require('./appdb.js'), DomainsError = require('./domains.js').DomainsError, eventlog = require('./eventlog.js'), fs = require('fs'), - groups = require('./groups.js'), mail = require('./mail.js'), mailboxdb = require('./mailboxdb.js'), manifestFormat = require('cloudron-manifestformat'), @@ -86,6 +85,7 @@ var appdb = require('./appdb.js'), TransformStream = require('stream').Transform, updateChecker = require('./updatechecker.js'), url = require('url'), + users = require('./users.js'), util = require('util'), uuid = require('uuid'), validator = require('validator'), @@ -358,20 +358,15 @@ function hasAccessTo(app, user, callback) { // check user access if (app.accessRestriction.users.some(function (e) { return e === user.id; })) return callback(null, true); - // check group access - groups.getMembership(user.id, function (error, groupIds) { - if (error) return callback(null, false); + const isAdmin = users.isAdmin(user); - const isAdmin = groupIds.indexOf(constants.ADMIN_GROUP_ID) !== -1; + if (isAdmin) return callback(null, true); // admins can always access any app - if (isAdmin) return callback(null, true); // admins can always access any app + if (!app.accessRestriction.groups) return callback(null, false); - if (!app.accessRestriction.groups) return callback(null, false); + if (app.accessRestriction.groups.some(function (gid) { return user.groupIds.indexOf(gid) !== -1; })) return callback(null, true); - if (app.accessRestriction.groups.some(function (gid) { return groupIds.indexOf(gid) !== -1; })) return callback(null, true); - - callback(null, false); - }); + callback(null, false); } function get(appId, callback) { diff --git a/src/ldap.js b/src/ldap.js index 16be48715..1b9d56fd2 100644 --- a/src/ldap.js +++ b/src/ldap.js @@ -52,16 +52,16 @@ function getUsersWithAccessToApp(req, callback) { getAppByRequest(req, function (error, app) { if (error) return callback(error); - users.list(function (error, result){ + users.list(function (error, result) { if (error) return callback(new ldap.OperationsError(error.toString())); - async.filter(result, apps.hasAccessTo.bind(null, app), function (error, result) { + async.filter(result, apps.hasAccessTo.bind(null, app), function (error, allowedUsers) { if (error) return callback(new ldap.OperationsError(error.toString())); // TODO: in the long run, we should probably get rid of this "admin" integration altogether - result.forEach(function (r) { r.admin = r.groupIds.indexOf(constants.ADMIN_GROUP_ID) !== -1; }); + allowedUsers.forEach(function (r) { r.admin = users.isAdmin(r); }); - callback(null, result); + callback(null, allowedUsers); }); }); }); diff --git a/src/test/apps-test.js b/src/test/apps-test.js index 0972527c7..71062562c 100644 --- a/src/test/apps-test.js +++ b/src/test/apps-test.js @@ -32,7 +32,8 @@ describe('Apps', function () { createdAt: 'sometime back', modifiedAt: 'now', resetToken: hat(256), - displayName: '' + displayName: '', + groupIds: [ 'admin' ] }; var USER_0 = { @@ -45,7 +46,8 @@ describe('Apps', function () { createdAt: 'sometime back', modifiedAt: 'now', resetToken: hat(256), - displayName: '' + displayName: '', + groupIds: [] }; var USER_1 = { @@ -58,7 +60,8 @@ describe('Apps', function () { createdAt: 'sometime back', modifiedAt: 'now', resetToken: hat(256), - displayName: '' + displayName: '', + groupIds: [ 'somegroup' ] }; var GROUP_0 = { @@ -287,8 +290,11 @@ describe('Apps', function () { }); describe('hasAccessTo', function () { + const someuser = { id: 'someuser', groupIds: [] }; + const adminuser = { id: 'adminuser', groupIds: [ 'admin' ] }; + it('returns true for unrestricted access', function (done) { - apps.hasAccessTo({ accessRestriction: null }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: null }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(true); done(); @@ -296,7 +302,7 @@ describe('Apps', function () { }); it('returns true for allowed user', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ 'someuser' ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ 'someuser' ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(true); done(); @@ -304,7 +310,7 @@ describe('Apps', function () { }); it('returns true for allowed user with multiple allowed', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ 'foo', 'someuser', 'anotheruser' ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ 'foo', 'someuser', 'anotheruser' ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(true); done(); @@ -312,7 +318,7 @@ describe('Apps', function () { }); it('returns false for not allowed user', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ 'foo' ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ 'foo' ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(false); done(); @@ -320,7 +326,7 @@ describe('Apps', function () { }); it('returns false for not allowed user with multiple allowed', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ 'foo', 'anotheruser' ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ 'foo', 'anotheruser' ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(false); done(); @@ -328,7 +334,7 @@ describe('Apps', function () { }); it('returns false for no group or user', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ ], groups: [ ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ ], groups: [ ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(false); done(); @@ -336,12 +342,20 @@ describe('Apps', function () { }); it('returns false for invalid group or user', function (done) { - apps.hasAccessTo({ accessRestriction: { users: [ ], groups: [ 'nop' ] } }, { id: 'someuser' }, function (error, access) { + apps.hasAccessTo({ accessRestriction: { users: [ ], groups: [ 'nop' ] } }, someuser, function (error, access) { expect(error).to.be(null); expect(access).to.be(false); done(); }); }); + + it('returns true for admin user', function (done) { + apps.hasAccessTo({ accessRestriction: { users: [ ], groups: [ 'nop' ] } }, adminuser, function (error, access) { + expect(error).to.be(null); + expect(access).to.be(true); + done(); + }); + }); }); describe('getAllByUser', function () {