Skip to content

Commit eb951fb

Browse files
shepilovshepilov
committed
feat: repair multiple permissions docs (fixes)
1 parent e170e2d commit eb951fb

5 files changed

Lines changed: 79 additions & 53 deletions

File tree

‎model/permission/permissions.go‎

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,12 @@ import (
77
"encoding/json"
88
"fmt"
99
"net/http"
10+
"slices"
1011
"strings"
1112
"time"
1213

1314
build "github.com/cozy/cozy-stack/pkg/config"
14-
stackconfig "github.com/cozy/cozy-stack/pkg/config/config"
15+
"github.com/cozy/cozy-stack/pkg/config/config"
1516
"github.com/cozy/cozy-stack/pkg/consts"
1617
"github.com/cozy/cozy-stack/pkg/couchdb"
1718
"github.com/cozy/cozy-stack/pkg/couchdb/mango"
@@ -250,12 +251,6 @@ func GetForSharePreview(db prefixer.Prefixer, sharingID string) (*Permission, er
250251
// docs as part of the read by creating/updating the canonical permission doc
251252
// and deleting duplicate legacy docs.
252253
func GetForShareInteract(db prefixer.Prefixer, sharingID string) (*Permission, error) {
253-
mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID))
254-
if err := mu.Lock(); err != nil {
255-
return nil, err
256-
}
257-
defer mu.Unlock()
258-
259254
return getOrRepairShareInteractPermissions(db, sharingID)
260255
}
261256

@@ -303,21 +298,35 @@ func getOrRepairShareInteractPermissions(db prefixer.Prefixer, sharingID string)
303298
return canonical, err
304299
}
305300

306-
perm, retry, err := repairShareInteractPermissions(db, sharingID, perms)
307-
if err == nil && !retry {
308-
return perm, nil
309-
}
310-
if retry && err == nil {
311-
return nil, &couchdb.Error{
312-
StatusCode: http.StatusConflict,
313-
Name: "conflict",
314-
Reason: "could not repair share-interact permissions after retries",
315-
}
301+
mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
302+
if err := mu.Lock(); err != nil {
303+
return nil, err
316304
}
317-
return nil, err
305+
defer mu.Unlock()
306+
307+
return getOrRepairShareInteractPermissionsLocked(db, sharingID)
318308
})
319309
}
320310

311+
func getOrRepairShareInteractPermissionsLocked(db prefixer.Prefixer, sharingID string) (*Permission, error) {
312+
perms, err := getShareInteractPermissions(db, sharingID)
313+
if err != nil {
314+
return nil, err
315+
}
316+
if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
317+
return canonical, err
318+
}
319+
320+
perm, retry, err := repairShareInteractPermissions(db, sharingID, perms)
321+
if err == nil && !retry {
322+
return perm, nil
323+
}
324+
if retry && err == nil {
325+
return nil, fmt.Errorf("could not repair share-interact permissions after retries")
326+
}
327+
return nil, err
328+
}
329+
321330
func shareInteractRetryOptions() utils.RetryOptions {
322331
return utils.RetryOptions{
323332
Attempts: 5,
@@ -344,15 +353,17 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission
344353
usable := 0
345354
hasDuplicate := false
346355
for i := range perms {
347-
if perms[i].ID() != canonicalID {
356+
p := &perms[i]
357+
isCanonical := p.ID() == canonicalID
358+
if !isCanonical {
348359
hasDuplicate = true
349360
}
350-
if perms[i].Expired() {
361+
if p.Expired() {
351362
continue
352363
}
353364
usable++
354-
if perms[i].ID() == canonicalID {
355-
canonical = &perms[i]
365+
if isCanonical {
366+
canonical = p
356367
}
357368
}
358369
if usable == 0 {
@@ -391,7 +402,7 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm
391402
hasUsablePermission = true
392403

393404
if len(merged.Permissions) == 0 && len(p.Permissions) > 0 {
394-
merged.Permissions = p.Clone().(*Permission).Permissions
405+
merged.Permissions = slices.Clone(p.Permissions)
395406
}
396407
if merged.Metadata == nil && p.Metadata != nil {
397408
merged.Metadata = p.Metadata.Clone()
@@ -820,14 +831,14 @@ func CreateShareInteractSet(db prefixer.Prefixer, sharingID string, codes map[st
820831
doc.Codes = make(map[string]string)
821832
}
822833

823-
mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID))
834+
mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
824835
if err := mu.Lock(); err != nil {
825836
return nil, err
826837
}
827838
defer mu.Unlock()
828839

829840
return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
830-
existing, err := getOrRepairShareInteractPermissions(db, sharingID)
841+
existing, err := getOrRepairShareInteractPermissionsLocked(db, sharingID)
831842
if err != nil && !couchdb.IsNotFoundError(err) {
832843
return nil, err
833844
}

‎model/permission/permissions_test.go‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ import (
1717
"github.com/stretchr/testify/require"
1818
)
1919

20-
var testPrefix = prefixer.NewPrefixer(0, "test", "permission-tests")
20+
func testPrefix(t *testing.T) prefixer.Prefixer {
21+
t.Helper()
22+
return prefixer.NewPrefixer(0, "test", t.Name())
23+
}
2124

2225
func TestCheckDoctypeName(t *testing.T) {
2326
assert.NoError(t, CheckDoctypeName("io.cozy.files", false))
@@ -438,7 +441,8 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
438441
}
439442

440443
config.UseTestFile(t)
441-
require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
444+
db := testPrefix(t)
445+
require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
442446

443447
const sharingID = "sharing-duplicate-interact-permissions"
444448
perms := Permission{
@@ -450,7 +454,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
450454
}},
451455
}
452456

453-
err := couchdb.CreateDoc(testPrefix, &Permission{
457+
err := couchdb.CreateDoc(db, &Permission{
454458
Type: TypeShareInteract,
455459
Permissions: perms.Permissions,
456460
Codes: map[string]string{
@@ -459,7 +463,7 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
459463
SourceID: consts.Sharings + "/" + sharingID,
460464
})
461465
require.NoError(t, err)
462-
err = couchdb.CreateDoc(testPrefix, &Permission{
466+
err = couchdb.CreateDoc(db, &Permission{
463467
Type: TypeShareInteract,
464468
Permissions: perms.Permissions,
465469
Codes: map[string]string{
@@ -469,27 +473,27 @@ func TestGetForShareInteractRepairsDuplicateDocs(t *testing.T) {
469473
})
470474
require.NoError(t, err)
471475

472-
repaired, err := GetForShareInteract(testPrefix, sharingID)
476+
repaired, err := GetForShareInteract(db, sharingID)
473477
require.NoError(t, err)
474478
require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID())
475479
require.Equal(t, map[string]string{
476480
"alice@example.test": "alice-token",
477481
"bob@example.test": "bob-token",
478482
}, repaired.Codes)
479483

480-
all, err := getShareInteractPermissions(testPrefix, sharingID)
484+
all, err := getShareInteractPermissions(db, sharingID)
481485
require.NoError(t, err)
482486
require.Len(t, all, 1)
483487
require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())
484488

485-
repaired, err = GetForShareInteract(testPrefix, sharingID)
489+
repaired, err = GetForShareInteract(db, sharingID)
486490
require.NoError(t, err)
487491
require.Equal(t, map[string]string{
488492
"alice@example.test": "alice-token",
489493
"bob@example.test": "bob-token",
490494
}, repaired.Codes)
491495

492-
all, err = getShareInteractPermissions(testPrefix, sharingID)
496+
all, err = getShareInteractPermissions(db, sharingID)
493497
require.NoError(t, err)
494498
require.Len(t, all, 1)
495499
}
@@ -500,7 +504,8 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
500504
}
501505

502506
config.UseTestFile(t)
503-
require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
507+
db := testPrefix(t)
508+
require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
504509

505510
const sharingID = "sharing-expired-duplicate-interact-permissions"
506511
rules := Set{{
@@ -511,7 +516,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
511516
}}
512517
expiredAt := time.Now().Add(-time.Hour).Format(time.RFC3339)
513518

514-
err := couchdb.CreateNamedDoc(testPrefix, &Permission{
519+
err := couchdb.CreateNamedDoc(db, &Permission{
515520
PID: ShareInteractPermissionID(sharingID),
516521
Type: TypeShareInteract,
517522
Permissions: rules,
@@ -521,7 +526,7 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
521526
SourceID: consts.Sharings + "/" + sharingID,
522527
})
523528
require.NoError(t, err)
524-
err = couchdb.CreateDoc(testPrefix, &Permission{
529+
err = couchdb.CreateDoc(db, &Permission{
525530
Type: TypeShareInteract,
526531
Permissions: rules,
527532
Codes: map[string]string{
@@ -532,14 +537,14 @@ func TestGetForShareInteractRepairsExpiredDuplicateDocs(t *testing.T) {
532537
})
533538
require.NoError(t, err)
534539

535-
repaired, err := GetForShareInteract(testPrefix, sharingID)
540+
repaired, err := GetForShareInteract(db, sharingID)
536541
require.NoError(t, err)
537542
require.Equal(t, ShareInteractPermissionID(sharingID), repaired.ID())
538543
require.Equal(t, map[string]string{
539544
"alice@example.test": "alice-token",
540545
}, repaired.Codes)
541546

542-
all, err := getShareInteractPermissions(testPrefix, sharingID)
547+
all, err := getShareInteractPermissions(db, sharingID)
543548
require.NoError(t, err)
544549
require.Len(t, all, 1)
545550
require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())
@@ -551,7 +556,8 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
551556
}
552557

553558
config.UseTestFile(t)
554-
require.NoError(t, couchdb.ResetDB(testPrefix, consts.Permissions))
559+
db := testPrefix(t)
560+
require.NoError(t, couchdb.ResetDB(db, consts.Permissions))
555561

556562
const sharingID = "sharing-canonical-interact-permissions"
557563
md := metadata.New()
@@ -566,13 +572,13 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
566572
Metadata: md,
567573
}
568574

569-
first, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{
575+
first, err := CreateShareInteractSet(db, sharingID, map[string]string{
570576
"alice@example.test": "alice-token",
571577
}, perms)
572578
require.NoError(t, err)
573579
require.Equal(t, ShareInteractPermissionID(sharingID), first.ID())
574580

575-
second, err := CreateShareInteractSet(testPrefix, sharingID, map[string]string{
581+
second, err := CreateShareInteractSet(db, sharingID, map[string]string{
576582
"bob@example.test": "bob-token",
577583
}, perms)
578584
require.NoError(t, err)
@@ -585,7 +591,7 @@ func TestCreateShareInteractSetUsesCanonicalDoc(t *testing.T) {
585591
require.NotNil(t, second.Metadata)
586592
require.True(t, second.Metadata.UpdatedAt.After(first.Metadata.UpdatedAt))
587593

588-
all, err := getShareInteractPermissions(testPrefix, sharingID)
594+
all, err := getShareInteractPermissions(db, sharingID)
589595
require.NoError(t, err)
590596
require.Len(t, all, 1)
591597
require.Equal(t, ShareInteractPermissionID(sharingID), all[0].ID())

‎model/sharing/sharing.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe
400400
if stored, ok := updated.Codes[key]; ok {
401401
return stored, nil
402402
}
403-
return code, nil
403+
return "", fmt.Errorf("share-interact code for %s was not stored in sharing %s", key, s.SID)
404404
}
405405

406406
func (s *Sharing) createInteractPermissions(inst *instance.Instance, m *Member) (string, error) {

‎pkg/utils/retry.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ package utils
22

33
import (
44
"context"
5-
"math/rand"
5+
"math/rand/v2"
66
"time"
77
)
88

@@ -19,7 +19,8 @@ type RetryOptions struct {
1919
// MaxDelay caps the exponential backoff delay before jitter is applied.
2020
MaxDelay time.Duration
2121
// JitterFactor adds a random delay between 0 and the current backoff delay
22-
// multiplied by JitterFactor. For example, 0.25 adds up to 25% jitter.
22+
// multiplied by JitterFactor. This jitter is one-sided: it can only extend
23+
// the delay, never shorten it. For example, 0.25 adds up to 25% jitter.
2324
// Values lower than or equal to 0 disable jitter.
2425
JitterFactor float64
2526
// 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
109110
delay := initial
110111
for i := 0; i < attempt; i++ {
111112
if delay > maxRetryDelay/2 {
112-
delay = maxRetryDelay
113-
} else {
114-
delay *= 2
115-
}
116-
if maxDelay > 0 && delay > maxDelay {
117-
return maxDelay
113+
return cappedDelay(maxRetryDelay, maxDelay)
118114
}
115+
delay *= 2
119116
}
117+
return cappedDelay(delay, maxDelay)
118+
}
119+
120+
func cappedDelay(delay time.Duration, maxDelay time.Duration) time.Duration {
120121
if maxDelay > 0 && delay > maxDelay {
121122
return maxDelay
122123
}
@@ -141,7 +142,7 @@ func addJitter(delay time.Duration, jitterFactor float64) time.Duration {
141142
if maxJitter <= 0 {
142143
return delay
143144
}
144-
jitter := time.Duration(rand.Int63n(int64(maxJitter)))
145+
jitter := time.Duration(rand.Int64N(int64(maxJitter)))
145146
if maxRetryDelay-delay < jitter {
146147
return maxRetryDelay
147148
}

‎pkg/utils/retry_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ func TestRetryWithBackoffCapsDelay(t *testing.T) {
9292
}, delays)
9393
}
9494

95+
func TestRetryDelaySaturatesOnOverflow(t *testing.T) {
96+
assert.Equal(t, maxRetryDelay, backoffDelay(maxRetryDelay, 1, 0))
97+
assert.Equal(t, time.Hour, backoffDelay(maxRetryDelay, 1, time.Hour))
98+
}
99+
95100
func TestRetryWithBackoffStopsWhenContextIsCanceled(t *testing.T) {
96101
errTemporary := errors.New("temporary")
97102
ctx, cancel := context.WithCancel(context.Background())
@@ -114,13 +119,16 @@ func TestRetryWithBackoffStopsWhenContextIsCanceled(t *testing.T) {
114119

115120
func TestRetryDelayWithJitter(t *testing.T) {
116121
base := 4 * time.Second
122+
hasJitter := false
117123

118-
for i := 0; i < 10; i++ {
124+
for i := 0; i < 100; i++ {
119125
delay := retryDelay(base, 0, 0, 0.25)
120126

121127
assert.GreaterOrEqual(t, delay, base)
122128
assert.Less(t, delay, base+time.Second)
129+
hasJitter = hasJitter || delay > base
123130
}
131+
assert.True(t, hasJitter)
124132
}
125133

126134
func TestRetryWithExpBackoffRunsAtLeastOnce(t *testing.T) {

0 commit comments

Comments
 (0)