From 15d2be9210304e0d885c58ce754f61070765d8a2 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Thu, 30 Oct 2025 22:59:52 -0500 Subject: [PATCH] clean up duplicate functionality around converting hold did to url --- pkg/appview/storage/manifest_store.go | 26 +-- pkg/appview/storage/manifest_store_test.go | 48 ------ pkg/atproto/resolver.go | 31 ++++ pkg/atproto/resolver_test.go | 186 ++++++++++++++++++++ pkg/atproto/utils.go | 33 ---- pkg/atproto/utils_test.go | 189 --------------------- pkg/auth/hold_remote.go | 38 +---- pkg/auth/hold_remote_test.go | 85 --------- 8 files changed, 220 insertions(+), 416 deletions(-) delete mode 100644 pkg/atproto/utils.go delete mode 100644 pkg/atproto/utils_test.go diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index 21896dd..7401158 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -134,11 +134,6 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, manifestRecord.ManifestBlob = blobRef manifestRecord.HoldDID = s.ctx.HoldDID // Primary reference (DID) - // Resolve hold endpoint from DID for backward compatibility - if holdEndpoint, err := resolveDIDToHTTPSEndpoint(s.ctx.HoldDID); err == nil { - manifestRecord.HoldEndpoint = holdEndpoint // Legacy reference (URL) for backward compat - } - // Extract Dockerfile labels from config blob and add to annotations // Only for image manifests (not manifest lists which don't have config blobs) isManifestList := strings.Contains(manifestRecord.MediaType, "manifest.list") || @@ -283,22 +278,6 @@ func (s *ManifestStore) extractConfigLabels(ctx context.Context, configDigestStr return configJSON.Config.Labels, nil } -// resolveDIDToHTTPSEndpoint resolves a DID to an HTTPS endpoint -// Currently supports did:web only (e.g., did:web:hold01.atcr.io → https://hold01.atcr.io) -func resolveDIDToHTTPSEndpoint(did string) (string, error) { - if !strings.HasPrefix(did, "did:web:") { - return "", fmt.Errorf("only did:web is supported, got: %s", did) - } - - // Extract hostname from did:web - hostname := strings.TrimPrefix(did, "did:web:") - - // Handle port notation (did:web:example.com:8080 → https://example.com:8080) - hostname = strings.ReplaceAll(hostname, ":", ":") - - return "https://" + hostname, nil -} - // notifyHoldAboutManifest notifies the hold service about a manifest upload // This enables the hold to create layer records and Bluesky posts func (s *ManifestStore) notifyHoldAboutManifest(ctx context.Context, manifestRecord *atproto.ManifestRecord, tag, manifestDigest string) error { @@ -309,10 +288,7 @@ func (s *ManifestStore) notifyHoldAboutManifest(ctx context.Context, manifestRec // Resolve hold DID to HTTP endpoint // For did:web, this is straightforward (e.g., did:web:hold01.atcr.io → https://hold01.atcr.io) - holdEndpoint, err := resolveDIDToHTTPSEndpoint(s.ctx.HoldDID) - if err != nil { - return fmt.Errorf("failed to resolve hold DID %s: %w", s.ctx.HoldDID, err) - } + holdEndpoint := atproto.ResolveHoldURL(s.ctx.HoldDID) // Use service token from middleware (already cached and validated) serviceToken := s.ctx.ServiceToken diff --git a/pkg/appview/storage/manifest_store_test.go b/pkg/appview/storage/manifest_store_test.go index 8177a8e..f83c262 100644 --- a/pkg/appview/storage/manifest_store_test.go +++ b/pkg/appview/storage/manifest_store_test.go @@ -912,51 +912,3 @@ func TestManifestStore_Delete(t *testing.T) { }) } } - -// TestResolveDIDToHTTPSEndpoint tests DID to HTTPS URL conversion -func TestResolveDIDToHTTPSEndpoint(t *testing.T) { - tests := []struct { - name string - did string - want string - wantErr bool - }{ - { - name: "did:web without port", - did: "did:web:hold01.atcr.io", - want: "https://hold01.atcr.io", - wantErr: false, - }, - { - name: "did:web with port", - did: "did:web:localhost:8080", - want: "https://localhost:8080", - wantErr: false, - }, - { - name: "did:plc not supported", - did: "did:plc:abc123", - want: "", - wantErr: true, - }, - { - name: "invalid did format", - did: "not-a-did", - want: "", - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := resolveDIDToHTTPSEndpoint(tt.did) - if (err != nil) != tt.wantErr { - t.Errorf("resolveDIDToHTTPSEndpoint() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("resolveDIDToHTTPSEndpoint() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/atproto/resolver.go b/pkg/atproto/resolver.go index 69d9ae4..3ddf847 100644 --- a/pkg/atproto/resolver.go +++ b/pkg/atproto/resolver.go @@ -3,10 +3,41 @@ package atproto import ( "context" "fmt" + "strings" "github.com/bluesky-social/indigo/atproto/syntax" ) +// ResolveHoldURL converts a hold identifier (DID or URL) to an HTTP/HTTPS URL +// Handles both formats for backward compatibility: +// - DID format: did:web:hold01.atcr.io → https://hold01.atcr.io +// - DID with port: did:web:172.28.0.3:8080 → http://172.28.0.3:8080 +// - URL format: https://hold.example.com → https://hold.example.com (passthrough) +func ResolveHoldURL(holdIdentifier string) string { + // If it's already a URL (has scheme), return as-is + if strings.HasPrefix(holdIdentifier, "http://") || strings.HasPrefix(holdIdentifier, "https://") { + return holdIdentifier + } + + // If it's a DID, convert to URL + if after, ok := strings.CutPrefix(holdIdentifier, "did:web:"); ok { + hostname := after + + // Use HTTP for localhost/IP addresses with ports, HTTPS for domains + if strings.Contains(hostname, ":") || + strings.Contains(hostname, "127.0.0.1") || + strings.Contains(hostname, "localhost") || + // Check if it's an IP address (contains only digits and dots in first part) + (len(hostname) > 0 && hostname[0] >= '0' && hostname[0] <= '9') { + return "http://" + hostname + } + return "https://" + hostname + } + + // Fallback: assume it's a hostname and use HTTPS + return "https://" + holdIdentifier +} + // ResolveDIDToPDS resolves a DID to its PDS endpoint. // Uses the shared identity directory with cache TTL and event-driven invalidation. func ResolveDIDToPDS(ctx context.Context, did string) (string, error) { diff --git a/pkg/atproto/resolver_test.go b/pkg/atproto/resolver_test.go index 70e886b..fd74e0d 100644 --- a/pkg/atproto/resolver_test.go +++ b/pkg/atproto/resolver_test.go @@ -6,6 +6,192 @@ import ( "testing" ) +func TestResolveHoldURL(t *testing.T) { + tests := []struct { + name string + holdIdentifier string + want string + }{ + // URL passthrough tests + { + name: "http URL passthrough", + holdIdentifier: "http://hold.example.com", + want: "http://hold.example.com", + }, + { + name: "https URL passthrough", + holdIdentifier: "https://hold.example.com", + want: "https://hold.example.com", + }, + { + name: "http URL with port passthrough", + holdIdentifier: "http://hold.example.com:8080", + want: "http://hold.example.com:8080", + }, + { + name: "https URL with port passthrough", + holdIdentifier: "https://hold.example.com:8443", + want: "https://hold.example.com:8443", + }, + { + name: "http URL with path passthrough", + holdIdentifier: "http://hold.example.com/some/path", + want: "http://hold.example.com/some/path", + }, + + // did:web to HTTPS (domain names) + { + name: "did:web domain to https", + holdIdentifier: "did:web:hold01.atcr.io", + want: "https://hold01.atcr.io", + }, + { + name: "did:web subdomain to https", + holdIdentifier: "did:web:my-hold.example.com", + want: "https://my-hold.example.com", + }, + { + name: "did:web simple domain to https", + holdIdentifier: "did:web:example.com", + want: "https://example.com", + }, + + // did:web to HTTP (ports) + { + name: "did:web with port to http", + holdIdentifier: "did:web:172.28.0.3:8080", + want: "http://172.28.0.3:8080", + }, + { + name: "did:web domain with port to http", + holdIdentifier: "did:web:hold.example.com:8080", + want: "http://hold.example.com:8080", + }, + { + name: "did:web localhost with port to http", + holdIdentifier: "did:web:localhost:8080", + want: "http://localhost:8080", + }, + + // did:web to HTTP (localhost) + { + name: "did:web localhost to http", + holdIdentifier: "did:web:localhost", + want: "http://localhost", + }, + + // did:web to HTTP (127.0.0.1) + { + name: "did:web 127.0.0.1 to http", + holdIdentifier: "did:web:127.0.0.1", + want: "http://127.0.0.1", + }, + { + name: "did:web 127.0.0.1 with port to http", + holdIdentifier: "did:web:127.0.0.1:8080", + want: "http://127.0.0.1:8080", + }, + + // did:web to HTTP (IP addresses) + { + name: "did:web IPv4 address to http", + holdIdentifier: "did:web:192.168.1.1", + want: "http://192.168.1.1", + }, + { + name: "did:web IPv4 with port to http", + holdIdentifier: "did:web:10.0.0.5:3000", + want: "http://10.0.0.5:3000", + }, + { + name: "did:web private IP to http", + holdIdentifier: "did:web:172.16.0.1", + want: "http://172.16.0.1", + }, + + // Fallback behavior (plain hostname) + { + name: "plain hostname fallback to https", + holdIdentifier: "hold.example.com", + want: "https://hold.example.com", + }, + { + name: "plain single word fallback to https", + holdIdentifier: "myhold", + want: "https://myhold", + }, + + // Edge cases + { + name: "empty string fallback", + holdIdentifier: "", + want: "https://", + }, + { + name: "did:web empty hostname", + holdIdentifier: "did:web:", + want: "https://", + }, + { + name: "just did:web prefix", + holdIdentifier: "did:web", + want: "https://did:web", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ResolveHoldURL(tt.holdIdentifier) + if got != tt.want { + t.Errorf("ResolveHoldURL(%q) = %q, want %q", tt.holdIdentifier, got, tt.want) + } + }) + } +} + +// TestResolveHoldURLRoundTrip tests that converting back and forth works +func TestResolveHoldURLRoundTrip(t *testing.T) { + tests := []struct { + name string + input string + wantHTTP bool // true if result should be http, false for https + }{ + {"domain to https and idempotent", "did:web:hold.atcr.io", false}, + {"IP to http and idempotent", "did:web:192.168.1.1", true}, + {"port to http and idempotent", "did:web:example.com:8080", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // First conversion + first := ResolveHoldURL(tt.input) + + // Second conversion (should be idempotent since output is URL) + second := ResolveHoldURL(first) + + if first != second { + t.Errorf("ResolveHoldURL is not idempotent: first=%q, second=%q", first, second) + } + + // Verify correct protocol + if tt.wantHTTP { + if !hasPrefix(first, "http://") { + t.Errorf("Expected http:// prefix, got %q", first) + } + } else { + if !hasPrefix(first, "https://") { + t.Errorf("Expected https:// prefix, got %q", first) + } + } + }) + } +} + +// Helper function to check prefix +func hasPrefix(s, prefix string) bool { + return len(s) >= len(prefix) && s[:len(prefix)] == prefix +} + // TestResolveIdentity tests resolving identifiers to DID, handle, and PDS endpoint func TestResolveIdentity(t *testing.T) { tests := []struct { diff --git a/pkg/atproto/utils.go b/pkg/atproto/utils.go deleted file mode 100644 index 06cf613..0000000 --- a/pkg/atproto/utils.go +++ /dev/null @@ -1,33 +0,0 @@ -package atproto - -import "strings" - -// ResolveHoldURL converts a hold identifier (DID or URL) to an HTTP/HTTPS URL -// Handles both formats for backward compatibility: -// - DID format: did:web:hold01.atcr.io → https://hold01.atcr.io -// - DID with port: did:web:172.28.0.3:8080 → http://172.28.0.3:8080 -// - URL format: https://hold.example.com → https://hold.example.com (passthrough) -func ResolveHoldURL(holdIdentifier string) string { - // If it's already a URL (has scheme), return as-is - if strings.HasPrefix(holdIdentifier, "http://") || strings.HasPrefix(holdIdentifier, "https://") { - return holdIdentifier - } - - // If it's a DID, convert to URL - if after, ok := strings.CutPrefix(holdIdentifier, "did:web:"); ok { - hostname := after - - // Use HTTP for localhost/IP addresses with ports, HTTPS for domains - if strings.Contains(hostname, ":") || - strings.Contains(hostname, "127.0.0.1") || - strings.Contains(hostname, "localhost") || - // Check if it's an IP address (contains only digits and dots in first part) - (len(hostname) > 0 && hostname[0] >= '0' && hostname[0] <= '9') { - return "http://" + hostname - } - return "https://" + hostname - } - - // Fallback: assume it's a hostname and use HTTPS - return "https://" + holdIdentifier -} diff --git a/pkg/atproto/utils_test.go b/pkg/atproto/utils_test.go deleted file mode 100644 index 849082c..0000000 --- a/pkg/atproto/utils_test.go +++ /dev/null @@ -1,189 +0,0 @@ -package atproto - -import "testing" - -func TestResolveHoldURL(t *testing.T) { - tests := []struct { - name string - holdIdentifier string - want string - }{ - // URL passthrough tests - { - name: "http URL passthrough", - holdIdentifier: "http://hold.example.com", - want: "http://hold.example.com", - }, - { - name: "https URL passthrough", - holdIdentifier: "https://hold.example.com", - want: "https://hold.example.com", - }, - { - name: "http URL with port passthrough", - holdIdentifier: "http://hold.example.com:8080", - want: "http://hold.example.com:8080", - }, - { - name: "https URL with port passthrough", - holdIdentifier: "https://hold.example.com:8443", - want: "https://hold.example.com:8443", - }, - { - name: "http URL with path passthrough", - holdIdentifier: "http://hold.example.com/some/path", - want: "http://hold.example.com/some/path", - }, - - // did:web to HTTPS (domain names) - { - name: "did:web domain to https", - holdIdentifier: "did:web:hold01.atcr.io", - want: "https://hold01.atcr.io", - }, - { - name: "did:web subdomain to https", - holdIdentifier: "did:web:my-hold.example.com", - want: "https://my-hold.example.com", - }, - { - name: "did:web simple domain to https", - holdIdentifier: "did:web:example.com", - want: "https://example.com", - }, - - // did:web to HTTP (ports) - { - name: "did:web with port to http", - holdIdentifier: "did:web:172.28.0.3:8080", - want: "http://172.28.0.3:8080", - }, - { - name: "did:web domain with port to http", - holdIdentifier: "did:web:hold.example.com:8080", - want: "http://hold.example.com:8080", - }, - { - name: "did:web localhost with port to http", - holdIdentifier: "did:web:localhost:8080", - want: "http://localhost:8080", - }, - - // did:web to HTTP (localhost) - { - name: "did:web localhost to http", - holdIdentifier: "did:web:localhost", - want: "http://localhost", - }, - - // did:web to HTTP (127.0.0.1) - { - name: "did:web 127.0.0.1 to http", - holdIdentifier: "did:web:127.0.0.1", - want: "http://127.0.0.1", - }, - { - name: "did:web 127.0.0.1 with port to http", - holdIdentifier: "did:web:127.0.0.1:8080", - want: "http://127.0.0.1:8080", - }, - - // did:web to HTTP (IP addresses) - { - name: "did:web IPv4 address to http", - holdIdentifier: "did:web:192.168.1.1", - want: "http://192.168.1.1", - }, - { - name: "did:web IPv4 with port to http", - holdIdentifier: "did:web:10.0.0.5:3000", - want: "http://10.0.0.5:3000", - }, - { - name: "did:web private IP to http", - holdIdentifier: "did:web:172.16.0.1", - want: "http://172.16.0.1", - }, - - // Fallback behavior (plain hostname) - { - name: "plain hostname fallback to https", - holdIdentifier: "hold.example.com", - want: "https://hold.example.com", - }, - { - name: "plain single word fallback to https", - holdIdentifier: "myhold", - want: "https://myhold", - }, - - // Edge cases - { - name: "empty string fallback", - holdIdentifier: "", - want: "https://", - }, - { - name: "did:web empty hostname", - holdIdentifier: "did:web:", - want: "https://", - }, - { - name: "just did:web prefix", - holdIdentifier: "did:web", - want: "https://did:web", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := ResolveHoldURL(tt.holdIdentifier) - if got != tt.want { - t.Errorf("ResolveHoldURL(%q) = %q, want %q", tt.holdIdentifier, got, tt.want) - } - }) - } -} - -// TestResolveHoldURLRoundTrip tests that converting back and forth works -func TestResolveHoldURLRoundTrip(t *testing.T) { - tests := []struct { - name string - input string - wantHTTP bool // true if result should be http, false for https - }{ - {"domain to https and idempotent", "did:web:hold.atcr.io", false}, - {"IP to http and idempotent", "did:web:192.168.1.1", true}, - {"port to http and idempotent", "did:web:example.com:8080", true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // First conversion - first := ResolveHoldURL(tt.input) - - // Second conversion (should be idempotent since output is URL) - second := ResolveHoldURL(first) - - if first != second { - t.Errorf("ResolveHoldURL is not idempotent: first=%q, second=%q", first, second) - } - - // Verify correct protocol - if tt.wantHTTP { - if !hasPrefix(first, "http://") { - t.Errorf("Expected http:// prefix, got %q", first) - } - } else { - if !hasPrefix(first, "https://") { - t.Errorf("Expected https:// prefix, got %q", first) - } - } - }) - } -} - -// Helper function to check prefix -func hasPrefix(s, prefix string) bool { - return len(s) >= len(prefix) && s[:len(prefix)] == prefix -} diff --git a/pkg/auth/hold_remote.go b/pkg/auth/hold_remote.go index 2df51ce..8fdd685 100644 --- a/pkg/auth/hold_remote.go +++ b/pkg/auth/hold_remote.go @@ -9,7 +9,6 @@ import ( "log/slog" "net/http" "net/url" - "strings" "sync" "time" @@ -219,10 +218,7 @@ func (a *RemoteHoldAuthorizer) setCachedCaptainRecord(holdDID string, record *at // fetchCaptainRecordFromXRPC queries the hold's XRPC endpoint for captain record func (a *RemoteHoldAuthorizer) fetchCaptainRecordFromXRPC(ctx context.Context, holdDID string) (*atproto.CaptainRecord, error) { // Resolve DID to URL - holdURL, err := a.resolveDIDToURL(holdDID) - if err != nil { - return nil, fmt.Errorf("failed to resolve hold DID: %w", err) - } + holdURL := atproto.ResolveHoldURL(holdDID) // Build XRPC request URL // GET /xrpc/com.atproto.repo.getRecord?repo={did}&collection=io.atcr.hold.captain&rkey=self @@ -326,10 +322,7 @@ func (a *RemoteHoldAuthorizer) IsCrewMember(ctx context.Context, holdDID, userDI // isCrewMemberNoCache queries XRPC without caching (internal helper) func (a *RemoteHoldAuthorizer) isCrewMemberNoCache(ctx context.Context, holdDID, userDID string) (bool, error) { // Resolve DID to URL - holdURL, err := a.resolveDIDToURL(holdDID) - if err != nil { - return false, fmt.Errorf("failed to resolve hold DID: %w", err) - } + holdURL := atproto.ResolveHoldURL(holdDID) // Build XRPC request URL // GET /xrpc/com.atproto.repo.listRecords?repo={did}&collection=io.atcr.hold.crew @@ -407,33 +400,6 @@ func (a *RemoteHoldAuthorizer) CheckWriteAccess(ctx context.Context, holdDID, us return CheckWriteAccessWithCaptain(captain, userDID, isCrew), nil } -// resolveDIDToURL converts a did:web DID to an HTTP/HTTPS URL -// Example: did:web:hold01.atcr.io → https://hold01.atcr.io -// Example (test mode): did:web:172.28.0.3:8080 → http://172.28.0.3:8080 -func (a *RemoteHoldAuthorizer) resolveDIDToURL(did string) (string, error) { - // Handle did:web format - if !strings.HasPrefix(did, "did:web:") { - return "", fmt.Errorf("only did:web is supported, got: %s", did) - } - - // Extract hostname from did:web:hostname - hostname := strings.TrimPrefix(did, "did:web:") - - // In test mode OR for local addresses, use HTTP instead of HTTPS - // This matches the logic in pkg/appview/storage/proxy_blob_store.go:resolveHoldURL - if a.testMode || - strings.Contains(hostname, ":") || - strings.Contains(hostname, "127.0.0.1") || - strings.Contains(hostname, "localhost") || - // Check if it's an IP address (contains only digits and dots) - (len(hostname) > 0 && (hostname[0] >= '0' && hostname[0] <= '9')) { - return "http://" + hostname, nil - } - - // Convert to HTTPS URL for production domains - return "https://" + hostname, nil -} - // nullString converts a string to sql.NullString func nullString(s string) sql.NullString { if s == "" { diff --git a/pkg/auth/hold_remote_test.go b/pkg/auth/hold_remote_test.go index ee2308f..35b1d9c 100644 --- a/pkg/auth/hold_remote_test.go +++ b/pkg/auth/hold_remote_test.go @@ -52,91 +52,6 @@ func setupTestDB(t *testing.T) *sql.DB { return testDB } -func TestResolveDIDToURL_ProductionDomain(t *testing.T) { - remote := &RemoteHoldAuthorizer{ - testMode: false, - } - - url, err := remote.resolveDIDToURL("did:web:hold01.atcr.io") - if err != nil { - t.Fatalf("resolveDIDToURL() error = %v", err) - } - - expected := "https://hold01.atcr.io" - if url != expected { - t.Errorf("Expected URL %q, got %q", expected, url) - } -} - -func TestResolveDIDToURL_LocalhostHTTP(t *testing.T) { - remote := &RemoteHoldAuthorizer{ - testMode: false, - } - - tests := []struct { - name string - did string - expected string - }{ - { - name: "localhost", - did: "did:web:localhost:8080", - expected: "http://localhost:8080", - }, - { - name: "127.0.0.1", - did: "did:web:127.0.0.1:8080", - expected: "http://127.0.0.1:8080", - }, - { - name: "IP address", - did: "did:web:172.28.0.3:8080", - expected: "http://172.28.0.3:8080", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - url, err := remote.resolveDIDToURL(tt.did) - if err != nil { - t.Fatalf("resolveDIDToURL() error = %v", err) - } - - if url != tt.expected { - t.Errorf("Expected URL %q, got %q", tt.expected, url) - } - }) - } -} - -func TestResolveDIDToURL_TestMode(t *testing.T) { - remote := &RemoteHoldAuthorizer{ - testMode: true, - } - - // In test mode, even production domains should use HTTP - url, err := remote.resolveDIDToURL("did:web:hold01.atcr.io") - if err != nil { - t.Fatalf("resolveDIDToURL() error = %v", err) - } - - expected := "http://hold01.atcr.io" - if url != expected { - t.Errorf("Expected HTTP URL in test mode, got %q", url) - } -} - -func TestResolveDIDToURL_InvalidDID(t *testing.T) { - remote := &RemoteHoldAuthorizer{ - testMode: false, - } - - _, err := remote.resolveDIDToURL("did:plc:invalid") - if err == nil { - t.Error("Expected error for non-did:web DID") - } -} - func TestFetchCaptainRecordFromXRPC(t *testing.T) { // Create mock HTTP server server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {