fix: abort search on path if cycle detected (#81)

Author: sjvans

CHANGELOG.md

@@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/).
 ### Fixed
 
 - Automatic personal data modification logging for data subject details with renamed keys
+- Data subject resolution in circular models
 
 ## Version 0.5.2 - 2023-12-08
 

lib/utils.js

@@ -1,18 +1,27 @@
 const WRITE = { CREATE: 1, UPDATE: 1, DELETE: 1 }
 
+const $hasPersonalData = Symbol('@cap-js/audit-logging:hasPersonalData')
+const $dataSubject = Symbol('@cap-js/audit-logging:dataSubject')
+const $parents = Symbol('@cap-js/audit-logging:parents')
+const $visitedUp = Symbol('@cap-js/audit-logging:visitedUp')
+const $visitedDown = Symbol('@cap-js/audit-logging:visitedDown')
+
 const hasPersonalData = entity => {
-  if (!entity['@PersonalData.EntitySemantics']) return
-  // default role to entity name
-  if (entity['@PersonalData.EntitySemantics'] === 'DataSubject' && !entity['@PersonalData.DataSubjectRole'])
-    entity['@PersonalData.DataSubjectRole'] = entity.name.match(/\w+/g).pop()
-  // prettier-ignore
-  const hasPersonalData = !!Object.values(entity.elements).some(element =>
-    element['@PersonalData.IsPotentiallyPersonal'] ||
-    element['@PersonalData.IsPotentiallySensitive'] ||
-    (element['@PersonalData.FieldSemantics'] && element['@PersonalData.FieldSemantics'] === 'DataSubjectID'))
-  // cache result
-  entity.own('_hasPersonalData', () => hasPersonalData)
-  return hasPersonalData
+  if (!entity.own($hasPersonalData)) {
+    if (!entity['@PersonalData.EntitySemantics']) entity.set($hasPersonalData, false)
+    else {
+      // default role to entity name
+      if (entity['@PersonalData.EntitySemantics'] === 'DataSubject' && !entity['@PersonalData.DataSubjectRole'])
+        entity['@PersonalData.DataSubjectRole'] = entity.name.match(/\w+/g).pop()
+      // prettier-ignore
+      const hasPersonalData = !!Object.values(entity.elements).some(element =>
+        element['@PersonalData.IsPotentiallyPersonal'] ||
+        element['@PersonalData.IsPotentiallySensitive'] ||
+        (element['@PersonalData.FieldSemantics'] && element['@PersonalData.FieldSemantics'] === 'DataSubjectID'))
+      entity.set($hasPersonalData, hasPersonalData)
+    }
+  }
+  return entity.own($hasPersonalData)
 }
 
 const getMapKeyForCurrentRequest = req => {
@@ -136,21 +145,25 @@ const _getDataSubjectIdQuery = ({ dataSubjectEntity, subs }, row, model) => {
 }
 
 const _getUps = (entity, model) => {
-  if (entity.own('__parents')) return entity.__parents
+  if (entity.own($parents)) return entity[$parents]
   const ups = []
   for (const def of Object.values(model.definitions)) {
     if (def.kind !== 'entity' || !def.associations) continue
     for (const element of Object.values(def.associations)) {
-      if (element.target !== entity.name || element._isBacklink) continue
-      if (element.name === 'SiblingEntity') continue
+      if (element.target !== entity.name || element._isBacklink || element.name === 'SiblingEntity') continue
       ups.push(element)
     }
   }
-  return entity.set('__parents', ups)
+  return entity.set($parents, ups)
 }
 
-const _getDataSubjectUp = (model, entity, prev, next, result) => {
+const _getDataSubjectUp = (root, model, entity, prev, next, result) => {
   for (const element of _getUps(entity, model)) {
+    // cycle detection
+    if (!element.own($visitedUp)) element.set($visitedUp, new Set())
+    if (element.own($visitedUp).has(root)) continue
+    element.own($visitedUp).add(root)
+
     const me = { entity, relative: element.parent, element }
     if (prev) prev.next = me
     if (element.parent['@PersonalData.EntitySemantics'] === 'DataSubject') {
@@ -159,38 +172,42 @@ const _getDataSubjectUp = (model, entity, prev, next, result) => {
       return result
     } else {
       // dfs is a must here
-      result = _getDataSubjectUp(model, element.parent, me, next || me, result)
+      result = _getDataSubjectUp(root, model, element.parent, me, next || me, result)
     }
   }
   return result
 }
 
-const _getDataSubjectDown = (entity, prev, next) => {
+const _getDataSubjectDown = (root, entity, prev, next) => {
   const associations = Object.values(entity.associations || {}).filter(e => !e._isBacklink)
+  // bfs makes more sense here -> check all own assocs first before going deeper
   for (const element of associations) {
     const me = { entity, relative: entity, element }
     if (element._target['@PersonalData.EntitySemantics'] === 'DataSubject') {
       if (prev) prev.next = me
       return { dataSubjectEntity: element._target, subs: [next || me] }
     }
   }
-  // bfs makes more sense here
   for (const element of associations) {
+    // cycle detection
+    if (!element.own($visitedDown)) element.set($visitedDown, new Set())
+    if (element.own($visitedDown).has(root)) continue
+    element.own($visitedDown).add(root)
+
     const me = { entity, relative: entity, element }
     if (prev) prev.next = me
-    const dataSubject = _getDataSubjectDown(element._target, me, next || me)
+    const dataSubject = _getDataSubjectDown(root, element._target, me, next || me)
     if (dataSubject) return dataSubject
   }
 }
 
 const getDataSubject = (entity, model) => {
-  const hash = '__dataSubject'
-  if (entity.own(hash)) return entity[hash]
+  if (entity.own($dataSubject)) return entity[$dataSubject]
   // entities with EntitySemantics 'DataSubjectDetails' or 'Other' must not necessarily
   // be always below or always above 'DataSubject' entity in CSN tree
-  let dataSubjectInfo = _getDataSubjectUp(model, entity)
-  if (!dataSubjectInfo) dataSubjectInfo = _getDataSubjectDown(entity)
-  return entity.set(hash, dataSubjectInfo)
+  let dataSubjectInfo = _getDataSubjectUp(entity.name, model, entity)
+  if (!dataSubjectInfo) dataSubjectInfo = _getDataSubjectDown(entity.name, entity)
+  return entity.set($dataSubject, dataSubjectInfo)
 }
 
 const _getDataSubjectsMap = req => {

test/personal-data/crud.test.js

@@ -2141,4 +2141,9 @@ describe('personal data audit logging in CRUD', () => {
       })
     })
   })
+
+  test('with cycles', async () => {
+    await POST('/crud-5/A', { text: 'foo' }, { auth: ALICE })
+    expect(_logs.length).toBe(1)
+  })
 })

test/personal-data/db/schema.cds

@@ -137,19 +137,49 @@ entity SubEntities {
       mainEntity : Association to MainEntities;
 }
 
-
 annotate MainEntities with @PersonalData: {
   EntitySemantics: 'DataSubject',
-  DataSubjectRole: 'MainEntity',
+  DataSubjectRole: 'MainEntity'
 } {
   ID   @PersonalData.FieldSemantics     : 'DataSubjectID';
   name @PersonalData.IsPotentiallyPersonal;
 }
 
-annotate SubEntities with @PersonalData  : {
-  EntitySemantics: 'DataSubjectDetails',
-  DataSubjectRole: 'MainEntity'
-} {
+annotate SubEntities with @PersonalData  : {EntitySemantics: 'DataSubjectDetails'} {
   mainEntity @PersonalData.FieldSemantics: 'DataSubjectID';
   name       @PersonalData.IsPotentiallyPersonal;
 }
+
+entity A {
+  key ID   : UUID;
+      text : String;
+      b    : Association to B;
+      c    : Association to C;
+}
+
+entity B {
+  key ID   : UUID;
+      text : String;
+      a    : Association to A;
+      c    : Association to C;
+}
+
+entity C {
+  key ID   : UUID;
+      text : String;
+}
+
+annotate A with @PersonalData      : {EntitySemantics: 'DataSubjectDetails'} {
+  c    @PersonalData.FieldSemantics: 'DataSubjectID';
+  text @PersonalData.IsPotentiallyPersonal;
+}
+
+annotate B with @PersonalData      : {EntitySemantics: 'DataSubjectDetails'} {
+  c    @PersonalData.FieldSemantics: 'DataSubjectID';
+  text @PersonalData.IsPotentiallyPersonal;
+}
+
+annotate C with @PersonalData      : {EntitySemantics: 'DataSubject'} {
+  ID   @PersonalData.FieldSemantics: 'DataSubjectID';
+  text @PersonalData.IsPotentiallyPersonal;
+}

test/personal-data/fiori.test.js

@@ -497,13 +497,13 @@ describe('personal data audit logging in Fiori', () => {
       // reset logs
       _logs = []
 
-      // draft data is never logged. however, the read of the active data is logged.
+      // draft data is never logged and since cds^7.6, we only read actives for which no draft exists -> now 0 logs
       response = await GET(
         '/fiori-1/Customers?$filter=(IsActiveEntity eq false or SiblingEntity/IsActiveEntity eq null)',
         { auth: ALICE }
       )
       expect(response).toMatchObject({ status: 200 })
-      expect(_logs.length).toBe(1)
+      expect(_logs.length).toBe(0)
 
       // reset logs
       _logs = []

test/personal-data/srv/crud-service.cds

@@ -198,3 +198,13 @@ service CRUD_4 {
     };
 
 }
+
+@path    : '/crud-5'
+@requires: 'admin'
+service CRUD_5 {
+
+  entity A                     as projection on db.A;
+  entity B                     as projection on db.B;
+  entity C                     as projection on db.C;
+
+}