From f83e6d13e0952794fefec745beaa4bcb4593ad09 Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 00:40:04 +0100 Subject: [PATCH 1/5] feat: respect field level permissions on bulk-update --- .../bulk-document/bulk-document.service.ts | 52 ++++++++++++++++--- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts index 99c74e1..d16b95f 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts @@ -29,6 +29,8 @@ import { CouchdbService } from '../../../couchdb/couchdb.service'; */ @Injectable() export class BulkDocumentService { + private DEFAULT_FIELDS = ['_id', '_rev', '_revisions', 'updated', 'created']; + constructor( private permissionService: PermissionService, private couchdbService: CouchdbService, @@ -96,16 +98,54 @@ 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 removeFieldsWithoutPermissions( + updatedDoc: DatabaseDocument, + existingDoc: DocMetaInf, + ability: DocumentAbility, + ): DatabaseDocument { + let action: any; + + if (existingDoc) { + if (updatedDoc._deleted) { + return updatedDoc; + } else { + action = 'update'; + } + } else { + action = 'create'; + } + + const fieldKeys = Object.keys(updatedDoc).filter( + (key: string) => !this.DEFAULT_FIELDS.includes(key), + ); + + for (let i = 0; i < fieldKeys.length; i++) { + if (!ability.can(action, updatedDoc, fieldKeys[i])) { + delete updatedDoc[fieldKeys[i]]; + } + } + + return Object.assign(existingDoc.doc, updatedDoc); + } + private hasPermissionsForDoc( updatedDoc: DatabaseDocument, existingDoc: DocMetaInf, From faa6c41d4188701c25f78f714242ef0e171f367c Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 09:52:38 +0100 Subject: [PATCH 2/5] feat: fix null pointer --- .../replication/bulk-document/bulk-document.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts index d16b95f..654363a 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts @@ -143,7 +143,7 @@ export class BulkDocumentService { } } - return Object.assign(existingDoc.doc, updatedDoc); + return Object.assign(existingDoc ? existingDoc.doc : {}, updatedDoc); } private hasPermissionsForDoc( From 22bc1bb748d515136e0c9b1db06192b7783bdb6b Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 09:59:37 +0100 Subject: [PATCH 3/5] feat: add tests --- .../bulk-document.service.spec.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts index 535e2c2..eefabbe 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts @@ -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, @@ -209,6 +235,7 @@ describe('BulkDocumentService', () => { _rev: 'someRev', _revisions: { start: 1, ids: ['someRev'] }, someProperty: 'someValue', + otherProperty: 'otherValue', }; } From 9908d91dd067e33943cc43e0a76adddac40937cd Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 15:16:45 +0100 Subject: [PATCH 4/5] feat: add tests --- .../bulk-document.service.spec.ts | 108 ++++++++++++++++++ .../bulk-document/bulk-document.service.ts | 36 +++++- 2 files changed, 141 insertions(+), 3 deletions(-) diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts index eefabbe..3898900 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.ts @@ -154,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], @@ -179,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'] }, + { 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' }, { @@ -193,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, }; + const request: BulkDocsRequest = { new_edits: false, docs: [updatedPrivateSchool, deletedPublicSchool], diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts index 654363a..d0dfc5b 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts @@ -29,7 +29,14 @@ import { CouchdbService } from '../../../couchdb/couchdb.service'; */ @Injectable() export class BulkDocumentService { - private DEFAULT_FIELDS = ['_id', '_rev', '_revisions', 'updated', 'created']; + private DEFAULT_FIELDS = [ + '_id', + '_rev', + '_revisions', + '_deleted', + 'updated', + 'created', + ]; constructor( private permissionService: PermissionService, @@ -116,6 +123,28 @@ export class BulkDocumentService { }; } + private deleteEmptyValues( + updatedDoc: DatabaseDocument, + existingDoc: DocMetaInf, + ) { + const fieldKeys = Object.keys(updatedDoc).filter( + (key: string) => !this.DEFAULT_FIELDS.includes(key), + ); + + 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]]; + } + } + + return Object.assign(existingDoc ? existingDoc.doc : {}, updatedDoc); + } + private removeFieldsWithoutPermissions( updatedDoc: DatabaseDocument, existingDoc: DocMetaInf, @@ -125,7 +154,8 @@ export class BulkDocumentService { if (existingDoc) { if (updatedDoc._deleted) { - return updatedDoc; + existingDoc.doc._deleted = true; + return existingDoc.doc; } else { action = 'update'; } @@ -143,7 +173,7 @@ export class BulkDocumentService { } } - return Object.assign(existingDoc ? existingDoc.doc : {}, updatedDoc); + return this.deleteEmptyValues(updatedDoc, existingDoc); } private hasPermissionsForDoc( From 0ac7d6e4eb9b7387b139b936a561d99690de401e Mon Sep 17 00:00:00 2001 From: Tom Winter Date: Thu, 18 Jan 2024 15:20:01 +0100 Subject: [PATCH 5/5] refactor: extract function --- .../bulk-document/bulk-document.service.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts index d0dfc5b..c04afd0 100644 --- a/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts +++ b/src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts @@ -127,9 +127,7 @@ export class BulkDocumentService { updatedDoc: DatabaseDocument, existingDoc: DocMetaInf, ) { - const fieldKeys = Object.keys(updatedDoc).filter( - (key: string) => !this.DEFAULT_FIELDS.includes(key), - ); + const fieldKeys = this.getCustomFieldKeys(updatedDoc); for (let i = 0; i < fieldKeys.length; i++) { if ( @@ -163,9 +161,7 @@ export class BulkDocumentService { action = 'create'; } - const fieldKeys = Object.keys(updatedDoc).filter( - (key: string) => !this.DEFAULT_FIELDS.includes(key), - ); + const fieldKeys = this.getCustomFieldKeys(updatedDoc); for (let i = 0; i < fieldKeys.length; i++) { if (!ability.can(action, updatedDoc, fieldKeys[i])) { @@ -176,6 +172,12 @@ export class BulkDocumentService { 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,