From 72e0a6ce146501337cbb901a34b52ed7c7df4964 Mon Sep 17 00:00:00 2001 From: Lewis Date: Tue, 07 Jul 2026 12:12:55 +0000 Subject: [PATCH] knotserver,appview,lexicons: resolve merge/forkSync target by repodid Lewis: May this revision serve well! --- appview/ingester_repo.go | 4 ++-- appview/ingester_repo_test.go | 9 +++++---- repoident/repoident.go | 31 +++++++++++++++++++++++++++++++ repoident/repoident_test.go | 37 +++++++++++++++++++++++++++++++++++++ repoverify/verify.go | 38 +++++++------------------------------- repoverify/verify_test.go | 17 ----------------- spindle/server.go | 3 ++- api/tangled/repoforkSync.go | 2 ++ api/tangled/repomerge.go | 2 ++ api/tangled/repomergeCheck.go | 2 ++ appview/models/repo.go | 14 ++++++++------ appview/pulls/compose.go | 1 + appview/pulls/merge.go | 1 + appview/pulls/single.go | 1 + appview/repo/repo.go | 1 + knotserver/xrpc/fork_sync.go | 12 ++++-------- knotserver/xrpc/merge.go | 12 ++++-------- knotserver/xrpc/merge_check.go | 8 ++------ knotserver/xrpc/resolve_repo_did_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ knotserver/xrpc/xrpc.go | 26 ++++++++++++++++++++++++++ lexicons/repo/forkSync.json | 5 +++++ lexicons/repo/merge.json | 5 +++++ lexicons/repo/mergeCheck.json | 5 +++++ 23 file(s) changed, 236 insertion(s)(+), 83 deletion(s)(-) diff --git a/appview/ingester_repo.go b/appview/ingester_repo.go --- a/appview/ingester_repo.go +++ b/appview/ingester_repo.go @@ -16,7 +16,7 @@ "tangled.org/core/appview/db" "tangled.org/core/appview/models" "tangled.org/core/orm" - "tangled.org/core/repoverify" + "tangled.org/core/repoident" ) func (i *Ingester) ingestRepo(ctx context.Context, e *jmodels.Event, l *slog.Logger) error { @@ -422,7 +422,7 @@ if i.Verifier == nil { return false, fmt.Errorf("ingester has no repo ownership verifier configured") } - rd, err := repoverify.NewRepoDid(repoDid) + rd, err := repoident.NewRepoDid(repoDid) if err != nil { l.Warn("rejecting repo event: invalid repoDid on record", "repoDid", repoDid, "err", err) return false, nil diff --git a/appview/ingester_repo_test.go b/appview/ingester_repo_test.go --- a/appview/ingester_repo_test.go +++ b/appview/ingester_repo_test.go @@ -18,6 +18,7 @@ "tangled.org/core/appview/notify" "tangled.org/core/orm" "tangled.org/core/rbac" + "tangled.org/core/repoident" "tangled.org/core/repoverify" ) @@ -33,17 +34,17 @@ func acceptOwner(t *testing.T, e *jmodels.Event) repoverify.Verifier { t.Helper() knot := mustKnotURL(t, "https://knot.example") - return func(_ context.Context, repoDid repoverify.RepoDid) (repoverify.Result, error) { + return func(_ context.Context, repoDid repoident.RepoDid) (repoverify.Result, error) { return repoverify.Result{ RepoDid: repoDid, - OwnerDid: repoverify.OwnerDid(e.Did), + OwnerDid: repoident.OwnerDid(e.Did), KnotURL: knot, }, nil } } func stubVerifier(result repoverify.Result, err error) repoverify.Verifier { - return func(_ context.Context, _ repoverify.RepoDid) (repoverify.Result, error) { + return func(_ context.Context, _ repoident.RepoDid) (repoverify.Result, error) { return result, err } } @@ -691,7 +692,7 @@ }) verifierCalled := false - withVerifier(ing, func(_ context.Context, _ repoverify.RepoDid) (repoverify.Result, error) { + withVerifier(ing, func(_ context.Context, _ repoident.RepoDid) (repoverify.Result, error) { verifierCalled = true return repoverify.Result{}, nil }) diff --git a/repoident/repoident.go b/repoident/repoident.go new file mode 100644 --- /dev/null +++ b/repoident/repoident.go @@ -0,0 +1,31 @@ +package repoident + +import ( + "fmt" + + "github.com/bluesky-social/indigo/atproto/syntax" +) + +type RepoDid syntax.DID + +func (r RepoDid) String() string { return string(r) } + +func NewRepoDid(s string) (RepoDid, error) { + did, err := syntax.ParseDID(s) + if err != nil { + return "", fmt.Errorf("invalid repoDid %q: %w", s, err) + } + return RepoDid(did), nil +} + +type OwnerDid syntax.DID + +func (o OwnerDid) String() string { return string(o) } + +func NewOwnerDid(s string) (OwnerDid, error) { + did, err := syntax.ParseDID(s) + if err != nil { + return "", fmt.Errorf("invalid ownerDid %q: %w", s, err) + } + return OwnerDid(did), nil +} diff --git a/repoident/repoident_test.go b/repoident/repoident_test.go new file mode 100644 --- /dev/null +++ b/repoident/repoident_test.go @@ -0,0 +1,37 @@ +package repoident + +import "testing" + +func TestNewRepoDid_RejectsInvalid(t *testing.T) { + if _, err := NewRepoDid(""); err == nil { + t.Error("NewRepoDid(\"\") err = nil, want error") + } +} + +func TestNewRepoDid_AcceptsValid(t *testing.T) { + raw := "did:plc:boltless" + got, err := NewRepoDid(raw) + if err != nil { + t.Fatalf("NewRepoDid(%q): %v", raw, err) + } + if got.String() != raw { + t.Errorf("got %q, want %q", got, raw) + } +} + +func TestNewOwnerDid_RejectsInvalid(t *testing.T) { + if _, err := NewOwnerDid("not-a-did"); err == nil { + t.Error("NewOwnerDid(\"not-a-did\") err = nil, want error") + } +} + +func TestNewOwnerDid_AcceptsValid(t *testing.T) { + raw := "did:plc:akshay" + got, err := NewOwnerDid(raw) + if err != nil { + t.Fatalf("NewOwnerDid(%q): %v", raw, err) + } + if got.String() != raw { + t.Errorf("got %q, want %q", got, raw) + } +} diff --git a/repoverify/verify.go b/repoverify/verify.go --- a/repoverify/verify.go +++ b/repoverify/verify.go @@ -10,36 +10,12 @@ "syscall" "time" - "github.com/bluesky-social/indigo/atproto/syntax" indigoxrpc "github.com/bluesky-social/indigo/xrpc" "tangled.org/core/api/tangled" "tangled.org/core/idresolver" + "tangled.org/core/repoident" "tangled.org/core/xrpc/xrpcclient" ) - -type RepoDid syntax.DID - -func (r RepoDid) String() string { return string(r) } - -func NewRepoDid(s string) (RepoDid, error) { - did, err := syntax.ParseDID(s) - if err != nil { - return "", fmt.Errorf("invalid repoDid %q: %w", s, err) - } - return RepoDid(did), nil -} - -type OwnerDid syntax.DID - -func (o OwnerDid) String() string { return string(o) } - -func NewOwnerDid(s string) (OwnerDid, error) { - did, err := syntax.ParseDID(s) - if err != nil { - return "", fmt.Errorf("invalid ownerDid %q: %w", s, err) - } - return OwnerDid(did), nil -} func ParseKnotEndpoint(raw string, dev bool) (*url.URL, error) { if raw == "" { @@ -65,15 +41,15 @@ } type Result struct { - RepoDid RepoDid - OwnerDid OwnerDid + RepoDid repoident.RepoDid + OwnerDid repoident.OwnerDid KnotURL *url.URL // Rkey of the sh.tangled.repo record tracked by the knot; empty when the // knot does not support describeRepo. Rkey string } -type Verifier func(ctx context.Context, repoDid RepoDid) (Result, error) +type Verifier func(ctx context.Context, repoDid repoident.RepoDid) (Result, error) const verifyTimeout = 10 * time.Second @@ -86,7 +62,7 @@ Transport: transport, } - return func(ctx context.Context, repoDid RepoDid) (Result, error) { + return func(ctx context.Context, repoDid repoident.RepoDid) (Result, error) { ctx, cancel := context.WithTimeout(ctx, verifyTimeout) defer cancel() return resolveAndDescribe(ctx, resolver, httpClient, repoDid, dev) @@ -97,7 +73,7 @@ ctx context.Context, resolver *idresolver.Resolver, httpClient *http.Client, - repoDid RepoDid, + repoDid repoident.RepoDid, dev bool, ) (Result, error) { ident, err := resolver.ResolveIdent(ctx, repoDid.String()) @@ -123,7 +99,7 @@ return Result{}, fmt.Errorf("knot %s returned mismatched repoDid: got %q, want %q", knot, out.RepoDid, repoDid) } - ownerDid, err := NewOwnerDid(out.OwnerDid) + ownerDid, err := repoident.NewOwnerDid(out.OwnerDid) if err != nil { return Result{}, fmt.Errorf("describeRepo on %s returned invalid ownerDid: %w", knot, err) } diff --git a/repoverify/verify_test.go b/repoverify/verify_test.go --- a/repoverify/verify_test.go +++ b/repoverify/verify_test.go @@ -2,23 +2,6 @@ import "testing" -func TestNewRepoDid_RejectsInvalid(t *testing.T) { - if _, err := NewRepoDid(""); err == nil { - t.Error("NewRepoDid(\"\") err = nil, want error") - } -} - -func TestNewRepoDid_AcceptsValid(t *testing.T) { - raw := "did:plc:abc123abc123abc123abc123" - got, err := NewRepoDid(raw) - if err != nil { - t.Fatalf("NewRepoDid(%q): %v", raw, err) - } - if got.String() != raw { - t.Errorf("got %q, want %q", got, raw) - } -} - func TestParseKnotEndpoint_RejectsHttpInProd(t *testing.T) { if _, err := ParseKnotEndpoint("http://knot.example", false); err == nil { t.Error("http:// knot URL accepted in prod") diff --git a/spindle/server.go b/spindle/server.go --- a/spindle/server.go +++ b/spindle/server.go @@ -30,6 +30,7 @@ "tangled.org/core/log" "tangled.org/core/notifier" "tangled.org/core/rbac" + "tangled.org/core/repoident" "tangled.org/core/repoverify" "tangled.org/core/spindle/config" "tangled.org/core/spindle/db" @@ -583,7 +584,7 @@ } // verify repo, we don't want git sync to point to arbitrary endpoints - res, err := s.verify(ctx, repoverify.RepoDid(repoDid)) + res, err := s.verify(ctx, repoident.RepoDid(repoDid)) if err != nil { return nil, fmt.Errorf("verify sourceRepo %s: %w", repoDid, err) } diff --git a/api/tangled/repoforkSync.go b/api/tangled/repoforkSync.go --- a/api/tangled/repoforkSync.go +++ b/api/tangled/repoforkSync.go @@ -22,6 +22,8 @@ Did string `json:"did" cborgen:"did"` // name: Name of the forked repository Name string `json:"name" cborgen:"name"` + // repo: DID of the repository + Repo *string `json:"repo,omitempty" cborgen:"repo,omitempty"` // source: AT-URI of the source repository Source string `json:"source" cborgen:"source"` } diff --git a/api/tangled/repomerge.go b/api/tangled/repomerge.go --- a/api/tangled/repomerge.go +++ b/api/tangled/repomerge.go @@ -32,6 +32,8 @@ Name string `json:"name" cborgen:"name"` // patch: Patch content to merge Patch string `json:"patch" cborgen:"patch"` + // repo: DID of the repository + Repo *string `json:"repo,omitempty" cborgen:"repo,omitempty"` } // RepoMerge calls the XRPC method "sh.tangled.repo.merge". diff --git a/api/tangled/repomergeCheck.go b/api/tangled/repomergeCheck.go --- a/api/tangled/repomergeCheck.go +++ b/api/tangled/repomergeCheck.go @@ -32,6 +32,8 @@ Name string `json:"name" cborgen:"name"` // patch: Patch or pull request to check for merge conflicts Patch string `json:"patch" cborgen:"patch"` + // repo: DID of the repository + Repo *string `json:"repo,omitempty" cborgen:"repo,omitempty"` } // RepoMergeCheck_Output is the output of a sh.tangled.repo.mergeCheck call. diff --git a/appview/models/repo.go b/appview/models/repo.go --- a/appview/models/repo.go +++ b/appview/models/repo.go @@ -51,11 +51,6 @@ website = &r.Website } - var repoDid *string - if r.RepoDid != "" { - repoDid = &r.RepoDid - } - return tangled.Repo{ Knot: r.Knot, Name: r.cosmeticName(), @@ -66,7 +61,7 @@ Source: source, Spindle: spindle, Labels: r.Labels, - RepoDid: repoDid, + RepoDid: r.RepoDidPtr(), } } @@ -101,6 +96,13 @@ return r.RepoDid } return string(r.RepoAt()) +} + +func (r Repo) RepoDidPtr() *string { + if r.RepoDid == "" { + return nil + } + return &r.RepoDid } func (r Repo) TopicStr() string { diff --git a/appview/pulls/compose.go b/appview/pulls/compose.go --- a/appview/pulls/compose.go +++ b/appview/pulls/compose.go @@ -467,6 +467,7 @@ resp, err := tangled.RepoMergeCheck(ctx, xrpcc, &tangled.RepoMergeCheck_Input{ Did: repo.Did, Name: repo.Name, + Repo: repo.RepoDidPtr(), Branch: targetBranch, Patch: patch, }) diff --git a/appview/pulls/merge.go b/appview/pulls/merge.go --- a/appview/pulls/merge.go +++ b/appview/pulls/merge.go @@ -74,6 +74,7 @@ mergeInput := &tangled.RepoMerge_Input{ Did: f.Did, Name: f.Name, + Repo: f.RepoDidPtr(), Branch: pull.TargetBranch, Patch: patch, CommitMessage: &pull.Title, diff --git a/appview/pulls/single.go b/appview/pulls/single.go --- a/appview/pulls/single.go +++ b/appview/pulls/single.go @@ -344,6 +344,7 @@ &tangled.RepoMergeCheck_Input{ Did: f.Did, Name: f.Name, + Repo: f.RepoDidPtr(), Branch: pull.TargetBranch, Patch: patch, }, diff --git a/appview/repo/repo.go b/appview/repo/repo.go --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -1383,6 +1383,7 @@ &tangled.RepoForkSync_Input{ Did: user.Did, Name: f.Name, + Repo: f.RepoDidPtr(), Source: f.Source, Branch: ref, }, diff --git a/knotserver/xrpc/fork_sync.go b/knotserver/xrpc/fork_sync.go --- a/knotserver/xrpc/fork_sync.go +++ b/knotserver/xrpc/fork_sync.go @@ -40,19 +40,15 @@ return } - repoDid, err := x.Db.GetRepoDid(did, name) + repoDid, repoPath, err := x.resolveRepoDID(data.Repo, did, name) if err != nil { - fail(xrpcerr.RepoNotFoundError) - return - } - repoPath, _, _, err := x.Db.ResolveRepoDIDOnDisk(x.Config.Repo.ScanPath, repoDid) - if err != nil { + l.Error("failed to resolve repo", "err", err) fail(xrpcerr.RepoNotFoundError) return } - if ok, err := x.Enforcer.IsPushAllowed(actorDid.String(), rbac.ThisServer, repoDid); !ok || err != nil { - l.Error("insufficient permissions", "did", actorDid.String(), "repo", repoDid) + if ok, err := x.Enforcer.IsPushAllowed(actorDid.String(), rbac.ThisServer, repoDid.String()); !ok || err != nil { + l.Error("insufficient permissions", "did", actorDid.String(), "repo", repoDid.String()) writeError(w, xrpcerr.AccessControlError(actorDid.String()), http.StatusUnauthorized) return } diff --git a/knotserver/xrpc/merge.go b/knotserver/xrpc/merge.go --- a/knotserver/xrpc/merge.go +++ b/knotserver/xrpc/merge.go @@ -42,19 +42,15 @@ return } - repoDid, err := x.Db.GetRepoDid(did, name) + repoDid, repoPath, err := x.resolveRepoDID(data.Repo, did, name) if err != nil { - fail(xrpcerr.RepoNotFoundError) - return - } - repoPath, _, _, err := x.Db.ResolveRepoDIDOnDisk(x.Config.Repo.ScanPath, repoDid) - if err != nil { + l.Error("failed to resolve repo", "err", err) fail(xrpcerr.RepoNotFoundError) return } - if ok, err := x.Enforcer.IsPushAllowed(actorDid.String(), rbac.ThisServer, repoDid); !ok || err != nil { - l.Error("insufficient permissions", "did", actorDid.String(), "repo", repoDid) + if ok, err := x.Enforcer.IsPushAllowed(actorDid.String(), rbac.ThisServer, repoDid.String()); !ok || err != nil { + l.Error("insufficient permissions", "did", actorDid.String(), "repo", repoDid.String()) writeError(w, xrpcerr.AccessControlError(actorDid.String()), http.StatusUnauthorized) return } diff --git a/knotserver/xrpc/merge_check.go b/knotserver/xrpc/merge_check.go --- a/knotserver/xrpc/merge_check.go +++ b/knotserver/xrpc/merge_check.go @@ -33,13 +33,9 @@ return } - repoDid, err := x.Db.GetRepoDid(did, name) + _, repoPath, err := x.resolveRepoDID(data.Repo, did, name) if err != nil { - fail(xrpcerr.RepoNotFoundError) - return - } - repoPath, _, _, err := x.Db.ResolveRepoDIDOnDisk(x.Config.Repo.ScanPath, repoDid) - if err != nil { + l.Error("failed to resolve repo", "err", err) fail(xrpcerr.RepoNotFoundError) return } diff --git a/knotserver/xrpc/resolve_repo_did_test.go b/knotserver/xrpc/resolve_repo_did_test.go new file mode 100644 --- /dev/null +++ b/knotserver/xrpc/resolve_repo_did_test.go @@ -0,0 +1,83 @@ +package xrpc + +import ( + "os" + "path/filepath" + "testing" +) + +const ( + resolveOwnerDid = "did:plc:akshay" + resolveRepoDid = "did:plc:squid" + resolveStoredKey = "squidbot" +) + +func setupResolveRepo(t *testing.T) (*Xrpc, string) { + t.Helper() + x := newTestXrpc(t) + scanPath := t.TempDir() + x.Config.Repo.ScanPath = scanPath + + if err := x.Db.StoreRepoKey(resolveRepoDid, []byte("k256"), resolveOwnerDid, resolveStoredKey); err != nil { + t.Fatalf("StoreRepoKey: %v", err) + } + if err := os.MkdirAll(filepath.Join(scanPath, resolveRepoDid), 0o755); err != nil { + t.Fatalf("mkdir repo dir: %v", err) + } + return x, scanPath +} + +func TestResolveRepoDID_PrefersRepoOverName(t *testing.T) { + x, scanPath := setupResolveRepo(t) + + repo := resolveRepoDid + gotDid, gotPath, err := x.resolveRepoDID(&repo, resolveOwnerDid, "SquidBot") + if err != nil { + t.Fatalf("resolveRepoDID with repo set: %v", err) + } + if gotDid != resolveRepoDid { + t.Errorf("repoDid = %q, want %q", gotDid, resolveRepoDid) + } + if want := filepath.Join(scanPath, resolveRepoDid); gotPath != want { + t.Errorf("repoPath = %q, want %q", gotPath, want) + } +} + +func TestResolveRepoDID_RejectsMalformedRepoDid(t *testing.T) { + x, _ := setupResolveRepo(t) + + malformed := "not-a-did" + if _, _, err := x.resolveRepoDID(&malformed, resolveOwnerDid, resolveStoredKey); err == nil { + t.Fatal("resolveRepoDID with malformed repo DID: got nil error, want failure") + } +} + +func TestResolveRepoDID_UnknownRepoDidDoesNotFallBackToName(t *testing.T) { + x, _ := setupResolveRepo(t) + + unknown := "did:plc:limpet" + if _, _, err := x.resolveRepoDID(&unknown, resolveOwnerDid, resolveStoredKey); err == nil { + t.Fatal("resolveRepoDID with unknown repo DID and resolvable name: got nil error, want failure") + } +} + +func TestResolveRepoDID_NameFallbackIsCaseSensitive(t *testing.T) { + x, _ := setupResolveRepo(t) + + if _, _, err := x.resolveRepoDID(nil, resolveOwnerDid, "SquidBot"); err == nil { + t.Fatal("resolveRepoDID with mismatched-case name: got nil error, want failure") + } + + empty := "" + if _, _, err := x.resolveRepoDID(&empty, resolveOwnerDid, "SquidBot"); err == nil { + t.Fatal("resolveRepoDID with empty repo and mismatched-case name: got nil error, want failure") + } + + gotDid, _, err := x.resolveRepoDID(nil, resolveOwnerDid, resolveStoredKey) + if err != nil { + t.Fatalf("resolveRepoDID with exact-case name: %v", err) + } + if gotDid != resolveRepoDid { + t.Errorf("repoDid = %q, want %q", gotDid, resolveRepoDid) + } +} diff --git a/knotserver/xrpc/xrpc.go b/knotserver/xrpc/xrpc.go --- a/knotserver/xrpc/xrpc.go +++ b/knotserver/xrpc/xrpc.go @@ -19,6 +19,7 @@ "tangled.org/core/knotserver/sandbox" "tangled.org/core/notifier" "tangled.org/core/rbac" + "tangled.org/core/repoident" xrpcerr "tangled.org/core/xrpc/errors" "tangled.org/core/xrpc/serviceauth" ) @@ -131,6 +132,31 @@ return "", xrpcerr.RepoNotFoundError } return repoPath, nil +} + +func (x *Xrpc) resolveRepoDID(repo *string, ownerDid, name string) (repoident.RepoDid, string, error) { + raw, err := x.selectRepoDID(repo, ownerDid, name) + if err != nil { + return "", "", err + } + + repoDid, err := repoident.NewRepoDid(raw) + if err != nil { + return "", "", err + } + + repoPath, _, _, err := x.Db.ResolveRepoDIDOnDisk(x.Config.Repo.ScanPath, repoDid.String()) + if err != nil { + return "", "", err + } + return repoDid, repoPath, nil +} + +func (x *Xrpc) selectRepoDID(repo *string, ownerDid, name string) (string, error) { + if repo != nil && *repo != "" { + return *repo, nil + } + return x.Db.GetRepoDid(ownerDid, name) } func writeError(w http.ResponseWriter, e xrpcerr.XrpcError, status int) { diff --git a/lexicons/repo/forkSync.json b/lexicons/repo/forkSync.json --- a/lexicons/repo/forkSync.json +++ b/lexicons/repo/forkSync.json @@ -30,6 +30,11 @@ "type": "string", "description": "Name of the forked repository" }, + "repo": { + "type": "string", + "format": "did", + "description": "DID of the repository" + }, "branch": { "type": "string", "description": "Branch to sync" diff --git a/lexicons/repo/merge.json b/lexicons/repo/merge.json --- a/lexicons/repo/merge.json +++ b/lexicons/repo/merge.json @@ -20,6 +20,11 @@ "type": "string", "description": "Name of the repository" }, + "repo": { + "type": "string", + "format": "did", + "description": "DID of the repository" + }, "patch": { "type": "string", "description": "Patch content to merge" diff --git a/lexicons/repo/mergeCheck.json b/lexicons/repo/mergeCheck.json --- a/lexicons/repo/mergeCheck.json +++ b/lexicons/repo/mergeCheck.json @@ -20,6 +20,11 @@ "type": "string", "description": "Name of the repository" }, + "repo": { + "type": "string", + "format": "did", + "description": "DID of the repository" + }, "patch": { "type": "string", "description": "Patch or pull request to check for merge conflicts" -- tangled.sh