diff --git a/pkg/appview/db/oauth_store.go b/pkg/appview/db/oauth_store.go index 76d894c..4fd5cfb 100644 --- a/pkg/appview/db/oauth_store.go +++ b/pkg/appview/db/oauth_store.go @@ -377,7 +377,7 @@ func (s *OAuthStore) CleanupExpiredAuthRequests(ctx context.Context) { } } -// InvalidateSessionsWithMismatchedScopes removes all sessions whose scopes don't match the desired scopes +// InvalidateSessionsWithMismatchedScopes removes all sessions that don't cover the desired scopes // This is called on AppView startup to ensure all sessions have current scopes // Returns the count of invalidated sessions func (s *OAuthStore) InvalidateSessionsWithMismatchedScopes(ctx context.Context, desiredScopes []string) (int, error) { @@ -408,14 +408,15 @@ func (s *OAuthStore) InvalidateSessionsWithMismatchedScopes(ctx context.Context, continue } - // Check if scopes match (expands include: scopes before comparing) - if !atoauth.ScopesMatch(sessionData.Scopes, desiredScopes) { - slog.Debug("Session has mismatched scopes", + // Evict sessions missing a scope we now ask for. Extra grants are + // fine: a PDS may add its own, and dropping a scope from the desired + // set shouldn't sign everyone out. + if missing := atoauth.MissingScopes(sessionData.Scopes, desiredScopes); len(missing) > 0 { + slog.Debug("Session is missing scopes", "component", "oauth/store", "session_key", sessionKey, "account_did", accountDID, - "session_scopes", sessionData.Scopes, - "desired_scopes", desiredScopes, + "missing", missing, ) sessionsToDelete = append(sessionsToDelete, sessionKey) } diff --git a/pkg/appview/db/oauth_store_test.go b/pkg/appview/db/oauth_store_test.go index cd12552..58e4d53 100644 --- a/pkg/appview/db/oauth_store_test.go +++ b/pkg/appview/db/oauth_store_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - atcroauth "atcr.io/pkg/auth/oauth" "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/bluesky-social/indigo/atproto/syntax" ) @@ -91,7 +90,7 @@ func TestInvalidateSessionsWithMismatchedScopes(t *testing.T) { t.Error("Expected session to be deleted (should error), but got no error") } - // Test 4: Session with extra scopes - should be invalidated + // Test 4: Session with extra scopes - kept; only missing scopes evict extraScopeSession := createSession("did:plc:test3", "session3", []string{"atproto", "blob:image/png", "extra:scope"}) if err := store.SaveSession(ctx, extraScopeSession); err != nil { t.Fatalf("Failed to save extra scope session: %v", err) @@ -101,8 +100,8 @@ func TestInvalidateSessionsWithMismatchedScopes(t *testing.T) { if err != nil { t.Fatalf("Expected no error, got: %v", err) } - if count != 1 { - t.Errorf("Expected 1 invalidated session (extra scope), got %d", count) + if count != 0 { + t.Errorf("Expected 0 invalidated sessions (extra scope), got %d", count) } // Test 5: Multiple sessions with mixed matches - only mismatch should be invalidated @@ -161,75 +160,6 @@ func TestInvalidateSessionsWithMismatchedScopes(t *testing.T) { } } -func TestScopesMatch(t *testing.T) { - // Test oauth.ScopesMatch function including include: scope expansion - tests := []struct { - name string - stored []string - desired []string - expected bool - }{ - { - name: "exact match", - stored: []string{"atproto", "blob:image/png"}, - desired: []string{"atproto", "blob:image/png"}, - expected: true, - }, - { - name: "different order", - stored: []string{"blob:image/png", "atproto"}, - desired: []string{"atproto", "blob:image/png"}, - expected: true, - }, - { - name: "missing scope", - stored: []string{"atproto"}, - desired: []string{"atproto", "blob:image/png"}, - expected: false, - }, - { - name: "extra scope", - stored: []string{"atproto", "blob:image/png", "extra"}, - desired: []string{"atproto", "blob:image/png"}, - expected: false, - }, - { - name: "both empty", - stored: []string{}, - desired: []string{}, - expected: true, - }, - { - name: "nil vs empty", - stored: nil, - desired: []string{}, - expected: true, - }, - { - name: "include scope expansion", - stored: []string{ - "atproto", - "repo?collection=io.atcr.manifest&collection=io.atcr.repo.page&collection=io.atcr.sailor.profile&collection=io.atcr.sailor.star&collection=io.atcr.tag", - }, - desired: []string{ - "atproto", - "include:io.atcr.authFullApp", - }, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := atcroauth.ScopesMatch(tt.stored, tt.desired) - if result != tt.expected { - t.Errorf("ScopesMatch(%v, %v) = %v, want %v", - tt.stored, tt.desired, result, tt.expected) - } - }) - } -} - func TestOAuthStoreSessionLifecycle(t *testing.T) { // Basic test to ensure SaveSession, GetSession, DeleteSession work correctly db, err := InitDB(":memory:", LibsqlConfig{}) diff --git a/pkg/appview/handlers/api.go b/pkg/appview/handlers/api.go index 50d13b2..94e1ed7 100644 --- a/pkg/appview/handlers/api.go +++ b/pkg/appview/handlers/api.go @@ -67,6 +67,10 @@ func (h *StarRepositoryHandler) ServeHTTP(w http.ResponseWriter, r *http.Request http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } slog.Error("Failed to create star record", "error", err) http.Error(w, fmt.Sprintf("Failed to create star: %v", err), http.StatusInternalServerError) return @@ -137,6 +141,10 @@ func (h *UnstarRepositoryHandler) ServeHTTP(w http.ResponseWriter, r *http.Reque http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } slog.Error("Failed to delete star record", "error", err) http.Error(w, fmt.Sprintf("Failed to delete star: %v", err), http.StatusInternalServerError) return diff --git a/pkg/appview/handlers/images.go b/pkg/appview/handlers/images.go index d0e505f..e10722d 100644 --- a/pkg/appview/handlers/images.go +++ b/pkg/appview/handlers/images.go @@ -74,6 +74,10 @@ func (h *DeleteTagHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, fmt.Sprintf("Failed to delete tag from PDS: %v", err), http.StatusInternalServerError) return } @@ -200,6 +204,10 @@ func (h *DeleteManifestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, fmt.Sprintf("Failed to delete tag '%s' from PDS: %v", tag, err), http.StatusInternalServerError) return } @@ -250,6 +258,10 @@ func (h *DeleteManifestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, fmt.Sprintf("Failed to delete manifest from PDS: %v", err), http.StatusInternalServerError) return } @@ -467,6 +479,10 @@ func (h *UploadAvatarHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, fmt.Sprintf("Failed to upload image: %v", err), http.StatusInternalServerError) return } @@ -505,6 +521,10 @@ func (h *UploadAvatarHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, fmt.Sprintf("Failed to update repository page: %v", err), http.StatusInternalServerError) return } diff --git a/pkg/appview/handlers/oauth_errors.go b/pkg/appview/handlers/oauth_errors.go index 313d7e0..1ee49bc 100644 --- a/pkg/appview/handlers/oauth_errors.go +++ b/pkg/appview/handlers/oauth_errors.go @@ -26,16 +26,19 @@ func isOAuthError(err error) bool { } // Check structured error types first + // A 403 is deliberately not here: the PDS returns it for a session that + // is alive but lacks a permission (InsufficientScope), and deleting the + // session over that signs the user out without fixing anything. var xrpcErr *xrpc.Error - if errors.As(err, &xrpcErr) && (xrpcErr.StatusCode == 401 || xrpcErr.StatusCode == 403) { + if errors.As(err, &xrpcErr) && xrpcErr.StatusCode == 401 { return true } var apiErr *atclient.APIError if errors.As(err, &apiErr) { - if apiErr.StatusCode == 401 || apiErr.StatusCode == 403 { + if apiErr.StatusCode == 401 { return true } - if apiErr.Name == "InvalidToken" || apiErr.Name == "InsufficientScope" || apiErr.Name == "InvalidGrant" { + if apiErr.Name == "InvalidToken" || apiErr.Name == "InvalidGrant" { return true } } @@ -88,3 +91,24 @@ func handleOAuthError(ctx context.Context, refresher *oauth.Refresher, did strin return true } + +// pdsDenied reports whether err is the user's PDS refusing a write (HTTP 403), +// and the message to show them. The session is fine; its grant is missing a +// permission, which PDSes spell differently (InsufficientScope, +// ScopeMissingError), so only the status is checked. Callers answer 403 with +// the message instead of a 500 that explains nothing. +func pdsDenied(err error) (string, bool) { + var apiErr *atclient.APIError + if errors.As(err, &apiErr) && apiErr.StatusCode == 403 { + reason := apiErr.Message + if reason == "" { + reason = apiErr.Name + } + return "Your PDS refused this change: " + reason + ". Log in again and approve all requested permissions.", true + } + var xrpcErr *xrpc.Error + if errors.As(err, &xrpcErr) && xrpcErr.StatusCode == 403 { + return "Your PDS refused this change. Log in again and approve all requested permissions.", true + } + return "", false +} diff --git a/pkg/appview/handlers/oauth_errors_test.go b/pkg/appview/handlers/oauth_errors_test.go new file mode 100644 index 0000000..686fe67 --- /dev/null +++ b/pkg/appview/handlers/oauth_errors_test.go @@ -0,0 +1,65 @@ +package handlers + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/bluesky-social/indigo/atproto/atclient" + "github.com/bluesky-social/indigo/xrpc" +) + +func TestIsOAuthError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"canceled", fmt.Errorf("putRecord: %w", context.Canceled), false}, + {"api error 401", &atclient.APIError{StatusCode: 401}, true}, + {"xrpc 401", &xrpc.Error{StatusCode: 401}, true}, + {"api error InvalidGrant", &atclient.APIError{StatusCode: 400, Name: "InvalidGrant"}, true}, + // A 403 is a missing permission on a live session, not a dead one. + {"api error 403 InsufficientScope", &atclient.APIError{StatusCode: 403, Name: "InsufficientScope"}, false}, + {"xrpc 403", &xrpc.Error{StatusCode: 403}, false}, + {"plain error", errors.New("boom"), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isOAuthError(tt.err); got != tt.want { + t.Errorf("isOAuthError(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +func TestPDSDenied(t *testing.T) { + tests := []struct { + name string + err error + want bool + contains string + }{ + {"tranquil InsufficientScope", fmt.Errorf("putRecord failed: %w", &atclient.APIError{StatusCode: 403, Name: "InsufficientScope", Message: "Insufficient scope to create records in io.atcr.sailor.star"}), true, "io.atcr.sailor.star"}, + {"reference ScopeMissingError", &atclient.APIError{StatusCode: 403, Name: "ScopeMissingError", Message: `Missing required scope "repo:io.atcr.sailor.star?action=create"`}, true, "repo:io.atcr.sailor.star"}, + {"name only", &atclient.APIError{StatusCode: 403, Name: "Forbidden"}, true, "Forbidden"}, + {"xrpc 403", &xrpc.Error{StatusCode: 403}, true, "refused"}, + {"401 is not a denial", &atclient.APIError{StatusCode: 401, Name: "InvalidToken"}, false, ""}, + {"500", &atclient.APIError{StatusCode: 500}, false, ""}, + {"plain", errors.New("boom"), false, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg, ok := pdsDenied(tt.err) + if ok != tt.want { + t.Fatalf("pdsDenied ok = %v, want %v", ok, tt.want) + } + if !strings.Contains(msg, tt.contains) { + t.Errorf("message %q should contain %q", msg, tt.contains) + } + }) + } +} diff --git a/pkg/appview/handlers/oauth_pages.go b/pkg/appview/handlers/oauth_pages.go new file mode 100644 index 0000000..cb2d19f --- /dev/null +++ b/pkg/appview/handlers/oauth_pages.go @@ -0,0 +1,73 @@ +package handlers + +import ( + "bytes" + "net/http" + + "atcr.io/pkg/auth/oauth" +) + +// OAuthPages renders the browser-facing pages of the OAuth authorize and +// callback endpoints (pkg/auth/oauth.Server) in the site layout. It is set on +// the server with SetPageRenderer; the server keeps ownership of the status +// codes and falls back to its inline templates if a render fails. +type OAuthPages struct { + BaseUIHandler +} + +var _ oauth.PageRenderer = (*OAuthPages)(nil) + +func (h *OAuthPages) meta(title string) *PageMeta { + return NewPageMeta(title+" - "+h.ClientShortName, ""). + WithRobots("noindex"). + WithSiteName(h.ClientShortName) +} + +func (h *OAuthPages) execute(name string, data any) ([]byte, error) { + var buf bytes.Buffer + if err := h.Templates.ExecuteTemplate(&buf, name, data); err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +// RenderOAuthError implements oauth.PageRenderer. +func (h *OAuthPages) RenderOAuthError(r *http.Request, message string) ([]byte, error) { + return h.execute("oauth-error", struct { + PageData + Meta *PageMeta + Message string + }{ + PageData: NewPageData(r, &h.BaseUIHandler), + Meta: h.meta("Authorization failed"), + Message: message, + }) +} + +// RenderMissingScopes implements oauth.PageRenderer. +func (h *OAuthPages) RenderMissingScopes(r *http.Request, missing []string, retryURL string) ([]byte, error) { + return h.execute("oauth-missing-scopes", struct { + PageData + Meta *PageMeta + Missing []string + RetryURL string + }{ + PageData: NewPageData(r, &h.BaseUIHandler), + Meta: h.meta("Missing permissions"), + Missing: missing, + RetryURL: retryURL, + }) +} + +// RenderAuthorized implements oauth.PageRenderer. +func (h *OAuthPages) RenderAuthorized(r *http.Request, handle string) ([]byte, error) { + return h.execute("oauth-authorized", struct { + PageData + Meta *PageMeta + Handle string + }{ + PageData: NewPageData(r, &h.BaseUIHandler), + Meta: h.meta("Authorization successful"), + Handle: handle, + }) +} diff --git a/pkg/appview/handlers/oauth_pages_test.go b/pkg/appview/handlers/oauth_pages_test.go new file mode 100644 index 0000000..44dae19 --- /dev/null +++ b/pkg/appview/handlers/oauth_pages_test.go @@ -0,0 +1,113 @@ +package handlers_test + +import ( + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "atcr.io/pkg/appview" + "atcr.io/pkg/appview/handlers" + _ "atcr.io/themes/seamark" +) + +// TestOAuthPages_Render renders each OAuth page in the site layout, for both +// the default templates and the seamark theme overlay. Set +// ATCR_OAUTH_PAGES_DUMP= to write the HTML out for inspection. +func TestOAuthPages_Render(t *testing.T) { + seamark, err := appview.LookupTheme("seamark") + if err != nil { + t.Fatalf("lookup seamark theme: %v", err) + } + + themes := []struct { + name string + overrides *appview.BrandingOverrides + shortName string + }{ + {"default", nil, "ATCR"}, + {"seamark", seamark, "Seamark"}, + } + + for _, theme := range themes { + t.Run(theme.name, func(t *testing.T) { + tmpl, err := appview.Templates(theme.overrides) + if err != nil { + t.Fatalf("load templates: %v", err) + } + pages := &handlers.OAuthPages{BaseUIHandler: handlers.BaseUIHandler{ + Templates: tmpl, + RegistryURL: "registry.example", + SiteURL: "site.example", + ClientName: "Test Registry", + ClientShortName: theme.shortName, + }} + req := httptest.NewRequest("GET", "/auth/oauth/callback", nil) + + cases := []struct { + page string + render func() ([]byte, error) + want []string + }{ + {"error", func() ([]byte, error) { + return pages.RenderOAuthError(req, "OAuth error: access_denied - ") + }, []string{ + "Authorization failed", + "OAuth error: access_denied - <script>nope</script>", + `href="/"`, + }}, + {"missing-scopes", func() ([]byte, error) { + return pages.RenderMissingScopes(req, []string{"repo:io.atcr.manifest", "blob:*/*"}, "/auth/oauth/authorize?handle=did%3Aplc%3Aabc") + }, []string{ + "Some permissions weren't granted", + ">repo:io.atcr.manifest", + ">blob:*/*", + `href="/auth/oauth/authorize?handle=did%3Aplc%3Aabc"`, + "Try again", + "Return to home", + "didn't grant everything " + theme.shortName + " needs", + }}, + {"authorized", func() ([]byte, error) { + return pages.RenderAuthorized(req, "alice.test") + }, []string{ + "Authorization successful", + "alice.test", + `content="3;url=/settings"`, + }}, + } + + for _, c := range cases { + t.Run(c.page, func(t *testing.T) { + out, err := c.render() + if err != nil { + t.Fatalf("render: %v", err) + } + body := string(out) + + // Site chrome: shared head, simple nav, footer. + for _, chrome := range []string{``, `id="main-content"`, `class="navbar`, "", `name="robots" content="noindex"`} { + if !strings.Contains(body, chrome) { + t.Errorf("missing site chrome %q", chrome) + } + } + for _, w := range c.want { + if !strings.Contains(body, w) { + t.Errorf("missing %q", w) + } + } + if strings.Contains(body, "—") { + t.Error("page contains an em dash") + } + + if dir := os.Getenv("ATCR_OAUTH_PAGES_DUMP"); dir != "" { + name := filepath.Join(dir, theme.name+"-"+c.page+".html") + if err := os.WriteFile(name, out, 0o644); err != nil { + t.Fatalf("dump: %v", err) + } + } + }) + } + }) + } +} diff --git a/pkg/appview/handlers/repo_editor.go b/pkg/appview/handlers/repo_editor.go index 91d355c..e083381 100644 --- a/pkg/appview/handlers/repo_editor.go +++ b/pkg/appview/handlers/repo_editor.go @@ -80,6 +80,10 @@ func (h *SaveRepoPageHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) http.Error(w, "Authentication failed, please log in again", http.StatusUnauthorized) return } + if msg, ok := pdsDenied(err); ok { + http.Error(w, msg, http.StatusForbidden) + return + } http.Error(w, "Failed to save description", http.StatusInternalServerError) return } diff --git a/pkg/appview/routes/routes.go b/pkg/appview/routes/routes.go index 166c178..0e9381a 100644 --- a/pkg/appview/routes/routes.go +++ b/pkg/appview/routes/routes.go @@ -53,47 +53,15 @@ type UIDependencies struct { CredHelper installscript.Brand // Credential helper brand (credHelpers value, binary name, config dir) } +// NewOAuthPages returns the site-styled renderer for the OAuth endpoints' +// browser pages, for oauth.Server.SetPageRenderer. +func NewOAuthPages(deps UIDependencies) *uihandlers.OAuthPages { + return &uihandlers.OAuthPages{BaseUIHandler: newBaseUIHandler(deps)} +} + // RegisterUIRoutes registers all web UI and API routes on the provided router func RegisterUIRoutes(router chi.Router, deps UIDependencies) { - // SiteURL is always derived from BaseURL (the website domain) - siteURL := trimRegistryURL(deps.BaseURL) - - // RegistryURL is the registry domain if configured, otherwise same as SiteURL - registryURL := siteURL - if deps.RegistryDomain != "" { - registryURL = deps.RegistryDomain - } - - // Create base with all dependencies - handlers just embed this - base := uihandlers.BaseUIHandler{ - Templates: deps.Templates, - RegistryURL: registryURL, - RegistryDomains: deps.RegistryDomains, - SiteURL: siteURL, - DB: deps.Database, - ReadOnlyDB: deps.ReadOnlyDB, - Refresher: deps.Refresher, - HealthChecker: deps.HealthChecker, - ReadmeFetcher: deps.ReadmeFetcher, - Directory: deps.OAuthClientApp.Dir, - OAuthClientApp: deps.OAuthClientApp, - SessionStore: deps.SessionStore, - DeviceStore: deps.DeviceStore, - OAuthStore: deps.OAuthStore, - BillingManager: deps.BillingManager, - WebhookDispatcher: deps.WebhookDispatcher, - DefaultHoldDID: deps.DefaultHoldDID, - ManagedHolds: deps.ManagedHolds, - CompanyName: deps.LegalConfig.CompanyName, - Jurisdiction: deps.LegalConfig.Jurisdiction, - ClientName: deps.ClientName, - ClientShortName: deps.ClientShortName, - BillingEnabled: deps.BillingManager != nil && deps.BillingManager.Enabled(), - AIAdvisorEnabled: deps.BillingManager != nil && deps.BillingManager.Enabled() && deps.ClaudeAPIKey != "", - SourceURL: deps.SourceURL, - BlueskyProfile: deps.BlueskyProfile, - CredHelper: deps.CredHelper, - } + base := newBaseUIHandler(deps) // OAuth login routes (public) router.Get("/auth/oauth/login", (&uihandlers.LoginHandler{BaseUIHandler: base}).ServeHTTP) @@ -275,6 +243,49 @@ func RegisterUIRoutes(router chi.Router, deps UIDependencies) { ).ServeHTTP) } +// newBaseUIHandler builds the dependencies every UI handler embeds. +func newBaseUIHandler(deps UIDependencies) uihandlers.BaseUIHandler { + // SiteURL is always derived from BaseURL (the website domain) + siteURL := trimRegistryURL(deps.BaseURL) + + // RegistryURL is the registry domain if configured, otherwise same as SiteURL + registryURL := siteURL + if deps.RegistryDomain != "" { + registryURL = deps.RegistryDomain + } + + // Create base with all dependencies - handlers just embed this + return uihandlers.BaseUIHandler{ + Templates: deps.Templates, + RegistryURL: registryURL, + RegistryDomains: deps.RegistryDomains, + SiteURL: siteURL, + DB: deps.Database, + ReadOnlyDB: deps.ReadOnlyDB, + Refresher: deps.Refresher, + HealthChecker: deps.HealthChecker, + ReadmeFetcher: deps.ReadmeFetcher, + Directory: deps.OAuthClientApp.Dir, + OAuthClientApp: deps.OAuthClientApp, + SessionStore: deps.SessionStore, + DeviceStore: deps.DeviceStore, + OAuthStore: deps.OAuthStore, + BillingManager: deps.BillingManager, + WebhookDispatcher: deps.WebhookDispatcher, + DefaultHoldDID: deps.DefaultHoldDID, + ManagedHolds: deps.ManagedHolds, + CompanyName: deps.LegalConfig.CompanyName, + Jurisdiction: deps.LegalConfig.Jurisdiction, + ClientName: deps.ClientName, + ClientShortName: deps.ClientShortName, + BillingEnabled: deps.BillingManager != nil && deps.BillingManager.Enabled(), + AIAdvisorEnabled: deps.BillingManager != nil && deps.BillingManager.Enabled() && deps.ClaudeAPIKey != "", + SourceURL: deps.SourceURL, + BlueskyProfile: deps.BlueskyProfile, + CredHelper: deps.CredHelper, + } +} + // CORSMiddleware returns a middleware that sets CORS headers for API endpoints func CORSMiddleware() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { diff --git a/pkg/appview/server.go b/pkg/appview/server.go index 9091c0a..90422ab 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -400,7 +400,7 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, } // Register UI routes - routes.RegisterUIRoutes(mainRouter, routes.UIDependencies{ + uiDeps := routes.UIDependencies{ Database: s.Database, ReadOnlyDB: s.ReadOnlyDB, SessionStore: s.SessionStore, @@ -428,7 +428,8 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, CompanyName: cfg.Legal.CompanyName, Jurisdiction: cfg.Legal.Jurisdiction, }, - }) + } + routes.RegisterUIRoutes(mainRouter, uiDeps) // Register Stripe webhook route (if billing enabled) s.BillingManager.RegisterRoutes(mainRouter) @@ -439,6 +440,9 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, if s.SessionStore != nil { s.OAuthServer.SetUISessionStore(s.SessionStore) } + // Draw the OAuth error, missing-permissions and success pages in the + // site layout instead of the package's bare fallback HTML. + s.OAuthServer.SetPageRenderer(routes.NewOAuthPages(uiDeps)) // Register OAuth post-auth callback (closure captures s for hook dispatch) s.OAuthServer.SetPostAuthCallback(func(ctx context.Context, did, handle, pdsEndpoint, sessionID string) error { diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index e4ca525..066dd5d 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -16,25 +16,40 @@ import ( "atcr.io/pkg/appview/holdpurge" "atcr.io/pkg/appview/readme" "atcr.io/pkg/atproto" + "github.com/bluesky-social/indigo/atproto/atclient" "github.com/distribution/distribution/v3" "github.com/distribution/distribution/v3/registry/api/errcode" "github.com/opencontainers/go-digest" ) -// rateLimitToErrcode converts an upstream PDS rate-limit error into a -// distribution errcode.Error (HTTP 429). When ctx contains a -// RetryAfterCarrier, also stashes the retry-after duration so HTTP -// middleware can emit a Retry-After response header. Returns the original -// error untouched when it isn't a rate-limit error. -func rateLimitToErrcode(ctx context.Context, err error) error { +// pdsErrorToErrcode converts an upstream PDS error the client can act on into +// a distribution errcode.Error, and returns any other error untouched. +// +// A rate limit becomes a 429. When ctx contains a RetryAfterCarrier, the +// retry-after duration is stashed too so HTTP middleware can emit a +// Retry-After response header. +// +// A 403 becomes DENIED, carrying the PDS's own message. It almost always means +// the session wasn't granted this write (InsufficientScope), and left alone it +// reaches the client as a 500 "unknown error" that explains nothing. +func pdsErrorToErrcode(ctx context.Context, err error) error { var rl *atproto.RateLimitError - if !errors.As(err, &rl) { - return err + if errors.As(err, &rl) { + if rl.RetryAfter > 0 { + SetRetryAfter(ctx, rl.RetryAfter) + } + return errcode.ErrorCodeTooManyRequests.WithMessage(rl.Error()) } - if rl.RetryAfter > 0 { - SetRetryAfter(ctx, rl.RetryAfter) + var apiErr *atclient.APIError + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden { + msg := apiErr.Message + if msg == "" { + msg = apiErr.Name + } + return errcode.ErrorCodeDenied.WithMessage( + "your PDS refused the write: " + msg + ". Log in to ATCR again and approve all requested permissions.") } - return errcode.ErrorCodeTooManyRequests.WithMessage(rl.Error()) + return err } // pullDedup deduplicates pull notifications per puller+owner+repo within a 5-minute window. @@ -200,7 +215,7 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, // Upload manifest as blob to PDS blobRef, err := s.ctx.ATProtoClient.UploadBlob(ctx, payload, mediaType) if err != nil { - if rl := rateLimitToErrcode(ctx, err); rl != err { + if rl := pdsErrorToErrcode(ctx, err); rl != err { return "", rl } return "", fmt.Errorf("failed to upload manifest blob: %w", err) @@ -287,7 +302,7 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, rkey := digestToRKey(dgst) _, err = s.ctx.ATProtoClient.PutRecord(ctx, atproto.ManifestCollection, rkey, manifestRecord) if err != nil { - if rl := rateLimitToErrcode(ctx, err); rl != err { + if rl := pdsErrorToErrcode(ctx, err); rl != err { return "", rl } return "", fmt.Errorf("failed to store manifest record in ATProto: %w", err) @@ -316,7 +331,7 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, tagRecord := atproto.NewTagRecord(s.ctx.ATProtoClient.DID(), s.ctx.Repository, tag, dgst.String(), mediaType) _, err = s.ctx.ATProtoClient.PutRecord(ctx, atproto.TagCollection, tagRKey, tagRecord) if err != nil { - if rl := rateLimitToErrcode(ctx, err); rl != err { + if rl := pdsErrorToErrcode(ctx, err); rl != err { return "", rl } return "", fmt.Errorf("failed to store tag in ATProto: %w", err) @@ -462,7 +477,7 @@ func (s *ManifestStore) Delete(ctx context.Context, dgst digest.Digest) error { for _, tag := range inRepo { tagRKey := atproto.RepositoryTagToRKey(s.ctx.Repository, tag) if err := s.ctx.ATProtoClient.DeleteRecord(ctx, atproto.TagCollection, tagRKey); err != nil { - if rl := rateLimitToErrcode(ctx, err); rl != err { + if rl := pdsErrorToErrcode(ctx, err); rl != err { return rl } return err @@ -480,7 +495,7 @@ func (s *ManifestStore) Delete(ctx context.Context, dgst digest.Digest) error { rkey := digestToRKey(dgst) if err := s.ctx.ATProtoClient.DeleteRecord(ctx, atproto.ManifestCollection, rkey); err != nil { - if rl := rateLimitToErrcode(ctx, err); rl != err { + if rl := pdsErrorToErrcode(ctx, err); rl != err { return rl } return err diff --git a/pkg/appview/storage/pds_error_test.go b/pkg/appview/storage/pds_error_test.go new file mode 100644 index 0000000..fd78faf --- /dev/null +++ b/pkg/appview/storage/pds_error_test.go @@ -0,0 +1,52 @@ +package storage + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/bluesky-social/indigo/atproto/atclient" + "github.com/distribution/distribution/v3/registry/api/errcode" +) + +// TestPDSErrorToErrcode_InsufficientScopeIsDenied pins the fix for a push that +// failed at putRecord with 403 InsufficientScope and reached Docker as +// "500 unknown: unknown error". +func TestPDSErrorToErrcode_InsufficientScopeIsDenied(t *testing.T) { + pdsErr := fmt.Errorf("putRecord failed: %w", &atclient.APIError{ + StatusCode: http.StatusForbidden, + Name: "InsufficientScope", + Message: "Insufficient scope to create records in io.atcr.manifest", + }) + + got := pdsErrorToErrcode(context.Background(), pdsErr) + + var ec errcode.Error + if !errors.As(got, &ec) { + t.Fatalf("got %T (%v), want errcode.Error", got, got) + } + if ec.Code != errcode.ErrorCodeDenied { + t.Errorf("code = %v, want DENIED", ec.Code) + } + if ec.Code.Descriptor().HTTPStatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403", ec.Code.Descriptor().HTTPStatusCode) + } + if !strings.Contains(ec.Message, "io.atcr.manifest") { + t.Errorf("message %q should carry the PDS's reason", ec.Message) + } +} + +func TestPDSErrorToErrcode_OtherErrorsUntouched(t *testing.T) { + for _, err := range []error{ + errors.New("boom"), + &atclient.APIError{StatusCode: http.StatusInternalServerError, Name: "InternalServerError"}, + &atclient.APIError{StatusCode: http.StatusUnauthorized, Name: "InvalidToken"}, + } { + if got := pdsErrorToErrcode(context.Background(), err); got != err { + t.Errorf("pdsErrorToErrcode(%v) = %v, want it untouched", err, got) + } + } +} diff --git a/pkg/appview/templates/pages/oauth.html b/pkg/appview/templates/pages/oauth.html new file mode 100644 index 0000000..36b79a0 --- /dev/null +++ b/pkg/appview/templates/pages/oauth.html @@ -0,0 +1,121 @@ +{{/* + Pages rendered for the OAuth authorize/callback endpoints, which live in + pkg/auth/oauth. That package owns the status codes and falls back to its + own inline HTML when no appview renderer is set; these templates are the + site-styled versions, filled in by handlers.OAuthPages. + Plain language only on these screens (no maritime copy). +*/}} + +{{ define "oauth-error" }} + + + + {{ template "head" . }} + {{ template "meta" .Meta }} + + + {{ template "nav-simple" . }} + +
+
+

Authorization failed

+ + + +
+ Sign in again + Return to home +
+
+
+ + {{ template "footer" . }} + + +{{ end }} + +{{ define "oauth-missing-scopes" }} + + + + {{ template "head" . }} + {{ template "meta" .Meta }} + + + {{ template "nav-simple" . }} + +
+
+

Some permissions weren't granted

+ + + +

Missing

+
    + {{ range .Missing }} +
  • {{ . }}
    • + {{ end }} +
+ +

+ If your PDS let you untick permissions, try again and approve all of them. + If a permission wasn't shown to you at all, your PDS may not support it yet. + Updating the PDS usually fixes this. +

+ +
+ Try again + Return to home +
+
+
+ + {{ template "footer" . }} + + +{{ end }} + +{{ define "oauth-authorized" }} + + + + {{ template "head" . }} + {{ template "meta" .Meta }} + + + + {{ template "nav-simple" . }} + +
+
+

Authorization successful

+ +
+ {{ icon "check-circle" "size-5" }} + You have authorized {{ .ClientShortName }} to access your account: {{ .Handle }} +
+ +

+ Redirecting to the settings page to generate your API key. If nothing happens, + go to settings. +

+ +

Next steps

+
    +
  1. Generate an API key on the settings page
    2. +
  2. Copy the API key (it's shown once)
    3. +
  3. Use it with: docker login {{ .RegistryURL }} -u {{ .Handle }} -p [your-api-key]
    4. +
+
+
+ + {{ template "footer" . }} + + +{{ end }} diff --git a/pkg/auth/oauth/client.go b/pkg/auth/oauth/client.go index a4e01d0..a9a92f3 100644 --- a/pkg/auth/oauth/client.go +++ b/pkg/auth/oauth/client.go @@ -169,38 +169,6 @@ func GetDefaultScopes(did string) []string { } } -// ScopesMatch checks if two scope lists are equivalent (order-independent) -// Returns true if both lists contain the same scopes, regardless of order. -// Expands any "include:" prefixed scopes in the desired list before comparing, -// since the PDS returns expanded scopes in the stored session. -func ScopesMatch(stored, desired []string) bool { - // Expand any include: scopes in desired before comparing - expandedDesired := ExpandIncludeScopes(desired) - - // Handle nil/empty cases - if len(stored) == 0 && len(expandedDesired) == 0 { - return true - } - if len(stored) != len(expandedDesired) { - return false - } - - // Build map of desired scopes for O(1) lookup - desiredMap := make(map[string]bool, len(expandedDesired)) - for _, scope := range expandedDesired { - desiredMap[scope] = true - } - - // Check if all stored scopes exist in desired - for _, scope := range stored { - if !desiredMap[scope] { - return false - } - } - - return true -} - // isLocalhost checks if a base URL is a localhost address func isLocalhost(baseURL string) bool { return strings.Contains(baseURL, "127.0.0.1") || strings.Contains(baseURL, "localhost") @@ -483,8 +451,12 @@ func IsSessionInvalidError(err error) bool { // have fixed. A genuinely dead session still gets caught by the 401 // status check above. isAuthError (below) omits it for the same reason; // the two classifiers must agree about the same condition. + // Also absent: InsufficientScope. It means the session is alive but + // wasn't granted this permission, and signing in again at the same + // PDS gets the same grant, so deleting it only loops the user through + // login. The caller should surface it as a 403 instead. switch apiErr.Name { - case "InvalidToken", "InvalidGrant", "InsufficientScope": + case "InvalidToken", "InvalidGrant": return true } } @@ -501,7 +473,6 @@ func IsSessionInvalidError(err error) bool { errStr := strings.ToLower(err.Error()) return strings.Contains(errStr, "invalid_grant") || strings.Contains(errStr, "invalid_token") || - strings.Contains(errStr, "insufficient_scope") || strings.Contains(errStr, "token expired") } @@ -529,7 +500,8 @@ func isAuthError(err error) bool { if apiErr.StatusCode == 401 { return true } - if apiErr.Name == "InvalidToken" || apiErr.Name == "InsufficientScope" { + // Not InsufficientScope: see IsSessionInvalidError. + if apiErr.Name == "InvalidToken" { return true } } @@ -538,7 +510,6 @@ func isAuthError(err error) bool { // appear in digests or URIs errStr := strings.ToLower(err.Error()) return strings.Contains(errStr, "invalid_token") || - strings.Contains(errStr, "insufficient_scope") || strings.Contains(errStr, "token expired") } @@ -566,15 +537,15 @@ func (r *Refresher) resumeSession(ctx context.Context, did string) (*oauth.Clien return nil, fmt.Errorf("no session found for DID: %s", did) } - // Log scope differences for debugging, but don't delete session - // The PDS will reject requests if scopes are insufficient - // (Permission-sets get expanded by PDS, so exact matching doesn't work) - desiredScopes := r.clientApp.Config.Scopes - if !ScopesMatch(sessionData.Scopes, desiredScopes) { - slog.Debug("Session scopes differ from desired (may be permission-set expansion)", + // A session missing a scope is refused at login and evicted on boot, so + // one reaching here predates that check or the desired set changed since + // the last boot. Log it but carry on: the PDS rejects whatever the session + // can't do, and that surfaces as a 403 rather than a sign-out. + if missing := MissingScopes(sessionData.Scopes, r.clientApp.Config.Scopes); len(missing) > 0 { + slog.Warn("Session is missing scopes", + "component", "oauth/refresher", "did", did, - "storedScopes", sessionData.Scopes, - "desiredScopes", desiredScopes) + "missing", missing) } // Resume session diff --git a/pkg/auth/oauth/client_test.go b/pkg/auth/oauth/client_test.go index 84f50f7..1c597cb 100644 --- a/pkg/auth/oauth/client_test.go +++ b/pkg/auth/oauth/client_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "slices" + "strings" "testing" "github.com/bluesky-social/indigo/atproto/atclient" @@ -55,68 +57,146 @@ func TestNewClientAppWithCustomScopes(t *testing.T) { } } -func TestScopesMatch(t *testing.T) { +func TestMissingScopes(t *testing.T) { + fullApp := "repo?collection=io.atcr.manifest&collection=io.atcr.repo.page&collection=io.atcr.sailor.profile&collection=io.atcr.sailor.star&collection=io.atcr.tag" + tests := []struct { - name string - stored []string - desired []string - expected bool + name string + granted []string + desired []string + missing []string }{ { - name: "exact match", - stored: []string{"atproto", "blob:image/png"}, - desired: []string{"atproto", "blob:image/png"}, - expected: true, + name: "exact match", + granted: []string{"atproto", "blob:image/png"}, + desired: []string{"atproto", "blob:image/png"}, }, { - name: "different order", - stored: []string{"blob:image/png", "atproto"}, - desired: []string{"atproto", "blob:image/png"}, - expected: true, + name: "different order", + granted: []string{"blob:image/png", "atproto"}, + desired: []string{"atproto", "blob:image/png"}, }, { - name: "missing scope in stored", - stored: []string{"atproto"}, - desired: []string{"atproto", "blob:image/png"}, - expected: false, + name: "both empty", + granted: nil, + desired: []string{}, }, { - name: "extra scope in stored", - stored: []string{"atproto", "blob:image/png", "extra"}, - desired: []string{"atproto", "blob:image/png"}, - expected: false, + name: "extra grants are fine", + granted: []string{"atproto", "blob:image/png", "repo:app.bsky.feed.post", "extra"}, + desired: []string{"atproto", "blob:image/png"}, }, { - name: "both empty", - stored: []string{}, - desired: []string{}, - expected: true, + name: "missing atproto", + granted: []string{"blob:image/png"}, + desired: []string{"atproto", "blob:image/png"}, + missing: []string{"atproto"}, }, { - name: "nil vs empty", - stored: nil, - desired: []string{}, - expected: true, + name: "missing blob", + granted: []string{"atproto"}, + desired: []string{"atproto", "blob:image/png"}, + missing: []string{"blob:image/png"}, }, { - name: "completely different", - stored: []string{"foo", "bar"}, - desired: []string{"baz", "qux"}, - expected: false, + name: "blob wildcard covers a type", + granted: []string{"atproto", "blob:image/*"}, + desired: []string{"atproto", "blob:image/png", "blob:image/*"}, + }, + { + name: "blob */* covers anything", + granted: []string{"atproto", "blob:*/*"}, + desired: []string{"atproto", "blob:application/vnd.oci.image.manifest.v1+json"}, + }, + { + name: "blob wildcard of another type does not cover", + granted: []string{"atproto", "blob:image/*"}, + desired: []string{"atproto", "blob:application/json"}, + missing: []string{"blob:application/json"}, + }, + { + name: "include expanded by the PDS", + granted: []string{"atproto", fullApp}, + desired: []string{"atproto", "include:io.atcr.authFullApp"}, + }, + { + name: "include echoed back unexpanded", + granted: []string{"atproto", "include:io.atcr.authFullApp"}, + desired: []string{"atproto", "include:io.atcr.authFullApp"}, + }, + { + name: "include expanded into one scope per collection, any order", + granted: []string{"atproto", + "repo:io.atcr.tag", "repo:io.atcr.sailor.star", "repo:io.atcr.manifest", + "repo:io.atcr.sailor.profile", "repo:io.atcr.repo.page"}, + desired: []string{"atproto", "include:io.atcr.authFullApp"}, + }, + { + // The tranquil.farm case: the consent screen dropped the manifest + // collection, so the push failed at putRecord. + name: "collection missing from the permission set", + granted: []string{"atproto", + "repo?collection=io.atcr.repo.page&collection=io.atcr.sailor.profile&collection=io.atcr.sailor.star&collection=io.atcr.tag"}, + desired: []string{"atproto", "include:io.atcr.authFullApp"}, + missing: []string{"repo:io.atcr.manifest?action=create&action=update&action=delete"}, + }, + { + name: "action subset granted", + granted: []string{"atproto", "repo:io.atcr.manifest?action=create"}, + desired: []string{"atproto", "repo:io.atcr.manifest"}, + missing: []string{"repo:io.atcr.manifest?action=update&action=delete"}, + }, + { + name: "wildcard collection covers", + granted: []string{"atproto", "repo:*"}, + desired: []string{"atproto", "include:io.atcr.authFullApp"}, + }, + { + name: "transition:generic covers repo, blob and rpc", + granted: []string{"atproto", "transition:generic"}, + desired: []string{"atproto", "include:io.atcr.authFullApp", "blob:image/*", "rpc:com.atproto.repo.getRecord?aud=*"}, + }, + { + name: "rpc match", + granted: []string{"atproto", "rpc:com.atproto.repo.getRecord?aud=*"}, + desired: []string{"atproto", "rpc:com.atproto.repo.getRecord?aud=*"}, + }, + { + name: "rpc with narrower aud does not cover aud=*", + granted: []string{"atproto", "rpc:com.atproto.repo.getRecord?aud=did:web:hold.example.com%23atcr_hold"}, + desired: []string{"atproto", "rpc:com.atproto.repo.getRecord?aud=*"}, + missing: []string{"rpc:com.atproto.repo.getRecord?aud=*"}, + }, + { + name: "default scopes fully granted", + granted: append([]string{fullApp}, withoutInclude(GetDefaultScopes("*"))...), + desired: GetDefaultScopes("*"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ScopesMatch(tt.stored, tt.desired) - if result != tt.expected { - t.Errorf("ScopesMatch(%v, %v) = %v, want %v", - tt.stored, tt.desired, result, tt.expected) + got := MissingScopes(tt.granted, tt.desired) + if !slices.Equal(got, tt.missing) { + t.Errorf("MissingScopes(%v, %v) = %v, want %v", tt.granted, tt.desired, got, tt.missing) + } + if ScopesCover(tt.granted, tt.desired) != (len(tt.missing) == 0) { + t.Errorf("ScopesCover disagrees with MissingScopes") } }) } } +func withoutInclude(scopes []string) []string { + var out []string + for _, s := range scopes { + if !strings.HasPrefix(s, "include:") { + out = append(out, s) + } + } + return out +} + // ---------------------------------------------------------------------------- // Session Management (Refresher) Tests // ---------------------------------------------------------------------------- @@ -198,6 +278,11 @@ func TestIsSessionInvalidError(t *testing.T) { {"plain invalid_token string", errors.New("auth server request failed (HTTP 401): invalid_token"), true}, {"connection refused", errors.New(`Post "https://pds.example.com/oauth/token": dial tcp: connection refused`), false}, {"generic 500", errors.New("token refresh failed (HTTP 500): server exploded"), false}, + // A live session the PDS didn't grant this permission to. Deleting it + // signed the user out mid-push, and logging in again at the same PDS + // got the same grant, so they looped (tranquil.farm, 2026-09-21). + {"api error 403 InsufficientScope is alive", &atclient.APIError{StatusCode: 403, Name: "InsufficientScope", Message: "Insufficient scope to create records in io.atcr.manifest"}, false}, + {"wrapped InsufficientScope string", errors.New("putRecord failed: API request failed (HTTP 403): InsufficientScope: insufficient_scope"), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -207,3 +292,31 @@ func TestIsSessionInvalidError(t *testing.T) { }) } } + +// TestIsAuthError covers the gate DoWithSession uses to delete a session after +// a failed PDS call. It must agree with IsSessionInvalidError. +func TestIsAuthError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"canceled", fmt.Errorf("putRecord failed: %w", context.Canceled), false}, + {"api error 401", &atclient.APIError{StatusCode: 401}, true}, + {"api error InvalidToken", &atclient.APIError{StatusCode: 400, Name: "InvalidToken"}, true}, + {"api error 403 InsufficientScope", fmt.Errorf("putRecord failed: %w", &atclient.APIError{StatusCode: 403, Name: "InsufficientScope", Message: "Insufficient scope to create records in io.atcr.manifest"}), false}, + {"insufficient_scope string", errors.New("request failed: insufficient_scope"), false}, + {"plain invalid_token string", errors.New("auth server request failed (HTTP 401): invalid_token"), true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isAuthError(tt.err); got != tt.want { + t.Errorf("isAuthError(%v) = %v, want %v", tt.err, got, tt.want) + } + if tt.err != nil && isAuthError(tt.err) != IsSessionInvalidError(tt.err) { + t.Errorf("isAuthError and IsSessionInvalidError disagree on %v", tt.err) + } + }) + } +} diff --git a/pkg/auth/oauth/scopes.go b/pkg/auth/oauth/scopes.go new file mode 100644 index 0000000..23df09f --- /dev/null +++ b/pkg/auth/oauth/scopes.go @@ -0,0 +1,189 @@ +package oauth + +import ( + "slices" + "sort" + "strings" + + atauth "github.com/bluesky-social/indigo/atproto/auth" +) + +// repoActions are the actions a repo permission grants when it names none. +var repoActions = []string{"create", "update", "delete"} + +// MissingScopes reports which of the desired scopes the granted set does not +// cover. It is an "at least" check: extra grants are ignored, and a grant +// counts if it is broader than what was asked for (a wildcard collection, a +// `type/*` blob accept, an rpc aud of `*`, or the legacy transition:generic). +// +// Scopes are compared by what they grant, not by how they are spelled. A PDS +// may expand a permission-set into one repo scope or several, list collections +// in any order, or echo the include: back unexpanded, and none of those should +// count as a missing permission. Both sides go through ExpandIncludeScopes +// first; a desired scope that still doesn't parse is matched by exact string. +// +// Each missing entry is rendered as a scope string narrowed to what is +// actually absent, e.g. "repo:io.atcr.manifest?action=create", so it can be +// shown to the user as-is. +func MissingScopes(granted, desired []string) []string { + g := parseGranted(ExpandIncludeScopes(granted)) + + var missing []string + missingRepo := map[string][]string{} // collection -> actions + + for _, scope := range ExpandIncludeScopes(desired) { + if scope == "" { + continue + } + if scope == "atproto" { + if !g.raw["atproto"] { + missing = append(missing, scope) + } + continue + } + perm, err := atauth.ParsePermissionString(scope) + if err != nil { + if !g.raw[scope] { + missing = append(missing, scope) + } + continue + } + + switch perm.Resource { + case "repo": + actions := perm.Action + if len(actions) == 0 { + actions = repoActions + } + for _, coll := range perm.Collection { + for _, act := range actions { + if !g.coversRepo(coll, act) { + missingRepo[coll] = append(missingRepo[coll], act) + } + } + } + case "blob": + for _, accept := range perm.Accept { + if !g.coversBlob(accept) { + missing = append(missing, "blob:"+accept) + } + } + case "rpc": + for _, lxm := range perm.Endpoint { + if !g.coversRPC(lxm, perm.Audience) { + missing = append(missing, "rpc:"+lxm+"?aud="+perm.Audience) + } + } + default: + // include: that we couldn't expand, account:, identity: — nothing + // finer-grained to compare, so it has to be granted verbatim. + if !g.raw[scope] { + missing = append(missing, scope) + } + } + } + + colls := make([]string, 0, len(missingRepo)) + for coll := range missingRepo { + colls = append(colls, coll) + } + sort.Strings(colls) + for _, coll := range colls { + missing = append(missing, "repo:"+coll+"?action="+strings.Join(missingRepo[coll], "&action=")) + } + + return missing +} + +// ScopesCover reports whether granted covers every desired scope. See +// MissingScopes. +func ScopesCover(granted, desired []string) bool { + return len(MissingScopes(granted, desired)) == 0 +} + +type grantedScopes struct { + raw map[string]bool + generic bool // transition:generic grants every repo, blob, and rpc permission + perms []atauth.Permission +} + +func parseGranted(scopes []string) grantedScopes { + g := grantedScopes{raw: make(map[string]bool, len(scopes))} + for _, s := range scopes { + g.raw[s] = true + if s == "transition:generic" { + g.generic = true + continue + } + if p, err := atauth.ParsePermissionString(s); err == nil { + g.perms = append(g.perms, *p) + } + } + return g +} + +func (g grantedScopes) coversRepo(coll, action string) bool { + if g.generic { + return true + } + for _, p := range g.perms { + if p.Resource != "repo" { + continue + } + if !slices.Contains(p.Collection, coll) && !slices.Contains(p.Collection, "*") { + continue + } + if len(p.Action) == 0 || slices.Contains(p.Action, action) { + return true + } + } + return false +} + +func (g grantedScopes) coversBlob(accept string) bool { + if g.generic { + return true + } + for _, p := range g.perms { + if p.Resource != "blob" { + continue + } + for _, a := range p.Accept { + if mimeCovers(a, accept) { + return true + } + } + } + return false +} + +func (g grantedScopes) coversRPC(lxm, aud string) bool { + if g.generic { + return true + } + for _, p := range g.perms { + if p.Resource != "rpc" { + continue + } + if p.Audience != aud && p.Audience != "*" { + continue + } + if slices.Contains(p.Endpoint, lxm) || slices.Contains(p.Endpoint, "*") { + return true + } + } + return false +} + +// mimeCovers reports whether a granted blob accept pattern covers a wanted +// one: an exact match, "*/*", or a "type/*" wildcard over the same type +// (which also covers a wanted "type/*"). +func mimeCovers(grant, want string) bool { + if grant == want || grant == "*/*" { + return true + } + if prefix, ok := strings.CutSuffix(grant, "*"); ok && strings.HasSuffix(prefix, "/") { + return strings.HasPrefix(want, prefix) + } + return false +} diff --git a/pkg/auth/oauth/server.go b/pkg/auth/oauth/server.go index 19e252c..2b01e90 100644 --- a/pkg/auth/oauth/server.go +++ b/pkg/auth/oauth/server.go @@ -1,12 +1,14 @@ package oauth import ( + "bytes" "context" "errors" "fmt" "html/template" "log/slog" "net/http" + "net/url" "strings" "time" @@ -78,12 +80,29 @@ type UserStore interface { // without coupling the OAuth package to AppView-specific dependencies. type PostAuthCallback func(ctx context.Context, did, handle, pdsEndpoint, sessionID string) error +// PageRenderer renders the HTML pages the OAuth endpoints show to a person in +// a browser, so an embedding application (the appview) can draw them in its +// own site layout. Each method returns the complete page body; Server owns the +// status code and headers. When a method returns an error, or no renderer is +// set, Server falls back to the plain inline templates in this file. +type PageRenderer interface { + // RenderOAuthError renders a failed authorization. message is plain text. + RenderOAuthError(r *http.Request, message string) ([]byte, error) + // RenderMissingScopes renders the refusal shown when the PDS granted + // fewer scopes than requested. retryURL restarts the flow. + RenderMissingScopes(r *http.Request, missing []string, retryURL string) ([]byte, error) + // RenderAuthorized renders the success page for the non-UI flow, which + // forwards to /settings. + RenderAuthorized(r *http.Request, handle string) ([]byte, error) +} + // Server handles OAuth authorization for the AppView type Server struct { clientApp *oauth.ClientApp refresher *Refresher uiSessionStore UISessionStore postAuthCallback PostAuthCallback + pages PageRenderer } // NewServer creates a new OAuth server @@ -109,6 +128,12 @@ func (s *Server) SetPostAuthCallback(callback PostAuthCallback) { s.postAuthCallback = callback } +// SetPageRenderer sets the renderer for the browser-facing OAuth pages. Pass +// nil to use the built-in inline templates. +func (s *Server) SetPageRenderer(pages PageRenderer) { + s.pages = pages +} + // ServeAuthorize handles GET /auth/oauth/authorize func (s *Server) ServeAuthorize(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { @@ -133,7 +158,7 @@ func (s *Server) ServeAuthorize(w http.ResponseWriter, r *http.Request) { // Check if error is about invalid_client_metadata (usually means PDS doesn't support required scopes) errMsg := err.Error() if strings.Contains(errMsg, "invalid_client_metadata") { - s.renderError(w, "OAuth authorization failed: Your PDS does not support one or more required OAuth scopes (likely the 'blob:' scope). Please update your PDS to the latest version and try again.") + s.renderError(w, r, "OAuth authorization failed: Your PDS does not support one or more required OAuth scopes (likely the 'blob:' scope). Please update your PDS to the latest version and try again.") return } @@ -158,7 +183,7 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { // Check for OAuth error if errorParam := r.URL.Query().Get("error"); errorParam != "" { errorDesc := r.URL.Query().Get("error_description") - s.renderError(w, fmt.Sprintf("OAuth error: %s - %s", errorParam, errorDesc)) + s.renderError(w, r, fmt.Sprintf("OAuth error: %s - %s", errorParam, errorDesc)) return } @@ -185,7 +210,7 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { "queryParams", r.URL.Query().Encode()) } - s.renderError(w, fmt.Sprintf("Failed to process OAuth callback: %v", err)) + s.renderError(w, r, fmt.Sprintf("Failed to process OAuth callback: %v", err)) return } @@ -194,6 +219,26 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { slog.Debug("OAuth callback successful", "did", did, "sessionID", sessionID) + // Refuse a partial grant. Some PDSes let the user untick permissions, or + // leave one off the consent screen, and a session missing one works until + // the first push that needs it. Signing in again only helps once the PDS + // offers the full set, so say what's missing now. This runs before the + // old-session cleanup below so a refused login leaves any working session + // the user already had untouched. + if missing := MissingScopes(sessionData.Scopes, s.clientApp.Config.Scopes); len(missing) > 0 { + slog.Warn("Refusing OAuth login: PDS did not grant all requested scopes", + "component", "oauth/server", + "did", did, + "host", sessionData.HostURL, + "missing", missing, + "granted", sessionData.Scopes) + if err := s.clientApp.Store.DeleteSession(r.Context(), sessionData.AccountDID, sessionID); err != nil { + slog.Warn("Failed to delete refused OAuth session", "did", did, "error", err) + } + s.renderMissingScopes(w, r, missing) + return + } + // Clean up old OAuth sessions for this DID BEFORE invalidating cache // This prevents accumulation of stale sessions with expired refresh tokens // Order matters: delete from DB first, then invalidate cache, so when cache reloads @@ -240,7 +285,7 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { }) if err != nil { slog.Error("Failed to create UI session", "error", err, "did", did) - s.renderError(w, "Something went wrong while logging you in. Please try again.") + s.renderError(w, r, "Something went wrong while logging you in. Please try again.") return } // Set UI session cookie and redirect (code below) @@ -264,7 +309,7 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { }) if err != nil { slog.Error("Failed to create UI session", "error", err, "did", did) - s.renderError(w, "Something went wrong while logging you in. Please try again.") + s.renderError(w, r, "Something went wrong while logging you in. Please try again.") return } // Set UI session cookie @@ -311,38 +356,85 @@ func (s *Server) ServeCallback(w http.ResponseWriter, r *http.Request) { } // Non-UI flow: redirect to settings to get API key - s.renderRedirectToSettings(w, handle) + s.renderRedirectToSettings(w, r, handle) } -// renderRedirectToSettings redirects to the settings page to generate an API key -func (s *Server) renderRedirectToSettings(w http.ResponseWriter, handle string) { - tmpl := template.Must(template.New("redirect").Parse(redirectToSettingsTemplate)) +// writePage writes an HTML page with the given status. It prefers the +// injected renderer and falls back to the inline template when there is none +// or it fails. +func (s *Server) writePage(w http.ResponseWriter, status int, render func(PageRenderer) ([]byte, error), fallbackName, fallback string, data any) { + var body []byte + if s.pages != nil { + b, err := render(s.pages) + if err == nil { + body = b + } else { + slog.Warn("OAuth page renderer failed, using fallback template", + "component", "oauth/server", "page", fallbackName, "error", err) + } + } + if body == nil { + var buf bytes.Buffer + tmpl := template.Must(template.New(fallbackName).Parse(fallback)) + if err := tmpl.Execute(&buf, data); err != nil { + http.Error(w, "failed to render template", http.StatusInternalServerError) + return + } + body = buf.Bytes() + } + + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(status) + _, _ = w.Write(body) +} + +// renderRedirectToSettings renders the success page for the non-UI flow, +// which forwards to the settings page. +func (s *Server) renderRedirectToSettings(w http.ResponseWriter, r *http.Request, handle string) { data := struct { Handle string }{ Handle: handle, } - - w.Header().Set("Content-Type", "text/html; charset=utf-8") - if err := tmpl.Execute(w, data); err != nil { - http.Error(w, "failed to render template", http.StatusInternalServerError) - } + s.writePage(w, http.StatusOK, func(p PageRenderer) ([]byte, error) { + return p.RenderAuthorized(r, handle) + }, "redirect", redirectToSettingsTemplate, data) } // renderError renders an error page -func (s *Server) renderError(w http.ResponseWriter, message string) { - tmpl := template.Must(template.New("error").Parse(errorTemplate)) +func (s *Server) renderError(w http.ResponseWriter, r *http.Request, message string) { data := struct { Message string }{ Message: message, } + s.writePage(w, http.StatusBadRequest, func(p PageRenderer) ([]byte, error) { + return p.RenderOAuthError(r, message) + }, "error", errorTemplate, data) +} - w.Header().Set("Content-Type", "text/html; charset=utf-8") - w.WriteHeader(http.StatusBadRequest) - if err := tmpl.Execute(w, data); err != nil { - http.Error(w, "failed to render template", http.StatusInternalServerError) +// renderMissingScopes renders the page shown when the PDS granted fewer +// permissions than ATCR asked for. +// +// "Try again" goes back through the login page, not straight to authorize: the +// login form is what sets the oauth_return_to cookie that gets a UI session +// created, and it carries any return_to in flight (the device page, during a +// credential-helper login) through to the retry. +func (s *Server) renderMissingScopes(w http.ResponseWriter, r *http.Request, missing []string) { + retryURL := "/auth/oauth/login" + if c, err := r.Cookie("oauth_return_to"); err == nil && c.Value != "" { + retryURL += "?return_to=" + url.QueryEscape(c.Value) } + data := struct { + Missing []string + RetryURL string + }{ + Missing: missing, + RetryURL: retryURL, + } + s.writePage(w, http.StatusForbidden, func(p PageRenderer) ([]byte, error) { + return p.RenderMissingScopes(r, data.Missing, data.RetryURL) + }, "missing-scopes", missingScopesTemplate, data) } // HTML templates @@ -399,3 +491,29 @@ const errorTemplate = ` ` + +const missingScopesTemplate = ` + + + + Missing Permissions - ATCR + + + +
+

Some permissions weren't granted

+

Your PDS signed you in, but didn't grant everything ATCR needs to push and manage images. You haven't been logged in.

+

Missing:

+ +

If your PDS let you untick permissions, try again and approve all of them. If a permission wasn't shown to you at all, your PDS may not support it yet. Updating the PDS usually fixes this.

+

Try again · Return to home

+
+ + +` diff --git a/pkg/auth/oauth/server_pages_test.go b/pkg/auth/oauth/server_pages_test.go new file mode 100644 index 0000000..7d87231 --- /dev/null +++ b/pkg/auth/oauth/server_pages_test.go @@ -0,0 +1,150 @@ +package oauth + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/bluesky-social/indigo/atproto/auth/oauth" +) + +// fakePages records calls and returns a marker body, or fails when err is set. +type fakePages struct { + err error + calls []string + missing []string + retryURL string + message string + handle string +} + +func (f *fakePages) RenderOAuthError(_ *http.Request, message string) ([]byte, error) { + f.calls = append(f.calls, "error") + f.message = message + if f.err != nil { + return nil, f.err + } + return []byte("

styled error: " + message + "

"), nil +} + +func (f *fakePages) RenderMissingScopes(_ *http.Request, missing []string, retryURL string) ([]byte, error) { + f.calls = append(f.calls, "missing") + f.missing = missing + f.retryURL = retryURL + if f.err != nil { + return nil, f.err + } + return []byte("

styled missing: " + strings.Join(missing, ",") + "

"), nil +} + +func (f *fakePages) RenderAuthorized(_ *http.Request, handle string) ([]byte, error) { + f.calls = append(f.calls, "authorized") + f.handle = handle + if f.err != nil { + return nil, f.err + } + return []byte("

styled authorized: " + handle + "

"), nil +} + +func newPagesTestServer(t *testing.T) *Server { + t.Helper() + clientApp, err := NewClientApp("http://localhost:5000", oauth.NewMemStore(), GetDefaultScopes("*"), "", "AT Container Registry") + if err != nil { + t.Fatalf("NewClientApp() error = %v", err) + } + return NewServer(clientApp) +} + +func pagesTestRequest() *http.Request { + return httptest.NewRequest(http.MethodGet, "/auth/oauth/callback", nil) +} + +func TestServer_PageRenderer_Used(t *testing.T) { + server := newPagesTestServer(t) + pages := &fakePages{} + server.SetPageRenderer(pages) + + tests := []struct { + name string + render func(w http.ResponseWriter) + status int + want string + }{ + {"error", func(w http.ResponseWriter) { server.renderError(w, pagesTestRequest(), "boom") }, http.StatusBadRequest, "styled error: boom"}, + {"missing", func(w http.ResponseWriter) { + req := pagesTestRequest() + req.AddCookie(&http.Cookie{Name: "oauth_return_to", Value: "/device?user_code=ABCD-EFGH"}) + server.renderMissingScopes(w, req, []string{"blob:*/*"}) + }, http.StatusForbidden, "styled missing: blob:*/*"}, + {"authorized", func(w http.ResponseWriter) { server.renderRedirectToSettings(w, pagesTestRequest(), "alice.test") }, http.StatusOK, "styled authorized: alice.test"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := httptest.NewRecorder() + tt.render(w) + if w.Code != tt.status { + t.Errorf("status = %d, want %d", w.Code, tt.status) + } + if ct := w.Header().Get("Content-Type"); ct != "text/html; charset=utf-8" { + t.Errorf("Content-Type = %q", ct) + } + if body := w.Body.String(); body != "

"+tt.want+"

" { + t.Errorf("body = %q, want the renderer's output", body) + } + }) + } + + if got := strings.Join(pages.calls, ","); got != "error,missing,authorized" { + t.Errorf("renderer calls = %q", got) + } + // Retry goes back through the login page and keeps the in-flight return_to. + if pages.retryURL != "/auth/oauth/login?return_to=%2Fdevice%3Fuser_code%3DABCD-EFGH" { + t.Errorf("retryURL = %q", pages.retryURL) + } +} + +func TestServer_PageRenderer_FallbackOnError(t *testing.T) { + server := newPagesTestServer(t) + pages := &fakePages{err: errors.New("template exploded")} + server.SetPageRenderer(pages) + + w := httptest.NewRecorder() + server.renderError(w, pagesTestRequest(), "boom") + + if len(pages.calls) != 1 { + t.Fatalf("renderer should have been tried once, calls = %v", pages.calls) + } + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", w.Code) + } + body := w.Body.String() + if !strings.Contains(body, "Authorization Failed") || !strings.Contains(body, "boom") { + t.Errorf("expected inline fallback page, got: %s", body) + } +} + +func TestServer_RenderMissingScopes_Fallback(t *testing.T) { + server := newPagesTestServer(t) // no renderer set + + w := httptest.NewRecorder() + missing := []string{"repo:io.atcr.manifest", "blob:*/*"} + server.renderMissingScopes(w, pagesTestRequest(), missing) + + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403", w.Code) + } + body := w.Body.String() + for _, s := range missing { + if !strings.Contains(body, ""+s+"") { + t.Errorf("missing scope %q not listed in body: %s", s, body) + } + } + if !strings.Contains(body, `href="/auth/oauth/login"`) { + t.Errorf("expected Try again link to the login page, got: %s", body) + } + if !strings.Contains(body, "Try again") || !strings.Contains(body, "Return to home") { + t.Errorf("expected Try again and Return to home links, got: %s", body) + } +} diff --git a/pkg/auth/oauth/server_test.go b/pkg/auth/oauth/server_test.go index f7a0f0d..6890ad6 100644 --- a/pkg/auth/oauth/server_test.go +++ b/pkg/auth/oauth/server_test.go @@ -288,7 +288,7 @@ func TestServer_RenderError(t *testing.T) { server := NewServer(clientApp) w := httptest.NewRecorder() - server.renderError(w, "Test error message") + server.renderError(w, httptest.NewRequest(http.MethodGet, "/auth/oauth/callback", nil), "Test error message") resp := w.Result() if resp.StatusCode != http.StatusBadRequest { @@ -317,7 +317,7 @@ func TestServer_RenderRedirectToSettings(t *testing.T) { server := NewServer(clientApp) w := httptest.NewRecorder() - server.renderRedirectToSettings(w, "alice.bsky.social") + server.renderRedirectToSettings(w, httptest.NewRequest(http.MethodGet, "/auth/oauth/callback", nil), "alice.bsky.social") resp := w.Result() if resp.StatusCode != http.StatusOK {