From 77b718c22cd06059c0852a7bdde2d7b1acda3e05 Mon Sep 17 00:00:00 2001
From: Pranav <pranav@chatwoot.com>
Date: Wed, 21 Aug 2024 11:36:26 +0530
Subject: [PATCH] fix: Remove user.permissions, resolve it from accounts
 (#9990)

Remove the `user.permissions` field and resolve the permissions directly
from the accounts array in the user. This change ensures that the cache
or previous values from the last active account don't affect the
permissions.

In this PR:
- Remove user.permissions usage, replace it with getUserPermissions
method.
- Remove json.permissions from user.json.jbuilder
---
 .../dashboard/components/layout/Sidebar.vue   | 10 ++++++--
 .../layout/sidebarComponents/Secondary.vue    | 10 ++++++--
 .../dashboard/components/policy.vue           | 19 +++++++++++----
 .../dashboard/helper/permissionsHelper.js     |  9 ++++++++
 .../dashboard/helper/routeHelpers.js          | 15 ++++++------
 .../helper/specs/permissionsHelper.spec.js    | 23 +++++++++++++++++++
 .../helper/specs/routeHelpers.spec.js         |  8 -------
 app/javascript/dashboard/routes/index.spec.js | 20 ++++++++++++----
 app/views/api/v1/models/_user.json.jbuilder   |  1 -
 .../devise/session_controller_spec.rb         |  2 +-
 10 files changed, 87 insertions(+), 30 deletions(-)

diff --git a/app/javascript/dashboard/components/layout/Sidebar.vue b/app/javascript/dashboard/components/layout/Sidebar.vue
index 1e1dfac25..9a2f5b028 100644
--- a/app/javascript/dashboard/components/layout/Sidebar.vue
+++ b/app/javascript/dashboard/components/layout/Sidebar.vue
@@ -8,7 +8,10 @@ import { useRoute, useRouter } from 'dashboard/composables/route';
 import PrimarySidebar from './sidebarComponents/Primary.vue';
 import SecondarySidebar from './sidebarComponents/Secondary.vue';
 import { routesWithPermissions } from '../../routes';
-import { hasPermissions } from '../../helper/permissionsHelper';
+import {
+  getUserPermissions,
+  hasPermissions,
+} from '../../helper/permissionsHelper';
 
 export default {
   components: {
@@ -113,7 +116,10 @@ export default {
       return getSidebarItems(this.accountId);
     },
     primaryMenuItems() {
-      const userPermissions = this.currentUser.permissions;
+      const userPermissions = getUserPermissions(
+        this.currentUser,
+        this.accountId
+      );
       const menuItems = this.sideMenuConfig.primaryMenu;
       return menuItems.filter(menuItem => {
         const isAvailableForTheUser = hasPermissions(
diff --git a/app/javascript/dashboard/components/layout/sidebarComponents/Secondary.vue b/app/javascript/dashboard/components/layout/sidebarComponents/Secondary.vue
index 6de03af53..0f12b48c3 100644
--- a/app/javascript/dashboard/components/layout/sidebarComponents/Secondary.vue
+++ b/app/javascript/dashboard/components/layout/sidebarComponents/Secondary.vue
@@ -4,7 +4,10 @@ import SecondaryNavItem from './SecondaryNavItem.vue';
 import AccountContext from './AccountContext.vue';
 import { mapGetters } from 'vuex';
 import { FEATURE_FLAGS } from '../../../featureFlags';
-import { hasPermissions } from '../../../helper/permissionsHelper';
+import {
+  getUserPermissions,
+  hasPermissions,
+} from '../../../helper/permissionsHelper';
 import { routesWithPermissions } from '../../../routes';
 
 export default {
@@ -59,7 +62,10 @@ export default {
     accessibleMenuItems() {
       const menuItemsFilteredByPermissions = this.menuConfig.menuItems.filter(
         menuItem => {
-          const { permissions: userPermissions = [] } = this.currentUser;
+          const userPermissions = getUserPermissions(
+            this.currentUser,
+            this.accountId
+          );
           return hasPermissions(
             routesWithPermissions[menuItem.toStateName],
             userPermissions
diff --git a/app/javascript/dashboard/components/policy.vue b/app/javascript/dashboard/components/policy.vue
index f888de5a2..e9ad2de1f 100644
--- a/app/javascript/dashboard/components/policy.vue
+++ b/app/javascript/dashboard/components/policy.vue
@@ -1,7 +1,11 @@
 <script setup>
 import { useStoreGetters } from 'dashboard/composables/store';
 import { computed } from 'vue';
-import { hasPermissions } from '../helper/permissionsHelper';
+import {
+  getUserPermissions,
+  hasPermissions,
+} from '../helper/permissionsHelper';
+
 const props = defineProps({
   permissions: {
     type: Array,
@@ -10,12 +14,17 @@ const props = defineProps({
 });
 
 const getters = useStoreGetters();
-const user = getters.getCurrentUser.value;
-const hasPermission = computed(() =>
-  hasPermissions(props.permissions, user.permissions)
-);
+const user = computed(() => getters.getCurrentUser.value);
+const accountId = computed(() => getters.getCurrentAccountId.value);
+const userPermissions = computed(() => {
+  return getUserPermissions(user.value, accountId.value);
+});
+const hasPermission = computed(() => {
+  return hasPermissions(props.permissions, userPermissions.value);
+});
 </script>
 
+<!-- eslint-disable vue/no-root-v-if -->
 <template>
   <div v-if="hasPermission">
     <slot />
diff --git a/app/javascript/dashboard/helper/permissionsHelper.js b/app/javascript/dashboard/helper/permissionsHelper.js
index 135238371..156e5ef4f 100644
--- a/app/javascript/dashboard/helper/permissionsHelper.js
+++ b/app/javascript/dashboard/helper/permissionsHelper.js
@@ -7,6 +7,15 @@ export const hasPermissions = (
   );
 };
 
+export const getCurrentAccount = ({ accounts } = {}, accountId = null) => {
+  return accounts.find(account => Number(account.id) === Number(accountId));
+};
+
+export const getUserPermissions = (user, accountId) => {
+  const currentAccount = getCurrentAccount(user, accountId) || {};
+  return currentAccount.permissions || [];
+};
+
 const isPermissionsPresentInRoute = route =>
   route.meta && route.meta.permissions;
 
diff --git a/app/javascript/dashboard/helper/routeHelpers.js b/app/javascript/dashboard/helper/routeHelpers.js
index 6c4036dd1..0a7615472 100644
--- a/app/javascript/dashboard/helper/routeHelpers.js
+++ b/app/javascript/dashboard/helper/routeHelpers.js
@@ -1,9 +1,8 @@
-import { hasPermissions } from './permissionsHelper';
-
-// eslint-disable-next-line default-param-last
-export const getCurrentAccount = ({ accounts } = {}, accountId) => {
-  return accounts.find(account => account.id === accountId);
-};
+import {
+  hasPermissions,
+  getUserPermissions,
+  getCurrentAccount,
+} from './permissionsHelper';
 
 export const routeIsAccessibleFor = (route, userPermissions = []) => {
   const { meta: { permissions: routePermissions = [] } = {} } = route;
@@ -19,7 +18,9 @@ const validateActiveAccountRoutes = (to, user) => {
     return accountDashboardURL;
   }
 
-  const isAccessible = routeIsAccessibleFor(to, user.permissions);
+  const userPermissions = getUserPermissions(user, to.params.accountId);
+
+  const isAccessible = routeIsAccessibleFor(to, userPermissions);
   // If the route is not accessible for the user, return to dashboard screen
   return isAccessible ? null : accountDashboardURL;
 };
diff --git a/app/javascript/dashboard/helper/specs/permissionsHelper.spec.js b/app/javascript/dashboard/helper/specs/permissionsHelper.spec.js
index 34f434f7d..ed8511862 100644
--- a/app/javascript/dashboard/helper/specs/permissionsHelper.spec.js
+++ b/app/javascript/dashboard/helper/specs/permissionsHelper.spec.js
@@ -1,8 +1,31 @@
 import {
   buildPermissionsFromRouter,
+  getCurrentAccount,
+  getUserPermissions,
   hasPermissions,
 } from '../permissionsHelper';
 
+describe('#getCurrentAccount', () => {
+  it('should return the current account', () => {
+    expect(getCurrentAccount({ accounts: [{ id: 1 }] }, 1)).toEqual({ id: 1 });
+    expect(getCurrentAccount({ accounts: [] }, 1)).toEqual(undefined);
+  });
+});
+
+describe('#getUserPermissions', () => {
+  it('should return the correct permissions', () => {
+    const user = {
+      accounts: [
+        { id: 1, permissions: ['conversations_manage'] },
+        { id: 3, permissions: ['contacts_manage'] },
+      ],
+    };
+    expect(getUserPermissions(user, 1)).toEqual(['conversations_manage']);
+    expect(getUserPermissions(user, '3')).toEqual(['contacts_manage']);
+    expect(getUserPermissions(user, 2)).toEqual([]);
+  });
+});
+
 describe('hasPermissions', () => {
   it('returns true if permission is present', () => {
     expect(
diff --git a/app/javascript/dashboard/helper/specs/routeHelpers.spec.js b/app/javascript/dashboard/helper/specs/routeHelpers.spec.js
index 5aa9c4ee2..dc5a01de1 100644
--- a/app/javascript/dashboard/helper/specs/routeHelpers.spec.js
+++ b/app/javascript/dashboard/helper/specs/routeHelpers.spec.js
@@ -1,19 +1,11 @@
 import {
   getConversationDashboardRoute,
-  getCurrentAccount,
   isAConversationRoute,
   routeIsAccessibleFor,
   validateLoggedInRoutes,
   isAInboxViewRoute,
 } from '../routeHelpers';
 
-describe('#getCurrentAccount', () => {
-  it('should return the current account', () => {
-    expect(getCurrentAccount({ accounts: [{ id: 1 }] }, 1)).toEqual({ id: 1 });
-    expect(getCurrentAccount({ accounts: [] }, 1)).toEqual(undefined);
-  });
-});
-
 describe('#routeIsAccessibleFor', () => {
   it('should return the correct access', () => {
     let route = { meta: { permissions: ['administrator'] } };
diff --git a/app/javascript/dashboard/routes/index.spec.js b/app/javascript/dashboard/routes/index.spec.js
index a169f8b76..ac67e8f2e 100644
--- a/app/javascript/dashboard/routes/index.spec.js
+++ b/app/javascript/dashboard/routes/index.spec.js
@@ -34,8 +34,14 @@ describe('#validateAuthenticateRoutePermission', () => {
             getCurrentUser: {
               account_id: 1,
               id: 1,
-              permissions: ['agent'],
-              accounts: [{ id: 1, role: 'agent', status: 'active' }],
+              accounts: [
+                {
+                  permissions: ['agent'],
+                  id: 1,
+                  role: 'agent',
+                  status: 'active',
+                },
+              ],
             },
           };
           validateAuthenticateRoutePermission(to, next, { getters });
@@ -55,8 +61,14 @@ describe('#validateAuthenticateRoutePermission', () => {
             getCurrentUser: {
               account_id: 1,
               id: 1,
-              permissions: ['administrator'],
-              accounts: [{ id: 1, role: 'administrator', status: 'active' }],
+              accounts: [
+                {
+                  id: 1,
+                  role: 'administrator',
+                  permissions: ['administrator'],
+                  status: 'active',
+                },
+              ],
             },
           };
           validateAuthenticateRoutePermission(to, next, { getters });
diff --git a/app/views/api/v1/models/_user.json.jbuilder b/app/views/api/v1/models/_user.json.jbuilder
index 0e8c95adb..3d8131f60 100644
--- a/app/views/api/v1/models/_user.json.jbuilder
+++ b/app/views/api/v1/models/_user.json.jbuilder
@@ -14,7 +14,6 @@ json.provider resource.provider
 json.pubsub_token resource.pubsub_token
 json.custom_attributes resource.custom_attributes if resource.custom_attributes.present?
 json.role resource.active_account_user&.role
-json.permissions resource.active_account_user&.permissions
 json.ui_settings resource.ui_settings
 json.uid resource.uid
 json.type resource.type
diff --git a/spec/controllers/devise/session_controller_spec.rb b/spec/controllers/devise/session_controller_spec.rb
index ad6d76bf9..f9c2ad1fd 100644
--- a/spec/controllers/devise/session_controller_spec.rb
+++ b/spec/controllers/devise/session_controller_spec.rb
@@ -50,7 +50,7 @@ RSpec.describe 'Session', type: :request do
              as: :json
 
         expect(response).to have_http_status(:success)
-        expect(response.parsed_body['data']['permissions']).to eq(['agent'])
+        expect(response.parsed_body['data']['accounts'].first['permissions']).to eq(['agent'])
       end
     end