Skip to content
This repository was archived by the owner on Nov 11, 2025. It is now read-only.

Fixes #9 include all attributes when no roles are specified to get #11

Open
hendrul wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fixes #9 include all attributes when no roles are specified to get #11

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb79593
Fixes #9 include all attributes when no roles are specified to get
Jul 23, 2016
cbf3814
invert polarity from whitelisting to blacklisting and add support for…
Jul 24, 2016
8e8c575
documentation update
Jul 24, 2016
044a156
disable io.js tests
Jul 27, 2016
8e4508f
Added argument method to discern between get/set on internal function…
Jul 27, 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
20 changes: 11 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ User = sequelize.define('user', {
email: {
type: Sequelize.STRING,
roles: {
admin: {get: true},
self: true
default: 'client',
system: {get: true},
client: true
}
},
password: {
Expand All @@ -37,16 +38,17 @@ User = sequelize.define('user', {
rank: {
type: Sequelize.STRING,
roles: {
self: {set: false, get: true}
admin: true
default: 'client',
client: {set: false, get: true}
system: true
}
}
});

user.get(); // Will never include email or password
user.get({role: 'admin'}); // Will include email but not password
user.get({raw: true}); // Ignores roles, will include all
user.get(); // Applies default roles and include all the rest
user.get({role: 'system'}); // Will include email and rank but not password

user.set({rank: 'UBER'}, {role: 'self'||undefined}); // Will be ignored
user.set({rank: 'UBER'}, {role: 'admin'}); // Will be set
user.set({rank: 'UBER'}, {role: 'client'}); // Will be ignored
user.set({rank: 'UBER'}, {role: 'system'}); // Will be set
user.set({rank: 'UBER'}); // Will be ignored, default is client
```
52 changes: 34 additions & 18 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,44 @@ init = function(target) {
Object.keys(target.rawAttributes).forEach(function (attr) {
if (target.rawAttributes[attr].roles !== undefined) {
if (target.rawAttributes[attr].roles === false) {
target.rawAttributes[attr].roles = {};
return;
return;
}

Object.keys(target.rawAttributes[attr].roles).forEach(function (role) {
if (typeof target.rawAttributes[attr].roles[role] === "boolean") {
target.rawAttributes[attr].roles[role] = {
set: target.rawAttributes[attr].roles[role],
get: target.rawAttributes[attr].roles[role]
};
Object.keys(target.rawAttributes[attr].roles).forEach(function (roleName) {
var role = target.rawAttributes[attr].roles[roleName];

if (roleName === 'default') {
if(typeof role === "string"){
role = target.rawAttributes[attr].roles.default = target.rawAttributes[attr].roles[role];
}
}

if (typeof role === "boolean") {
target.rawAttributes[attr].roles[roleName] = { set: role, get: role };
return;
}

if (target.rawAttributes[attr].roles[role].set === undefined) {
target.rawAttributes[attr].roles[role].set = false;
if (role.set === undefined) {
role.set = true;
}
if (target.rawAttributes[attr].roles[role].get === undefined) {
target.rawAttributes[attr].roles[role].get = false;
if (role.get === undefined) {
role.get = true;
}
});
}
});

var accessGranted = function(attr, options){
Copy link
Copy Markdown
Owner

@mickhansen mickhansen Jul 26, 2016

Choose a reason for hiding this comment

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

accessGrantedForGet or accept a method argument

return !attr
// If no role is given apply default if any
|| !options.role && (typeof attr.roles !== 'object' || (attr.roles.default||{}).get !== false)
// If no roles defined in attribute or set to true
|| attr.roles === undefined || attr.roles === true
// If role is given but not defined in attribute apply default if any
|| options.role && (typeof attr.roles === 'object' && attr.roles[options.role] === undefined && (attr.roles.default||{}).get !== false)
Copy link
Copy Markdown
Owner

@mickhansen mickhansen Jul 26, 2016

Choose a reason for hiding this comment

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

I think this case should rather throw, i don't like silently failing.

Copy link
Copy Markdown
Author

@hendrul hendrul Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean by silently failing?. Not returning boolean but rising an error instead?

Copy link
Copy Markdown
Owner

@mickhansen mickhansen Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@i don't think that applying the default role for a "bad" role is a good default.

Copy link
Copy Markdown
Author

@hendrul hendrul Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean that if I try to save some attribute defaulted to set: false, I'll get an error?. In my particular case the client could send all attributes and I need just omit those protected ones. I think throwing is obtrusive (at least in my case) I would be forced to enclose every operation in try-catch block when I just need to be careful when assigning roles. Maybe this should be configurable don't you think?

Copy link
Copy Markdown
Owner

@mickhansen mickhansen Jul 28, 2016

Choose a reason for hiding this comment

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

We should avoid configuration on call-site at all costs in favour of reasonable convention.

What i mean is that if you call get('attr', {role: 'roleThatDoesNotExist'}) it should throw, not default to using the default role.

// Apply given role if defined in attribute
|| (attr.roles[options.role]||{}).get === true;
};

target.Instance.prototype.get = function(key, options) {
if (typeof key === "object" && !options) {
Expand All @@ -43,12 +59,11 @@ init = function(target) {
options = {};
}

if (options.raw === true) {
return $get.call(this, key, options);
}
if (key !== undefined) {
var attr = target.rawAttributes[key];
if (!attr || !attr.roles || attr.roles && attr.roles[options.role] && attr.roles[options.role].get) {

if (accessGranted(attr, options)) {

return $get.call(this, key, options);
} else {
return undefined;
Expand All @@ -57,15 +72,16 @@ init = function(target) {

var values = $get.call(this, options)
, response = {};

Object.keys(values).forEach(function (key) {
var attr = target.rawAttributes[key];

var val = values[key];
if(!attr && val instanceof Sequelize.Instance) {
response[key] = val.get.call(val, options);
}
else if (!attr || !attr.roles || attr.roles && attr.roles[options.role] && attr.roles[options.role].get) {
else if ( accessGranted(attr, options) ) {

response[key] = val;
}
});
Expand Down
Loading