fix: align viewer permissions for shared links (#3335)

* refactor(content-explorer): clarify viewer permission for shared links * fix: align viewer role behavior for Content Picker * refactor: update param types * refactor: remove FlowFixMe --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
box · May 31, 2023 · 5d78c11 · 5d78c11
1 parent b81e976
commit 5d78c11
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 56 deletions.
diff --git a/src/api/Item.js b/src/api/Item.js
@@ -20,6 +20,7 @@ import {
 } from '../constants';
 import type { ElementsErrorCallback, RequestData, RequestOptions } from '../common/types/api';
 import type {
+    Access,
     BoxItem,
     BoxItemPermission,
     FlattenedBoxItem,
@@ -165,7 +166,7 @@ class Item extends Base {
     /**
      * API to delete an Item
      *
-     * @param {Object} item - Item to delete
+     * @param {BoxItem} item - Item to delete
      * @param {Function} successCallback - Success callback
      * @param {Function} errorCallback - Error callback
      * @return {void}
@@ -221,7 +222,7 @@ class Item extends Base {
     /**
      * API to rename an Item
      *
-     * @param {Object} item - Item to rename
+     * @param {BoxItem} item - Item to rename
      * @param {string} name - Item new name
      * @param {Function} successCallback - Success callback
      * @param {Function} errorCallback - Error callback
@@ -286,40 +287,46 @@ class Item extends Base {
     };
 
     /**
-     * Validate an item update request
+     * Validate request to update an item shared link
      *
      * @param {string|void} itemID - ID of item to share
      * @param {BoxItemPermission|void} itemPermissions - Permissions for item
-     * @param {boolean|void} canSkipSetShareAccess - skip the check for can_set_share_access when creating a new shared link
+     * @param {boolean|void} canSkipSetShareAccessPermission - skip `can_set_share_access` permission check
      * @throws {Error}
      * @return {void}
      */
-    validateRequest(itemID: ?string, itemPermissions: ?BoxItemPermission, canSkipSetShareAccess: boolean = false) {
+    validateSharedLinkRequest(
+        itemID: ?string,
+        itemPermissions: ?BoxItemPermission,
+        canSkipSetShareAccessPermission: boolean = false,
+    ) {
         if (!itemID || !itemPermissions) {
             this.errorCode = ERROR_CODE_SHARE_ITEM;
             throw getBadItemError();
         }
 
+        // It is sometimes necessary to skip `can_set_share_access` permission check
+        // e.g. Viewer permission can create shared links but cannot update access level
         const { can_share, can_set_share_access }: BoxItemPermission = itemPermissions;
-        if (!can_share || (!canSkipSetShareAccess && !can_set_share_access)) {
+        if (!can_share || (!canSkipSetShareAccessPermission && !can_set_share_access)) {
             this.errorCode = ERROR_CODE_SHARE_ITEM;
             throw getBadPermissionsError();
         }
     }
 
     /**
-     * API to create, modify (change access) or remove a shared link
+     * API to create, modify (change access), or remove a shared link
      *
-     * @param {Object} item - Item to share
-     * @param {string} access - Shared access level
+     * @param {BoxItem} item - Item to share
+     * @param {Access} access - Shared access level
      * @param {Function} successCallback - Success callback
      * @param {Function|void} errorCallback - Error callback
      * @param {Array<string>|void} [options.fields] - Optionally include specific fields
      * @return {Promise<void>}
      */
     async share(
         item: BoxItem,
-        access: ?string, // if "access" is undefined, the backend will set the default access level for the shared link
+        access?: Access, // if "access" is undefined, the backend will set the default access level for the shared link
         successCallback: Function,
         errorCallback: ElementsErrorCallback = noop,
         options: RequestOptions = {},
@@ -329,14 +336,14 @@ class Item extends Base {
         }
 
         try {
-            const { id, permissions }: BoxItem = item;
+            const { id, permissions, shared_link: sharedLink }: BoxItem = item;
             this.id = id;
             this.successCallback = successCallback;
             this.errorCallback = errorCallback;
 
-            // if we use the default access level, we don't need permission to set the access level
-            const canSkipSetShareAccess = access === undefined;
-            this.validateRequest(id, permissions, canSkipSetShareAccess);
+            // skip permission check when creating links with default access level
+            const canSkipSetShareAccessPermission = !sharedLink && access === undefined;
+            this.validateSharedLinkRequest(id, permissions, canSkipSetShareAccessPermission);
 
             const { fields } = options;
             const requestData: RequestData = {
@@ -383,7 +390,7 @@ class Item extends Base {
             this.successCallback = successCallback;
             this.errorCallback = errorCallback;
 
-            this.validateRequest(id, permissions);
+            this.validateSharedLinkRequest(id, permissions);
 
             const { fields } = options;
             const requestData: RequestData = {

diff --git a/src/api/__tests__/Item.test.js b/src/api/__tests__/Item.test.js
@@ -301,7 +301,7 @@ describe('api/Item', () => {
         });
     });
 
-    describe('validateRequest()', () => {
+    describe('validateSharedLinkRequest()', () => {
         const MOCK_ITEM_ERROR = new Error('missing item data');
         const MOCK_PERMISSIONS_ERROR = new Error('missing permissions');
         const MOCK_SUFFICIENT_PERMISSIONS = {
@@ -319,7 +319,7 @@ describe('api/Item', () => {
             ${undefined}    | ${MOCK_SUFFICIENT_PERMISSIONS} | ${'itemID is missing'}
             ${MOCK_ITEM_ID} | ${undefined}                   | ${'itemPermissions is missing'}
         `('should throw a bad item error if $description', ({ itemID, itemPermissions }) => {
-            expect(() => item.validateRequest(itemID, itemPermissions)).toThrowError(MOCK_ITEM_ERROR);
+            expect(() => item.validateSharedLinkRequest(itemID, itemPermissions)).toThrowError(MOCK_ITEM_ERROR);
             expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM);
         });
 
@@ -329,24 +329,26 @@ describe('api/Item', () => {
             ${{ can_share: false, can_set_share_access: true }}  | ${'set share access but not share'}
             ${{ can_share: true, can_set_share_access: false }}  | ${'share but not set share access'}
         `('should throw a bad permissions error when the user can $description', ({ itemPermissions }) => {
-            expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions)).toThrowError(MOCK_PERMISSIONS_ERROR);
+            expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions)).toThrowError(
+                MOCK_PERMISSIONS_ERROR,
+            );
             expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM);
         });
 
         test('should not throw an error if the request is valid', () => {
-            expect(() => item.validateRequest(MOCK_ITEM_ID, MOCK_SUFFICIENT_PERMISSIONS)).not.toThrow();
+            expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, MOCK_SUFFICIENT_PERMISSIONS)).not.toThrow();
             expect(item.errorCode).toBeUndefined();
         });
 
-        test('should skip `can_set_share_access` check when canSkipSetShareAccess is true', () => {
+        test('should skip `can_set_share_access` check when canSkipSetShareAccessPermission is true', () => {
             const itemPermissions = { can_share: true, can_set_share_access: false };
-            expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions, true)).not.toThrow();
+            expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions, true)).not.toThrow();
             expect(item.errorCode).toBeUndefined();
         });
 
-        test('should not skip `can_set_share_access` check when canSkipSetShareAccess is false', () => {
+        test('should not skip `can_set_share_access` check when canSkipSetShareAccessPermission is false', () => {
             const itemPermissions = { can_share: true, can_set_share_access: false };
-            expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions, false)).toThrowError(
+            expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions, false)).toThrowError(
                 MOCK_PERMISSIONS_ERROR,
             );
             expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM);
@@ -357,7 +359,7 @@ describe('api/Item', () => {
         let shareSuccessHandlerSpy;
         let errorHandlerSpy;
         let getUrlSpy;
-        let validateRequestSpy;
+        let validateSharedLinkRequestSpy;
         const MOCK_DATA = { shared_link: '', permissions: {} };
         beforeEach(() => {
             file = {
@@ -370,7 +372,7 @@ describe('api/Item', () => {
             shareSuccessHandlerSpy = jest.spyOn(item, 'shareSuccessHandler');
             errorHandlerSpy = jest.spyOn(item, 'errorHandler');
             getUrlSpy = jest.spyOn(item, 'getUrl').mockReturnValue('url');
-            validateRequestSpy = jest.spyOn(item, 'validateRequest').mockReturnValue(null);
+            validateSharedLinkRequestSpy = jest.spyOn(item, 'validateSharedLinkRequest').mockReturnValue(null);
             jest.spyOn(item, 'getCache').mockImplementation(() => ({
                 get: jest.fn().mockReturnValue('success'),
                 has: jest.fn().mockReturnValue(false),
@@ -391,7 +393,7 @@ describe('api/Item', () => {
         describe('with missing data', () => {
             const MOCK_MISSING_DATA_ERROR = new Error('missing data');
             beforeEach(() => {
-                validateRequestSpy.mockImplementation(() => {
+                validateSharedLinkRequestSpy.mockImplementation(() => {
                     throw MOCK_MISSING_DATA_ERROR;
                 });
             });
@@ -503,7 +505,7 @@ describe('api/Item', () => {
             };
             shareSuccessHandlerSpy = jest.spyOn(item, 'shareSuccessHandler');
             getUrlSpy = jest.spyOn(item, 'getUrl').mockReturnValue('url');
-            jest.spyOn(item, 'validateRequest').mockReturnValue(null);
+            jest.spyOn(item, 'validateSharedLinkRequest').mockReturnValue(null);
             jest.spyOn(item, 'getCache').mockImplementation(() => ({
                 get: jest.fn().mockReturnValue('success'),
                 has: jest.fn().mockReturnValue(false),

diff --git a/src/elements/content-explorer/ContentExplorer.js b/src/elements/content-explorer/ContentExplorer.js
@@ -818,7 +818,7 @@ class ContentExplorer extends Component<Props, State> {
     };
 
     /**
-     * Chages the sort by and sort direction
+     * Changes the sort by and sort direction
      *
      * @private
      * @param {string} sortBy - field to sort by
@@ -1275,33 +1275,23 @@ class ContentExplorer extends Component<Props, State> {
      * @param {Object} item file or folder
      * @returns {void}
      */
-    handleSharedLinkSuccess = (newItem: BoxItem) => {
+    handleSharedLinkSuccess = (item: BoxItem) => {
         const { currentCollection } = this.state;
 
-        if (!newItem[FIELD_SHARED_LINK]) {
-            const { canSetShareAccess }: Props = this.props;
-            if (!newItem || !canSetShareAccess) {
-                return;
-            }
-
-            const { permissions, type } = newItem;
-            if (!permissions || !type) {
-                return;
-            }
-
+        if (item.type && !item[FIELD_SHARED_LINK]) {
             // create a shared link with default access, and update the collection
             const access = undefined;
-            this.api.getAPI(type).share(newItem, access, (updatedItem: BoxItem) => {
+            this.api.getAPI(item.type).share(item, access, (updatedItem: BoxItem) => {
                 this.updateCollection(currentCollection, updatedItem, () => this.setState({ isShareModalOpen: true }));
             });
         } else {
             // update collection with existing shared link
-            this.updateCollection(currentCollection, newItem, () => this.setState({ isShareModalOpen: true }));
+            this.updateCollection(currentCollection, item, () => this.setState({ isShareModalOpen: true }));
         }
     };
 
     /**
-     * Chages the sort by and sort direction
+     * Callback for sharing an item
      *
      * @private
      * @return {void}

diff --git a/src/elements/content-explorer/__tests__/ContentExplorer.test.js b/src/elements/content-explorer/__tests__/ContentExplorer.test.js
@@ -560,7 +560,7 @@ describe('elements/content-explorer/ContentExplorer', () => {
     });
 
     describe('handleSharedLinkSuccess()', () => {
-        const getApiShareMock = jest.fn().mockImplementation((newItem, access, callback) => callback());
+        const getApiShareMock = jest.fn().mockImplementation((item, access, callback) => callback());
         const getApiMock = jest.fn().mockReturnValue({ share: getApiShareMock });
         const updateCollectionMock = jest.fn();
 
@@ -581,8 +581,8 @@ describe('elements/content-explorer/ContentExplorer', () => {
         });
 
         afterEach(() => {
-            getApiShareMock.mockClear();
             getApiMock.mockClear();
+            getApiShareMock.mockClear();
             updateCollectionMock.mockClear();
         });
 

diff --git a/src/elements/content-picker/ContentPicker.js b/src/elements/content-picker/ContentPicker.js
@@ -9,7 +9,6 @@ import React, { Component } from 'react';
 import type { Node } from 'react';
 import classNames from 'classnames';
 import debounce from 'lodash/debounce';
-import getProp from 'lodash/get';
 import uniqueid from 'lodash/uniqueId';
 import noop from 'lodash/noop';
 import Header from '../common/header';
@@ -850,7 +849,6 @@ class ContentPicker extends Component<Props, State> {
         const existing: BoxItem = selected[cacheKey];
         const existingFromCache: BoxItem = this.api.getCache().get(cacheKey);
         const existInSelected = selectedKeys.indexOf(cacheKey) !== -1;
-        const itemCanSetShareAccess = getProp(item, 'permissions.can_set_share_access', false);
 
         // Existing object could have mutated and we just need to update the
         // reference in the selected map. In that case treat it like a new selection.
@@ -863,7 +861,7 @@ class ContentPicker extends Component<Props, State> {
             // We are selecting a new item that was never
             // selected before. However if we are in a single
             // item selection mode, we should also unselect any
-            // prior item that was item that was selected.
+            // prior item that was selected.
 
             // Check if we hit the selection limit and if selection
             // is not already currently in the selected data structure.
@@ -886,7 +884,7 @@ class ContentPicker extends Component<Props, State> {
             // If can set share access, fetch the shared link properties of the item
             // In the case where another item is selected, any in flight XHR will get
             // cancelled
-            if (canSetShareAccess && itemCanSetShareAccess && forceSharedLink) {
+            if (canSetShareAccess && forceSharedLink) {
                 this.fetchSharedLinkInfo(item);
             }
         }
@@ -939,8 +937,7 @@ class ContentPicker extends Component<Props, State> {
     handleSharedLinkSuccess = (item: BoxItem) => {
         // if no shared link currently exists, create a shared link with enterprise default
         if (!item[FIELD_SHARED_LINK]) {
-            // $FlowFixMe
-            this.changeShareAccess(null, item);
+            this.changeShareAccess(undefined, item);
         } else {
             const { selected } = this.state;
             const { id, type } = item;
@@ -962,7 +959,7 @@ class ContentPicker extends Component<Props, State> {
      * @param {Object} item file or folder object
      * @return {void}
      */
-    changeShareAccess = (access: Access, item: BoxItem): void => {
+    changeShareAccess = (access?: Access, item: BoxItem): void => {
         const { canSetShareAccess }: Props = this.props;
         if (!item || !canSetShareAccess) {
             return;
@@ -974,7 +971,7 @@ class ContentPicker extends Component<Props, State> {
         }
 
         const { can_set_share_access }: BoxItemPermission = permissions;
-        if (!can_set_share_access) {
+        if (access !== undefined && !can_set_share_access) {
             return;
         }
 
@@ -1005,7 +1002,7 @@ class ContentPicker extends Component<Props, State> {
     };
 
     /**
-     * Chages the sort by and sort direction
+     * Changes the sort by and sort direction
      *
      * @private
      * @param {string} sortBy - field to sorty by

diff --git a/src/elements/content-sharing/hooks/useSharedLink.js b/src/elements/content-sharing/hooks/useSharedLink.js
@@ -122,6 +122,7 @@ function useSharedLink(
         // Shared link access level change function
         const updatedChangeSharedLinkAccessLevelFn: SharedLinkUpdateLevelFnType = () => (newAccessLevel: string) =>
             connectToItemShare({
+                // $FlowFixMe
                 access: transformAccess(newAccessLevel),
                 successFn: data => {
                     currentAccessLevel.current = newAccessLevel;

diff --git a/src/elements/content-sharing/types.js b/src/elements/content-sharing/types.js
@@ -1,5 +1,6 @@
 // @flow
 import type {
+    Access,
     BoxItemClassification,
     BoxItemPermission,
     Collaboration,
@@ -137,7 +138,7 @@ export type GetContactsByEmailFnType = () => (filterTerm: {
 export type SendInvitesFnType = () => InviteCollaboratorsRequest => Promise<null | Array<Function>>;
 
 export type ConnectToItemShareFnType = ({
-    access?: string,
+    access?: Access,
     errorFn?: Function,
     requestOptions?: RequestOptions,
     successFn?: Function,

diff --git a/src/utils/fields.js b/src/utils/fields.js
@@ -174,7 +174,7 @@ const FILE_VERSIONS_FIELDS_TO_FETCH = [
     FIELD_VERSION_NUMBER,
 ];
 
-// Fields needed for Content Picker to show shared link permissions
+// Fields needed to show shared link permissions
 const FILE_SHARED_LINK_FIELDS_TO_FETCH = [FIELD_ALLOWED_SHARED_LINK_ACCESS_LEVELS, FIELD_SHARED_LINK];
 
 // Fields needed to get tasks data