Closed m4v15 closed 6 years ago
for "Who is allowed to edit what field permissions", can we decide if we want to do this and if so, give examples.
new middleware:
fieldPermissions([
{
minRole: roles.SUPER,
field: '_id'
},
{
minRole: roles.ADMIN,
field: 'owner'
}
])
probably better
fieldPermissions({
_id: roles.SUPER,
username: roles.ADMIN
})
I've spent/wasted a lot of time on this. I think we need to finalise how we want the middleware to look when implemented on the routes. I still haven't come to a solid conclusion. Thoughts on the following @m4v15 @des-des ?
minRole
and ownerPermitted
would be the first permissions 'barrier'fields
, if present, would be the second 'barrier'So we would have:
PUT
:
permissions({
minRole: ADMIN,
ownerPermitted: true
fields: {
_id: { minRole: SUPER, ownerPermitted: false }
username: { minRole: ADMIN, ownerPermitted: true }
name: { minRole: ADMIN, ownerPermitted: true }
}
})
DELETE
:
permissions({
minRole: SUPER
})
I think that looks good, and we could have a fixtures file with all of the fields objects for each table to tidy it up:
PUT
:
permissions({
minRole: [ADMIN, OWNER],
fields: UserFieldPermissions
})
@m4v15 @mattlub
Hey both approaches seem fine. One thing I would say is that the name ownerPermitted
does not make lots of sense. It is not so much a question of whether the owner is permitted, but a question of whether a non-owner is permitted?
The owner permitted flag is as a secondary check - so if the route is only for super, But then the owner is permitted if the owner is not super they are still allowed through
ah this makes sense, ill go back to the other pull and see if it makes sense to me
@mattlub @m4v15 okay I have approved the (#122). Thinking about the best api for this, I think best is,
{
authorisedRoles: [SUPER, OWNER]
}
{
authorisedRoles: [ADMIN]
}
(ie similar to @m4v15's proposal).
I get that you are both getting worn down so I will not block. I think:
So are saying
{
authorisedRoles: [SUPER, OWNER]
}
instead of
{
minRole: SUPER,
ownerPermitted: true
}
Ok so I've realised we haven't documented properly exactly the permissions API. I don't know if @mattlub can do it better than me but I will give it ago:
As of pull #122 the resource owner check is added to the initial rolePermissions
middleware as so:
{
minRole: SUPER,
ownerPermitted: true
}
Because of this the middleware name has been changed from rolePermissions
to the broader permissions
. I prefer this to having authorisedRoles: [SUPER, OWNER]
as owner isn't actually a role and doesn't quite fit into the role hierarchy of our spec.
Pull #146 builds on top of this, adding a second middle ware with similar API but to test the fields modified - each field has it's own object with minRole
and ownerPermitted
keys.
@des-des @m4v15 ok my latest commit changes it to use
authorizedRoles: [ ADMIN, OWNER ]
can't remember why that idea was dropped in the first place, but it's back now.
closing as has been implemented
relates #27