fix: stale sharing shortcut counter after recipient removal (#4780)

## Summary

Fixes the Drive sharing counter after a recipient is removed from a
pending shared drive.

A recipient can have multiple shortcut files for the same sharing: for
example, one pending `new` shortcut and another shortcut created when
opening the sharing authorization flow. Previously, revocation only
removed the shortcut
pointed to by `ShortcutID`, leaving older referenced shortcuts counted
by `/sharings/news`.

  ## Changes

  - Clean up the shortcut referenced by `ShortcutID`.
- Also remove remaining shortcut files linked to the same sharing
through `referenced_by`.
  - Keep referenced cleanup limited to shortcut files.

Author: shepilov

model/sharing/invitation.go

@@ -356,24 +356,7 @@ func (s *Sharing) CreateDriveShortcut(inst *instance.Instance, seen bool) error
 		Type: consts.Sharings,
 	})
 
-	file, err := inst.VFS().CreateFile(fileDoc, nil)
-	if err != nil {
-		basename := fileDoc.DocName
-		for i := 2; i < 100; i++ {
-			fileDoc.DocName = fmt.Sprintf("%s (%d)", basename, i)
-			file, err = inst.VFS().CreateFile(fileDoc, nil)
-			if err == nil {
-				break
-			}
-		}
-		if err != nil {
-			return err
-		}
-	}
-	_, err = file.Write(body)
-	if cerr := file.Close(); cerr != nil && err == nil {
-		err = cerr
-	}
+	fileDoc, err = s.createOrUpdateShortcut(inst, fileDoc, body)
 	if err != nil {
 		return err
 	}
@@ -448,12 +431,59 @@ func (s *Sharing) CreateShortcut(inst *instance.Instance, previewURL string, see
 		Type: consts.Sharings,
 	})
 
-	file, err := inst.VFS().CreateFile(fileDoc, nil)
+	fileDoc, err = s.createOrUpdateShortcut(inst, fileDoc, body)
+	if err != nil {
+		return err
+	}
+
+	s.ShortcutID = fileDoc.DocID
+	if err := couchdb.UpdateDoc(inst, s); err != nil {
+		inst.Logger().Warnf("Cannot save shortcut id %s: %s", s.ShortcutID, err)
+	}
+
+	if !seen {
+		return s.SendShortcutNotification(inst, fileDoc, previewURL)
+	}
+	return nil
+}
+
+func (s *Sharing) createOrUpdateShortcut(inst *instance.Instance, fileDoc *vfs.FileDoc, body []byte) (*vfs.FileDoc, error) {
+	fs := inst.VFS()
+	if s.ShortcutID != "" {
+		if olddoc, err := fs.FileByID(s.ShortcutID); err == nil && isReusableShortcut(olddoc) {
+			newdoc := olddoc.Clone().(*vfs.FileDoc)
+			newdoc.ByteSize = int64(len(body))
+			newdoc.MD5Sum = nil
+			newdoc.Mime = fileDoc.Mime
+			newdoc.Class = fileDoc.Class
+			newdoc.Executable = fileDoc.Executable
+			newdoc.Encrypted = fileDoc.Encrypted
+			newdoc.UpdatedAt = fileDoc.UpdatedAt
+			newdoc.Metadata = fileDoc.Metadata
+			newdoc.ReferencedBy = fileDoc.ReferencedBy
+			newdoc.CozyMetadata = fileDoc.CozyMetadata
+			return newdoc, writeShortcutFile(fs, newdoc, olddoc, body)
+		}
+	}
+
+	if err := createShortcutFile(fs, fileDoc, body); err != nil {
+		return nil, err
+	}
+	return fileDoc, nil
+}
+
+func isReusableShortcut(file *vfs.FileDoc) bool {
+	return !file.Trashed &&
+		(file.Mime == consts.ShortcutMimeType || file.Class == "shortcut")
+}
+
+func createShortcutFile(fs vfs.VFS, fileDoc *vfs.FileDoc, body []byte) error {
+	file, err := fs.CreateFile(fileDoc, nil)
 	if err != nil {
 		basename := fileDoc.DocName
 		for i := 2; i < 100; i++ {
 			fileDoc.DocName = fmt.Sprintf("%s (%d)", basename, i)
-			file, err = inst.VFS().CreateFile(fileDoc, nil)
+			file, err = fs.CreateFile(fileDoc, nil)
 			if err == nil {
 				break
 			}
@@ -462,23 +492,23 @@ func (s *Sharing) CreateShortcut(inst *instance.Instance, previewURL string, see
 			return err
 		}
 	}
-	_, err = file.Write(body)
-	if cerr := file.Close(); cerr != nil && err == nil {
-		err = cerr
-	}
+	return writeShortcutContent(file, body)
+}
+
+func writeShortcutFile(fs vfs.VFS, newdoc, olddoc *vfs.FileDoc, body []byte) error {
+	file, err := fs.CreateFile(newdoc, olddoc)
 	if err != nil {
 		return err
 	}
+	return writeShortcutContent(file, body)
+}
 
-	s.ShortcutID = fileDoc.DocID
-	if err := couchdb.UpdateDoc(inst, s); err != nil {
-		inst.Logger().Warnf("Cannot save shortcut id %s: %s", s.ShortcutID, err)
-	}
-
-	if !seen {
-		return s.SendShortcutNotification(inst, fileDoc, previewURL)
+func writeShortcutContent(file vfs.File, body []byte) error {
+	_, err := file.Write(body)
+	if cerr := file.Close(); cerr != nil && err == nil {
+		err = cerr
 	}
-	return nil
+	return err
 }
 
 // SendShortcut sends the HTTP request to the cozy of the recipient for adding

model/sharing/sharing.go

@@ -1188,21 +1188,76 @@ func (s *Sharing) GetSharecodeFromShortcut(inst *instance.Instance) (string, err
 }
 
 func (s *Sharing) cleanShortcutID(inst *instance.Instance) string {
-	if s.ShortcutID == "" {
+	fs := inst.VFS()
+	// remove a shortcut by id, as it before
+	parentID, deletedByID, err := destroyShortcutByID(fs, s.ShortcutID)
+	if err != nil {
 		return ""
 	}
+	shouldClearShortcutID := deletedByID || s.ShortcutID != ""
+
+	// remove shortcut reference from sharing
+	referencedParentID, deletedByRef, err := destroyReferencedShortcutFiles(inst, fs, s.SID)
+	if err == nil && parentID == "" {
+		parentID = referencedParentID
+	}
+	if err == nil && deletedByRef {
+		shouldClearShortcutID = true
+	}
+
+	if shouldClearShortcutID {
+		s.ShortcutID = ""
+		_ = couchdb.UpdateDoc(inst, s)
+	}
+	return parentID
+}
+
+func destroyShortcutByID(fs vfs.VFS, shortcutID string) (string, bool, error) {
+	if shortcutID == "" {
+		return "", false, nil
+	}
+	file, err := fs.FileByID(shortcutID)
+	if err != nil {
+		return "", false, nil
+	}
+	if err := fs.DestroyFile(file); err != nil {
+		return "", false, err
+	}
+	return file.DirID, true, nil
+}
+
+func destroyReferencedShortcutFiles(inst *instance.Instance, fs vfs.VFS, sharingID string) (string, bool, error) {
+	key := []string{consts.Sharings, sharingID}
+	req := &couchdb.ViewRequest{
+		StartKey: key,
+		EndKey:   []string{key[0], key[1], couchdb.MaxString},
+	}
+	var res couchdb.ViewResponse
+	if err := couchdb.ExecView(inst, couchdb.FilesReferencedByView, req, &res); err != nil {
+		return "", false, err
+	}
 
 	var parentID string
-	fs := inst.VFS()
-	if file, err := fs.FileByID(s.ShortcutID); err == nil {
-		parentID = file.DirID
+	var deleted bool
+	for _, row := range res.Rows {
+		file, err := fs.FileByID(row.ID)
+		if err != nil || !isShortcutFile(file) {
+			continue
+		}
 		if err := fs.DestroyFile(file); err != nil {
-			return ""
+			return "", false, err
+		}
+		if parentID == "" {
+			parentID = file.DirID
 		}
+		deleted = true
 	}
-	s.ShortcutID = ""
-	_ = couchdb.UpdateDoc(inst, s)
-	return parentID
+	return parentID, deleted, nil
+}
+
+func isShortcutFile(file *vfs.FileDoc) bool {
+	return file.Type == consts.FileType &&
+		(file.Mime == consts.ShortcutMimeType || file.Class == "shortcut")
 }
 
 // GetPreviewURL asks the owner's Cozy the URL for previewing the sharing.

web/sharings/drives_test.go

@@ -4141,6 +4141,79 @@ func TestSharedDriveRevocation(t *testing.T) {
 	})
 }
 
+func TestOpeningPendingSharedDriveShortcutClearsNewsCounter(t *testing.T) {
+	if testing.Short() {
+		t.Skip("an instance is required for this test: test skipped due to the use of --short flag")
+	}
+
+	env := setupSharedDrivesEnv(t)
+	eA, eB, _ := env.createClients(t)
+	sharingID := createDirectRecipientDriveSharing(
+		t,
+		env.acme,
+		eA,
+		env.acmeToken,
+		"Betty Pending",
+		"betty@example.net",
+		env.tsB.URL,
+		"Pending Revocation Drive",
+		"Pending drive for revocation counter",
+	)
+
+	recipientSharing := waitForSharingOnRecipient(t, env.betty, sharingID)
+	require.NotEmpty(t, recipientSharing.ShortcutID)
+	newShortcutID := recipientSharing.ShortcutID
+
+	eB.GET("/sharings/news").
+		WithHeader("Authorization", "Bearer "+env.bettyToken).
+		Expect().Status(http.StatusOK).
+		JSON().Object().
+		Path("$.meta.count").Number().IsEqual(1)
+
+	loginSharingRecipient(t, eB)
+	FakeOwnerInstanceForSharing(t, env.betty, env.tsA.URL, sharingID)
+	require.NotEmpty(t, recipientSharing.Credentials)
+	require.NotEmpty(t, recipientSharing.Credentials[0].State)
+	authorizeLink := fmt.Sprintf(
+		"%s/auth/authorize/sharing?sharing_id=%s&state=%s",
+		env.tsB.URL,
+		sharingID,
+		url.QueryEscape(recipientSharing.Credentials[0].State),
+	)
+	openSharingAuthorize(t, eB, authorizeLink, sharingID)
+
+	recipientSharing, err := sharing.FindSharing(env.betty, sharingID)
+	require.NoError(t, err)
+	require.NotEmpty(t, recipientSharing.ShortcutID)
+	seenShortcutID := recipientSharing.ShortcutID
+	require.Equal(t, newShortcutID, seenShortcutID)
+
+	eB.GET("/sharings/news").
+		WithHeader("Authorization", "Bearer "+env.bettyToken).
+		Expect().Status(http.StatusOK).
+		JSON().Object().
+		Path("$.meta.count").Number().IsEqual(0)
+
+	require.Eventually(t, func() bool {
+		count, err := sharing.CountNewShortcuts(env.betty)
+		return err == nil && count == 0
+	}, 5*time.Second, 100*time.Millisecond, "Betty's sharing counter should not include the opened pending drive")
+
+	require.Eventually(t, func() bool {
+		_, err := env.betty.VFS().FileByID(newShortcutID)
+		return err == nil
+	}, 5*time.Second, 100*time.Millisecond, "Betty's opened shared-drive shortcut should still exist before revocation")
+
+	eA.DELETE("/sharings/"+sharingID+"/recipients/1").
+		WithHeader("Authorization", "Bearer "+env.acmeToken).
+		Expect().Status(http.StatusNoContent)
+
+	require.Eventually(t, func() bool {
+		_, err := env.betty.VFS().FileByID(newShortcutID)
+		return os.IsNotExist(err)
+	}, 5*time.Second, 100*time.Millisecond, "Betty's opened shared-drive shortcut should be removed after revocation")
+}
+
 func TestDirectoryRootSharedDriveOwnerDeletionRevokesRecipient(t *testing.T) {
 	if testing.Short() {
 		t.Skip("an instance is required for this test: test skipped due to the use of --short flag")