Skip to content

Commit e40924d

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

5 files changed

Lines changed: 95 additions & 76 deletions

File tree

‎model/permission/permissions.go‎

Lines changed: 52 additions & 48 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

@@ -294,30 +289,43 @@ func getShareInteractPermissions(db prefixer.Prefixer, sharingID string) ([]Perm
294289
}
295290

296291
func getOrRepairShareInteractPermissions(db prefixer.Prefixer, sharingID string) (*Permission, error) {
297-
return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
298-
perms, err := getShareInteractPermissions(db, sharingID)
299-
if err != nil {
300-
return nil, err
301-
}
302-
if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
303-
return canonical, err
304-
}
292+
perms, err := getShareInteractPermissions(db, sharingID)
293+
if err != nil {
294+
return nil, err
295+
}
296+
if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
297+
return canonical, err
298+
}
305299

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-
}
316-
}
300+
return repairShareInteractPermissionsWithLock(db, sharingID)
301+
}
302+
303+
func repairShareInteractPermissionsWithLock(db prefixer.Prefixer, sharingID string) (*Permission, error) {
304+
mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
305+
if err := mu.Lock(); err != nil {
317306
return nil, err
307+
}
308+
defer mu.Unlock()
309+
310+
return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
311+
return getOrRepairShareInteractPermissionsOnce(db, sharingID)
318312
})
319313
}
320314

315+
// getOrRepairShareInteractPermissionsOnce performs a single read+repair pass.
316+
// The caller must hold the share-interact write lock for sharingID.
317+
func getOrRepairShareInteractPermissionsOnce(db prefixer.Prefixer, sharingID string) (*Permission, error) {
318+
perms, err := getShareInteractPermissions(db, sharingID)
319+
if err != nil {
320+
return nil, err
321+
}
322+
if canonical, needsRepair, err := shareInteractRepairState(perms, sharingID); err != nil || !needsRepair {
323+
return canonical, err
324+
}
325+
326+
return repairShareInteractPermissions(db, sharingID, perms)
327+
}
328+
321329
func shareInteractRetryOptions() utils.RetryOptions {
322330
return utils.RetryOptions{
323331
Attempts: 5,
@@ -344,15 +352,17 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission
344352
usable := 0
345353
hasDuplicate := false
346354
for i := range perms {
347-
if perms[i].ID() != canonicalID {
355+
p := &perms[i]
356+
isCanonical := p.ID() == canonicalID
357+
if !isCanonical {
348358
hasDuplicate = true
349359
}
350-
if perms[i].Expired() {
360+
if p.Expired() {
351361
continue
352362
}
353363
usable++
354-
if perms[i].ID() == canonicalID {
355-
canonical = &perms[i]
364+
if isCanonical {
365+
canonical = p
356366
}
357367
}
358368
if usable == 0 {
@@ -364,7 +374,10 @@ func shareInteractRepairState(perms []Permission, sharingID string) (*Permission
364374
return nil, true, nil
365375
}
366376

367-
func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, bool, error) {
377+
// repairShareInteractPermissions merges usable share-interact permission docs
378+
// into the canonical doc and deletes non-canonical duplicates. The caller must
379+
// hold the share-interact write lock for sharingID.
380+
func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perms []Permission) (*Permission, error) {
368381
canonicalID := ShareInteractPermissionID(sharingID)
369382
merged := &Permission{
370383
PID: canonicalID,
@@ -391,7 +404,7 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm
391404
hasUsablePermission = true
392405

393406
if len(merged.Permissions) == 0 && len(p.Permissions) > 0 {
394-
merged.Permissions = p.Clone().(*Permission).Permissions
407+
merged.Permissions = slices.Clone(p.Permissions)
395408
}
396409
if merged.Metadata == nil && p.Metadata != nil {
397410
merged.Metadata = p.Metadata.Clone()
@@ -413,36 +426,27 @@ func repairShareInteractPermissions(db prefixer.Prefixer, sharingID string, perm
413426
}
414427
}
415428
if !hasUsablePermission {
416-
return nil, false, ErrExpiredToken
429+
return nil, ErrExpiredToken
417430
}
418431

419432
if !hasCanonical {
420433
if err := couchdb.CreateNamedDoc(db, merged); err != nil {
421-
if couchdb.IsConflictError(err) || couchdb.IsFileExists(err) {
422-
return nil, true, err
423-
}
424-
return nil, false, err
434+
return nil, err
425435
}
426436
} else if err := couchdb.UpdateDoc(db, merged); err != nil {
427-
if couchdb.IsConflictError(err) {
428-
return nil, true, err
429-
}
430-
return nil, false, err
437+
return nil, err
431438
}
432439

433440
for _, p := range duplicates {
434441
if err := couchdb.DeleteDoc(db, p); err != nil {
435442
if couchdb.IsNotFoundError(err) {
436443
continue
437444
}
438-
if couchdb.IsConflictError(err) {
439-
return nil, true, err
440-
}
441-
return nil, false, err
445+
return nil, err
442446
}
443447
}
444448

445-
return merged, false, nil
449+
return merged, nil
446450
}
447451

448452
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
820824
doc.Codes = make(map[string]string)
821825
}
822826

823-
mu := stackconfig.Lock().ReadWrite(db, shareInteractLockName(sharingID))
827+
mu := config.Lock().ReadWrite(db, shareInteractLockName(sharingID))
824828
if err := mu.Lock(); err != nil {
825829
return nil, err
826830
}
827831
defer mu.Unlock()
828832

829833
return utils.RetryWithBackoffValue(context.Background(), shareInteractRetryOptions(), func() (*Permission, error) {
830-
existing, err := getOrRepairShareInteractPermissions(db, sharingID)
834+
existing, err := getOrRepairShareInteractPermissionsOnce(db, sharingID)
831835
if err != nil && !couchdb.IsNotFoundError(err) {
832836
return nil, err
833837
}

‎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) {

0 commit comments

Comments
 (0)