mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-16 16:15:40 +00:00
Browser pairing authenticates users as RoleOperator, but every provider and OAuth route was wrapped in requireAuth(RoleAdmin). The /setup flow reads GET /v1/providers, so paired browsers got 403 and hung on the setup page. Split provider/OAuth route auth by operation: - providers.go: GET list/get/models/status/embedding/codex-activity now use a readAuth wrapper (method-derived min → Viewer); POST/PUT/DELETE and reconnect/verify stay Admin. Responses already mask API keys and queries stay tenant-scoped, so no secrets are exposed. - oauth.go: GET status/quota use readAuth (they return only connection state and quota, never tokens); start/callback/logout stay Admin (preserving the #450 admin-only-management decision for mutations). Updates the two auth tests to assert the new contract: an Operator can read providers/OAuth status (200) but still cannot mutate them (403).
This commit is contained in:
1 parent
cab639d15e
commit
dcf000ab6a
3 files changed
+70
-42
No files matched your search
+15
-8
@@ -59,27 +59,34 @@ func NewOAuthHandler(provStore store.ProviderStore, secretStore store.ConfigSecr
|
||||
|
||||
// RegisterRoutes registers OAuth routes on the given mux.
|
||||
func (h *OAuthHandler) RegisterRoutes(mux *http.ServeMux) {
|
||||
mux.HandleFunc("GET /v1/auth/chatgpt/{provider}/status", h.auth(h.handleStatus))
|
||||
mux.HandleFunc("GET /v1/auth/chatgpt/{provider}/quota", h.auth(h.handleQuota))
|
||||
// Read-only status/quota — needed by /setup for browser-paired Operators
|
||||
// (issue #1075). Managing OAuth (start/callback/logout) stays Admin-only.
|
||||
mux.HandleFunc("GET /v1/auth/chatgpt/{provider}/status", h.readAuth(h.handleStatus))
|
||||
mux.HandleFunc("GET /v1/auth/chatgpt/{provider}/quota", h.readAuth(h.handleQuota))
|
||||
mux.HandleFunc("POST /v1/auth/chatgpt/{provider}/start", h.auth(h.handleStart))
|
||||
mux.HandleFunc("POST /v1/auth/chatgpt/{provider}/callback", h.auth(h.handleManualCallback))
|
||||
mux.HandleFunc("POST /v1/auth/chatgpt/{provider}/logout", h.auth(h.handleLogout))
|
||||
|
||||
mux.HandleFunc("GET /v1/auth/openai/status", h.auth(h.handleStatus))
|
||||
mux.HandleFunc("GET /v1/auth/openai/quota", h.auth(h.handleQuota))
|
||||
mux.HandleFunc("GET /v1/auth/openai/status", h.readAuth(h.handleStatus))
|
||||
mux.HandleFunc("GET /v1/auth/openai/quota", h.readAuth(h.handleQuota))
|
||||
mux.HandleFunc("POST /v1/auth/openai/start", h.auth(h.handleStart))
|
||||
mux.HandleFunc("POST /v1/auth/openai/callback", h.auth(h.handleManualCallback))
|
||||
mux.HandleFunc("POST /v1/auth/openai/logout", h.auth(h.handleLogout))
|
||||
}
|
||||
|
||||
// auth requires RoleAdmin for all OAuth endpoints.
|
||||
// Breaking change from pre-#450: previously used requireAuth("", next) which
|
||||
// allowed any authenticated user including operators. Now only admins can
|
||||
// manage OAuth providers.
|
||||
// auth requires RoleAdmin for OAuth management (start/callback/logout).
|
||||
// Since #450 these mutating flows are admin-only; #1075 keeps that for writes
|
||||
// but splits reads out via readAuth so setup can check connection status.
|
||||
func (h *OAuthHandler) auth(next http.HandlerFunc) http.HandlerFunc {
|
||||
return requireAuth(permissions.RoleAdmin, next)
|
||||
}
|
||||
|
||||
// readAuth gates read-only OAuth status/quota at the method-derived minimum
|
||||
// (GET→Viewer). Responses expose only connection state and quota, never tokens.
|
||||
func (h *OAuthHandler) readAuth(next http.HandlerFunc) http.HandlerFunc {
|
||||
return requireAuth("", next)
|
||||
}
|
||||
|
||||
func oauthTenantID(ctx context.Context) uuid.UUID {
|
||||
if tid := store.TenantIDFromContext(ctx); tid != uuid.Nil {
|
||||
return tid
|
||||
|
||||
+34
-23
@@ -420,8 +420,11 @@ func TestOAuthHandlerProviderLogoutRoute(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestProvidersHandlerRequiresAdmin(t *testing.T) {
|
||||
token := "operator-key"
|
||||
// operatorAPIKeyRequest builds a request authenticated as a browser-pairing-style
|
||||
// Operator (operator.write scope → RoleOperator).
|
||||
func operatorAPIKeyRequest(t *testing.T, method, path string) *http.Request {
|
||||
t.Helper()
|
||||
token := "operator-key-" + method + path
|
||||
setupTestCache(t, map[string]*store.APIKeyData{
|
||||
crypto.HashAPIKey(token): {
|
||||
ID: uuid.New(),
|
||||
@@ -429,41 +432,49 @@ func TestProvidersHandlerRequiresAdmin(t *testing.T) {
|
||||
TenantID: store.MasterTenantID,
|
||||
},
|
||||
})
|
||||
req := httptest.NewRequest(method, path, nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
return req
|
||||
}
|
||||
|
||||
// TestProvidersHandler_OperatorReadsButCannotMutate is a regression for issue #1075:
|
||||
// browser-paired Operators must read providers so /setup can proceed, while
|
||||
// create/update/delete stay Admin-only.
|
||||
func TestProvidersHandler_OperatorReadsButCannotMutate(t *testing.T) {
|
||||
h := NewProvidersHandler(newMockProviderStore(), newMockSecretsStore(), nil, "")
|
||||
mux := http.NewServeMux()
|
||||
h.RegisterRoutes(mux)
|
||||
|
||||
req := httptest.NewRequest("GET", "/v1/providers", nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
w := httptest.NewRecorder()
|
||||
mux.ServeHTTP(w, req)
|
||||
wGet := httptest.NewRecorder()
|
||||
mux.ServeHTTP(wGet, operatorAPIKeyRequest(t, "GET", "/v1/providers"))
|
||||
if wGet.Code != http.StatusOK {
|
||||
t.Fatalf("operator GET /v1/providers = %d, want 200", wGet.Code)
|
||||
}
|
||||
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Fatalf("status code = %d, want %d", w.Code, http.StatusForbidden)
|
||||
wPost := httptest.NewRecorder()
|
||||
mux.ServeHTTP(wPost, operatorAPIKeyRequest(t, "POST", "/v1/providers"))
|
||||
if wPost.Code != http.StatusForbidden {
|
||||
t.Fatalf("operator POST /v1/providers = %d, want 403", wPost.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOAuthHandlerRequiresAdmin(t *testing.T) {
|
||||
token := "operator-key"
|
||||
setupTestCache(t, map[string]*store.APIKeyData{
|
||||
crypto.HashAPIKey(token): {
|
||||
ID: uuid.New(),
|
||||
Scopes: []string{"operator.write"},
|
||||
TenantID: store.MasterTenantID,
|
||||
},
|
||||
})
|
||||
// TestOAuthHandler_OperatorReadsStatusButCannotManage: Operator can read OAuth
|
||||
// status for setup; managing OAuth (start) remains Admin-only (issue #1075, #450).
|
||||
func TestOAuthHandler_OperatorReadsStatusButCannotManage(t *testing.T) {
|
||||
h := newTestOAuthHandler(t, "")
|
||||
mux := http.NewServeMux()
|
||||
h.RegisterRoutes(mux)
|
||||
|
||||
req := httptest.NewRequest("GET", "/v1/auth/openai/status", nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
w := httptest.NewRecorder()
|
||||
mux.ServeHTTP(w, req)
|
||||
wGet := httptest.NewRecorder()
|
||||
mux.ServeHTTP(wGet, operatorAPIKeyRequest(t, "GET", "/v1/auth/openai/status"))
|
||||
if wGet.Code != http.StatusOK {
|
||||
t.Fatalf("operator GET oauth status = %d, want 200", wGet.Code)
|
||||
}
|
||||
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Fatalf("status code = %d, want %d", w.Code, http.StatusForbidden)
|
||||
wPost := httptest.NewRecorder()
|
||||
mux.ServeHTTP(wPost, operatorAPIKeyRequest(t, "POST", "/v1/auth/openai/start"))
|
||||
if wPost.Code != http.StatusForbidden {
|
||||
t.Fatalf("operator POST oauth start = %d, want 403", wPost.Code)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+21
-11
@@ -158,35 +158,45 @@ func providerCacheTenantID(ctx context.Context, tenantID uuid.UUID) uuid.UUID {
|
||||
|
||||
// RegisterRoutes registers all provider management routes on the given mux.
|
||||
func (h *ProvidersHandler) RegisterRoutes(mux *http.ServeMux) {
|
||||
// Provider CRUD
|
||||
mux.HandleFunc("GET /v1/providers", h.auth(h.handleListProviders))
|
||||
// Provider CRUD — reads are needed by /setup for browser-paired Operators
|
||||
// (issue #1075), so GET routes use read-level auth (GET→Viewer) while
|
||||
// mutations stay Admin-only.
|
||||
mux.HandleFunc("GET /v1/providers", h.readAuth(h.handleListProviders))
|
||||
mux.HandleFunc("POST /v1/providers", h.auth(h.handleCreateProvider))
|
||||
mux.HandleFunc("GET /v1/providers/{id}", h.auth(h.handleGetProvider))
|
||||
mux.HandleFunc("GET /v1/providers/{id}", h.readAuth(h.handleGetProvider))
|
||||
mux.HandleFunc("PUT /v1/providers/{id}", h.auth(h.handleUpdateProvider))
|
||||
mux.HandleFunc("DELETE /v1/providers/{id}", h.auth(h.handleDeleteProvider))
|
||||
|
||||
// Model listing (proxied to upstream provider API)
|
||||
mux.HandleFunc("GET /v1/providers/{id}/models", h.auth(h.handleListProviderModels))
|
||||
mux.HandleFunc("GET /v1/providers/{id}/models", h.readAuth(h.handleListProviderModels))
|
||||
|
||||
// Provider + model verification (pre-flight check)
|
||||
// Provider + model verification (pre-flight check) — mutating actions, Admin.
|
||||
mux.HandleFunc("POST /v1/providers/{id}/reconnect", h.auth(h.handleReconnectProvider))
|
||||
mux.HandleFunc("POST /v1/providers/{id}/verify", h.auth(h.handleVerifyProvider))
|
||||
mux.HandleFunc("POST /v1/providers/{id}/verify-embedding", h.auth(h.handleVerifyEmbedding))
|
||||
|
||||
// Provider-scoped Codex pool activity monitor
|
||||
mux.HandleFunc("GET /v1/providers/{id}/codex-pool-activity", h.auth(h.handleProviderCodexPoolActivity))
|
||||
// Provider-scoped Codex pool activity monitor (read-only status)
|
||||
mux.HandleFunc("GET /v1/providers/{id}/codex-pool-activity", h.readAuth(h.handleProviderCodexPoolActivity))
|
||||
|
||||
// Embedding system status
|
||||
mux.HandleFunc("GET /v1/embedding/status", h.auth(h.handleEmbeddingStatus))
|
||||
// Embedding system status (read-only)
|
||||
mux.HandleFunc("GET /v1/embedding/status", h.readAuth(h.handleEmbeddingStatus))
|
||||
|
||||
// Claude CLI auth status (global — not per-provider)
|
||||
mux.HandleFunc("GET /v1/providers/claude-cli/auth-status", h.auth(h.handleClaudeCLIAuthStatus))
|
||||
// Claude CLI auth status (global — not per-provider; read-only status)
|
||||
mux.HandleFunc("GET /v1/providers/claude-cli/auth-status", h.readAuth(h.handleClaudeCLIAuthStatus))
|
||||
}
|
||||
|
||||
// auth gates provider mutations at Admin.
|
||||
func (h *ProvidersHandler) auth(next http.HandlerFunc) http.HandlerFunc {
|
||||
return requireAuth(permissions.RoleAdmin, next)
|
||||
}
|
||||
|
||||
// readAuth gates read-only provider endpoints at the method-derived minimum
|
||||
// (GET→Viewer), so browser-paired Operators can complete /setup (issue #1075).
|
||||
// Responses already mask API keys, and provider queries stay tenant-scoped.
|
||||
func (h *ProvidersHandler) readAuth(next http.HandlerFunc) http.HandlerFunc {
|
||||
return requireAuth("", next)
|
||||
}
|
||||
|
||||
// maskAPIKey replaces non-empty API keys with "***".
|
||||
func maskAPIKey(p *store.LLMProviderData) {
|
||||
if p.APIKey != "" {
|
||||
|
||||
Reference in new issue
Block a user