fix: handle both Set and array for currentUser.authorities when check…

…ing interpretations access (DHIS2-15964)
dhis2 · Oct 16, 2023 · 55aac63 · 55aac63
1 parent 014d43e
commit 55aac63
Show file tree

Hide file tree

Showing 3 changed files with 269 additions and 9 deletions.
diff --git a/src/components/Interpretations/InterpretationModal/InterpretationModal.js b/src/components/Interpretations/InterpretationModal/InterpretationModal.js
@@ -24,7 +24,7 @@ const modalCSS = css.resolve`
         max-width: calc(100vw - 128px) !important;
         max-height: calc(100vh - 128px) !important;
         width: auto !important;
-        height: calc(100vw - 128px) !important;
+        height: calc(100vh - 128px) !important;
         overflow-y: hidden;
     }
     aside.hidden {

diff --git a/src/components/Interpretations/common/__tests__/getInterpretationAccess.spec.js b/src/components/Interpretations/common/__tests__/getInterpretationAccess.spec.js
@@ -3,21 +3,36 @@ import {
     getCommentAccess,
 } from '../getInterpretationAccess.js'
 
-const superuser = {
+const superuserD2CurrentUser = {
     id: 'iamsuper',
     authorities: new Set(['ALL']),
 }
 
-const userJoe = {
+const userJoeD2CurrentUser = {
     id: 'johndoe',
     authorities: new Set(['Some']),
 }
 
-const userJane = {
+const userJaneD2CurrentUser = {
     id: 'jane',
     authorities: new Set(['Some']),
 }
 
+const superuser = {
+    id: 'iamsuper',
+    authorities: ['ALL'],
+}
+
+const userJoe = {
+    id: 'johndoe',
+    authorities: ['Some'],
+}
+
+const userJane = {
+    id: 'jane',
+    authorities: ['Some'],
+}
+
 describe('interpretation and comment access', () => {
     describe('getInterpretationAccess', () => {
         it('returns true for all accesses for superuser', () => {
@@ -113,6 +128,228 @@ describe('interpretation and comment access', () => {
                 delete: false,
             })
         })
+
+        it('throws an error for all accesses when no currentUser provided', () => {
+            const interpretation = {
+                access: {
+                    write: false,
+                    manage: false,
+                },
+                createdBy: userJane,
+            }
+
+            expect(() => getInterpretationAccess(interpretation)).toThrow(
+                '"hasAuthority" requires "authorities" to be an array or set of authorities (strings)'
+            )
+        })
+
+        it('throws an error when currentUser is missing authorities', () => {
+            const interpretation = {
+                access: {
+                    write: false,
+                    manage: false,
+                },
+                createdBy: userJane,
+            }
+
+            expect(() =>
+                getInterpretationAccess(interpretation, {
+                    id: 'usernoauthorties',
+                })
+            ).toThrow(
+                '"hasAuthority" requires "authorities" to be an array or set of authorities (strings)'
+            )
+        })
+    })
+
+    describe('getInterpretationAccess using D2.currentUser', () => {
+        it('returns true for all accesses for superuser', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                    manage: true,
+                },
+                createdBy: userJoeD2CurrentUser,
+            }
+
+            expect(
+                getInterpretationAccess(interpretation, superuserD2CurrentUser)
+            ).toMatchObject({
+                share: true,
+                comment: true,
+                edit: true,
+                delete: true,
+            })
+        })
+        it('returns true for all accesses for creator', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                    manage: true,
+                },
+                createdBy: userJoeD2CurrentUser,
+            }
+
+            expect(
+                getInterpretationAccess(interpretation, userJoeD2CurrentUser)
+            ).toMatchObject({
+                share: true,
+                comment: true,
+                edit: true,
+                delete: true,
+            })
+        })
+
+        it('returns false for edit/delete if user is not creator/superuser', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                    manage: true,
+                },
+                createdBy: userJaneD2CurrentUser,
+            }
+
+            expect(
+                getInterpretationAccess(interpretation, userJoeD2CurrentUser)
+            ).toMatchObject({
+                share: true,
+                comment: true,
+                edit: false,
+                delete: false,
+            })
+        })
+
+        it('returns false for comment/edit/delete if user is not creator/superuser and no write access', () => {
+            const interpretation = {
+                access: {
+                    write: false,
+                    manage: true,
+                },
+                createdBy: userJaneD2CurrentUser,
+            }
+
+            expect(
+                getInterpretationAccess(interpretation, userJoeD2CurrentUser)
+            ).toMatchObject({
+                share: true,
+                comment: false,
+                edit: false,
+                delete: false,
+            })
+        })
+
+        it('returns false for share/comment/edit/delete if user is not creator/superuser and no write or manage access', () => {
+            const interpretation = {
+                access: {
+                    write: false,
+                    manage: false,
+                },
+                createdBy: userJaneD2CurrentUser,
+            }
+
+            expect(
+                getInterpretationAccess(interpretation, userJoeD2CurrentUser)
+            ).toMatchObject({
+                share: false,
+                comment: false,
+                edit: false,
+                delete: false,
+            })
+        })
+    })
+
+    describe('getCommentAccess using D2.currentUser', () => {
+        it('returns true for all accesses for superuser', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                },
+            }
+
+            const comment = {
+                createdBy: userJoeD2CurrentUser,
+            }
+
+            expect(
+                getCommentAccess(
+                    comment,
+                    interpretation.access.write,
+                    superuserD2CurrentUser
+                )
+            ).toMatchObject({
+                edit: true,
+                delete: true,
+            })
+        })
+
+        it('returns true for all accesses for creator when interpretation has write access', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                },
+            }
+
+            const comment = {
+                createdBy: userJoeD2CurrentUser,
+            }
+
+            expect(
+                getCommentAccess(
+                    comment,
+                    interpretation.access.write,
+                    userJoeD2CurrentUser
+                )
+            ).toMatchObject({
+                edit: true,
+                delete: true,
+            })
+        })
+
+        it('returns true for edit and false for delete for creator when interpretation does not have write access', () => {
+            const interpretation = {
+                access: {
+                    write: false,
+                },
+            }
+
+            const comment = {
+                createdBy: userJoeD2CurrentUser,
+            }
+
+            expect(
+                getCommentAccess(
+                    comment,
+                    interpretation.access.write,
+                    userJoeD2CurrentUser
+                )
+            ).toMatchObject({
+                edit: true,
+                delete: false,
+            })
+        })
+
+        it('returns false for edit/delete for user who is not creator or superuser', () => {
+            const interpretation = {
+                access: {
+                    write: true,
+                },
+            }
+
+            const comment = {
+                createdBy: userJaneD2CurrentUser,
+            }
+
+            expect(
+                getCommentAccess(
+                    comment,
+                    interpretation.access.write,
+                    userJoeD2CurrentUser
+                )
+            ).toMatchObject({
+                edit: false,
+                delete: false,
+            })
+        })
     })
 
     describe('getCommentAccess', () => {

diff --git a/src/components/Interpretations/common/getInterpretationAccess.js b/src/components/Interpretations/common/getInterpretationAccess.js
@@ -1,9 +1,31 @@
-const isCreatorOrSuperuser = (object, currentUser) =>
-    object?.createdBy.id === currentUser?.id ||
-    currentUser?.authorities.has('ALL')
+// For backwards compatibility
+// accept both Set (from the old d2.currentUser object) and array
+const hasAuthority = (authorities, authority) => {
+    if (!authority || typeof authority !== 'string') {
+        throw new Error(
+            `"hasAuthority" requires "authority" to be a populated string but received ${authority}`
+        )
+    }
+    if (!(Array.isArray(authorities) || authorities instanceof Set)) {
+        throw new Error(
+            `"hasAuthority" requires "authorities" to be an array or set of authorities (strings)`
+        )
+    }
+
+    return Array.isArray(authorities)
+        ? authorities.includes(authority)
+        : authorities.has(authority)
+}
+
+const isSuperuser = (authorities) => hasAuthority(authorities, 'ALL')
+
+const isCreator = (object, currentUser) =>
+    object?.createdBy.id === currentUser?.id
 
 export const getInterpretationAccess = (interpretation, currentUser) => {
-    const canEditDelete = isCreatorOrSuperuser(interpretation, currentUser)
+    const canEditDelete =
+        isCreator(interpretation, currentUser) ||
+        isSuperuser(currentUser?.authorities)
     return {
         share: interpretation.access.manage,
         comment: interpretation.access.write,
@@ -17,7 +39,8 @@ export const getCommentAccess = (
     hasInterpretationReplyAccess,
     currentUser
 ) => {
-    const canEditDelete = isCreatorOrSuperuser(comment, currentUser)
+    const canEditDelete =
+        isCreator(comment, currentUser) || isSuperuser(currentUser?.authorities)
     return {
         edit: canEditDelete,
         delete: canEditDelete && hasInterpretationReplyAccess,