Skip to content

Backport/do not allow duplicate role names #2296

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.

Sign up for GitHub

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

Merged
superkhau merged 2 commits into from
May 5, 2016
Merged

Backport/do not allow duplicate role names #2296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4798b2f
Add feature to not allow duplicate role name
supasate Apr 29, 2016
e89fbd7
Clean up by removing unnecessary comments
supasate May 3, 2016
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: 2 additions & 0 deletions common/models/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,6 @@ module.exports = function(Role) {
if (callback) callback(err, roles);
});
};

Role.validatesUniquenessOf('name', { message: 'already exists' });
};
79 changes: 57 additions & 22 deletions test/role.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ var async = require('async');
var expect = require('chai').expect;

function checkResult(err, result) {
// console.log(err, result);
assert(!err);
}

Expand Down Expand Up @@ -65,19 +64,17 @@ describe('role model', function() {
});

it('should define role/user relations', function() {

User.create({name: 'Raymond', email: 'x@y.com', password: 'foobar'}, function(err, user) {
// console.log('User: ', user.id);
Role.create({name: 'userRole'}, function(err, role) {
role.principals.create({principalType: RoleMapping.USER, principalId: user.id}, function(err, p) {
User.create({ name: 'Raymond', email: 'x@y.com', password: 'foobar' }, function(err, user) {
Role.create({ name: 'userRole' }, function(err, role) {
role.principals.create({ principalType: RoleMapping.USER, principalId: user.id },
function(err, p) {
Role.find(function(err, roles) {
assert(!err);
assert.equal(roles.length, 1);
assert.equal(roles[0].name, 'userRole');
});
role.principals(function(err, principals) {
assert(!err);
// console.log(principals);
assert.equal(principals.length, 1);
assert.equal(principals[0].principalType, RoleMapping.USER);
assert.equal(principals[0].principalId, user.id);
Expand All @@ -93,11 +90,25 @@ describe('role model', function() {

});

it('should automatically generate role id', function() {
it('should not allow duplicate role name', function(done) {
Role.create({ name: 'userRole' }, function(err, role) {
if (err) return done(err);

User.create({name: 'Raymond', email: 'x@y.com', password: 'foobar'}, function(err, user) {
// console.log('User: ', user.id);
Role.create({name: 'userRole'}, function(err, role) {
Role.create({ name: 'userRole' }, function(err, role) {
expect(err).to.exist; //jshint ignore:line

@superkhau superkhau May 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos Do you know if there is a better way to fix this (besides ...to.exist())?

@bajtos bajtos May 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me, just squash the commit adding this line with the one you are fixing.

I think we should fix our eslint-config-loopback to reject lines like expect(err).to.exist too.

@bajtos bajtos May 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should fix our eslint-config-loopback to reject lines like expect(err).to.exist too.

loopbackio/eslint-config-loopback#6

expect(err).to.have.property('name', 'ValidationError');
expect(err).to.have.deep.property('details.codes.name');
expect(err.details.codes.name).to.contain('uniqueness');
expect(err).to.have.property('statusCode', 422);

done();
});
});
});

it('should automatically generate role id', function() {
User.create({ name: 'Raymond', email: 'x@y.com', password: 'foobar' }, function(err, user) {
Role.create({ name: 'userRole' }, function(err, role) {
assert(role.id);
role.principals.create({principalType: RoleMapping.USER, principalId: user.id}, function(err, p) {
assert(p.id);
Expand All @@ -109,7 +120,6 @@ describe('role model', function() {
});
role.principals(function(err, principals) {
assert(!err);
// console.log(principals);
assert.equal(principals.length, 1);
assert.equal(principals[0].principalType, RoleMapping.USER);
assert.equal(principals[0].principalId, user.id);
Expand All @@ -126,13 +136,12 @@ describe('role model', function() {
});

it('should support getRoles() and isInRole()', function() {
User.create({name: 'Raymond', email: 'x@y.com', password: 'foobar'}, function(err, user) {
// console.log('User: ', user.id);
Role.create({name: 'userRole'}, function(err, role) {
role.principals.create({principalType: RoleMapping.USER, principalId: user.id}, function(err, p) {
// Role.find(console.log);
// role.principals(console.log);
Role.isInRole('userRole', {principalType: RoleMapping.USER, principalId: user.id}, function(err, exists) {
User.create({ name: 'Raymond', email: 'x@y.com', password: 'foobar' }, function(err, user) {
Role.create({ name: 'userRole' }, function(err, role) {
role.principals.create({ principalType: RoleMapping.USER, principalId: user.id },
function(err, p) {
Role.isInRole('userRole', { principalType: RoleMapping.USER, principalId: user.id },
function(err, exists) {
assert(!err && exists === true);
});

Expand Down Expand Up @@ -209,9 +218,9 @@ describe('role model', function() {
assert(!err && yes);
});

// console.log('User: ', user.id);
Album.create({name: 'Album 1', userId: user.id}, function(err, album1) {
Role.isInRole(Role.OWNER, {principalType: ACL.USER, principalId: user.id, model: Album, id: album1.id}, function(err, yes) {
Album.create({ name: 'Album 1', userId: user.id }, function(err, album1) {
var role = { principalType: ACL.USER, principalId: user.id, model: Album, id: album1.id };
Role.isInRole(Role.OWNER, role, function(err, yes) {
assert(!err && yes);
});
Album.create({name: 'Album 2'}, function(err, album2) {
Expand All @@ -233,22 +242,26 @@ describe('role model', function() {
password: 'jpass'
}, function(err, u) {
if (err) return done(err);

user = u;
User.create({
username: 'mary',
email: 'mary@gmail.com',
password: 'mpass'
}, function(err, u) {
if (err) return done(err);

Application.create({
name: 'demo'
}, function(err, a) {
if (err) return done(err);

app = a;
Role.create({
name: 'admin'
}, function(err, r) {
if (err) return done(err);

role = r;
var principals = [
{
Expand All @@ -272,55 +285,69 @@ describe('role model', function() {
it('should resolve user by id', function(done) {
ACL.resolvePrincipal(ACL.USER, user.id, function(err, u) {
if (err) return done(err);

expect(u.id).to.eql(user.id);

done();
});
});

it('should resolve user by username', function(done) {
ACL.resolvePrincipal(ACL.USER, user.username, function(err, u) {
if (err) return done(err);

expect(u.username).to.eql(user.username);

done();
});
});

it('should resolve user by email', function(done) {
ACL.resolvePrincipal(ACL.USER, user.email, function(err, u) {
if (err) return done(err);

expect(u.email).to.eql(user.email);

done();
});
});

it('should resolve app by id', function(done) {
ACL.resolvePrincipal(ACL.APP, app.id, function(err, a) {
if (err) return done(err);

expect(a.id).to.eql(app.id);

done();
});
});

it('should resolve app by name', function(done) {
ACL.resolvePrincipal(ACL.APP, app.name, function(err, a) {
if (err) return done(err);

expect(a.name).to.eql(app.name);

done();
});
});

it('should report isMappedToRole by user.username', function(done) {
ACL.isMappedToRole(ACL.USER, user.username, 'admin', function(err, flag) {
if (err) return done(err);

expect(flag).to.eql(true);

done();
});
});

it('should report isMappedToRole by user.email', function(done) {
ACL.isMappedToRole(ACL.USER, user.email, 'admin', function(err, flag) {
if (err) return done(err);

expect(flag).to.eql(true);

done();
});
});
Expand All @@ -329,23 +356,29 @@ describe('role model', function() {
function(done) {
ACL.isMappedToRole(ACL.USER, 'mary', 'admin', function(err, flag) {
if (err) return done(err);

expect(flag).to.eql(false);

done();
});
});

it('should report isMappedToRole by app.name', function(done) {
ACL.isMappedToRole(ACL.APP, app.name, 'admin', function(err, flag) {
if (err) return done(err);

expect(flag).to.eql(true);

done();
});
});

it('should report isMappedToRole by app.name', function(done) {
ACL.isMappedToRole(ACL.APP, app.name, 'admin', function(err, flag) {
if (err) return done(err);

expect(flag).to.eql(true);

done();
});
});
Expand Down Expand Up @@ -383,6 +416,7 @@ describe('role model', function() {
role[pluralName](function(err, models) {
assert(!err);
assert.equal(models.length, 1);

if (++runs === mappings.length) {
done();
}
Expand All @@ -404,6 +438,7 @@ describe('role model', function() {
assert.equal(users.length, 1);
assert.equal(users[0].id, user.id);
assert(User.find.calledWith(query));

done();
});
});
Expand Down