diff --git a/api/content_oauth_admin_handlers.go b/api/content_oauth_admin_handlers.go index 56a18aac..14d95414 100644 --- a/api/content_oauth_admin_handlers.go +++ b/api/content_oauth_admin_handlers.go @@ -67,7 +67,7 @@ func (h *ContentOAuthHandlers) AdminList(c *gin.Context) { toks, err := h.Tokens.ListByUser(c.Request.Context(), userID) if err != nil { slogging.Get().WithContext(c).Error("admin list content tokens user=%s: %v", userID, err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "content tokens not found", "failed to list content tokens")) return } @@ -94,7 +94,7 @@ func (h *ContentOAuthHandlers) AdminDelete(c *gin.Context) { return } slogging.Get().WithContext(c).Error("admin delete content token user=%s provider=%s: %v", userID, providerID, err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "content token not found", "failed to delete content token")) return } diff --git a/api/content_oauth_handlers.go b/api/content_oauth_handlers.go index 666f80dc..c7d2d899 100644 --- a/api/content_oauth_handlers.go +++ b/api/content_oauth_handlers.go @@ -91,7 +91,7 @@ func (h *ContentOAuthHandlers) List(c *gin.Context) { toks, err := h.Tokens.ListByUser(c.Request.Context(), userID) if err != nil { slogging.Get().WithContext(c).Error("list content tokens: %v", err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "content tokens not found", "failed to list content tokens")) return } out := make([]contentTokenInfo, 0, len(toks)) @@ -104,7 +104,7 @@ func (h *ContentOAuthHandlers) List(c *gin.Context) { // Authorize handles POST /me/content_tokens/:provider_id/authorize. // Validates the provider and client_callback, generates PKCE, stores state in Redis, // and returns {authorization_url, expires_at}. -// SEM@462c9c5f31b3e00796bfbcea980a74ada200f7f7: initiate a PKCE OAuth flow for a content provider and return the authorization URL (mutates shared state) +// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: initiate a PKCE OAuth flow for a content provider and return the authorization URL; 503 when state store is down (mutates shared state) func (h *ContentOAuthHandlers) Authorize(c *gin.Context) { userID, ok := h.UserLookup(c) if !ok { @@ -139,7 +139,7 @@ func (h *ContentOAuthHandlers) Authorize(c *gin.Context) { verifier, err := NewPKCEVerifier() if err != nil { slogging.Get().WithContext(c).Error("generate PKCE verifier: %v", err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, ServerError("failed to start content provider authorization")) return } @@ -154,7 +154,8 @@ func (h *ContentOAuthHandlers) Authorize(c *gin.Context) { nonce, err := h.StateStore.Put(c.Request.Context(), payload, ttl) if err != nil { slogging.Get().WithContext(c).Error("put content oauth state: %v", err) - c.AbortWithStatus(http.StatusInternalServerError) + // A Redis outage is ErrContentOAuthStateUnavailable (ErrTransient) -> 503 (#1119). + HandleRequestError(c, StoreErrorToRequestError(err, "authorization state not found", "failed to start content provider authorization")) return } @@ -198,7 +199,7 @@ func (h *ContentOAuthHandlers) Delete(c *gin.Context) { return } slogging.Get().WithContext(c).Error("delete content token: %v", err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "content token not found", "failed to delete content token")) return } @@ -224,6 +225,12 @@ func (h *ContentOAuthHandlers) Callback(c *gin.Context) { } state, err := h.StateStore.Consume(ctx, nonce) + if errors.Is(err, ErrContentOAuthStateUnavailable) { + // The state store is down, so the nonce is unverifiable rather than invalid (#1119). + logger.Error("content oauth callback: state store unavailable: %v", err) + HandleRequestError(c, ServiceUnavailableError("Authorization state store temporarily unavailable - please retry")) + return + } if err != nil { logger.Warn("content oauth callback: invalid/expired state: %v", err) renderCallbackError(c, "invalid_state") diff --git a/api/content_oauth_state.go b/api/content_oauth_state.go index 16897166..5cb1082f 100644 --- a/api/content_oauth_state.go +++ b/api/content_oauth_state.go @@ -9,12 +9,18 @@ import ( "fmt" "time" + "github.com/ericfitz/tmi/internal/dberrors" "github.com/redis/go-redis/v9" ) // ErrContentOAuthStateNotFound is returned when the OAuth state nonce is not found or has expired. var ErrContentOAuthStateNotFound = errors.New("content oauth state not found or expired") +// ErrContentOAuthStateUnavailable is returned when the Redis state store cannot be +// reached or rejects the operation. It wraps dberrors.ErrTransient so handlers map +// it to 503 through StoreErrorToRequestError (#1119). +var ErrContentOAuthStateUnavailable = fmt.Errorf("content oauth state store unavailable: %w", dberrors.ErrTransient) + // ContentOAuthStatePayload holds the data associated with a pending OAuth authorization flow. // SEM@0ee4fc6b79c5a6c5a7dade501546a8eec4509aed: data held in a pending content OAuth authorization state nonce (pure) type ContentOAuthStatePayload struct { @@ -52,7 +58,7 @@ func (s *ContentOAuthStateStore) Put(ctx context.Context, p ContentOAuthStatePay return "", err } if err := s.rdb.Set(ctx, s.keyPrefix+nonce, payload, ttl).Err(); err != nil { - return "", fmt.Errorf("put state: %w", err) + return "", fmt.Errorf("put state: %w: %w", ErrContentOAuthStateUnavailable, err) } return nonce, nil } @@ -67,7 +73,7 @@ func (s *ContentOAuthStateStore) Consume(ctx context.Context, nonce string) (*Co return nil, ErrContentOAuthStateNotFound } if err != nil { - return nil, err + return nil, fmt.Errorf("consume state: %w: %w", ErrContentOAuthStateUnavailable, err) } var out ContentOAuthStatePayload if err := json.Unmarshal(val, &out); err != nil { diff --git a/api/content_oauth_unavailable_test.go b/api/content_oauth_unavailable_test.go new file mode 100644 index 00000000..1f5426e7 --- /dev/null +++ b/api/content_oauth_unavailable_test.go @@ -0,0 +1,276 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/ericfitz/tmi/internal/dberrors" + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Backend-unavailable paths for the content OAuth and picker handlers (#1119): +// a Redis or transient DB fault is a 503 with a JSON Error body and Retry-After, +// never a bare 500. + +// errTransientDB mimics a classified transient DB fault from the token repository. +var errTransientDB = fmt.Errorf("content token: %w", dberrors.ErrTransient) + +// assertServiceUnavailable checks for a documented 503: JSON Error body plus Retry-After. +func assertServiceUnavailable(t *testing.T, w *httptest.ResponseRecorder) { + t.Helper() + require.Equal(t, http.StatusServiceUnavailable, w.Code) + assert.NotEmpty(t, w.Header().Get("Retry-After")) + dec := json.NewDecoder(bytes.NewReader(w.Body.Bytes())) + dec.DisallowUnknownFields() + var body Error + require.NoError(t, dec.Decode(&body), "body must match the Error schema: %s", w.Body.String()) + assert.Equal(t, "service_unavailable", string(body.Error)) + assert.NotEmpty(t, body.ErrorDescription) +} + +func TestContentOAuthUnavailable_ContentOAuthStateStore_RedisDown_ReturnsUnavailable(t *testing.T) { + mr, store := newTestStateStoreWithServer(t) + mr.Close() + + _, err := store.Put(context.Background(), ContentOAuthStatePayload{UserID: "u"}, time.Minute) + require.Error(t, err) + assert.ErrorIs(t, err, ErrContentOAuthStateUnavailable) + assert.ErrorIs(t, err, dberrors.ErrTransient) + + _, err = store.Consume(context.Background(), "some-nonce") + require.Error(t, err) + assert.ErrorIs(t, err, ErrContentOAuthStateUnavailable) + assert.NotErrorIs(t, err, ErrContentOAuthStateNotFound) +} + +func TestContentOAuthUnavailable_ContentOAuthStateStore_MissingNonce_IsNotUnavailable(t *testing.T) { + store := newTestStateStore(t) + _, err := store.Consume(context.Background(), "missing") + assert.ErrorIs(t, err, ErrContentOAuthStateNotFound) + assert.NotErrorIs(t, err, ErrContentOAuthStateUnavailable) +} + +func TestContentOAuthUnavailable_ContentOAuthHandlers_Authorize_RedisDown_Returns503(t *testing.T) { + provider := &stubContentOAuthProvider{id: "mock", authURL: "https://auth.example.com/authorize"} + h, mr := newTestHandlers(t, &mockContentTokenRepo{}, provider) + mr.Close() + + r := ginTestRouter() + r.Use(func(c *gin.Context) { setUserContext(c, testUserID); c.Next() }) + r.POST("/me/content_tokens/:provider_id/authorize", h.Authorize) + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/me/content_tokens/mock/authorize", + strings.NewReader(`{"client_callback":"http://localhost:8079/"}`)) + req.Header.Set("Content-Type", "application/json") + r.ServeHTTP(w, req) + + assertServiceUnavailable(t, w) +} + +func TestContentOAuthUnavailable_ContentOAuthHandlers_Callback_RedisDown_Returns503(t *testing.T) { + h, mr := newTestHandlers(t, &mockContentTokenRepo{}, nil) + mr.Close() + + r := ginTestRouter() + r.GET("/oauth2/content_callback", h.Callback) + + params := url.Values{} + params.Set("state", "some-nonce") + params.Set("code", "c") + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, buildCallbackURL(params), nil)) + + assertServiceUnavailable(t, w) +} + +func TestContentOAuthUnavailable_ContentOAuthHandlers_TokenStoreErrors(t *testing.T) { + tests := []struct { + name string + err error + wantStatus int + }{ + {"transient DB fault is 503", errTransientDB, http.StatusServiceUnavailable}, + {"unclassified error is 500 with body", errors.New("boom"), http.StatusInternalServerError}, + } + routes := []struct { + name string + method string + path string + route string + pick func(h *ContentOAuthHandlers) gin.HandlerFunc + }{ + {"List", http.MethodGet, "/me/content_tokens", "/me/content_tokens", + func(h *ContentOAuthHandlers) gin.HandlerFunc { return h.List }}, + {"Delete", http.MethodDelete, "/me/content_tokens/mock", "/me/content_tokens/:provider_id", + func(h *ContentOAuthHandlers) gin.HandlerFunc { return h.Delete }}, + {"AdminList", http.MethodGet, "/admin/users/u2/content_tokens", "/admin/users/:user_id/content_tokens", + func(h *ContentOAuthHandlers) gin.HandlerFunc { return h.AdminList }}, + {"AdminDelete", http.MethodDelete, "/admin/users/u2/content_tokens/mock", "/admin/users/:user_id/content_tokens/:provider_id", + func(h *ContentOAuthHandlers) gin.HandlerFunc { return h.AdminDelete }}, + } + for _, rt := range routes { + for _, tc := range tests { + t.Run(rt.name+"/"+tc.name, func(t *testing.T) { + repo := &mockContentTokenRepo{ + listByUser: func(_ context.Context, _ string) ([]ContentToken, error) { return nil, tc.err }, + deleteByUserAndProv: func(_ context.Context, _, _ string) (*ContentToken, error) { + return nil, tc.err + }, + } + h, _ := newTestHandlers(t, repo, nil) + r := ginTestRouter() + r.Use(func(c *gin.Context) { setUserContext(c, testUserID); c.Next() }) + r.Handle(rt.method, rt.route, rt.pick(h)) + + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(rt.method, rt.path, nil)) + + if tc.wantStatus == http.StatusServiceUnavailable { + assertServiceUnavailable(t, w) + return + } + require.Equal(t, tc.wantStatus, w.Code) + var body Error + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, "server_error", string(body.Error)) + }) + } + } +} + +func TestContentOAuthUnavailable_MintPickerToken_TransientLookupFailure_Returns503(t *testing.T) { + repo := &mockContentTokenRepo{ + getByUserAndProvider: func(_ context.Context, _, _ string) (*ContentToken, error) { + return nil, errTransientDB + }, + } + registry := NewContentOAuthProviderRegistry() + registry.Register(&stubRefreshableProvider{id: pickerProviderID}) + r := pickerTestRouter(newPickerTestHandler(repo, registry, defaultPickerConfigs())) + + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/me/picker_tokens/"+pickerProviderID, nil)) + + assertServiceUnavailable(t, w) +} + +func TestContentOAuthUnavailable_MicrosoftPickerGrantHandler_Handle_TransientLookupFailure_Returns503(t *testing.T) { + repo := &mockContentTokenRepo{ + getByUserAndProvider: func(_ context.Context, _, _ string) (*ContentToken, error) { + return nil, errTransientDB + }, + } + h := NewMicrosoftPickerGrantHandler(repo, NewContentOAuthProviderRegistry(), "app-id", "", + func(_ *gin.Context) (string, bool) { return "u1", true }, permissiveLoopbackValidator()) + + body, _ := json.Marshal(MicrosoftPickerGrantRequest{DriveId: "b!abc", ItemId: "01XYZ"}) + c, rec := newPickerGrantGinContext(t, "POST", "/me/microsoft/picker_grants", bytes.NewReader(body)) + h.Handle(c) + + assertServiceUnavailable(t, rec) +} + +// assertPickerErrorBody decodes body strictly as the spec's Error schema (which +// forbids additional properties) and checks the error and details.code values. +// An empty wantDetail asserts that no details.code is present. +func assertPickerErrorBody(t *testing.T, body []byte, wantError, wantDetail string) { + t.Helper() + dec := json.NewDecoder(bytes.NewReader(body)) + dec.DisallowUnknownFields() + var e Error + require.NoError(t, dec.Decode(&e), "body must match the Error schema: %s", body) + assert.Equal(t, wantError, string(e.Error)) + assert.NotEmpty(t, e.ErrorDescription) + gotDetail := "" + if e.Details != nil && e.Details.Code != nil { + gotDetail = *e.Details.Code + } + assert.Equal(t, wantDetail, gotDetail) +} + +// expiredPickerToken returns a linked token that needs a refresh before use. +func expiredPickerToken(providerID string) *ContentToken { + past := time.Now().Add(-1 * time.Hour) + return &ContentToken{ + ID: "tok-expired", + UserID: testUserID, + ProviderID: providerID, + AccessToken: "old", + RefreshToken: "refresh", + Status: ContentTokenStatusActive, + ExpiresAt: &past, + } +} + +func TestContentOAuthUnavailable_MintPickerToken_RefreshErrors(t *testing.T) { + tests := []struct { + name string + refreshErr error // provider Refresh error + lockErr error // RefreshWithLock store error (skips the provider call) + wantStatus int + wantError string + wantDetail string + wantRetryAf bool + }{ + {"transient provider failure", errors.New("provider timeout"), nil, http.StatusServiceUnavailable, "service_unavailable", "transient_failure", true}, + {"transient DB fault in refresh lock", nil, errTransientDB, http.StatusServiceUnavailable, "service_unavailable", "", true}, + {"unclassified refresh lock error", nil, errors.New("boom"), http.StatusInternalServerError, "server_error", "", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tok := expiredPickerToken(pickerProviderID) + repo := &mockContentTokenRepo{ + getByUserAndProvider: func(_ context.Context, _, _ string) (*ContentToken, error) { return tok, nil }, + refreshWithLock: func(_ context.Context, _ string, fn func(*ContentToken) (*ContentToken, error)) (*ContentToken, error) { + if tc.lockErr != nil { + return nil, tc.lockErr + } + cur := *tok + return fn(&cur) + }, + } + registry := NewContentOAuthProviderRegistry() + registry.Register(&stubRefreshableProvider{id: pickerProviderID, refreshErr: tc.refreshErr}) + r := pickerTestRouter(newPickerTestHandler(repo, registry, defaultPickerConfigs())) + + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/me/picker_tokens/"+pickerProviderID, nil)) + + require.Equal(t, tc.wantStatus, w.Code) + assert.Equal(t, tc.wantRetryAf, w.Header().Get("Retry-After") != "") + assertPickerErrorBody(t, w.Body.Bytes(), tc.wantError, tc.wantDetail) + }) + } +} + +func TestContentOAuthUnavailable_MicrosoftPickerGrantHandler_RefreshLockTransient_Returns503(t *testing.T) { + tok := expiredPickerToken(ProviderMicrosoft) + repo := &mockContentTokenRepo{ + getByUserAndProvider: func(_ context.Context, _, _ string) (*ContentToken, error) { return tok, nil }, + refreshWithLock: func(_ context.Context, _ string, _ func(*ContentToken) (*ContentToken, error)) (*ContentToken, error) { + return nil, errTransientDB + }, + } + registry := NewContentOAuthProviderRegistry() + registry.Register(&stubRefreshableProvider{id: ProviderMicrosoft}) + h := NewMicrosoftPickerGrantHandler(repo, registry, "app-id", "", + func(_ *gin.Context) (string, bool) { return testUserID, true }, permissiveLoopbackValidator()) + + body, _ := json.Marshal(MicrosoftPickerGrantRequest{DriveId: "b!abc", ItemId: "01XYZ"}) + c, rec := newPickerGrantGinContext(t, "POST", "/me/microsoft/picker_grants", bytes.NewReader(body)) + h.Handle(c) + + assertServiceUnavailable(t, rec) +} diff --git a/api/microsoft_picker_grant_handler.go b/api/microsoft_picker_grant_handler.go index 81c4ddf0..09618d71 100644 --- a/api/microsoft_picker_grant_handler.go +++ b/api/microsoft_picker_grant_handler.go @@ -10,6 +10,7 @@ import ( "strings" "time" + "github.com/ericfitz/tmi/internal/errcode" "github.com/ericfitz/tmi/internal/slogging" "github.com/gin-gonic/gin" ) @@ -85,55 +86,41 @@ func NewMicrosoftPickerGrantHandler( // 6. Refresh if expired → 401 (permanent) or 503 (transient) on failure. // 7. Call Graph permissions API → 200 / 422 grant_failed / 503 transient_failure. // -// SEM@16390049ed01287835d8186f860dadff9c8c9288: handle a Microsoft File Picker grant request, issuing a Graph permission and returning the permission ID +// SEM@5e6e00bd650bd3fdec6654c4a7d0f26192c35f2d: handle a Microsoft File Picker grant request, issuing a Graph permission and returning the permission ID func (h *MicrosoftPickerGrantHandler) Handle(c *gin.Context) { log := slogging.Get().WithContext(c) if h.applicationObjectID == "" { - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "code": "picker_grants_not_configured", - "message": "Microsoft picker grants are not configured (application_object_id is empty)", - }) + HandleRequestError(c, WithDetailCode(UnprocessableEntityError("Microsoft picker grants are not configured (application_object_id is empty)"), + errcode.DetailPickerGrantsNotConfigured)) return } userID, ok := h.userLookup(c) if !ok { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "unauthenticated", - "message": "authentication required", - }) + HandleRequestError(c, UnauthorizedError("authentication required")) return } var req MicrosoftPickerGrantRequest if err := c.ShouldBindJSON(&req); err != nil || req.DriveId == "" || req.ItemId == "" { - c.JSON(http.StatusBadRequest, gin.H{ - "code": "invalid_request", - "message": "drive_id and item_id are required", - }) + HandleRequestError(c, InvalidInputError("drive_id and item_id are required")) return } tok, err := h.tokens.GetByUserAndProvider(c.Request.Context(), userID, ProviderMicrosoft) if err != nil { if errors.Is(err, ErrContentTokenNotFound) { - c.JSON(http.StatusNotFound, gin.H{ - "code": "not_linked", - "message": "no linked Microsoft token", - }) + HandleRequestError(c, WithDetailCode(NotFoundError("no linked Microsoft token"), errcode.DetailNotLinked)) return } log.Error("microsoft_picker_grant: token lookup user=%s: %v", userID, err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "no linked Microsoft token", "failed to look up linked token")) return } if tok.Status == ContentTokenStatusFailedRefresh { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "token_refresh_failed", - "message": "re-link Microsoft account", - }) + HandleRequestError(c, WithDetailCode(UnauthorizedError("re-link Microsoft account"), errcode.DetailTokenRefreshFailed)) return } @@ -151,21 +138,16 @@ func (h *MicrosoftPickerGrantHandler) Handle(c *gin.Context) { accessToken, _, err := h.refreshIfNeeded(c, tok) if err != nil { if errors.Is(err, ErrAuthRequired) { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "token_refresh_failed", - "message": "re-link Microsoft account", - }) + HandleRequestError(c, WithDetailCode(UnauthorizedError("re-link Microsoft account"), errcode.DetailTokenRefreshFailed)) return } if errors.Is(err, ErrTransient) { - c.JSON(http.StatusServiceUnavailable, gin.H{ - "code": "transient_failure", - "message": "retry later", - }) + HandleRequestError(c, WithDetailCode(ServiceUnavailableError("retry later"), errcode.DetailTransientFailure)) return } log.Error("microsoft_picker_grant: refresh user=%s: %v", userID, err) - c.AbortWithStatus(http.StatusInternalServerError) + // Remaining errors come from the token store's RefreshWithLock. + HandleRequestError(c, StoreErrorToRequestError(err, "no linked Microsoft token", "failed to refresh linked token")) return } @@ -176,23 +158,17 @@ func (h *MicrosoftPickerGrantHandler) Handle(c *gin.Context) { // Transport error reaching Graph (DNS, connection refused, TLS, timeout): // canonically transient. log.Warn("microsoft_picker_grant: transport error to graph user=%s: %v", userID, gerr) - c.JSON(http.StatusServiceUnavailable, gin.H{"code": "transient_failure", "message": "could not reach Graph"}) + HandleRequestError(c, WithDetailCode(ServiceUnavailableError("could not reach Graph"), errcode.DetailTransientFailure)) case status >= 400 && status < 500: - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "code": "grant_failed", - "message": "Graph rejected the permission grant", - "graph_status": status, - }) + HandleRequestError(c, WithDetailContext(WithDetailCode(UnprocessableEntityError("Graph rejected the permission grant"), + errcode.DetailGrantFailed), "graph_status", status)) case status >= 500: - c.JSON(http.StatusServiceUnavailable, gin.H{ - "code": "transient_failure", - "message": "Graph 5xx", - }) + HandleRequestError(c, WithDetailCode(ServiceUnavailableError("Graph returned a server error"), errcode.DetailTransientFailure)) default: // Truly unexpected: status >= 200 but < 400 with a non-nil error // (e.g., decode failure on success body). Log and return 500. log.Error("microsoft_picker_grant: graph user=%s status=%d: %v", userID, status, gerr) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, ServerError("unexpected response from Graph")) } return } diff --git a/api/microsoft_picker_grant_handler_test.go b/api/microsoft_picker_grant_handler_test.go index 7a39692c..be27c453 100644 --- a/api/microsoft_picker_grant_handler_test.go +++ b/api/microsoft_picker_grant_handler_test.go @@ -112,9 +112,7 @@ func TestMicrosoftPickerGrantHandler_Handle_NoAppObjectID(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusUnprocessableEntity, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "picker_grants_not_configured", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "unprocessable_entity", "picker_grants_not_configured") } // Case 3: No authenticated user → 401 unauthenticated. @@ -128,9 +126,7 @@ func TestMicrosoftPickerGrantHandler_Handle_Unauthenticated(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusUnauthorized, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "unauthenticated", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "unauthorized", "") } // Case 4: No linked Microsoft token → 404 not_linked. @@ -144,9 +140,7 @@ func TestMicrosoftPickerGrantHandler_Handle_NotLinked(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusNotFound, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "not_linked", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "not_found", "not_linked") } // Case 5: Graph returns 403 → 422 grant_failed. @@ -175,9 +169,7 @@ func TestMicrosoftPickerGrantHandler_Handle_GraphReturns403(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusUnprocessableEntity, rec.Code) - var resp map[string]any - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "grant_failed", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "unprocessable_entity", "grant_failed") } // Case 6: Token in failed_refresh status → 401 token_refresh_failed (no Graph call). @@ -200,9 +192,7 @@ func TestMicrosoftPickerGrantHandler_Handle_FailedRefreshStatus(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusUnauthorized, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "token_refresh_failed", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "unauthorized", "token_refresh_failed") } // Case 7: Missing drive_id → 400 invalid_request. @@ -227,9 +217,7 @@ func TestMicrosoftPickerGrantHandler_Handle_MissingDriveID(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusBadRequest, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "invalid_request", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "invalid_input", "") } // Case 8: Transport error reaching Graph (connection closed without response) → 503 transient_failure. @@ -265,9 +253,7 @@ func TestMicrosoftPickerGrantHandler_Handle_TransportError(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusServiceUnavailable, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "transient_failure", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "service_unavailable", "transient_failure") } // Case 10: Consumer Microsoft account (personal MSA) — handler skips Graph @@ -344,7 +330,5 @@ func TestMicrosoftPickerGrantHandler_Handle_GraphReturns500(t *testing.T) { h.Handle(c) assert.Equal(t, http.StatusServiceUnavailable, rec.Code) - var resp map[string]string - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) - assert.Equal(t, "transient_failure", resp["code"]) + assertPickerErrorBody(t, rec.Body.Bytes(), "service_unavailable", "transient_failure") } diff --git a/api/picker_token_handler.go b/api/picker_token_handler.go index bbad5563..fa6489a1 100644 --- a/api/picker_token_handler.go +++ b/api/picker_token_handler.go @@ -7,6 +7,7 @@ import ( "net/http" "time" + "github.com/ericfitz/tmi/internal/errcode" "github.com/ericfitz/tmi/internal/slogging" "github.com/gin-gonic/gin" ) @@ -83,7 +84,7 @@ func NewPickerTokenHandler( // 6. refreshIfNeeded → 401/503 on error. // 7. 200 with access_token, expires_at, developer_key, app_id. // -// SEM@67569fbeb577336dd278978473f2ea666cf1543f: validate and dispatch a picker token request, refreshing the OAuth token if needed (reads DB) +// SEM@5e6e00bd650bd3fdec6654c4a7d0f26192c35f2d: validate and dispatch a picker token request, refreshing the OAuth token if needed (reads DB) func (h *PickerTokenHandler) Handle(c *gin.Context) { log := slogging.Get().WithContext(c) providerID := c.Param("provider_id") @@ -91,32 +92,23 @@ func (h *PickerTokenHandler) Handle(c *gin.Context) { // Step 1: Check picker configuration. cfg, hasCfg := h.configs[providerID] if !hasCfg || (len(cfg.ProviderConfig) == 0 && cfg.DeveloperKey == "" && cfg.AppID == "") { - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "code": "picker_not_configured", - "message": "picker is not configured for this provider", - "provider_id": providerID, - }) + HandleRequestError(c, WithDetailContext(WithDetailCode(UnprocessableEntityError("picker is not configured for this provider"), + errcode.DetailPickerNotConfigured), "provider_id", providerID)) return } // Step 2: Check provider registry. _, ok := h.registry.Get(providerID) if !ok { - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "code": "provider_not_registered", - "message": "OAuth provider is not registered", - "provider_id": providerID, - }) + HandleRequestError(c, WithDetailContext(WithDetailCode(UnprocessableEntityError("OAuth provider is not registered"), + errcode.DetailProviderNotRegistered), "provider_id", providerID)) return } // Step 3: Authenticate the caller. userID, ok := h.userLookup(c) if !ok { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "unauthenticated", - "message": "authentication required", - }) + HandleRequestError(c, UnauthorizedError("authentication required")) return } @@ -124,24 +116,19 @@ func (h *PickerTokenHandler) Handle(c *gin.Context) { tok, err := h.tokens.GetByUserAndProvider(c.Request.Context(), userID, providerID) if err != nil { if errors.Is(err, ErrContentTokenNotFound) { - c.JSON(http.StatusNotFound, gin.H{ - "code": "not_linked", - "message": "no linked token for this provider", - "provider_id": providerID, - }) + HandleRequestError(c, WithDetailContext(WithDetailCode(NotFoundError("no linked token for this provider"), + errcode.DetailNotLinked), "provider_id", providerID)) return } log.Error("picker_token: GetByUserAndProvider user=%s provider=%s: %v", userID, providerID, err) - c.AbortWithStatus(http.StatusInternalServerError) + HandleRequestError(c, StoreErrorToRequestError(err, "no linked token for this provider", "failed to look up linked token")) return } // Step 5: Short-circuit on permanently failed token. if tok.Status == ContentTokenStatusFailedRefresh { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "token_refresh_failed", - "message": "token is in a failed refresh state; please re-authorize", - }) + HandleRequestError(c, WithDetailCode(UnauthorizedError("token is in a failed refresh state; please re-authorize"), + errcode.DetailTokenRefreshFailed)) return } @@ -149,21 +136,18 @@ func (h *PickerTokenHandler) Handle(c *gin.Context) { accessToken, expiresAt, err := h.refreshIfNeeded(c, tok, providerID) if err != nil { if errors.Is(err, ErrAuthRequired) { - c.JSON(http.StatusUnauthorized, gin.H{ - "code": "token_refresh_failed", - "message": "token refresh failed; please re-authorize", - }) + HandleRequestError(c, WithDetailCode(UnauthorizedError("token refresh failed; please re-authorize"), + errcode.DetailTokenRefreshFailed)) return } if errors.Is(err, ErrTransient) { - c.JSON(http.StatusServiceUnavailable, gin.H{ - "code": "transient_failure", - "message": "a transient error occurred; please retry", - }) + HandleRequestError(c, WithDetailCode(ServiceUnavailableError("a transient error occurred; please retry"), + errcode.DetailTransientFailure)) return } log.Error("picker_token: refreshIfNeeded user=%s provider=%s: %v", userID, providerID, err) - c.AbortWithStatus(http.StatusInternalServerError) + // Remaining errors come from the token store's RefreshWithLock. + HandleRequestError(c, StoreErrorToRequestError(err, "no linked token for this provider", "failed to refresh linked token")) return } diff --git a/api/picker_token_handler_test.go b/api/picker_token_handler_test.go index 8226af85..aeaa2218 100644 --- a/api/picker_token_handler_test.go +++ b/api/picker_token_handler_test.go @@ -198,9 +198,7 @@ func TestMintPickerToken_NoLinkedToken(t *testing.T) { require.Equal(t, http.StatusNotFound, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "not_linked", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "not_found", "not_linked") } // Case 3: Token in failed_refresh status → 401 with code "token_refresh_failed". @@ -228,9 +226,7 @@ func TestMintPickerToken_FailedRefreshStatus(t *testing.T) { require.Equal(t, http.StatusUnauthorized, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "token_refresh_failed", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "unauthorized", "token_refresh_failed") } // Case 4: Provider not registered in registry → 422 with code "provider_not_registered". @@ -248,9 +244,7 @@ func TestMintPickerToken_ProviderNotRegistered(t *testing.T) { require.Equal(t, http.StatusUnprocessableEntity, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "provider_not_registered", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "unprocessable_entity", "provider_not_registered") } // Case 5: Picker inputs missing from config → 422 with code "picker_not_configured". @@ -273,9 +267,7 @@ func TestMintPickerToken_PickerNotConfigured(t *testing.T) { require.Equal(t, http.StatusUnprocessableEntity, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "picker_not_configured", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "unprocessable_entity", "picker_not_configured") } // Case 6: No user in context → 401 with code "unauthenticated". @@ -297,9 +289,7 @@ func TestMintPickerToken_Unauthenticated(t *testing.T) { require.Equal(t, http.StatusUnauthorized, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "unauthenticated", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "unauthorized", "") } // Case 7: Token expired but refresh succeeds → 200 with refreshed token. @@ -510,7 +500,5 @@ func TestMintPickerToken_RefreshPermanentFailure(t *testing.T) { require.Equal(t, http.StatusUnauthorized, w.Code) - var body map[string]string - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - assert.Equal(t, "token_refresh_failed", body["code"]) + assertPickerErrorBody(t, w.Body.Bytes(), "unauthorized", "token_refresh_failed") } diff --git a/api/request_utils.go b/api/request_utils.go index 18aa7645..4a97628e 100644 --- a/api/request_utils.go +++ b/api/request_utils.go @@ -670,6 +670,19 @@ func WithDetailCode(err *RequestError, code errcode.Code) *RequestError { return err } +// WithDetailContext adds one key/value to err's details.context and returns it. +// SEM@5e6e00bd650bd3fdec6654c4a7d0f26192c35f2d: attach a key/value pair to a RequestError's details context (pure) +func WithDetailContext(err *RequestError, key string, value any) *RequestError { + if err.Details == nil { + err.Details = &ErrorDetails{} + } + if err.Details.Context == nil { + err.Details.Context = map[string]any{} + } + err.Details.Context[key] = value + return err +} + // SEM@074f3ca8790e600162273d06152aff41b7223b07: build a RequestError from status, code and message (pure) func newRequestError(status int, code errcode.Code, message string) *RequestError { return &RequestError{Status: status, Code: code, Message: message} diff --git a/internal/errcode/errcode.go b/internal/errcode/errcode.go index f3a33afd..90eb2303 100644 --- a/internal/errcode/errcode.go +++ b/internal/errcode/errcode.go @@ -8,6 +8,7 @@ package errcode import "regexp" // Code is a machine-readable error code. +// SEM@db94db3c8118cffb5220bf9a44c2fde0ada20427: machine-readable error code emitted in API error bodies type Code string // Tier 1: REST codes (Error.error enum in the OpenAPI spec). @@ -153,6 +154,12 @@ const ( DetailLlmBusy Code = "llm_busy" DetailLlmNotConfigured Code = "llm_not_configured" DetailCollaborationSessionExists Code = "collaboration_session_exists" + DetailPickerNotConfigured Code = "picker_not_configured" + DetailPickerGrantsNotConfigured Code = "picker_grants_not_configured" + DetailNotLinked Code = "not_linked" + DetailTokenRefreshFailed Code = "token_refresh_failed" + DetailTransientFailure Code = "transient_failure" + DetailGrantFailed Code = "grant_failed" ) // transportCodes are emitted by route-agnostic middleware before dispatch, so @@ -217,6 +224,12 @@ var detailCodes = []Code{ DetailLlmBusy, DetailLlmNotConfigured, DetailCollaborationSessionExists, + DetailPickerNotConfigured, + DetailPickerGrantsNotConfigured, + DetailNotLinked, + DetailTokenRefreshFailed, + DetailTransientFailure, + DetailGrantFailed, } var (