From 61a934debba6a1ccbba37d820ea548e639914877 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 9 Sep 2026 09:31:16 -0500 Subject: [PATCH] hold: look crew members up by rkey instead of walking the collection ValidateBlobWriteAccess, ValidateBlobReadAccess, ValidateOwnerOrCrewAdmin and getCrewTier each listed every crew record to find one member: open a carstore session, walk the MST, CBOR-decode each record, compare DIDs. That ran on every multipart call from the appview, including the part URL request for every 10MB, and on every getBlob presign, so on a hold with hundreds of crew each part cost hundreds of decodes. lookupCrewMember tries the deterministic rkey first (one record read) and only falls back to the walk on a not-found miss. The fallback is required: records created before the hash-rkey scheme sit at a TID rkey, and the boot-time migration that rekeyed them only existed between e0a2dda and b2d6842, so a hold that upgraded across that window still has them. Members hit the O(1) path; only genuine non-members pay for the walk, and they are denied anyway. Every authorization decision and error string is unchanged. Tests cover the deterministic hit, a legacy TID-keyed member found only through the fallback, a non-member, and a storage error surfacing as an error rather than a silent denial. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5 --- pkg/hold/pds/auth.go | 52 ++++++++--------- pkg/hold/pds/crew.go | 37 ++++++++++++ pkg/hold/pds/crew_test.go | 119 ++++++++++++++++++++++++++++++++++++++ pkg/hold/pds/layer.go | 12 +--- 4 files changed, 182 insertions(+), 38 deletions(-) diff --git a/pkg/hold/pds/auth.go b/pkg/hold/pds/auth.go index 677a990..0eca3dd 100644 --- a/pkg/hold/pds/auth.go +++ b/pkg/hold/pds/auth.go @@ -276,20 +276,18 @@ func ValidateOwnerOrCrewAdmin(r *http.Request, pds *HoldPDS, httpClient HTTPClie } // Check if user is crew with admin permission - crew, err := pds.ListCrewMembers(r.Context()) + member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID) if err != nil { return nil, fmt.Errorf("failed to check crew membership: %w", err) } - for _, member := range crew { - if member.Record.Member == user.DID { - // Check if this crew member has crew:admin permission - if slices.Contains(member.Record.Permissions, "crew:admin") { - return user, nil - } - // User is crew but doesn't have admin permission - return nil, NewAuthError("crew:admin", "crew member lacks permission", "crew:admin") + if isCrew { + // Check if this crew member has crew:admin permission + if slices.Contains(member.Permissions, "crew:admin") { + return user, nil } + // User is crew but doesn't have admin permission + return nil, NewAuthError("crew:admin", "crew member lacks permission", "crew:admin") } // User is neither owner nor authorized crew @@ -408,20 +406,18 @@ func ValidateBlobWriteAccess(r *http.Request, pds *HoldPDS, httpClient HTTPClien } // Check if user is crew with blob:write permission - crew, err := pds.ListCrewMembers(r.Context()) + member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID) if err != nil { return nil, fmt.Errorf("failed to check crew membership: %w", err) } - for _, member := range crew { - if member.Record.Member == user.DID { - // Check if this crew member has blob:write permission - if slices.Contains(member.Record.Permissions, "blob:write") { - return user, nil - } - // User is crew but doesn't have write permission - return nil, NewAuthError("blob:write", "crew member lacks permission", "blob:write") + if isCrew { + // Check if this crew member has blob:write permission + if slices.Contains(member.Permissions, "blob:write") { + return user, nil } + // User is crew but doesn't have write permission + return nil, NewAuthError("blob:write", "crew member lacks permission", "blob:write") } // User is neither owner nor authorized crew @@ -477,22 +473,20 @@ func ValidateBlobReadAccess(r *http.Request, pds *HoldPDS, httpClient HTTPClient // Check if user is crew with blob:read or blob:write permission // Note: blob:write implicitly grants blob:read access - crew, err := pds.ListCrewMembers(r.Context()) + member, isCrew, err := pds.lookupCrewMember(r.Context(), user.DID) if err != nil { return nil, fmt.Errorf("failed to check crew membership: %w", err) } - for _, member := range crew { - if member.Record.Member == user.DID { - // Check if this crew member has blob:read or blob:write permission - // blob:write implicitly grants read access (can't push without pulling) - if slices.Contains(member.Record.Permissions, "blob:read") || - slices.Contains(member.Record.Permissions, "blob:write") { - return user, nil - } - // User is crew but doesn't have read or write permission - return nil, NewAuthError("blob:read", "crew member lacks permission", "blob:read", "blob:write") + if isCrew { + // Check if this crew member has blob:read or blob:write permission + // blob:write implicitly grants read access (can't push without pulling) + if slices.Contains(member.Permissions, "blob:read") || + slices.Contains(member.Permissions, "blob:write") { + return user, nil } + // User is crew but doesn't have read or write permission + return nil, NewAuthError("blob:read", "crew member lacks permission", "blob:read", "blob:write") } // User is neither owner nor authorized crew diff --git a/pkg/hold/pds/crew.go b/pkg/hold/pds/crew.go index 329e87d..a766351 100644 --- a/pkg/hold/pds/crew.go +++ b/pkg/hold/pds/crew.go @@ -71,6 +71,43 @@ func (p *HoldPDS) GetCrewMemberByDID(ctx context.Context, memberDID string) (cid return p.GetCrewMember(ctx, rkey) } +// lookupCrewMember resolves this hold's crew record for a DID, preferring the +// O(1) deterministic-rkey read over a full walk of the crew collection. +// +// Every current writer (AddCrewMember, UpdateCrewMemberTier, bootstrap, crew +// import, the join endpoint) stores crew records at atproto.CrewRecordKey(did), +// but records created before that scheme landed sit at a TID rkey. The boot-time +// rekey migration that fixed those only shipped between 2026-01-06 and +// 2026-05-04, so a hold upgrading straight across that window still has +// TID-keyed crew records. A miss therefore falls back to the full walk rather +// than denying access to a legacy member. +// +// Returns (nil, false, nil) when the DID is genuinely not crew, and a non-nil +// error only for a real storage failure. +func (p *HoldPDS) lookupCrewMember(ctx context.Context, memberDID string) (*atproto.CrewRecord, bool, error) { + _, record, err := p.GetCrewMemberByDID(ctx, memberDID) + if err == nil { + return record, true, nil + } + if !errors.Is(err, ErrCrewMemberNotFound) { + return nil, false, err + } + + // Deterministic rkey absent: fall back to the walk in case this member + // predates the hash-rkey scheme. + crew, err := p.ListCrewMembers(ctx) + if err != nil { + return nil, false, err + } + for _, member := range crew { + if member.Record.Member == memberDID { + return member.Record, true, nil + } + } + + return nil, false, nil +} + // CrewMemberWithKey pairs a crew record with its rkey and CID type CrewMemberWithKey struct { Rkey string diff --git a/pkg/hold/pds/crew_test.go b/pkg/hold/pds/crew_test.go index 8b42556..531a130 100644 --- a/pkg/hold/pds/crew_test.go +++ b/pkg/hold/pds/crew_test.go @@ -2,7 +2,10 @@ package pds import ( "bytes" + "context" "errors" + "os" + "path/filepath" "strings" "testing" @@ -592,3 +595,119 @@ func TestListCrewMembers_MixedDIDs(t *testing.T) { } } } + +// TestLookupCrewMember_DeterministicRkey verifies the O(1) fast path finds a +// crew member written by AddCrewMember. +func TestLookupCrewMember_DeterministicRkey(t *testing.T) { + pds, ctx := setupTestPDS(t) + defer pds.Close() + + memberDID := "did:plc:fastpath" + if _, err := pds.AddCrewMember(ctx, memberDID, "writer", []string{"blob:write"}, "bosun"); err != nil { + t.Fatalf("AddCrewMember failed: %v", err) + } + + record, isCrew, err := pds.lookupCrewMember(ctx, memberDID) + if err != nil { + t.Fatalf("lookupCrewMember returned error: %v", err) + } + if !isCrew { + t.Fatal("Expected member to be found") + } + if record.Member != memberDID { + t.Errorf("Expected member %s, got %s", memberDID, record.Member) + } + if record.Tier != "bosun" { + t.Errorf("Expected tier bosun, got %s", record.Tier) + } +} + +// TestLookupCrewMember_LegacyTIDRkey verifies the fallback walk still finds a +// crew record left at a TID rkey by a pre-migration hold. Without the fallback +// these members would silently lose authorization. +func TestLookupCrewMember_LegacyTIDRkey(t *testing.T) { + pds, ctx := setupTestPDS(t) + defer pds.Close() + + memberDID := "did:plc:legacytid" + + // Write the record the way pre-2026-01-06 holds did: auto-generated TID rkey. + legacy := &atproto.CrewRecord{ + Type: atproto.CrewCollection, + Member: memberDID, + Role: "writer", + Permissions: []string{"blob:write"}, + Tier: "deckhand", + } + rkey, _, err := pds.repomgr.CreateRecord(ctx, pds.uid, atproto.CrewCollection, legacy) + if err != nil { + t.Fatalf("CreateRecord failed: %v", err) + } + if rkey == atproto.CrewRecordKey(memberDID) { + t.Fatalf("Test setup is wrong: CreateRecord produced the deterministic rkey %s", rkey) + } + + // The O(1) read must miss, proving the fallback is what found the record. + if _, _, err := pds.GetCrewMemberByDID(ctx, memberDID); !errors.Is(err, ErrCrewMemberNotFound) { + t.Fatalf("Expected ErrCrewMemberNotFound from deterministic lookup, got %v", err) + } + + record, isCrew, err := pds.lookupCrewMember(ctx, memberDID) + if err != nil { + t.Fatalf("lookupCrewMember returned error: %v", err) + } + if !isCrew { + t.Fatal("Expected legacy TID-keyed member to be found via fallback walk") + } + if record.Tier != "deckhand" { + t.Errorf("Expected tier deckhand, got %s", record.Tier) + } +} + +// TestLookupCrewMember_NotFound verifies a genuine non-member reports +// (nil, false, nil) rather than an error, so callers deny with an AuthError. +func TestLookupCrewMember_NotFound(t *testing.T) { + pds, ctx := setupTestPDS(t) + defer pds.Close() + + if _, err := pds.AddCrewMember(ctx, "did:plc:someone", "writer", []string{"blob:write"}, ""); err != nil { + t.Fatalf("AddCrewMember failed: %v", err) + } + + record, isCrew, err := pds.lookupCrewMember(ctx, "did:plc:stranger") + if err != nil { + t.Fatalf("Expected nil error for a non-member, got %v", err) + } + if isCrew { + t.Error("Expected non-member to report isCrew=false") + } + if record != nil { + t.Errorf("Expected nil record for a non-member, got %+v", record) + } +} + +// TestLookupCrewMember_StorageError verifies a real storage failure surfaces as +// an error instead of being mistaken for "not a crew member", which would turn +// a broken CAR store into a silent authorization denial. +func TestLookupCrewMember_StorageError(t *testing.T) { + ctx := context.Background() + keyPath := filepath.Join(t.TempDir(), "signing-key") + if err := os.WriteFile(keyPath, sharedTestKey, 0600); err != nil { + t.Fatalf("Failed to write signing key: %v", err) + } + + // Deliberately skip InitNewActor, so the repo has no head to read. + pds, err := NewHoldPDS(ctx, "did:web:hold.example.com", "https://hold.example.com", "https://atcr.io", ":memory:", keyPath, false) + if err != nil { + t.Fatalf("Failed to create test PDS: %v", err) + } + defer pds.Close() + + record, isCrew, err := pds.lookupCrewMember(ctx, "did:plc:anyone") + if err == nil { + t.Fatalf("Expected a storage error, got (%+v, %v, nil)", record, isCrew) + } + if isCrew { + t.Error("Expected isCrew=false alongside a storage error") + } +} diff --git a/pkg/hold/pds/layer.go b/pkg/hold/pds/layer.go index 882d8e3..e1e8784 100644 --- a/pkg/hold/pds/layer.go +++ b/pkg/hold/pds/layer.go @@ -216,18 +216,12 @@ func (p *HoldPDS) GetQuotaForUserWithTier(ctx context.Context, userDID string, q // getCrewTier returns the tier for a crew member, or empty string if not found func (p *HoldPDS) getCrewTier(ctx context.Context, userDID string) string { - crewMembers, err := p.ListCrewMembers(ctx) - if err != nil { + member, isCrew, err := p.lookupCrewMember(ctx, userDID) + if err != nil || !isCrew { return "" } - for _, member := range crewMembers { - if member.Record.Member == userDID { - return member.Record.Tier - } - } - - return "" + return member.Tier } // ListLayerRecordsForUser returns all layer records uploaded by a specific user