From d3d45abd352a6b9609d732c24a718fef83b076c7 Mon Sep 17 00:00:00 2001
From: Jen Jones Arnesen <jennifer@dhis2.org>
Date: Thu, 5 Oct 2023 11:56:30 +0200
Subject: [PATCH] fix: handle both Set and array for currentUser.authorities
 when checking interpretations access (DHIS2-15964)

---
 .../InterpretationModal.js                    |   2 +-
 .../InterpretationThread.js                   |  84 +++---
 .../Interpretations/common/Message/Message.js |   1 +
 .../__tests__/getInterpretationAccess.spec.js | 243 +++++++++++++++++-
 .../common/getInterpretationAccess.js         |  35 ++-
 ...IfNumber.js => addToTotalIfNumber.spec.js} |   0
 yarn.lock                                     |  41 +--
 7 files changed, 311 insertions(+), 95 deletions(-)
 rename src/modules/pivotTable/__tests__/{addToTotalIfNumber.js => addToTotalIfNumber.spec.js} (100%)

diff --git a/src/components/Interpretations/InterpretationModal/InterpretationModal.js b/src/components/Interpretations/InterpretationModal/InterpretationModal.js
index 6853ed4ed..5cc5050bd 100644
--- 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/InterpretationModal/InterpretationThread.js b/src/components/Interpretations/InterpretationModal/InterpretationThread.js
index ec4a8b624..03686cd7b 100644
--- a/src/components/Interpretations/InterpretationModal/InterpretationThread.js
+++ b/src/components/Interpretations/InterpretationModal/InterpretationThread.js
@@ -35,57 +35,47 @@ const InterpretationThread = ({
 
     return (
         <div className={cx('container', { fetching })}>
-            <div className={'scrollbox'}>
-                <div className={'title'}>
-                    <IconClock16 color={colors.grey700} />
-                    {moment(fromServerDate(interpretation.created)).format(
-                        'LLL'
-                    )}
-                </div>
-                {DownloadMenu && (
-                    <DownloadMenu relativePeriodDate={interpretation.created} />
-                )}
-                <div className={'thread'}>
-                    <Interpretation
-                        currentUser={currentUser}
-                        interpretation={interpretation}
-                        onReplyIconClick={
-                            interpretationAccess.comment
-                                ? () => focusRef.current?.focus()
-                                : null
-                        }
-                        onUpdated={() => onThreadUpdated(true)}
-                        onDeleted={onInterpretationDeleted}
-                        isInThread={true}
-                    />
-                    <div className={'comments'}>
-                        {interpretation.comments.map((comment) => (
-                            <Comment
-                                key={comment.id}
-                                comment={comment}
-                                currentUser={currentUser}
-                                interpretationId={interpretation.id}
-                                onThreadUpdated={onThreadUpdated}
-                                canComment={interpretationAccess.comment}
-                            />
-                        ))}
-                    </div>
-                    {interpretationAccess.comment && (
-                        <CommentAddForm
+            <div className={'title'}>
+                <IconClock16 color={colors.grey700} />
+                {moment(fromServerDate(interpretation.created)).format('LLL')}
+            </div>
+            {DownloadMenu && (
+                <DownloadMenu relativePeriodDate={interpretation.created} />
+            )}
+            <div className={'thread'}>
+                <Interpretation
+                    currentUser={currentUser}
+                    interpretation={interpretation}
+                    onReplyIconClick={
+                        interpretationAccess.comment
+                            ? () => focusRef.current?.focus()
+                            : null
+                    }
+                    onUpdated={() => onThreadUpdated(true)}
+                    onDeleted={onInterpretationDeleted}
+                    isInThread={true}
+                />
+                <div className={'comments'}>
+                    {interpretation.comments.map((comment) => (
+                        <Comment
+                            key={comment.id}
+                            comment={comment}
                             currentUser={currentUser}
                             interpretationId={interpretation.id}
-                            onSave={() => onThreadUpdated(true)}
-                            focusRef={focusRef}
+                            onThreadUpdated={onThreadUpdated}
+                            canComment={interpretationAccess.comment}
                         />
-                    )}
+                    ))}
                 </div>
             </div>
-            <CommentAddForm
-                currentUser={currentUser}
-                interpretationId={interpretation.id}
-                onSave={() => onThreadUpdated(true)}
-                focusRef={focusRef}
-            />
+            {interpretationAccess.comment && (
+                <CommentAddForm
+                    currentUser={currentUser}
+                    interpretationId={interpretation.id}
+                    onSave={() => onThreadUpdated(true)}
+                    focusRef={focusRef}
+                />
+            )}
             <style jsx>{`
                 .thread {
                     margin-top: var(--spacers-dp16);
@@ -95,7 +85,7 @@ const InterpretationThread = ({
 
                 .container {
                     position: relative;
-                    overflow: hidden;
+                    overflow: auto;
                     max-height: calc(100vh - 285px);
                     display: flex;
                     flex-direction: column;
diff --git a/src/components/Interpretations/common/Message/Message.js b/src/components/Interpretations/common/Message/Message.js
index a965d2965..50613ab2b 100644
--- a/src/components/Interpretations/common/Message/Message.js
+++ b/src/components/Interpretations/common/Message/Message.js
@@ -48,6 +48,7 @@ const Message = ({ children, text, created, username }) => {
                     font-size: 14px;
                     line-height: 19px;
                     color: ${colors.grey900};
+                    word-break: break-word;
                 }
 
                 .content :global(p:first-child) {
diff --git a/src/components/Interpretations/common/__tests__/getInterpretationAccess.spec.js b/src/components/Interpretations/common/__tests__/getInterpretationAccess.spec.js
index 45f75feff..3dc62633f 100644
--- 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
index d95e82af3..11123bc63 100644
--- a/src/components/Interpretations/common/getInterpretationAccess.js
+++ b/src/components/Interpretations/common/getInterpretationAccess.js
@@ -1,9 +1,33 @@
-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) || typeof authorities?.has === 'function')
+    ) {
+        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 +41,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,
diff --git a/src/modules/pivotTable/__tests__/addToTotalIfNumber.js b/src/modules/pivotTable/__tests__/addToTotalIfNumber.spec.js
similarity index 100%
rename from src/modules/pivotTable/__tests__/addToTotalIfNumber.js
rename to src/modules/pivotTable/__tests__/addToTotalIfNumber.spec.js
diff --git a/yarn.lock b/yarn.lock
index 5e6d9a3ef..08374ba9f 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -15261,14 +15261,7 @@ node-dir@^0.1.10:
   dependencies:
     minimatch "^3.0.2"
 
-node-fetch@^2.6.0:
-  version "2.6.11"
-  resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.11.tgz#cde7fc71deef3131ef80a738919f999e6edfff25"
-  integrity sha512-4I6pdBY1EthSqDmJkiNk3JIT8cswwR9nfeW/cPdUagJYEQG7R95WRH74wpz7ma8Gh/9dI9FP+OU+0E4FvtA55w==
-  dependencies:
-    whatwg-url "^5.0.0"
-
-node-fetch@^2.6.1, node-fetch@^2.6.7:
+node-fetch@^2.6.0, node-fetch@^2.6.1, node-fetch@^2.6.7:
   version "2.6.12"
   resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.12.tgz#02eb8e22074018e3d5a83016649d04df0e348fba"
   integrity sha512-C/fGU2E8ToujUivIO0H+tpQ6HWo4eEmchoPIoXtxCrVghxdKq+QOHqEZW7tuP3KlV3bC8FRMO5nMCC7Zm1VP6g==
@@ -21044,7 +21037,7 @@ webpack@4.44.2:
     watchpack "^1.7.4"
     webpack-sources "^1.4.1"
 
-"webpack@>=4.43.0 <6.0.0":
+"webpack@>=4.43.0 <6.0.0", webpack@^5.41.1:
   version "5.88.1"
   resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.88.1.tgz#21eba01e81bd5edff1968aea726e2fbfd557d3f8"
   integrity sha512-FROX3TxQnC/ox4N+3xQoWZzvGXSuscxR32rbzjpXgEzWudJFEJBpdlkkob2ylrv5yzzufD1zph1OoFsLtm6stQ==
@@ -21074,36 +21067,6 @@ webpack@4.44.2:
     watchpack "^2.4.0"
     webpack-sources "^3.2.3"
 
-webpack@^5.41.1:
-  version "5.88.0"
-  resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.88.0.tgz#a07aa2f8e7a64a8f1cec0c6c2e180e3cb34440c8"
-  integrity sha512-O3jDhG5e44qIBSi/P6KpcCcH7HD+nYIHVBhdWFxcLOcIGN8zGo5nqF3BjyNCxIh4p1vFdNnreZv2h2KkoAw3lw==
-  dependencies:
-    "@types/eslint-scope" "^3.7.3"
-    "@types/estree" "^1.0.0"
-    "@webassemblyjs/ast" "^1.11.5"
-    "@webassemblyjs/wasm-edit" "^1.11.5"
-    "@webassemblyjs/wasm-parser" "^1.11.5"
-    acorn "^8.7.1"
-    acorn-import-assertions "^1.9.0"
-    browserslist "^4.14.5"
-    chrome-trace-event "^1.0.2"
-    enhanced-resolve "^5.15.0"
-    es-module-lexer "^1.2.1"
-    eslint-scope "5.1.1"
-    events "^3.2.0"
-    glob-to-regexp "^0.4.1"
-    graceful-fs "^4.2.9"
-    json-parse-even-better-errors "^2.3.1"
-    loader-runner "^4.2.0"
-    mime-types "^2.1.27"
-    neo-async "^2.6.2"
-    schema-utils "^3.2.0"
-    tapable "^2.1.1"
-    terser-webpack-plugin "^5.3.7"
-    watchpack "^2.4.0"
-    webpack-sources "^3.2.3"
-
 websocket-driver@>=0.5.1, websocket-driver@^0.7.4:
   version "0.7.4"
   resolved "https://registry.yarnpkg.com/websocket-driver/-/websocket-driver-0.7.4.tgz#89ad5295bbf64b480abcba31e4953aca706f5760"