fix: remove a repair flow on share-interact permission creation

Author: shepilov

model/permission/permissions.go

@@ -827,50 +827,51 @@ func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[st
 	defer mu.Unlock()
 
 	return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
-		existing, err := getOrRepairShareInteractPermissionsOnce(db, sharingID)
-		if err != nil && !couchdb.IsNotFoundError(err) {
-			return nil, err
-		}
-		if err == nil {
-			merged := existing.Clone().(*Permission)
-			merged.Type = TypeShareInteract
-			merged.SourceID = consts.Sharings + "/" + sharingID
-			merged.SetID(ShareInteractPermissionID(sharingID))
-			if len(subdoc.Permissions) > 0 {
-				// Callers pass the complete interact rule set, so replace instead of merging rules.
-				merged.Permissions = subdoc.Permissions
-			}
-			if merged.Metadata == nil && subdoc.Metadata != nil {
-				merged.Metadata = subdoc.Metadata.Clone()
-			}
-			if merged.Metadata != nil {
-				merged.Metadata.ChangeUpdatedAt()
+		existing, err := GetPermissionByIDIncludingExpired(db, ShareInteractPermissionID(sharingID))
+		if err != nil {
+			if !couchdb.IsNotFoundError(err) {
+				return nil, err
 			}
-			if merged.Codes == nil {
-				merged.Codes = make(map[string]string)
+			err = couchdb.CreateNamedDoc(db, doc)
+			if err == nil {
+				return doc, nil
 			}
-			for key, code := range codes {
-				if key == "" {
-					continue
-				}
-				if existingCode, ok := merged.Codes[key]; ok && existingCode != code {
-					logger.WithDomain(db.DomainName()).WithNamespace("permissions").
-						Warnf("keeping existing share-interact code for %s in sharing %s", key, sharingID)
-					continue
-				}
-				merged.Codes[key] = code
+			return nil, err
+		}
+
+		merged := existing.Clone().(*Permission)
+		merged.Type = TypeShareInteract
+		merged.SourceID = consts.Sharings + "/" + sharingID
+		merged.SetID(ShareInteractPermissionID(sharingID))
+		merged.ExpiresAt = nil
+		if len(subdoc.Permissions) > 0 {
+			// Callers pass the complete interact rule set, so replace instead of merging rules.
+			merged.Permissions = subdoc.Permissions
+		}
+		if merged.Metadata == nil && subdoc.Metadata != nil {
+			merged.Metadata = subdoc.Metadata.Clone()
+		}
+		if merged.Metadata != nil {
+			merged.Metadata.ChangeUpdatedAt()
+		}
+		if merged.Codes == nil {
+			merged.Codes = make(map[string]string)
+		}
+		for key, code := range codes {
+			if key == "" {
+				continue
 			}
-			if err := couchdb.UpdateDoc(db, merged); err != nil {
-				return nil, err
+			if existingCode, ok := merged.Codes[key]; ok && existingCode != code {
+				logger.WithDomain(db.DomainName()).WithNamespace("permissions").
+					Warnf("keeping existing share-interact code for %s in sharing %s", key, sharingID)
+				continue
 			}
-			return merged, nil
+			merged.Codes[key] = code
 		}
-
-		err = couchdb.CreateNamedDoc(db, doc)
-		if err == nil {
-			return doc, nil
+		if err := couchdb.UpdateDoc(db, merged); err != nil {
+			return nil, err
 		}
-		return nil, err
+		return merged, nil
 	})
 }
 

model/permission/permissions_test.go

@@ -597,6 +597,92 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
 	require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())
 }
 
+func TestCreateShareInteractSetReactivatesExpiredCanonicalDoc(t *testing.T) {
+	if testing.Short() {
+		t.Skip("an instance is required for this test: test skipped due to the use of --short flag")
+	}
+
+	config.UseTestFile(t)
+	db := testPrefix(t)
+	require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
+
+	const sharingID = "sharing-expired-canonical-interact-permissions"
+	rules := Set{{
+		Title:  "Shared drive",
+		Type:   consts.Files,
+		Values: []string{"shared-drive-root"},
+		Verbs:  ALL,
+	}}
+	expiredAt := time.Now().Add(-time.Hour).Format(time.RFC3339)
+
+	err := couchdb.CreateNamedDoc(db, &Permission{
+		PID:         ShareInteractPermissionID(sharingID),
+		Type:        TypeShareInteract,
+		Permissions: rules,
+		Codes: map[string]string{
+			"alice@example.test": "alice-token",
+		},
+		ExpiresAt: expiredAt,
+		SourceID:  consts.Sharings + "/" + sharingID,
+	})
+	require.NoError(t, err)
+
+	updated, err := CreateShareInteractSet(db, sharingID, map[string]string{
+		"bob@example.test": "bob-token",
+	}, Permission{Permissions: rules})
+	require.NoError(t, err)
+	require.Nil(t, updated.ExpiresAt)
+	require.Equal(t, map[string]string{
+		"alice@example.test": "alice-token",
+		"bob@example.test":   "bob-token",
+	}, updated.Codes)
+
+	stored, err := GetForShareInteract(db, sharingID)
+	require.NoError(t, err)
+	require.Nil(t, stored.ExpiresAt)
+}
+
+func TestCreateShareInteractSetDoesNotRepairLegacyDuplicates(t *testing.T) {
+	if testing.Short() {
+		t.Skip("an instance is required for this test: test skipped due to the use of --short flag")
+	}
+
+	config.UseTestFile(t)
+	db := testPrefix(t)
+	require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
+
+	const sharingID = "sharing-create-with-legacy-duplicate"
+	rules := Set{{
+		Title:  "Shared drive",
+		Type:   consts.Files,
+		Values: []string{"shared-drive-root"},
+		Verbs:  ALL,
+	}}
+
+	err := couchdb.CreateDoc(db, &Permission{
+		Type:        TypeShareInteract,
+		Permissions: rules,
+		Codes: map[string]string{
+			"alice@example.test": "alice-token",
+		},
+		SourceID: consts.Sharings + "/" + sharingID,
+	})
+	require.NoError(t, err)
+
+	created, err := CreateShareInteractSet(db, sharingID, map[string]string{
+		"bob@example.test": "bob-token",
+	}, Permission{Permissions: rules})
+	require.NoError(t, err)
+	require.Equal(t, ShareInteractPermissionID(sharingID), created.ID())
+	require.Equal(t, map[string]string{
+		"bob@example.test": "bob-token",
+	}, created.Codes)
+
+	all, err := getShareInteractPermissions(db, sharingID)
+	require.NoError(t, err)
+	require.Len(t, all, 2)
+}
+
 func TestCreateShareSetBlocklist(t *testing.T) {
 	s := Set{Rule{Type: "io.cozy.notifications"}}
 	subdoc := Permission{