feat: respect field level permissions on bulk-update #120
Conversation
There was a problem hiding this comment.
Functionality seems got. But lets
- add a test for removing some properties
- merge the logic for the singe doc and the bulk docs request so they behave the same
- also add a check for posting attachments (
/{db}/{docId}/{property}) this should only work if the users has permissions for the property a attachment refers to.
A
| }; | ||
|
|
||
| jest.spyOn(mockRulesService, 'getRulesForUser').mockReturnValue([ | ||
| { action: 'delete', subject: 'Child', fields: ['name'] }, |
There was a problem hiding this comment.
I am not sure whether this kind of rule makes sense for us to support. I am not even sure how I should understand the rule here: Does it mean I am allowed to delete the name property or does it mean that I am allowed to delete the entity and change the name property?. How we (or PouchDB) currently delete is by only sending a doc with the _deleted property set without the remaining properties (see screenshot below). So what you are describing here is more like a update and delete. I think it would make sense that for delete rule we do not implement any field level logic but only use this for read and update (and also create?) rules.
| // User deletes a document, permissions can't be checked directly | ||
| const deletedPublicSchool: DatabaseDocument = { | ||
| _id: publicSchool._id, | ||
| _rev: publicSchool._rev, | ||
| _revisions: publicSchool._revisions, | ||
| _deleted: true, | ||
| anotherProperty: 'anotherValue', | ||
| privateSchool: false, |
There was a problem hiding this comment.
We explicitly implemented the _delete endpoint so it does not keep the other properties to support GDPR regulations. The document needs to be kept for synchronisation reasons but all other info on the doc should be removed.
There was a problem hiding this comment.
I was not aware of this regulation, thanks.
| updatedDoc[fieldKeys[i]] === null | ||
| ) { | ||
| delete existingDoc.doc[fieldKeys[i]]; | ||
| delete updatedDoc[fieldKeys[i]]; |
There was a problem hiding this comment.
Can you add a test for this behavior?
| return Object.assign(existingDoc ? existingDoc.doc : {}, updatedDoc); | ||
| } | ||
|
|
||
| private removeFieldsWithoutPermissions( |
There was a problem hiding this comment.
Can we move this logic into a separate service so that it can be used by all endpoints that send changes to the CouchDB?
| if (existingDoc) { | ||
| if (updatedDoc._deleted) { | ||
| existingDoc.doc._deleted = true; | ||
| return existingDoc.doc; |
There was a problem hiding this comment.
Let's make sure to delete all properties on the doc too (maybe keeping updated and created)
| for (let i = 0; i < fieldKeys.length; i++) { | ||
| if (!ability.can(action, updatedDoc, fieldKeys[i])) { | ||
| delete updatedDoc[fieldKeys[i]]; | ||
| } | ||
| } |
There was a problem hiding this comment.
| for (let i = 0; i < fieldKeys.length; i++) { | |
| if (!ability.can(action, updatedDoc, fieldKeys[i])) { | |
| delete updatedDoc[fieldKeys[i]]; | |
| } | |
| } | |
| for (const key of fieldKeys) { | |
| if (!ability.can(action, updatedDoc, key)) { | |
| delete updatedDoc[key]; | |
| } | |
| } |
There was a problem hiding this comment.
Actually, any reason why you did not got with the approach we already implemented in the applyPermissions function inside the DocumentController? Using permittedFieldsOf and pick the code seems much shorter and clearer.
There was a problem hiding this comment.
Why is this function in a Controller? Did not saw it there.
There was a problem hiding this comment.
I think I showed it to you initially when we talked about the requirements for this feature. It was only in the controller because we did not yet use it in other places.
| action = 'create'; | ||
| } | ||
|
|
||
| const fieldKeys = this.getCustomFieldKeys(updatedDoc); |
There was a problem hiding this comment.
I think using the CASL function permittedFieldsOf and combining the results with the DEFAULT_FIELDS would make this a bit more explicit.
| updatedDoc: DatabaseDocument, | ||
| existingDoc: DocMetaInf, | ||
| ) { | ||
| const fieldKeys = this.getCustomFieldKeys(updatedDoc); |
There was a problem hiding this comment.
Its a bit weird that you have to call this again. You could just pass the values from the other function.
| }; | ||
| } | ||
|
|
||
| private deleteEmptyValues( |
There was a problem hiding this comment.
For the review and later following the code I think it would help to sort the functions in a file according to their importance and call order. Having this function before removeFieldsWithoutPermissions doesn't really make sense.
sleidig
added
the
Status: Core Team Priority
tomwwinter
force-pushed
the
master
branch
3 times, most recently
from
660be7f
to
189badc
Compare
related: Aam-Digital/ndb-core#1912