Bookmarks and siteUtil hardening

js/about/bookmarks.js

@@ -34,15 +34,10 @@ class BookmarkFolderItem extends ImmutableComponent {
     e.preventDefault()
     e.dataTransfer.dropEffect = 'move'
   }
-
+  // NOTE: both folders and bookmarks can be dropped here
   onDrop (e) {
     const bookmark = dndData.getDragData(e.dataTransfer, dragTypes.BOOKMARK)
-
-    if (bookmark) {
-      // Don't allow a bookmark folder to be moved into itself
-      if (bookmark.get('folderId') === this.props.bookmarkFolder.get('folderId')) {
-        return
-      }
+    if (bookmark && siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, this.props.bookmarkFolder)) {
       aboutActions.moveSite(bookmark.toJS(), this.props.bookmarkFolder.toJS(), dndData.shouldPrependVerticalItem(e.target, e.clientY), true)
     }
   }
@@ -75,7 +70,9 @@ class BookmarkFolderItem extends ImmutableComponent {
       </div>
       {
         childBookmarkFolders.size > 0
-        ? <BookmarkFolderList onChangeSelectedFolder={this.props.onChangeSelectedFolder}
+        ? <BookmarkFolderList
+          search={this.props.search}
+          onChangeSelectedFolder={this.props.onChangeSelectedFolder}
           bookmarkFolders={childBookmarkFolders}
           selectedFolderId={this.props.selectedFolderId}
           allBookmarkFolders={this.props.allBookmarkFolders} />
@@ -98,7 +95,9 @@ class BookmarkFolderList extends ImmutableComponent {
       }
       {
         this.props.isRoot
-        ? <BookmarkFolderItem selected={!this.props.search && this.props.selectedFolderId === 0}
+        ? <BookmarkFolderItem
+          search={this.props.search}
+          selected={!this.props.search && this.props.selectedFolderId === 0}
           dataL10nId='bookmarksToolbar'
           draggable={false}
           onChangeSelectedFolder={this.props.onChangeSelectedFolder}
@@ -113,13 +112,16 @@ class BookmarkFolderList extends ImmutableComponent {
           ? null
           : <BookmarkFolderItem bookmarkFolder={bookmarkFolder}
             allBookmarkFolders={this.props.allBookmarkFolders}
+            search={this.props.search}
             selected={!this.props.search && this.props.selectedFolderId === bookmarkFolder.get('folderId')}
             selectedFolderId={this.props.selectedFolderId}
             onChangeSelectedFolder={this.props.onChangeSelectedFolder} />)
       }
       {
         this.props.isRoot
-        ? <BookmarkFolderItem selected={this.props.selectedFolderId === -1}
+        ? <BookmarkFolderItem
+          search={this.props.search}
+          selected={!this.props.search && this.props.selectedFolderId === -1}
           dataL10nId='otherBookmarks'
           draggable={false}
           onChangeSelectedFolder={this.props.onChangeSelectedFolder}
@@ -188,12 +190,24 @@ class BookmarksList extends ImmutableComponent {
   }
   onDrop (siteDetail, e) {
     const bookmark = dndData.getDragData(e.dataTransfer, dragTypes.BOOKMARK)
+    let destinationIsParent = false
     if (bookmark) {
-      aboutActions.moveSite(bookmark.toJS(), siteDetail.toJS(), dndData.shouldPrependVerticalItem(e.target, e.clientY), false)
+      // If source is folder, destination needs to be a folder too
+      if (siteUtil.isFolder(bookmark)) {
+        siteDetail = siteDetail.get('parentFolderId')
+          ? this.props.allBookmarkFolders.find((folder) => folder.get('folderId') === siteDetail.get('parentFolderId'))
+          : Immutable.fromJS({folderId: 0, tags: [siteTags.BOOKMARK_FOLDER]})
+        destinationIsParent = true
+      }
+
+      if (siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, siteDetail)) {
+        aboutActions.moveSite(bookmark.toJS(), siteDetail.toJS(), dndData.shouldPrependVerticalItem(e.target, e.clientY), destinationIsParent)
+      }
     }
   }
   render () {
     const props = !this.props.draggable ? {
+      onDragStart: this.onDragStart,
       sortingDisabled: !this.props.sortable
     } : {
       onDoubleClick: this.onDoubleClick,
@@ -223,6 +237,7 @@ class BookmarksList extends ImmutableComponent {
         onDoubleClick={this.onDoubleClick}
         {...props}
         contextMenuName='bookmark'
+        thisArg={this}
         onContextMenu={aboutActions.contextMenu} />
     </div>
   }
@@ -315,6 +330,7 @@ class AboutBookmarks extends React.Component {
               ? this.searchedBookmarks(this.state.search, this.state.bookmarks)
               : this.bookmarksInFolder
             }
+            allBookmarkFolders={this.state.bookmarkFolders}
             sortable={false}
             draggable={!this.state.search}
             tableID={this.selectedFolderId} />

js/components/sortableTable.js

@@ -143,22 +143,23 @@ class SortableTable extends ImmutableComponent {
       rowAttributes.onContextMenu = this.props.onContextMenu.bind(this, handlerInput, this.props.contextMenuName)
     }
     // Bindings for row-specific event handlers
+    const thisArg = this.props.thisArg || this
     if (typeof this.props.onClick === 'function') {
-      rowAttributes.onClick = this.props.onClick.bind(this, handlerInput)
+      rowAttributes.onClick = this.props.onClick.bind(thisArg, handlerInput)
     }
     if (typeof this.props.onDoubleClick === 'function') {
-      rowAttributes.onDoubleClick = this.props.onDoubleClick.bind(this, handlerInput)
+      rowAttributes.onDoubleClick = this.props.onDoubleClick.bind(thisArg, handlerInput)
     }
     if (typeof this.props.onDragStart === 'function') {
-      rowAttributes.onDragStart = this.props.onDragStart.bind(this, Immutable.fromJS(handlerInput))
+      rowAttributes.onDragStart = this.props.onDragStart.bind(thisArg, Immutable.fromJS(handlerInput))
       rowAttributes.draggable = true
     }
     if (typeof this.props.onDragOver === 'function') {
-      rowAttributes.onDragOver = this.props.onDragOver.bind(this, Immutable.fromJS(handlerInput))
+      rowAttributes.onDragOver = this.props.onDragOver.bind(thisArg, Immutable.fromJS(handlerInput))
       rowAttributes.draggable = true
     }
     if (typeof this.props.onDrop === 'function') {
-      rowAttributes.onDrop = this.props.onDrop.bind(this, Immutable.fromJS(handlerInput))
+      rowAttributes.onDrop = this.props.onDrop.bind(thisArg, Immutable.fromJS(handlerInput))
       rowAttributes.draggable = true
     }
     return rowAttributes

js/state/siteUtil.js

@@ -216,16 +216,43 @@ module.exports.removeSite = function (sites, siteDetail, tag) {
     .setIn([index, 'tags'], tags.toSet().remove(tag).toList())
 }
 
-function fillParentFolders (parentFolderIds, bookmarkFolder, allBookmarks) {
+/**
+ * Called by isMoveAllowed
+ * Trace a folder's ancestory, collecting all parent folderIds until reaching Bookmarks Toolbar (folderId=0)
+ */
+const getAncestorFolderIds = (parentFolderIds, bookmarkFolder, allBookmarks) => {
   if (bookmarkFolder.get('parentFolderId')) {
     parentFolderIds.push(bookmarkFolder.get('parentFolderId'))
     const nextItem = allBookmarks.find((item) => item.get('folderId') === bookmarkFolder.get('parentFolderId'))
     if (nextItem) {
-      fillParentFolders(parentFolderIds, nextItem, allBookmarks)
+      getAncestorFolderIds(parentFolderIds, nextItem, allBookmarks)
     }
   }
 }
 
+/**
+ * Determine if a proposed move is valid
+ *
+ * @param sites The application state's Immutable sites list
+ * @param siteDetail The site detail to move
+ * @param destinationDetail The site detail to move to
+ */
+module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => {
+  if (typeof destinationDetail.get('parentFolderId') === 'number' && typeof sourceDetail.get('folderId') === 'number') {
+    // Folder can't be its own parent
+    if (sourceDetail.get('folderId') === destinationDetail.get('folderId')) {
+      return false
+    }
+    // Ancestor folder can't be moved into a descendant
+    let ancestorFolderIds = []
+    getAncestorFolderIds(ancestorFolderIds, destinationDetail, sites)
+    if (ancestorFolderIds.includes(sourceDetail.get('folderId'))) {
+      return false
+    }
+  }
+  return true
+}
+
 /**
  * Moves the specified site from one location to another
  *
@@ -238,21 +265,14 @@ function fillParentFolders (parentFolderIds, bookmarkFolder, allBookmarks) {
  * @return The new sites Immutable object
  */
 module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, destinationIsParent, disallowReparent) {
-  // Disallow loops
-  let parentFolderIds = []
-  if (typeof destinationDetail.get('parentFolderId') === 'number' &&
-      typeof sourceDetail.get('folderId') === 'number') {
-    fillParentFolders(parentFolderIds, destinationDetail, sites)
-    if (sourceDetail.get('folderId') === destinationDetail.get('folderId') ||
-        parentFolderIds.includes(sourceDetail.get('folderId'))) {
-      return sites
-    }
+  if (!module.exports.isMoveAllowed(sites, sourceDetail, destinationDetail)) {
+    return sites
   }
 
   const sourceSiteIndex = module.exports.getSiteIndex(sites, sourceDetail, sourceDetail.get('tags'))
   let destinationSiteIndex
   if (destinationIsParent) {
-    // When the destinatiaon is the parent we want to put it at the end
+    // When the destination is the parent we want to put it at the end
     destinationSiteIndex = sites.size - 1
     prepend = false
   } else {

test/unit/state/siteUtilTest.js

@@ -426,28 +426,35 @@ describe('siteUtil', function () {
     })
   })
 
-  describe('moveSite', function () {
+  describe('isMoveAllowed', function () {
+    // NOTE: usage taken from Bookmark Manager, which calls aboutActions.moveSite
     it('does not allow you to move a bookmark folder into itself', function () {
       // Add a new bookmark folder
       let processedSites = siteUtil.addSite(emptySites, folderMinFields)
       const folderId = processedSites.getIn([0, 'folderId'])
-      const bookmark = Immutable.fromJS({
-        lastAccessedTime: 123,
-        title: 'bookmark1',
-        parentFolderId: folderId,
-        location: testUrl1,
-        tags: [siteTags.BOOKMARK]
-      })
       // Add a bookmark into that folder
-      processedSites = siteUtil.addSite(processedSites, bookmark)
+      processedSites = siteUtil.addSite(processedSites, bookmarkAllFields.set('parentFolderId', folderId))
       const bookmarkFolder = processedSites.get(0)
-
-      // Usage taken from Bookmark Manager, which calls aboutActions.moveSite
-      processedSites = siteUtil.moveSite(processedSites, bookmarkFolder, bookmarkFolder, false, true, false)
-      const siteIndex = siteUtil.getSiteIndex(processedSites, bookmarkFolder, siteTags.BOOKMARK_FOLDER)
-
-      assert.notEqual(processedSites.getIn([siteIndex, 'parentFolderId']), processedSites.getIn([siteIndex, 'folderId']))
+      // Should NOT be able to move bookmark folder into itself
+      assert.equal(false, siteUtil.isMoveAllowed(processedSites, bookmarkFolder, bookmarkFolder))
     })
+    it('does not allow you to move an ancestor folder into a descendant folder', function () {
+      // Add a new bookmark folder
+      let processedSites = siteUtil.addSite(emptySites, folderMinFields)
+      const folderId1 = processedSites.getIn([0, 'folderId'])
+      // Add a child below that folder
+      processedSites = siteUtil.addSite(processedSites, folderMinFields.set('parentFolderId', folderId1))
+      const folderId2 = processedSites.getIn([1, 'folderId'])
+      // Add a folder below the previous child
+      processedSites = siteUtil.addSite(processedSites, folderMinFields.set('parentFolderId', folderId2))
+      const bookmarkFolder1 = processedSites.get(0)
+      const bookmarkFolder3 = processedSites.get(2)
+      // Should NOT be able to move grandparent folder into its grandchild
+      assert.equal(false, siteUtil.isMoveAllowed(processedSites, bookmarkFolder1, bookmarkFolder3))
+    })
+  })
+
+  describe('moveSite', function () {
   })
 
   describe('getDetailFromFrame', function () {