Merge pull request #1414 from UniversityOfHelsinkiCS/access-right-mayhem

Feedbacktarget multiple access right adaption
UniversityOfHelsinkiCS · Dec 12, 2024 · e665961 · e665961
2 parents 5f5f846 + d5fb03f
commit e665961
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 51 deletions.
diff --git a/src/client/pages/FeedbackTarget/FeedbackTargetContext.jsx b/src/client/pages/FeedbackTarget/FeedbackTargetContext.jsx
@@ -9,9 +9,9 @@ export const FeedbackTargetContextProvider = ({ id, isAdmin, organisation, feedb
     const orgAccess = organisation?.access
     const accessStatus = feedbackTarget?.accessStatus
 
-    const isResponsibleTeacher = accessStatus === 'RESPONSIBLE_TEACHER' || isAdmin
-    const isTeacher = accessStatus === 'TEACHER' || isResponsibleTeacher || isAdmin
-    const isStudent = accessStatus === 'STUDENT'
+    const isResponsibleTeacher = accessStatus.includes('RESPONSIBLE_TEACHER') || isAdmin
+    const isTeacher = accessStatus.includes('TEACHER') || isResponsibleTeacher || isAdmin
+    const isStudent = accessStatus.includes('STUDENT')
     const isOrganisationAdmin = orgAccess?.admin || isAdmin
     const isOrganisationReader = orgAccess?.read || isAdmin
 

diff --git a/src/client/pages/FeedbackTarget/tabs/FeedbackView/FeedbackView.jsx b/src/client/pages/FeedbackTarget/tabs/FeedbackView/FeedbackView.jsx
@@ -104,7 +104,7 @@ const FeedbackView = () => {
   const { language } = i18n
   const { accessStatus, opensAt, closesAt, feedback, continuousFeedbackEnabled } = feedbackTarget
 
-  const isOutsider = accessStatus === 'NONE'
+  const isOutsider = accessStatus.includes('NONE')
   const isEnded = feedbackTargetIsEnded(feedbackTarget)
   const isOpen = feedbackTargetIsOpen(feedbackTarget)
   const isOngoing = !isOpen && !isEnded

diff --git a/src/client/pages/GuestUser/GuestFeedbackTargetView.jsx b/src/client/pages/GuestUser/GuestFeedbackTargetView.jsx
@@ -89,7 +89,7 @@ const GuestFeedbackTargetView = () => {
   const isOpen = feedbackTargetIsOpen(feedbackTarget)
   const isEnded = feedbackTargetIsEnded(feedbackTarget)
   const isStarted = new Date() >= new Date(opensAt)
-  const isTeacher = accessStatus === 'TEACHER' || accessStatus === 'RESPONSIBLE_TEACHER'
+  const isTeacher = accessStatus.includes('TEACHER') || accessStatus.includes('RESPONSIBLE_TEACHER')
   const showFeedbacksTab = (isTeacher && isStarted) || feedback || isEnded
 
   const coursePeriod = getCoursePeriod(courseRealisation)

diff --git a/src/server/services/feedbackTargets/Access.js b/src/server/services/feedbackTargets/Access.js
@@ -65,7 +65,7 @@ const RIGHTS = {
     UPDATE_ORGANISATION_SURVEYS,
     ENABLE_TOKEN_ENROLMENT,
   ],
-  ORGANISATION_READ: [PUBLIC_FEEDBACKS, GIVE_CONTINUOUS_FEEDBACK, GIVE_FEEDBACK],
+  ORGANISATION_READ: [PUBLIC_FEEDBACKS, GIVE_CONTINUOUS_FEEDBACK],
   RESPONSIBLE_TEACHER: [
     UPDATE,
     UPDATE_RESPONSE,
@@ -91,7 +91,7 @@ Object.freeze(RIGHTS)
 /**
  * Checks whether given access status allows given action
  */
-const hasRight = (accessStatus, action) => (RIGHTS[accessStatus] ?? []).includes(action)
+const hasRight = (accessStatuses, action) => accessStatuses.some(accessStatus => RIGHTS[accessStatus].includes(action))
 
 class Access {
   constructor(accessStatus) {
@@ -172,21 +172,27 @@ class Access {
 
   // Role enum
 
-  static ADMIN = new Access('ADMIN')
+  static ADMIN = new Access(['ADMIN'])
 
-  static RESPONSIBLE_TEACHER = new Access('RESPONSIBLE_TEACHER')
+  static RESPONSIBLE_TEACHER = new Access(['RESPONSIBLE_TEACHER'])
 
-  static TEACHER = new Access('TEACHER')
+  static TEACHER = new Access(['TEACHER'])
 
-  static ORGANISATION_ADMIN = new Access('ORGANISATION_ADMIN')
+  static ORGANISATION_ADMIN = new Access(['ORGANISATION_ADMIN'])
 
-  static ORGANISATION_READ = new Access('ORGANISATION_READ')
+  static ORGANISATION_READ = new Access(['ORGANISATION_READ'])
 
-  static STUDENT = new Access('STUDENT')
+  static STUDENT = new Access(['STUDENT'])
 
-  static NONE = new Access('NONE')
+  static NONE = new Access(['NONE'])
 
-  static For(accessStatus) {
+  static mergeAccesses(accesses) {
+    const accessStatuses = accesses.map(a => a.accessStatus)
+
+    return new Access(accessStatuses.flat())
+  }
+
+  static For(accessStatuses) {
     return (
       [
         this.ADMIN,
@@ -195,7 +201,7 @@ class Access {
         this.ORGANISATION_ADMIN,
         this.ORGANISATION_READ,
         this.STUDENT,
-      ].find(a => a.accessStatus === accessStatus) ?? this.NONE
+      ].find(a => accessStatuses.includes(a.accessStatus)) ?? this.NONE
     )
   }
 

diff --git a/src/server/services/feedbackTargets/getAccess.js b/src/server/services/feedbackTargets/getAccess.js
@@ -1,51 +1,33 @@
 const { Access } = require('./Access')
 
 const getAccess = async ({ userFeedbackTarget, user, feedbackTarget }) => {
-  if (user.dataValues.isAdmin) return Access.ADMIN
+  const accesses = []
+
+  if (user.dataValues.isAdmin) {
+    accesses.push(Access.ADMIN)
+  }
 
   const accessStatus = userFeedbackTarget?.accessStatus
-  let uftAccess = null
   if (accessStatus) {
-    uftAccess = Access.For(accessStatus)
+    accesses.push(Access.For(accessStatus))
   }
 
-  // User not directly associated. Lets check if they have access through organisation
+  // User not directly associated. Let's check if they have access through organisation
   const organisationAccess = await user.getOrganisationAccessByCourseUnitId(feedbackTarget.courseUnitId)
-
-  let orgAccess = null
   if (organisationAccess) {
     if (organisationAccess.admin) {
-      orgAccess = Access.ORGANISATION_ADMIN
+      accesses.push(Access.ORGANISATION_ADMIN)
     } else if (organisationAccess.read) {
-      orgAccess = Access.ORGANISATION_READ
+      accesses.push(Access.ORGANISATION_READ)
     }
   }
 
-  // only direct access, return that
-  if (uftAccess !== null && orgAccess === null) {
-    return uftAccess
-  }
-  // access only through organisation, return that
-  if (uftAccess === null && orgAccess !== null) {
-    return orgAccess
-  }
-  // both direct access and access through organisation, return highest
-  if (uftAccess !== null && orgAccess !== null) {
-    if (orgAccess === Access.ORGANISATION_ADMIN) {
-      if (uftAccess === Access.ADMIN || uftAccess === Access.RESPONSIBLE_TEACHER) {
-        return uftAccess
-      }
-      return orgAccess
-    }
-    if (orgAccess === Access.ORGANISATION_READ) {
-      if (uftAccess === Access.STUDENT) {
-        return orgAccess
-      }
-      return uftAccess
-    }
+  if (accesses.length === 0) {
+    return Access.NONE
   }
 
-  return null
+  // Merge all accesses into one Access object
+  return Access.mergeAccesses(accesses)
 }
 
 module.exports = { getAccess }
diff --git a/src/server/services/feedbackTargets/getOneForUser.js b/src/server/services/feedbackTargets/getOneForUser.js
@@ -190,18 +190,19 @@ const getOneForUser = async (id, user) => {
     throw new ApplicationError('Not found', 404)
   }
 
-  const access = await getAccess({
+  const { accessStatus } = await getAccess({
     userFeedbackTarget,
     user,
     feedbackTarget,
   })
-  if (!access) {
+
+  if (accessStatus.includes('NONE')) {
     throw new ApplicationError('No access', 403)
   }
 
   return {
     ...additionalData,
-    accessStatus: access,
+    accessStatus,
     feedback: userFeedbackTarget?.feedback ?? null,
     groupIds: userFeedbackTarget?.groupIds ?? [],
     ...feedbackTarget.toJSON(),

diff --git a/src/server/util/sentry.ts b/src/server/util/sentry.ts
@@ -13,6 +13,6 @@ export const initializeSentry = () => {
     release: GIT_SHA,
     integrations: [Sentry.httpIntegration({ breadcrumbs: true }), Sentry.expressIntegration()],
     tracesSampleRate: 1.0,
-    ignoreErrors: ['No access', 'jwt expired', 'Not found', 'Forbidden'],
+    ignoreErrors: ['No access', 'jwt expired', 'Not found'],
   })
 }