From 96f022548ac578c45b6d28f320ad59621694a261 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Mon, 18 May 2026 16:48:18 +0200 Subject: [PATCH 1/8] test: verify but with multiple permissions docs for a shared drive --- model/sharing/member_test.go | 70 ++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 model/sharing/member_test.go diff --git a/model/sharing/member_test.go b/model/sharing/member_test.go new file mode 100644 index 00000000000..b5d2cbbe151 --- /dev/null +++ b/model/sharing/member_test.go @@ -0,0 +1,70 @@ +package sharing + +import ( + "testing" + + "github.com/cozy/cozy-stack/model/permission" + "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/consts" + "github.com/cozy/cozy-stack/pkg/couchdb" + "github.com/cozy/cozy-stack/tests/testutils" + "github.com/stretchr/testify/require" +) + +func TestFindMemberByInteractCodeWithDuplicatePermissions(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) + testutils.NeedCouchdb(t) + setup := testutils.NewSetup(t, t.Name()) + inst := setup.GetTestInstance() + require.NoError(t, couchdb.ResetDB(inst, consts.Permissions)) + + const sharingID = "sharing-duplicate-interact-permissions" + const aliceEmail = "alice@example.test" + const bobEmail = "bob@example.test" + + aliceToken := "alice-interact-token" + bobToken := "bob-interact-token" + perms := permission.Permission{ + Permissions: permission.Set{{ + Title: "Shared drive", + Type: consts.Files, + Values: []string{"shared-drive-root"}, + Verbs: permission.ALL, + }}, + } + + _, err := permission.CreateShareInteractSet(inst, sharingID, map[string]string{ + aliceEmail: aliceToken, + }, perms) + require.NoError(t, err) + _, err = permission.CreateShareInteractSet(inst, sharingID, map[string]string{ + bobEmail: bobToken, + }, perms) + require.NoError(t, err) + + first, err := permission.GetForShareInteract(inst, sharingID) + require.NoError(t, err) + + targetEmail := bobEmail + targetToken := bobToken + if _, ok := first.Codes[bobEmail]; ok { + targetEmail = aliceEmail + targetToken = aliceToken + } + + s := Sharing{ + SID: sharingID, + Members: []Member{ + {Email: "owner@example.test", Status: MemberStatusOwner}, + {Email: aliceEmail, Status: MemberStatusReady}, + {Email: bobEmail, Status: MemberStatusReady}, + }, + } + member, err := s.FindMemberByInteractCode(inst, targetToken) + require.NoError(t, err) + require.Equal(t, targetEmail, member.Email) +} From 246b8e21e36e33f6b41e03f1b52dbf0fe1eccaec Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Mon, 18 May 2026 17:11:02 +0200 Subject: [PATCH 2/8] feat: repair multiple permissions docs for a shared drive --- model/permission/permissions.go | 138 +++++++++++++++++++++++++++ model/permission/permissions_test.go | 59 ++++++++++++ 2 files changed, 197 insertions(+) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index 5a5b1e324a8..2d1a8fa3b10 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -14,6 +14,7 @@ import ( "github.com/cozy/cozy-stack/pkg/couchdb" "github.com/cozy/cozy-stack/pkg/couchdb/mango" "github.com/cozy/cozy-stack/pkg/crypto" + "github.com/cozy/cozy-stack/pkg/logger" "github.com/cozy/cozy-stack/pkg/metadata" "github.com/cozy/cozy-stack/pkg/prefixer" "github.com/labstack/echo/v4" @@ -247,6 +248,143 @@ func GetForShareInteract(db prefixer.Prefixer, sharingID string) (*Permission, e return getFromSource(db, TypeShareInteract, consts.Sharings, sharingID) } +// ShareInteractPermissionID returns the canonical permission document ID for a +// share-interact permission set. +func ShareInteractPermissionID(sharingID string) string { + return TypeShareInteract + "-" + sharingID +} + +func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Permission, error) { + var res []Permission + req := couchdb.FindRequest{ + UseIndex: "by-source-and-type", + Selector: mango.And( + mango.Equal("type", TypeShareInteract), + mango.Equal("source_id", consts.Sharings+"/"+sharingID), + ), + Limit: 1000, + } + err := couchdb.FindDocs(db, consts.Permissions, &req, &res) + if err != nil { + return nil, err + } + return res, nil +} + +// RepairShareInteractPermissions merges duplicate share-interact permission +// documents for a sharing into the canonical document and deletes the legacy +// duplicates. +func RepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) { + var lastErr error + for attempt := 0; attempt < 5; attempt++ { + perm, retry, err := repairShareInteractPermissions(db, sharingID) + if err == nil && !retry { + return perm, nil + } + lastErr = err + } + if lastErr != nil { + return nil, lastErr + } + return nil, &couchdb.Error{ + StatusCode: http.StatusConflict, + Name: "conflict", + Reason: "could not repair share-interact permissions after retries", + } +} + +func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, bool, error) { + perms, err := getShareInteractPermissions(db, sharingID) + if err != nil { + return nil, false, err + } + if len(perms) == 0 { + return nil, false, &couchdb.Error{ + StatusCode: http.StatusNotFound, + Name: "not_found", + Reason: fmt.Sprintf("no permission doc for %v", sharingID), + } + } + + canonicalID := ShareInteractPermissionID(sharingID) + merged := &Permission{ + PID: canonicalID, + Type: TypeShareInteract, + SourceID: consts.Sharings + "/" + sharingID, + Codes: make(map[string]string), + } + hasUsablePermission := false + hasCanonical := false + duplicates := make([]*Permission, 0, len(perms)) + for i := range perms { + p := &perms[i] + if p.Expired() { + continue + } + + hasUsablePermission = true + if p.ID() == canonicalID { + hasCanonical = true + merged.SetRev(p.Rev()) + } else { + duplicates = append(duplicates, p) + } + + if len(merged.Permissions) == 0 && len(p.Permissions) > 0 { + merged.Permissions = p.Clone().(*Permission).Permissions + } + if merged.Metadata == nil && p.Metadata != nil { + merged.Metadata = p.Metadata.Clone() + } + if merged.ExpiresAt == nil && p.ExpiresAt != nil { + merged.ExpiresAt = p.ExpiresAt + } + + for key, code := range p.Codes { + if key == "" { + continue + } + if existing, ok := merged.Codes[key]; ok && existing != code { + logger.WithDomain(db.DomainName()).WithNamespace("permissions"). + Warnf("conflicting share-interact code for %s in sharing %s", key, sharingID) + continue + } + merged.Codes[key] = code + } + } + if !hasUsablePermission { + return nil, false, ErrExpiredToken + } + + if !hasCanonical { + if err := couchdb.CreateNamedDoc(db, merged); err != nil { + if couchdb.IsConflictError(err) || couchdb.IsFileExists(err) { + return nil, true, err + } + return nil, false, err + } + } else if err := couchdb.UpdateDoc(db, merged); err != nil { + if couchdb.IsConflictError(err) { + return nil, true, err + } + return nil, false, err + } + + for _, p := range duplicates { + if err := couchdb.DeleteDoc(db, p); err != nil { + if couchdb.IsNotFoundError(err) { + continue + } + if couchdb.IsConflictError(err) { + return nil, true, err + } + return nil, false, err + } + } + + return merged, false, nil +} + func getFromSource(db prefixer.Prefixer, permType, docType, slug string) (*Permission, error) { var res []Permission req := couchdb.FindRequest{ diff --git a/model/permission/permissions_test.go b/model/permission/permissions_test.go index 6a8a17f3e8c..a2ca5f06c2d 100644 --- a/model/permission/permissions_test.go +++ b/model/permission/permissions_test.go @@ -6,10 +6,17 @@ import ( "strings" "testing" + "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/consts" + "github.com/cozy/cozy-stack/pkg/couchdb" + "github.com/cozy/cozy-stack/pkg/prefixer" "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +var testPrefix = prefixer.NewPrefixer(0, "test", "permission-tests") + func TestCheckDoctypeName(t *testing.T) { assert.NoError(t, CheckDoctypeName("io.cozy.files", false)) assert.NoError(t, CheckDoctypeName("io.cozy.account_types", false)) @@ -423,6 +430,58 @@ func TestShareSetPermissions(t *testing.T) { assert.Error(t, err) } +func TestRepairShareInteractPermissionsMergesDuplicateDocs(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) + require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + + const sharingID = "sharing-duplicate-interact-permissions" + perms := Permission{ + Permissions: Set{{ + Title: "Shared drive", + Type: consts.Files, + Values: []string{"shared-drive-root"}, + Verbs: ALL, + }}, + } + + _, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + "alice@example.test": "alice-token", + }, perms) + require.NoError(t, err) + _, err = CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + "bob@example.test": "bob-token", + }, perms) + require.NoError(t, err) + + repaired, err := RepairShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID()) + require.Equal(t, map[string]string{ + "alice@example.test": "alice-token", + "bob@example.test": "bob-token", + }, repaired.Codes) + + all, err := getShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Len(t, all, 1) + require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) + + repaired, err = RepairShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Equal(t, map[string]string{ + "alice@example.test": "alice-token", + "bob@example.test": "bob-token", + }, repaired.Codes) + + all, err = getShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Len(t, all, 1) +} + func TestCreateShareSetBlocklist(t *testing.T) { s := Set{Rule{Type: "io.cozy.notifications"}} subdoc := Permission{ From fd87addf352ae22534483086369b5b5db93ef1f1 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Mon, 18 May 2026 17:48:46 +0200 Subject: [PATCH 3/8] feat: repair multiple permissions docs for a shared drive on documents get(permissions check) --- model/permission/permissions.go | 168 ++++++++++++++++++++++----- model/permission/permissions_test.go | 76 ++++++++++-- model/sharing/member_test.go | 109 ++++++++++++++--- model/sharing/sharing.go | 31 +++-- pkg/utils/retry.go | 167 ++++++++++++++++++++++++-- pkg/utils/retry_test.go | 137 ++++++++++++++++++++++ 6 files changed, 613 insertions(+), 75 deletions(-) create mode 100644 pkg/utils/retry_test.go diff --git a/model/permission/permissions.go b/model/permission/permissions.go index 2d1a8fa3b10..cb33dc02ad3 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -3,6 +3,7 @@ package permission import ( + "context" "encoding/json" "fmt" "net/http" @@ -10,6 +11,7 @@ import ( "time" build "github.com/cozy/cozy-stack/pkg/config" + stackconfig "github.com/cozy/cozy-stack/pkg/config/config" "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/couchdb" "github.com/cozy/cozy-stack/pkg/couchdb/mango" @@ -17,6 +19,7 @@ import ( "github.com/cozy/cozy-stack/pkg/logger" "github.com/cozy/cozy-stack/pkg/metadata" "github.com/cozy/cozy-stack/pkg/prefixer" + "github.com/cozy/cozy-stack/pkg/utils" "github.com/labstack/echo/v4" ) @@ -243,9 +246,17 @@ func GetForSharePreview(db prefixer.Prefixer, sharingID string) (*Permission, er } // GetForShareInteract retrieves the Permission doc for a given sharing to -// read/write a note +// read/write a note. It may repair legacy duplicate share-interact permission +// docs as part of the read by creating/updating the canonical permission doc +// and deleting duplicate legacy docs. func GetForShareInteract(db prefixer.Prefixer, sharingID string) (*Permission, error) { - return getFromSource(db, TypeShareInteract, consts.Sharings, sharingID) + mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID)) + if err := mu.Lock(); err != nil { + return nil, err + } + defer mu.Unlock() + + return getOrRepairShareInteractPermissions(db, sharingID) } // ShareInteractPermissionID returns the canonical permission document ID for a @@ -254,6 +265,10 @@ func ShareInteractPermissionID(sharingID string) string { return TypeShareInteract + "-" + sharingID } +func shareInteractLockName(sharingID string) string { + return "permissions/share-interact/" + sharingID +} + func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Permission, error) { var res []Permission req := couchdb.FindRequest{ @@ -271,33 +286,44 @@ func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Perm return res, nil } -// RepairShareInteractPermissions merges duplicate share-interact permission -// documents for a sharing into the canonical document and deletes the legacy -// duplicates. -func RepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) { - var lastErr error - for attempt := 0; attempt < 5; attempt++ { - perm, retry, err := repairShareInteractPermissions(db, sharingID) +func getOrRepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) { + return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) { + perms, err := getShareInteractPermissions(db, sharingID) + if err != nil { + return nil, err + } + if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair { + return canonical, err + } + + perm, retry, err := repairShareInteractPermissions(db, sharingID, perms) if err == nil && !retry { return perm, nil } - lastErr = err - } - if lastErr != nil { - return nil, lastErr - } - return nil, &couchdb.Error{ - StatusCode: http.StatusConflict, - Name: "conflict", - Reason: "could not repair share-interact permissions after retries", - } + if retry && err == nil { + return nil, &couchdb.Error{ + StatusCode: http.StatusConflict, + Name: "conflict", + Reason: "could not repair share-interact permissions after retries", + } + } + return nil, err + }) } -func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, bool, error) { - perms, err := getShareInteractPermissions(db, sharingID) - if err != nil { - return nil, false, err +func shareInteractRetryOptions() utils.RetryOptions { + return utils.RetryOptions{ + Attempts: 5, + Delay: 10 * time.Millisecond, + MaxDelay: 100 * time.Millisecond, + JitterFactor: 0.25, + ShouldRetry: func(err error) bool { + return couchdb.IsConflictError(err) || couchdb.IsFileExists(err) + }, } +} + +func shareInteractRepairState(perms []Permission, sharingID string) (*Permission, bool, error) { if len(perms) == 0 { return nil, false, &couchdb.Error{ StatusCode: http.StatusNotFound, @@ -306,6 +332,28 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Pe } } + canonicalID := ShareInteractPermissionID(sharingID) + var canonical *Permission + usable := 0 + for i := range perms { + if perms[i].Expired() { + continue + } + usable++ + if perms[i].ID() == canonicalID { + canonical = &perms[i] + } + } + if usable == 0 { + return nil, false, ErrExpiredToken + } + if usable == 1 && canonical != nil { + return canonical, false, nil + } + return nil, true, nil +} + +func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, bool, error) { canonicalID := ShareInteractPermissionID(sharingID) merged := &Permission{ PID: canonicalID, @@ -318,15 +366,17 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Pe duplicates := make([]*Permission, 0, len(perms)) for i := range perms { p := &perms[i] + if p.ID() == canonicalID { + hasCanonical = true + // If the canonical doc is expired, update that doc ID but rebuild its content from non-expired docs. + merged.SetRev(p.Rev()) + } if p.Expired() { continue } hasUsablePermission = true - if p.ID() == canonicalID { - hasCanonical = true - merged.SetRev(p.Rev()) - } else { + if p.ID() != canonicalID { duplicates = append(duplicates, p) } @@ -743,21 +793,75 @@ func CreateSharePreviewSet(db prefixer.Prefixer, sharingID string, codes, shortc return doc, nil } -// CreateShareInteractSet creates a Permission doc for reading/writing a note -// inside a sharing +// CreateShareInteractSet creates or updates the Permission doc for reading and +// writing a note inside a sharing. When subdoc.Permissions is not empty, it +// replaces the existing permission rules with that full set. func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[string]string, subdoc Permission) (*Permission, error) { doc := &Permission{ + PID: ShareInteractPermissionID(sharingID), Type: TypeShareInteract, Permissions: subdoc.Permissions, Codes: codes, SourceID: consts.Sharings + "/" + sharingID, Metadata: subdoc.Metadata, } - err := couchdb.CreateDoc(db, doc) - if err != nil { + + if doc.Codes == nil { + doc.Codes = make(map[string]string) + } + + mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID)) + if err := mu.Lock(); err != nil { return nil, err } - return doc, nil + defer mu.Unlock() + + return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) { + existing, err := getOrRepairShareInteractPermissions(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() + } + if merged.Codes == nil { + merged.Codes = make(map[string]string) + } + 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 + } + if err := couchdb.UpdateDoc(db, merged); err != nil { + return nil, err + } + return merged, nil + } + + err = couchdb.CreateNamedDoc(db, doc) + if err == nil { + return doc, nil + } + return nil, err + }) } // ForceWebapp creates or updates a Permission doc for a given webapp diff --git a/model/permission/permissions_test.go b/model/permission/permissions_test.go index a2ca5f06c2d..fce0ff8bd8e 100644 --- a/model/permission/permissions_test.go +++ b/model/permission/permissions_test.go @@ -5,10 +5,12 @@ import ( "encoding/json" "strings" "testing" + "time" "github.com/cozy/cozy-stack/pkg/config/config" "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/couchdb" + "github.com/cozy/cozy-stack/pkg/metadata" "github.com/cozy/cozy-stack/pkg/prefixer" "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" @@ -430,7 +432,7 @@ func TestShareSetPermissions(t *testing.T) { assert.Error(t, err) } -func TestRepairShareInteractPermissionsMergesDuplicateDocs(t *testing.T) { +func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { if testing.Short() { t.Skip("an instance is required for this test: test skipped due to the use of --short flag") } @@ -448,16 +450,26 @@ func TestRepairShareInteractPermissionsMergesDuplicateDocs(t *testing.T) { }}, } - _, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ - "alice@example.test": "alice-token", - }, perms) + err := couchdb.CreateDoc(testPrefix, &Permission{ + Type: TypeShareInteract, + Permissions: perms.Permissions, + Codes: map[string]string{ + "alice@example.test": "alice-token", + }, + SourceID: consts.Sharings + "/" + sharingID, + }) require.NoError(t, err) - _, err = CreateShareInteractSet(testPrefix, sharingID, map[string]string{ - "bob@example.test": "bob-token", - }, perms) + err = couchdb.CreateDoc(testPrefix, &Permission{ + Type: TypeShareInteract, + Permissions: perms.Permissions, + Codes: map[string]string{ + "bob@example.test": "bob-token", + }, + SourceID: consts.Sharings + "/" + sharingID, + }) require.NoError(t, err) - repaired, err := RepairShareInteractPermissions(testPrefix, sharingID) + repaired, err := GetForShareInteract(testPrefix, sharingID) require.NoError(t, err) require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID()) require.Equal(t, map[string]string{ @@ -470,7 +482,7 @@ func TestRepairShareInteractPermissionsMergesDuplicateDocs(t *testing.T) { require.Len(t, all, 1) require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) - repaired, err = RepairShareInteractPermissions(testPrefix, sharingID) + repaired, err = GetForShareInteract(testPrefix, sharingID) require.NoError(t, err) require.Equal(t, map[string]string{ "alice@example.test": "alice-token", @@ -482,6 +494,52 @@ func TestRepairShareInteractPermissionsMergesDuplicateDocs(t *testing.T) { require.Len(t, all, 1) } +func TestCreateShareInteractSetUsesCanonicalDoc(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) + require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + + const sharingID = "sharing-canonical-interact-permissions" + md := metadata.New() + md.UpdatedAt = time.Now().Add(-time.Hour) + perms := Permission{ + Permissions: Set{{ + Title: "Shared drive", + Type: consts.Files, + Values: []string{"shared-drive-root"}, + Verbs: ALL, + }}, + Metadata: md, + } + + first, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + "alice@example.test": "alice-token", + }, perms) + require.NoError(t, err) + require.Equal(t, ShareInteractPermissionID(sharingID), first.ID()) + + second, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + "bob@example.test": "bob-token", + }, perms) + require.NoError(t, err) + require.Equal(t, ShareInteractPermissionID(sharingID), second.ID()) + require.Equal(t, map[string]string{ + "alice@example.test": "alice-token", + "bob@example.test": "bob-token", + }, second.Codes) + require.NotNil(t, first.Metadata) + require.NotNil(t, second.Metadata) + require.True(t, second.Metadata.UpdatedAt.After(first.Metadata.UpdatedAt)) + + all, err := getShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Len(t, all, 1) + require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) +} + func TestCreateShareSetBlocklist(t *testing.T) { s := Set{Rule{Type: "io.cozy.notifications"}} subdoc := Permission{ diff --git a/model/sharing/member_test.go b/model/sharing/member_test.go index b5d2cbbe151..b1d15b3ced1 100644 --- a/model/sharing/member_test.go +++ b/model/sharing/member_test.go @@ -1,12 +1,16 @@ package sharing import ( + "fmt" + "os" + "sync" "testing" "github.com/cozy/cozy-stack/model/permission" "github.com/cozy/cozy-stack/pkg/config/config" "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/couchdb" + "github.com/cozy/cozy-stack/pkg/couchdb/mango" "github.com/cozy/cozy-stack/tests/testutils" "github.com/stretchr/testify/require" ) @@ -37,24 +41,27 @@ func TestFindMemberByInteractCodeWithDuplicatePermissions(t *testing.T) { }}, } - _, err := permission.CreateShareInteractSet(inst, sharingID, map[string]string{ - aliceEmail: aliceToken, - }, perms) - require.NoError(t, err) - _, err = permission.CreateShareInteractSet(inst, sharingID, map[string]string{ - bobEmail: bobToken, - }, perms) + err := couchdb.CreateDoc(inst, &permission.Permission{ + Type: permission.TypeShareInteract, + Permissions: perms.Permissions, + Codes: map[string]string{ + aliceEmail: aliceToken, + }, + SourceID: consts.Sharings + "/" + sharingID, + }) require.NoError(t, err) - - first, err := permission.GetForShareInteract(inst, sharingID) + err = couchdb.CreateDoc(inst, &permission.Permission{ + Type: permission.TypeShareInteract, + Permissions: perms.Permissions, + Codes: map[string]string{ + bobEmail: bobToken, + }, + SourceID: consts.Sharings + "/" + sharingID, + }) require.NoError(t, err) targetEmail := bobEmail targetToken := bobToken - if _, ok := first.Codes[bobEmail]; ok { - targetEmail = aliceEmail - targetToken = aliceToken - } s := Sharing{ SID: sharingID, @@ -68,3 +75,79 @@ func TestFindMemberByInteractCodeWithDuplicatePermissions(t *testing.T) { require.NoError(t, err) require.Equal(t, targetEmail, member.Email) } + +func TestGetInteractCodeConcurrentCalls(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) + testutils.NeedCouchdb(t) + setup := testutils.NewSetup(t, t.Name()) + inst := setup.GetTestInstance() + require.NoError(t, couchdb.ResetDB(inst, consts.Permissions)) + + calls := 100 + if os.Getenv("COZY_STRESS_TESTS") == "1" { + calls = 1000 + } + sharingID := "sharing-concurrent-interact-permissions" + s := Sharing{ + SID: sharingID, + AppSlug: "drive", + Rules: []Rule{{ + Title: "Shared drive", + DocType: consts.Files, + Values: []string{"shared-drive-root"}, + }}, + } + + members := make([]Member, calls) + codes := make([]string, calls) + errs := make(chan error, calls) + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(calls) + + for i := 0; i < calls; i++ { + members[i] = Member{Email: fmt.Sprintf("member-%04d@example.test", i)} + go func(i int) { + defer wg.Done() + <-start + code, err := s.GetInteractCode(inst, &members[i], i+1) + if err != nil { + errs <- err + return + } + codes[i] = code + }(i) + } + + close(start) + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err) + } + + interact, err := permission.GetForShareInteract(inst, sharingID) + require.NoError(t, err) + require.Len(t, interact.Codes, calls) + for i, member := range members { + require.NotEmpty(t, codes[i]) + require.Equal(t, codes[i], interact.Codes[member.Email]) + } + + var perms []permission.Permission + req := couchdb.FindRequest{ + UseIndex: "by-source-and-type", + Selector: mango.And( + mango.Equal("type", permission.TypeShareInteract), + mango.Equal("source_id", consts.Sharings+"/"+sharingID), + ), + Limit: calls, + } + require.NoError(t, couchdb.FindDocs(inst, consts.Permissions, &req, &perms)) + require.Len(t, perms, 1) + require.Equal(t, permission.ShareInteractPermissionID(sharingID), perms[0].ID()) +} diff --git a/model/sharing/sharing.go b/model/sharing/sharing.go index 845d969352c..cca9a642243 100644 --- a/model/sharing/sharing.go +++ b/model/sharing/sharing.go @@ -342,7 +342,7 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe interact, err := permission.GetForShareInteract(inst, s.ID()) if err != nil { if couchdb.IsNotFoundError(err) { - return s.CreateInteractPermissions(inst, member) + return s.createInteractPermissions(inst, member) } return "", err } @@ -364,7 +364,11 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe } if key == member.Instance || key == member.Email || key == indexKey { if needUpdate { - if err := couchdb.UpdateDoc(inst, interact); err != nil { + _, err := permission.CreateShareInteractSet(inst, s.SID, nil, permission.Permission{ + Permissions: set, + Metadata: interact.Metadata, + }) + if err != nil { return "", err } } @@ -385,19 +389,21 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe if err != nil { return "", err } - if interact.Codes == nil { - interact.Codes = make(map[string]string) - } - interact.Codes[key] = code - if err := couchdb.UpdateDoc(inst, interact); err != nil { + + updated, err := permission.CreateShareInteractSet(inst, s.SID, map[string]string{key: code}, permission.Permission{ + Permissions: set, + Metadata: interact.Metadata, + }) + if err != nil { return "", err } + if stored, ok := updated.Codes[key]; ok { + return stored, nil + } return code, nil } -// CreateInteractPermissions creates the permissions doc for reading and -// writing a note inside this sharing. -func (s *Sharing) CreateInteractPermissions(inst *instance.Instance, m *Member) (string, error) { +func (s *Sharing) createInteractPermissions(inst *instance.Instance, m *Member) (string, error) { key := m.Email if key == "" { key = m.Instance @@ -417,10 +423,13 @@ func (s *Sharing) CreateInteractPermissions(inst *instance.Instance, m *Member) Metadata: md, } - _, err = permission.CreateShareInteractSet(inst, s.SID, codes, doc) + interact, err := permission.CreateShareInteractSet(inst, s.SID, codes, doc) if err != nil { return "", err } + if stored, ok := interact.Codes[key]; ok { + return stored, nil + } return code, nil } diff --git a/pkg/utils/retry.go b/pkg/utils/retry.go index 85218b6bcff..0de8884b72b 100644 --- a/pkg/utils/retry.go +++ b/pkg/utils/retry.go @@ -1,23 +1,170 @@ package utils -import "time" +import ( + "context" + "math/rand" + "time" +) + +const maxRetryDelay = time.Duration(1<<63 - 1) + +// RetryOptions configures RetryWithBackoff. +type RetryOptions struct { + // Attempts is the maximum number of calls to fn. Values lower than 1 are + // treated as 1. + Attempts int + // Delay is the wait duration before the first retry. It doubles after each + // failed attempt. + Delay time.Duration + // MaxDelay caps the exponential backoff delay before jitter is applied. + MaxDelay time.Duration + // JitterFactor adds a random delay between 0 and the current backoff delay + // multiplied by JitterFactor. For example, 0.25 adds up to 25% jitter. + // Values lower than or equal to 0 disable jitter. + JitterFactor float64 + // ShouldRetry can be used to retry only some errors. When nil, every + // non-nil error is retried until Attempts is exhausted. + ShouldRetry func(error) bool + // OnRetry is called after a failed attempt and before sleeping. The attempt + // argument is the number of attempts already made, starting at 1. + OnRetry func(attempt int, err error, delay time.Duration) +} + +// RetryWithBackoff calls fn until it succeeds, ShouldRetry rejects the returned +// error, the context is done, or the maximum number of attempts is reached. +func RetryWithBackoff(ctx context.Context, opts RetryOptions, fn func() error) error { + _, err := RetryWithBackoffValue(ctx, opts, func() (struct{}, error) { + return struct{}{}, fn() + }) + return err +} + +// RetryWithBackoffValue calls fn until it succeeds, ShouldRetry rejects the +// returned error, the context is done, or the maximum number of attempts is +// reached. It returns the value returned by the successful call. +func RetryWithBackoffValue[T any](ctx context.Context, opts RetryOptions, fn func() (T, error)) (T, error) { + var zero T + if ctx == nil { + ctx = context.Background() + } + + attempts := opts.Attempts + if attempts < 1 { + attempts = 1 + } + + var err error + var value T + for attempt := 0; attempt < attempts; attempt++ { + select { + case <-ctx.Done(): + return zero, ctx.Err() + default: + } + + value, err = fn() + if err == nil { + return value, nil + } + if attempt == attempts-1 { + return zero, err + } + if opts.ShouldRetry != nil && !opts.ShouldRetry(err) { + return zero, err + } + + delay := retryDelay(opts.Delay, attempt, opts.MaxDelay, opts.JitterFactor) + if opts.OnRetry != nil { + opts.OnRetry(attempt+1, err, delay) + } + if err := sleepWithContext(ctx, delay); err != nil { + return zero, err + } + } + + return zero, err +} // RetryWithExpBackoff can be used to call several times a function until it // returns no error or the maximum count of calls has been reached. Between two // calls, it will wait, first by the given delay, and after that, the delay // will double after each failure. func RetryWithExpBackoff(count int, delay time.Duration, fn func() error) error { - err := fn() - if err == nil { - return nil + return RetryWithBackoff(context.Background(), RetryOptions{ + Attempts: count, + Delay: delay, + }, fn) +} + +func retryDelay(initial time.Duration, attempt int, maxDelay time.Duration, jitterFactor float64) time.Duration { + delay := backoffDelay(initial, attempt, maxDelay) + return addJitter(delay, jitterFactor) +} + +func backoffDelay(initial time.Duration, attempt int, maxDelay time.Duration) time.Duration { + if initial <= 0 { + return 0 } - for i := 1; i < count; i++ { - time.Sleep(delay) - delay *= 2 - err = fn() - if err == nil { + + delay := initial + for i := 0; i < attempt; i++ { + if delay > maxRetryDelay/2 { + delay = maxRetryDelay + } else { + delay *= 2 + } + if maxDelay > 0 && delay > maxDelay { + return maxDelay + } + } + if maxDelay > 0 && delay > maxDelay { + return maxDelay + } + return delay +} + +func addJitter(delay time.Duration, jitterFactor float64) time.Duration { + if delay <= 0 || jitterFactor <= 0 { + return delay + } + + maxJitterFloat := float64(delay) * jitterFactor + if maxJitterFloat < 1 { + return delay + } + var maxJitter time.Duration + if maxJitterFloat >= float64(maxRetryDelay) { + maxJitter = maxRetryDelay + } else { + maxJitter = time.Duration(maxJitterFloat) + } + if maxJitter <= 0 { + return delay + } + jitter := time.Duration(rand.Int63n(int64(maxJitter))) + if maxRetryDelay-delay < jitter { + return maxRetryDelay + } + return delay + jitter +} + +func sleepWithContext(ctx context.Context, delay time.Duration) error { + if delay <= 0 { + select { + case <-ctx.Done(): + return ctx.Err() + default: return nil } } - return err + + timer := time.NewTimer(delay) + defer timer.Stop() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } } diff --git a/pkg/utils/retry_test.go b/pkg/utils/retry_test.go new file mode 100644 index 00000000000..b385aa123b4 --- /dev/null +++ b/pkg/utils/retry_test.go @@ -0,0 +1,137 @@ +package utils + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRetryWithBackoffSucceedsAfterRetry(t *testing.T) { + errTemporary := errors.New("temporary") + calls := 0 + + err := RetryWithBackoff(context.Background(), RetryOptions{ + Attempts: 3, + }, func() error { + calls++ + if calls < 3 { + return errTemporary + } + return nil + }) + + require.NoError(t, err) + assert.Equal(t, 3, calls) +} + +func TestRetryWithBackoffValueReturnsSuccessfulValue(t *testing.T) { + errTemporary := errors.New("temporary") + calls := 0 + + value, err := RetryWithBackoffValue(context.Background(), RetryOptions{ + Attempts: 3, + }, func() (string, error) { + calls++ + if calls < 2 { + return "", errTemporary + } + return "done", nil + }) + + require.NoError(t, err) + assert.Equal(t, "done", value) + assert.Equal(t, 2, calls) +} + +func TestRetryWithBackoffStopsOnNonRetryableError(t *testing.T) { + errRetryable := errors.New("retryable") + errFatal := errors.New("fatal") + calls := 0 + + err := RetryWithBackoff(context.Background(), RetryOptions{ + Attempts: 5, + ShouldRetry: func(err error) bool { + return errors.Is(err, errRetryable) + }, + }, func() error { + calls++ + if calls == 1 { + return errRetryable + } + return errFatal + }) + + require.ErrorIs(t, err, errFatal) + assert.Equal(t, 2, calls) +} + +func TestRetryWithBackoffCapsDelay(t *testing.T) { + errTemporary := errors.New("temporary") + var delays []time.Duration + + err := RetryWithBackoff(context.Background(), RetryOptions{ + Attempts: 4, + Delay: time.Nanosecond, + MaxDelay: 2 * time.Nanosecond, + OnRetry: func(_ int, _ error, delay time.Duration) { + delays = append(delays, delay) + }, + }, func() error { + return errTemporary + }) + + require.ErrorIs(t, err, errTemporary) + assert.Equal(t, []time.Duration{ + time.Nanosecond, + 2 * time.Nanosecond, + 2 * time.Nanosecond, + }, delays) +} + +func TestRetryWithBackoffStopsWhenContextIsCanceled(t *testing.T) { + errTemporary := errors.New("temporary") + ctx, cancel := context.WithCancel(context.Background()) + calls := 0 + + err := RetryWithBackoff(ctx, RetryOptions{ + Attempts: 3, + Delay: time.Hour, + OnRetry: func(_ int, _ error, _ time.Duration) { + cancel() + }, + }, func() error { + calls++ + return errTemporary + }) + + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 1, calls) +} + +func TestRetryDelayWithJitter(t *testing.T) { + base := 4 * time.Second + + for i := 0; i < 10; i++ { + delay := retryDelay(base, 0, 0, 0.25) + + assert.GreaterOrEqual(t, delay, base) + assert.Less(t, delay, base+time.Second) + } +} + +func TestRetryWithExpBackoffRunsAtLeastOnce(t *testing.T) { + errTemporary := errors.New("temporary") + calls := 0 + + err := RetryWithExpBackoff(0, time.Nanosecond, func() error { + calls++ + return errTemporary + }) + + require.ErrorIs(t, err, errTemporary) + assert.Equal(t, 1, calls) +} From e170e2d5175d266d430cf3861aacbf4555da4977 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Tue, 19 May 2026 11:08:39 +0200 Subject: [PATCH 4/8] feat: repair multiple permissions docs for a shared drive on documents get(permissions check) --- model/permission/permissions.go | 20 ++++++++--- model/permission/permissions_test.go | 51 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index cb33dc02ad3..6f5d25ef1dd 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -281,7 +281,14 @@ func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Perm } err := couchdb.FindDocs(db, consts.Permissions, &req, &res) if err != nil { - return nil, err + // With a cluster of couchdb, we can have a race condition where we + // query an index before it has been updated for a doc that has just + // been created. Keep the same fallback as getFromSource. + time.Sleep(1 * time.Second) + err = couchdb.FindDocs(db, consts.Permissions, &req, &res) + if err != nil { + return nil, err + } } return res, nil } @@ -335,7 +342,11 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission canonicalID := ShareInteractPermissionID(sharingID) var canonical *Permission usable := 0 + hasDuplicate := false for i := range perms { + if perms[i].ID() != canonicalID { + hasDuplicate = true + } if perms[i].Expired() { continue } @@ -347,7 +358,7 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission if usable == 0 { return nil, false, ErrExpiredToken } - if usable == 1 && canonical != nil { + if usable == 1 && canonical != nil && !hasDuplicate { return canonical, false, nil } return nil, true, nil @@ -370,15 +381,14 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm hasCanonical = true // If the canonical doc is expired, update that doc ID but rebuild its content from non-expired docs. merged.SetRev(p.Rev()) + } else { + duplicates = append(duplicates, p) } if p.Expired() { continue } hasUsablePermission = true - if p.ID() != canonicalID { - duplicates = append(duplicates, p) - } if len(merged.Permissions) == 0 && len(p.Permissions) > 0 { merged.Permissions = p.Clone().(*Permission).Permissions diff --git a/model/permission/permissions_test.go b/model/permission/permissions_test.go index fce0ff8bd8e..227a90eca1e 100644 --- a/model/permission/permissions_test.go +++ b/model/permission/permissions_test.go @@ -494,6 +494,57 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { require.Len(t, all, 1) } +func TestGetForShareInteractRepairsExpiredDuplicateDocs(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) + require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + + const sharingID = "sharing-expired-duplicate-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(testPrefix, &Permission{ + PID: ShareInteractPermissionID(sharingID), + Type: TypeShareInteract, + Permissions: rules, + Codes: map[string]string{ + "alice@example.test": "alice-token", + }, + SourceID: consts.Sharings + "/" + sharingID, + }) + require.NoError(t, err) + err = couchdb.CreateDoc(testPrefix, &Permission{ + Type: TypeShareInteract, + Permissions: rules, + Codes: map[string]string{ + "expired@example.test": "expired-token", + }, + ExpiresAt: expiredAt, + SourceID: consts.Sharings + "/" + sharingID, + }) + require.NoError(t, err) + + repaired, err := GetForShareInteract(testPrefix, sharingID) + require.NoError(t, err) + require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID()) + require.Equal(t, map[string]string{ + "alice@example.test": "alice-token", + }, repaired.Codes) + + all, err := getShareInteractPermissions(testPrefix, sharingID) + require.NoError(t, err) + require.Len(t, all, 1) + require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) +} + func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) { if testing.Short() { t.Skip("an instance is required for this test: test skipped due to the use of --short flag") From e40924d7044000623a4c5736edbf8cad7cdc8646 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Tue, 19 May 2026 11:45:56 +0200 Subject: [PATCH 5/8] feat: repair multiple permissions docs (fixes) --- model/permission/permissions.go | 100 ++++++++++++++------------- model/permission/permissions_test.go | 40 ++++++----- model/sharing/sharing.go | 2 +- pkg/utils/retry.go | 19 ++--- pkg/utils/retry_test.go | 10 ++- 5 files changed, 95 insertions(+), 76 deletions(-) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index 6f5d25ef1dd..d8b4611a903 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -7,11 +7,12 @@ import ( "encoding/json" "fmt" "net/http" + "slices" "strings" "time" build "github.com/cozy/cozy-stack/pkg/config" - stackconfig "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/config/config" "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/couchdb" "github.com/cozy/cozy-stack/pkg/couchdb/mango" @@ -250,12 +251,6 @@ func GetForSharePreview(db prefixer.Prefixer, sharingID string) (*Permission, er // docs as part of the read by creating/updating the canonical permission doc // and deleting duplicate legacy docs. func GetForShareInteract(db prefixer.Prefixer, sharingID string) (*Permission, error) { - mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID)) - if err := mu.Lock(); err != nil { - return nil, err - } - defer mu.Unlock() - return getOrRepairShareInteractPermissions(db, sharingID) } @@ -294,30 +289,43 @@ func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Perm } func getOrRepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) { - return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) { - perms, err := getShareInteractPermissions(db, sharingID) - if err != nil { - return nil, err - } - if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair { - return canonical, err - } + perms, err := getShareInteractPermissions(db, sharingID) + if err != nil { + return nil, err + } + if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair { + return canonical, err + } - perm, retry, err := repairShareInteractPermissions(db, sharingID, perms) - if err == nil && !retry { - return perm, nil - } - if retry && err == nil { - return nil, &couchdb.Error{ - StatusCode: http.StatusConflict, - Name: "conflict", - Reason: "could not repair share-interact permissions after retries", - } - } + return repairShareInteractPermissionsWithLock(db, sharingID) +} + +func repairShareInteractPermissionsWithLock(db prefixer.Prefixer, sharingID string) (*Permission, error) { + mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID)) + if err := mu.Lock(); err != nil { return nil, err + } + defer mu.Unlock() + + return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) { + return getOrRepairShareInteractPermissionsOnce(db, sharingID) }) } +// getOrRepairShareInteractPermissionsOnce performs a single read+repair pass. +// The caller must hold the share-interact write lock for sharingID. +func getOrRepairShareInteractPermissionsOnce(db prefixer.Prefixer, sharingID string) (*Permission, error) { + perms, err := getShareInteractPermissions(db, sharingID) + if err != nil { + return nil, err + } + if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair { + return canonical, err + } + + return repairShareInteractPermissions(db, sharingID, perms) +} + func shareInteractRetryOptions() utils.RetryOptions { return utils.RetryOptions{ Attempts: 5, @@ -344,15 +352,17 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission usable := 0 hasDuplicate := false for i := range perms { - if perms[i].ID() != canonicalID { + p := &perms[i] + isCanonical := p.ID() == canonicalID + if !isCanonical { hasDuplicate = true } - if perms[i].Expired() { + if p.Expired() { continue } usable++ - if perms[i].ID() == canonicalID { - canonical = &perms[i] + if isCanonical { + canonical = p } } if usable == 0 { @@ -364,7 +374,10 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission return nil, true, nil } -func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, bool, error) { +// repairShareInteractPermissions merges usable share-interact permission docs +// into the canonical doc and deletes non-canonical duplicates. The caller must +// hold the share-interact write lock for sharingID. +func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, error) { canonicalID := ShareInteractPermissionID(sharingID) merged := &Permission{ PID: canonicalID, @@ -391,7 +404,7 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm hasUsablePermission = true if len(merged.Permissions) == 0 && len(p.Permissions) > 0 { - merged.Permissions = p.Clone().(*Permission).Permissions + merged.Permissions = slices.Clone(p.Permissions) } if merged.Metadata == nil && p.Metadata != nil { merged.Metadata = p.Metadata.Clone() @@ -413,21 +426,15 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm } } if !hasUsablePermission { - return nil, false, ErrExpiredToken + return nil, ErrExpiredToken } if !hasCanonical { if err := couchdb.CreateNamedDoc(db, merged); err != nil { - if couchdb.IsConflictError(err) || couchdb.IsFileExists(err) { - return nil, true, err - } - return nil, false, err + return nil, err } } else if err := couchdb.UpdateDoc(db, merged); err != nil { - if couchdb.IsConflictError(err) { - return nil, true, err - } - return nil, false, err + return nil, err } for _, p := range duplicates { @@ -435,14 +442,11 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm if couchdb.IsNotFoundError(err) { continue } - if couchdb.IsConflictError(err) { - return nil, true, err - } - return nil, false, err + return nil, err } } - return merged, false, nil + return merged, nil } func getFromSource(db prefixer.Prefixer, permType, docType, slug string) (*Permission, error) { @@ -820,14 +824,14 @@ func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[st doc.Codes = make(map[string]string) } - mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID)) + mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID)) if err := mu.Lock(); err != nil { return nil, err } defer mu.Unlock() return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) { - existing, err := getOrRepairShareInteractPermissions(db, sharingID) + existing, err := getOrRepairShareInteractPermissionsOnce(db, sharingID) if err != nil && !couchdb.IsNotFoundError(err) { return nil, err } diff --git a/model/permission/permissions_test.go b/model/permission/permissions_test.go index 227a90eca1e..cfcc54e5349 100644 --- a/model/permission/permissions_test.go +++ b/model/permission/permissions_test.go @@ -17,7 +17,10 @@ import ( "github.com/stretchr/testify/require" ) -var testPrefix = prefixer.NewPrefixer(0, "test", "permission-tests") +func testPrefix(t *testing.T) prefixer.Prefixer { + t.Helper() + return prefixer.NewPrefixer(0, "test", t.Name()) +} func TestCheckDoctypeName(t *testing.T) { assert.NoError(t, CheckDoctypeName("io.cozy.files", false)) @@ -438,7 +441,8 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { } config.UseTestFile(t) - require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + db := testPrefix(t) + require.NoError(t, couchdb.ResetDB(db, consts.Permissions)) const sharingID = "sharing-duplicate-interact-permissions" perms := Permission{ @@ -450,7 +454,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { }}, } - err := couchdb.CreateDoc(testPrefix, &Permission{ + err := couchdb.CreateDoc(db, &Permission{ Type: TypeShareInteract, Permissions: perms.Permissions, Codes: map[string]string{ @@ -459,7 +463,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { SourceID: consts.Sharings + "/" + sharingID, }) require.NoError(t, err) - err = couchdb.CreateDoc(testPrefix, &Permission{ + err = couchdb.CreateDoc(db, &Permission{ Type: TypeShareInteract, Permissions: perms.Permissions, Codes: map[string]string{ @@ -469,7 +473,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { }) require.NoError(t, err) - repaired, err := GetForShareInteract(testPrefix, sharingID) + repaired, err := GetForShareInteract(db, sharingID) require.NoError(t, err) require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID()) require.Equal(t, map[string]string{ @@ -477,19 +481,19 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) { "bob@example.test": "bob-token", }, repaired.Codes) - all, err := getShareInteractPermissions(testPrefix, sharingID) + all, err := getShareInteractPermissions(db, sharingID) require.NoError(t, err) require.Len(t, all, 1) require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) - repaired, err = GetForShareInteract(testPrefix, sharingID) + repaired, err = GetForShareInteract(db, sharingID) require.NoError(t, err) require.Equal(t, map[string]string{ "alice@example.test": "alice-token", "bob@example.test": "bob-token", }, repaired.Codes) - all, err = getShareInteractPermissions(testPrefix, sharingID) + all, err = getShareInteractPermissions(db, sharingID) require.NoError(t, err) require.Len(t, all, 1) } @@ -500,7 +504,8 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) { } config.UseTestFile(t) - require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + db := testPrefix(t) + require.NoError(t, couchdb.ResetDB(db, consts.Permissions)) const sharingID = "sharing-expired-duplicate-interact-permissions" rules := Set{{ @@ -511,7 +516,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) { }} expiredAt := time.Now().Add(-time.Hour).Format(time.RFC3339) - err := couchdb.CreateNamedDoc(testPrefix, &Permission{ + err := couchdb.CreateNamedDoc(db, &Permission{ PID: ShareInteractPermissionID(sharingID), Type: TypeShareInteract, Permissions: rules, @@ -521,7 +526,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) { SourceID: consts.Sharings + "/" + sharingID, }) require.NoError(t, err) - err = couchdb.CreateDoc(testPrefix, &Permission{ + err = couchdb.CreateDoc(db, &Permission{ Type: TypeShareInteract, Permissions: rules, Codes: map[string]string{ @@ -532,14 +537,14 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) { }) require.NoError(t, err) - repaired, err := GetForShareInteract(testPrefix, sharingID) + repaired, err := GetForShareInteract(db, sharingID) require.NoError(t, err) require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID()) require.Equal(t, map[string]string{ "alice@example.test": "alice-token", }, repaired.Codes) - all, err := getShareInteractPermissions(testPrefix, sharingID) + all, err := getShareInteractPermissions(db, sharingID) require.NoError(t, err) require.Len(t, all, 1) require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) @@ -551,7 +556,8 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) { } config.UseTestFile(t) - require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions)) + db := testPrefix(t) + require.NoError(t, couchdb.ResetDB(db, consts.Permissions)) const sharingID = "sharing-canonical-interact-permissions" md := metadata.New() @@ -566,13 +572,13 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) { Metadata: md, } - first, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + first, err := CreateShareInteractSet(db, sharingID, map[string]string{ "alice@example.test": "alice-token", }, perms) require.NoError(t, err) require.Equal(t, ShareInteractPermissionID(sharingID), first.ID()) - second, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{ + second, err := CreateShareInteractSet(db, sharingID, map[string]string{ "bob@example.test": "bob-token", }, perms) require.NoError(t, err) @@ -585,7 +591,7 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) { require.NotNil(t, second.Metadata) require.True(t, second.Metadata.UpdatedAt.After(first.Metadata.UpdatedAt)) - all, err := getShareInteractPermissions(testPrefix, sharingID) + all, err := getShareInteractPermissions(db, sharingID) require.NoError(t, err) require.Len(t, all, 1) require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID()) diff --git a/model/sharing/sharing.go b/model/sharing/sharing.go index cca9a642243..626b2971ce2 100644 --- a/model/sharing/sharing.go +++ b/model/sharing/sharing.go @@ -400,7 +400,7 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe if stored, ok := updated.Codes[key]; ok { return stored, nil } - return code, nil + return "", fmt.Errorf("share-interact code for %s was not stored in sharing %s", key, s.SID) } func (s *Sharing) createInteractPermissions(inst *instance.Instance, m *Member) (string, error) { diff --git a/pkg/utils/retry.go b/pkg/utils/retry.go index 0de8884b72b..f7502c66610 100644 --- a/pkg/utils/retry.go +++ b/pkg/utils/retry.go @@ -2,7 +2,7 @@ package utils import ( "context" - "math/rand" + "math/rand/v2" "time" ) @@ -19,7 +19,8 @@ type RetryOptions struct { // MaxDelay caps the exponential backoff delay before jitter is applied. MaxDelay time.Duration // JitterFactor adds a random delay between 0 and the current backoff delay - // multiplied by JitterFactor. For example, 0.25 adds up to 25% jitter. + // multiplied by JitterFactor. This jitter is one-sided: it can only extend + // the delay, never shorten it. For example, 0.25 adds up to 25% jitter. // Values lower than or equal to 0 disable jitter. JitterFactor float64 // ShouldRetry can be used to retry only some errors. When nil, every @@ -109,14 +110,14 @@ func backoffDelay(initial time.Duration, attempt int, maxDelay time.Duration) ti delay := initial for i := 0; i < attempt; i++ { if delay > maxRetryDelay/2 { - delay = maxRetryDelay - } else { - delay *= 2 - } - if maxDelay > 0 && delay > maxDelay { - return maxDelay + return cappedDelay(maxRetryDelay, maxDelay) } + delay *= 2 } + return cappedDelay(delay, maxDelay) +} + +func cappedDelay(delay time.Duration, maxDelay time.Duration) time.Duration { if maxDelay > 0 && delay > maxDelay { return maxDelay } @@ -141,7 +142,7 @@ func addJitter(delay time.Duration, jitterFactor float64) time.Duration { if maxJitter <= 0 { return delay } - jitter := time.Duration(rand.Int63n(int64(maxJitter))) + jitter := time.Duration(rand.Int64N(int64(maxJitter))) if maxRetryDelay-delay < jitter { return maxRetryDelay } diff --git a/pkg/utils/retry_test.go b/pkg/utils/retry_test.go index b385aa123b4..d3d29f846df 100644 --- a/pkg/utils/retry_test.go +++ b/pkg/utils/retry_test.go @@ -92,6 +92,11 @@ func TestRetryWithBackoffCapsDelay(t *testing.T) { }, delays) } +func TestRetryDelaySaturatesOnOverflow(t *testing.T) { + assert.Equal(t, maxRetryDelay, backoffDelay(maxRetryDelay, 1, 0)) + assert.Equal(t, time.Hour, backoffDelay(maxRetryDelay, 1, time.Hour)) +} + func TestRetryWithBackoffStopsWhenContextIsCanceled(t *testing.T) { errTemporary := errors.New("temporary") ctx, cancel := context.WithCancel(context.Background()) @@ -114,13 +119,16 @@ func TestRetryWithBackoffStopsWhenContextIsCanceled(t *testing.T) { func TestRetryDelayWithJitter(t *testing.T) { base := 4 * time.Second + hasJitter := false - for i := 0; i < 10; i++ { + for i := 0; i < 100; i++ { delay := retryDelay(base, 0, 0, 0.25) assert.GreaterOrEqual(t, delay, base) assert.Less(t, delay, base+time.Second) + hasJitter = hasJitter || delay > base } + assert.True(t, hasJitter) } func TestRetryWithExpBackoffRunsAtLeastOnce(t *testing.T) { From 177a4766dbadc4ee64b05a1131d1a2a83acc985f Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Tue, 19 May 2026 13:30:25 +0200 Subject: [PATCH 6/8] feat: repair multiple permissions docs (fixes) --- model/permission/permissions.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index d8b4611a903..ad1aba2c634 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -820,10 +820,6 @@ func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[st Metadata: subdoc.Metadata, } - if doc.Codes == nil { - doc.Codes = make(map[string]string) - } - mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID)) if err := mu.Lock(); err != nil { return nil, err From e1ff361cc017b3dea8dbd4ef2fbc4a8ffc93103c Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Tue, 19 May 2026 13:44:10 +0200 Subject: [PATCH 7/8] fix: remove a repair flow on share-interact permission creation --- model/permission/permissions.go | 77 +++++++++++++------------ model/permission/permissions_test.go | 86 ++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 38 deletions(-) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index ad1aba2c634..befbf392f48 100644 --- a/model/permission/permissions.go +++ b/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 }) } diff --git a/model/permission/permissions_test.go b/model/permission/permissions_test.go index cfcc54e5349..24974148672 100644 --- a/model/permission/permissions_test.go +++ b/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{ From 9b1bb9cd39403e5e9bbe79e447729ec920de81c3 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Wed, 20 May 2026 10:14:04 +0200 Subject: [PATCH 8/8] fix: remove a repair flow on share-interact(comment fix) --- model/permission/permissions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/permission/permissions.go b/model/permission/permissions.go index befbf392f48..86301ddaa97 100644 --- a/model/permission/permissions.go +++ b/model/permission/permissions.go @@ -392,7 +392,7 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm p := &perms[i] if p.ID() == canonicalID { hasCanonical = true - // If the canonical doc is expired, update that doc ID but rebuild its content from non-expired docs. + // Keep the canonical doc revision and then overwrite the document with content rebuilt from non-expired docs. merged.SetRev(p.Rev()) } else { duplicates = append(duplicates, p)