Skip to content
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

Jump to bottom

feat: respect field level permissions on bulk-update #120

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

feat: respect field level permissions on bulk-update #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f83e6d1
feat: respect field level permissions on bulk-update
tomwwinter Jan 17, 2024
faa6c41
feat: fix null pointer
tomwwinter Jan 18, 2024
22bc1bb
feat: add tests
tomwwinter Jan 18, 2024
9908d91
feat: add tests
tomwwinter Jan 18, 2024
0ac7d6e
refactor: extract function
tomwwinter Jan 18, 2024
e3b1851
Merge branch 'master' into tw/feat/1912-respect-field-level-permissions
TheSlimvReal Jan 22, 2024
879bea0
Merge branch 'master' into tw/feat/1912-respect-field-level-permissions
TheSlimvReal Jan 22, 2024
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
135 changes: 135 additions & 0 deletions src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,32 @@ describe('BulkDocumentService', () => {
});
});

it('should apply field permissions to CREATE operations in BulkDocs', async () => {
const request: BulkDocsRequest = {
new_edits: true,
docs: [Object.assign({}, childDoc), Object.assign(schoolDoc)],
};
jest.spyOn(mockRulesService, 'getRulesForUser').mockReturnValue([
{ action: 'create', subject: 'Child', fields: 'someProperty' },
{ action: ['read', 'update'], subject: 'School' },
]);
jest.spyOn(mockCouchDBService, 'post').mockReturnValue(of({ rows: [] }));

const result = await service.filterBulkDocsRequest(request, normalUser, '');

expect(result).toEqual({
new_edits: true,
docs: [
{
_id: 'Child:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
someProperty: 'someValue',
},
],
});
});

it('should apply permissions to UPDATE operations in BulkDocs', async () => {
const request: BulkDocsRequest = {
new_edits: false,
Expand All @@ -128,11 +154,64 @@ describe('BulkDocumentService', () => {
});
});

it('should apply field permissions to UPDATE operations in BulkDocs', async () => {
const request: BulkDocsRequest = {
new_edits: false,
docs: [
{
_id: 'Child:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
someProperty: 'newSomeValue',
otherProperty: 'newOtherValue',
},
{
_id: 'School:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
anotherProperty: 'newAnotherValue',
},
],
};

jest.spyOn(mockRulesService, 'getRulesForUser').mockReturnValue([
{ action: 'update', subject: 'Child', fields: 'someProperty' },
{ action: ['read', 'update'], subject: 'School' },
]);

jest
.spyOn(mockCouchDBService, 'post')
.mockReturnValue(of(createAllDocsResponse(childDoc, schoolDoc)));

const result = await service.filterBulkDocsRequest(request, normalUser, '');

expect(result).toEqual({
new_edits: false,
docs: [
{
_id: 'Child:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
someProperty: 'newSomeValue',
otherProperty: 'otherValue',
},
{
_id: 'School:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
anotherProperty: 'newAnotherValue',
},
],
});
});

it('should apply permissions to DELETE operations in BulkDocs', async () => {
const deletedChildDoc = getChildDoc();
deletedChildDoc._deleted = true;

const deletedSchoolDoc = getSchoolDoc();
deletedSchoolDoc._deleted = true;

const request: BulkDocsRequest = {
new_edits: false,
docs: [deletedChildDoc, deletedSchoolDoc],
Expand All @@ -153,12 +232,61 @@ describe('BulkDocumentService', () => {
});
});

it('should apply field permissions to DELETE operations in BulkDocs', async () => {
const request: BulkDocsRequest = {
new_edits: false,
docs: [
{
_id: 'Child:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
_deleted: true,
someProperty: 'newSomeValue',
otherProperty: 'otherValue',
},
{
_id: 'School:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
_deleted: true,
anotherProperty: 'newAnotherValue',
},
],
};

jest.spyOn(mockRulesService, 'getRulesForUser').mockReturnValue([
{ action: 'delete', subject: 'Child', fields: ['name'] },
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Screenshot 2024-01-22 at 10 39 42

{ action: ['read', 'update'], subject: 'School' },
]);
jest
.spyOn(mockCouchDBService, 'post')
.mockReturnValue(of(createAllDocsResponse(childDoc, schoolDoc)));

const result = await service.filterBulkDocsRequest(request, normalUser, '');

expect(result).toEqual({
new_edits: false,
docs: [
{
_id: 'Child:1',
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
_deleted: true,
someProperty: 'someValue',
otherProperty: 'otherValue',
},
],
});
});

it('should check the permissions on the document from the database', async () => {
const privateSchool = getSchoolDoc();
privateSchool.privateSchool = true;

const publicSchool = getSchoolDoc();
publicSchool._id = 'School:2';
publicSchool.privateSchool = false;

jest.spyOn(mockRulesService, 'getRulesForUser').mockReturnValue([
{ action: 'update', subject: 'Child' },
{
Expand All @@ -167,20 +295,26 @@ describe('BulkDocumentService', () => {
conditions: { privateSchool: false }, // User is only allowed to update/delete public schools
},
]);

jest
.spyOn(mockCouchDBService, 'post')
.mockReturnValue(of(createAllDocsResponse(privateSchool, publicSchool)));

// User makes change to a document on which no permissions are given
const updatedPrivateSchool = getSchoolDoc();
updatedPrivateSchool.privateSchool = false;
updatedPrivateSchool.name = 'Not so Private School';

// 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of this regulation, thanks.

};

const request: BulkDocsRequest = {
new_edits: false,
docs: [updatedPrivateSchool, deletedPublicSchool],
Expand Down Expand Up @@ -209,6 +343,7 @@ describe('BulkDocumentService', () => {
_rev: 'someRev',
_revisions: { start: 1, ids: ['someRev'] },
someProperty: 'someValue',
otherProperty: 'otherValue',
};
}

Expand Down
84 changes: 78 additions & 6 deletions src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ import { CouchdbService } from '../../../couchdb/couchdb.service';
*/
@Injectable()
export class BulkDocumentService {
private DEFAULT_FIELDS = [
'_id',
'_rev',
'_revisions',
'_deleted',
'updated',
'created',
];

constructor(
private permissionService: PermissionService,
private couchdbService: CouchdbService,
Expand Down Expand Up @@ -96,16 +105,79 @@ export class BulkDocumentService {
);
return {
new_edits: request.new_edits,
docs: request.docs.filter((doc) =>
this.hasPermissionsForDoc(
doc,
response.rows.find((responseDoc) => responseDoc.id === doc._id),
ability,
docs: request.docs
.filter((doc) =>
this.hasPermissionsForDoc(
doc,
response.rows.find((responseDoc) => responseDoc.id === doc._id),
ability,
),
)
.map((doc) =>
this.removeFieldsWithoutPermissions(
doc,
response.rows.find((responseDoc) => responseDoc.id === doc._id),
ability,
),
),
),
};
}

private deleteEmptyValues(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

updatedDoc: DatabaseDocument,
existingDoc: DocMetaInf,
) {
const fieldKeys = this.getCustomFieldKeys(updatedDoc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Its a bit weird that you have to call this again. You could just pass the values from the other function.


for (let i = 0; i < fieldKeys.length; i++) {
if (
updatedDoc[fieldKeys[i]] === '' ||
updatedDoc[fieldKeys[i]] === undefined ||
updatedDoc[fieldKeys[i]] === null
) {
delete existingDoc.doc[fieldKeys[i]];
delete updatedDoc[fieldKeys[i]];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this behavior?

}
}

return Object.assign(existingDoc ? existingDoc.doc : {}, updatedDoc);
}

private removeFieldsWithoutPermissions(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this logic into a separate service so that it can be used by all endpoints that send changes to the CouchDB?

updatedDoc: DatabaseDocument,
existingDoc: DocMetaInf,
ability: DocumentAbility,
): DatabaseDocument {
let action: any;

if (existingDoc) {
if (updatedDoc._deleted) {
existingDoc.doc._deleted = true;
return existingDoc.doc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure to delete all properties on the doc too (maybe keeping updated and created)

} else {
action = 'update';
}
} else {
action = 'create';
}

const fieldKeys = this.getCustomFieldKeys(updatedDoc);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using the CASL function permittedFieldsOf and combining the results with the DEFAULT_FIELDS would make this a bit more explicit.


for (let i = 0; i < fieldKeys.length; i++) {
if (!ability.can(action, updatedDoc, fieldKeys[i])) {
delete updatedDoc[fieldKeys[i]];
}
}
Comment on lines +166 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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];
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this function in a Controller? Did not saw it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return this.deleteEmptyValues(updatedDoc, existingDoc);
}

private getCustomFieldKeys(updatedDoc: DatabaseDocument) {
return Object.keys(updatedDoc).filter(
(key: string) => !this.DEFAULT_FIELDS.includes(key),
);
}

private hasPermissionsForDoc(
updatedDoc: DatabaseDocument,
existingDoc: DocMetaInf,
Expand Down