fix: portal nil S3Client panic, admin role prefix, and deep link URL (#191)

cjimti · web-flow · commit c7e71a41938b · 2026-03-05T23:58:02.000-08:00
* fix: portal nil S3Client panic, admin role prefix, and deep link URL

- Guard getAssetContent and fetchPublicAsset against nil S3Client,
  returning 503 instead of panicking when s3_connection is not configured
- Use AdminRoles from persona config for is_admin check instead of
  hardcoded "admin" string, fixing mismatch with prefixed roles like
  "dp_admin"
- Fix save_artifact deep link from /artifacts/{id} to /portal/assets/{id}

* fix: fail early on nil S3Client in portal toolkit write paths

Previously, handleSaveArtifact and uploadContentUpdate silently skipped
the S3 upload when s3Client was nil, creating ghost assets in the store
pointing to non-existent S3 content. Now both paths return a clear
"content storage not configured" error instead.
diff --git a/cmd/mcp-data-platform/main.go b/cmd/mcp-data-platform/main.go
@@ -489,6 +489,13 @@ func mountPortalAPI(mux *http.ServeMux, p *platform.Platform) {
 	}
 	portalAuth := portal.NewAuthenticator(p.Authenticator(), portalAuthOpts...)
 
+	var adminRoles []string
+	if pr := p.PersonaRegistry(); pr != nil {
+		if persona, ok := pr.Get(p.Config().Admin.Persona); ok {
+			adminRoles = persona.Roles
+		}
+	}
+
 	deps := portal.Deps{
 		AssetStore:    p.PortalAssetStore(),
 		ShareStore:    p.PortalShareStore(),
@@ -500,6 +507,7 @@ func mountPortalAPI(mux *http.ServeMux, p *platform.Platform) {
 			BurstSize:         p.Config().Portal.RateLimit.BurstSize,
 		},
 		OIDCEnabled: p.BrowserSessionFlow() != nil,
+		AdminRoles:  adminRoles,
 	}
 
 	handler := portal.NewHandler(deps, portal.RequirePortalAuth(portalAuth))
diff --git a/pkg/portal/handler.go b/pkg/portal/handler.go
@@ -30,6 +30,7 @@ type Deps struct {
 	PublicBaseURL string
 	RateLimit     RateLimitConfig
 	OIDCEnabled   bool
+	AdminRoles    []string // roles that grant admin access in the portal
 }
 
 // Handler provides portal REST API endpoints.
@@ -97,7 +98,7 @@ type meResponse struct {
 	IsAdmin bool     `json:"is_admin"`
 }
 
-func (*Handler) getMe(w http.ResponseWriter, r *http.Request) {
+func (h *Handler) getMe(w http.ResponseWriter, r *http.Request) {
 	user := GetUser(r.Context())
 	if user == nil {
 		writeError(w, http.StatusUnauthorized, errAuthRequired)
@@ -107,7 +108,7 @@ func (*Handler) getMe(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, meResponse{
 		UserID:  user.UserID,
 		Roles:   user.Roles,
-		IsAdmin: slices.Contains(user.Roles, "admin"),
+		IsAdmin: hasAnyRole(user.Roles, h.deps.AdminRoles),
 	})
 }
 
@@ -207,6 +208,11 @@ func (h *Handler) getAssetContent(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	if h.deps.S3Client == nil {
+		writeError(w, http.StatusServiceUnavailable, "content storage not configured")
+		return
+	}
+
 	data, contentType, err := h.deps.S3Client.GetObject(r.Context(), asset.S3Bucket, asset.S3Key)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, "failed to retrieve content")
@@ -510,6 +516,16 @@ func generateToken() (string, error) {
 	return hex.EncodeToString(b), nil
 }
 
+// hasAnyRole returns true if any role in userRoles is also in targetRoles.
+func hasAnyRole(userRoles, targetRoles []string) bool {
+	for _, r := range userRoles {
+		if slices.Contains(targetRoles, r) {
+			return true
+		}
+	}
+	return false
+}
+
 // isSharedWithUser checks whether an asset has been shared with a specific user.
 func (h *Handler) isSharedWithUser(r *http.Request, assetID, userID string) bool {
 	shares, err := h.deps.ShareStore.ListByAsset(r.Context(), assetID)
diff --git a/pkg/portal/handler_test.go b/pkg/portal/handler_test.go
@@ -449,6 +449,23 @@ func TestGetAssetContentNoUser(t *testing.T) {
 	assert.Equal(t, http.StatusUnauthorized, w.Code)
 }
 
+func TestGetAssetContentNilS3Client(t *testing.T) {
+	asset := &Asset{ID: "a1", OwnerID: "u1", S3Bucket: "b", S3Key: "k"}
+	h := NewHandler(Deps{
+		AssetStore: &mockAssetStore{getAsset: asset},
+		ShareStore: &mockShareStore{},
+		S3Client:   nil, // no S3 configured
+		S3Bucket:   "test-bucket",
+	}, testAuthMiddleware(&User{UserID: "u1"}))
+
+	req := httptest.NewRequest("GET", "/api/v1/portal/assets/a1/content", http.NoBody)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusServiceUnavailable, w.Code)
+	assert.Contains(t, w.Body.String(), "content storage not configured")
+}
+
 // --- updateAsset ---
 
 func TestUpdateAssetSuccess(t *testing.T) {
@@ -1107,6 +1124,28 @@ func TestListSharedWithMeNoUser(t *testing.T) {
 	assert.Equal(t, http.StatusUnauthorized, w.Code)
 }
 
+// --- hasAnyRole ---
+
+func TestHasAnyRole(t *testing.T) {
+	tests := []struct {
+		name        string
+		userRoles   []string
+		targetRoles []string
+		expected    bool
+	}{
+		{"match", []string{"dp_admin", "dp_analyst"}, []string{"dp_admin"}, true},
+		{"no match", []string{"dp_analyst"}, []string{"dp_admin"}, false},
+		{"empty user roles", nil, []string{"dp_admin"}, false},
+		{"empty target roles", []string{"dp_admin"}, nil, false},
+		{"both empty", nil, nil, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, hasAnyRole(tt.userRoles, tt.targetRoles))
+		})
+	}
+}
+
 // --- intParam ---
 
 func TestIntParam(t *testing.T) {
@@ -1221,8 +1260,14 @@ func TestIsSharedWithUserError(t *testing.T) {
 // --- Me handler tests ---
 
 func TestGetMeSuccess(t *testing.T) {
-	user := &User{UserID: "user-42", Roles: []string{"admin", "analyst"}}
-	h := newTestHandler(&mockAssetStore{}, &mockShareStore{}, &mockS3Client{}, user)
+	user := &User{UserID: "user-42", Roles: []string{"dp_admin", "analyst"}}
+	h := NewHandler(Deps{
+		AssetStore: &mockAssetStore{},
+		ShareStore: &mockShareStore{},
+		S3Client:   &mockS3Client{},
+		S3Bucket:   "test-bucket",
+		AdminRoles: []string{"dp_admin"},
+	}, testAuthMiddleware(user))
 
 	req := httptest.NewRequest("GET", "/api/v1/portal/me", http.NoBody)
 	w := httptest.NewRecorder()
@@ -1234,7 +1279,28 @@ func TestGetMeSuccess(t *testing.T) {
 	require.NoError(t, json.NewDecoder(w.Body).Decode(&resp))
 	assert.Equal(t, "user-42", resp.UserID)
 	assert.True(t, resp.IsAdmin)
-	assert.Contains(t, resp.Roles, "admin")
+	assert.Contains(t, resp.Roles, "dp_admin")
+}
+
+func TestGetMeNonAdminWithPrefixedRoles(t *testing.T) {
+	// Verify that roles like "dp_analyst" do NOT match admin when AdminRoles is ["dp_admin"]
+	user := &User{UserID: "user-99", Roles: []string{"dp_analyst"}}
+	h := NewHandler(Deps{
+		AssetStore: &mockAssetStore{},
+		ShareStore: &mockShareStore{},
+		S3Client:   &mockS3Client{},
+		AdminRoles: []string{"dp_admin"},
+	}, testAuthMiddleware(user))
+
+	req := httptest.NewRequest("GET", "/api/v1/portal/me", http.NoBody)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusOK, w.Code)
+
+	var resp meResponse
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&resp))
+	assert.False(t, resp.IsAdmin)
 }
 
 func TestGetMeNonAdmin(t *testing.T) {
diff --git a/pkg/portal/public.go b/pkg/portal/public.go
@@ -105,6 +105,10 @@ func (h *Handler) fetchPublicAsset(r *http.Request, assetID string) (*Asset, []b
 		return nil, nil, &publicAssetError{Message: "This asset has been deleted.", Status: http.StatusGone}
 	}
 
+	if h.deps.S3Client == nil {
+		return nil, nil, &publicAssetError{Message: "Content storage not configured.", Status: http.StatusServiceUnavailable}
+	}
+
 	data, _, err := h.deps.S3Client.GetObject(r.Context(), asset.S3Bucket, asset.S3Key)
 	if err != nil {
 		slog.Error("public view: failed to fetch content", "error", err, "asset_id", asset.ID) //nolint:gosec // structured log, not user-facing
diff --git a/pkg/portal/public_test.go b/pkg/portal/public_test.go
@@ -118,6 +118,22 @@ func TestPublicViewAssetDeleted(t *testing.T) {
 	assert.Equal(t, http.StatusGone, w.Code)
 }
 
+func TestPublicViewNilS3Client(t *testing.T) {
+	share := &Share{ID: "s1", AssetID: "a1", Token: "tok1"}
+	asset := &Asset{ID: "a1"}
+	h := NewHandler(Deps{
+		AssetStore: &mockAssetStore{getAsset: asset},
+		ShareStore: &mockShareStore{getByTokenRes: share},
+		S3Client:   nil, // no S3 configured
+	}, nil)
+
+	req := httptest.NewRequest("GET", "/portal/view/tok1", http.NoBody)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusServiceUnavailable, w.Code)
+}
+
 func TestPublicViewS3Error(t *testing.T) {
 	share := &Share{ID: "s1", AssetID: "a1", Token: "tok1"}
 	asset := &Asset{ID: "a1"}
diff --git a/pkg/toolkits/portal/toolkit.go b/pkg/toolkits/portal/toolkit.go
@@ -173,10 +173,11 @@ func (t *Toolkit) handleSaveArtifact(ctx context.Context, _ *mcp.CallToolRequest
 
 	s3Key := t.buildS3Key(userID, assetID, input.ContentType)
 
-	if t.s3Client != nil {
-		if err := t.s3Client.PutObject(ctx, t.s3Bucket, s3Key, []byte(input.Content), input.ContentType); err != nil {
-			return errorResult("failed to upload content: " + err.Error()), nil, nil //nolint:nilerr // MCP protocol
-		}
+	if t.s3Client == nil {
+		return errorResult("content storage not configured"), nil, nil
+	}
+	if err := t.s3Client.PutObject(ctx, t.s3Bucket, s3Key, []byte(input.Content), input.ContentType); err != nil {
+		return errorResult("failed to upload content: " + err.Error()), nil, nil //nolint:nilerr // MCP protocol
 	}
 
 	prov := buildProvenance(ctx, userID, sessionID)
@@ -314,10 +315,11 @@ func (t *Toolkit) uploadContentUpdate(ctx context.Context, asset *portal.Asset,
 	ext := extensionForContentType(ct)
 	s3Key := path.Join(t.s3Prefix, asset.OwnerID, asset.ID, "content"+ext)
 
-	if t.s3Client != nil {
-		if err := t.s3Client.PutObject(ctx, t.s3Bucket, s3Key, []byte(input.Content), ct); err != nil {
-			return fmt.Errorf("s3 put: %w", err)
-		}
+	if t.s3Client == nil {
+		return fmt.Errorf("content storage not configured")
+	}
+	if err := t.s3Client.PutObject(ctx, t.s3Bucket, s3Key, []byte(input.Content), ct); err != nil {
+		return fmt.Errorf("s3 put: %w", err)
 	}
 	updates.S3Key = s3Key
 	updates.SizeBytes = int64(len(input.Content))
@@ -393,7 +395,7 @@ func (t *Toolkit) buildSaveOutput(assetID string, prov portal.Provenance) saveAr
 		ToolCallsRecorded:  len(prov.ToolCalls),
 	}
 	if t.baseURL != "" {
-		out.PortalURL = t.baseURL + "/artifacts/" + assetID
+		out.PortalURL = t.baseURL + "/portal/assets/" + assetID
 	}
 	return out
 }
diff --git a/pkg/toolkits/portal/toolkit_test.go b/pkg/toolkits/portal/toolkit_test.go
@@ -120,7 +120,7 @@ func TestSaveArtifact_ValidationErrors(t *testing.T) {
 
 func TestSaveArtifact_WithProvenance(t *testing.T) {
 	store := newInMemoryAssetStore()
-	tk := New(Config{Name: "test", AssetStore: store, S3Bucket: "bucket"})
+	tk := New(Config{Name: "test", AssetStore: store, S3Client: &mockS3Client{}, S3Bucket: "bucket"})
 
 	provCalls := []middleware.ProvenanceToolCall{
 		{ToolName: "trino_query", Timestamp: "2024-01-01T00:00:00Z", Summary: "SELECT 1"},
@@ -305,6 +305,26 @@ func TestManageArtifact_MissingAssetID(t *testing.T) {
 	}
 }
 
+func TestBuildSaveOutput(t *testing.T) {
+	tk := New(Config{Name: "test", BaseURL: "https://example.com", S3Bucket: "bucket"})
+	out := tk.buildSaveOutput("abc123", portal.Provenance{
+		ToolCalls: []portal.ProvenanceToolCall{{ToolName: "trino_query"}},
+	})
+
+	assert.Equal(t, "abc123", out.AssetID)
+	assert.Equal(t, "https://example.com/portal/assets/abc123", out.PortalURL)
+	assert.True(t, out.ProvenanceCaptured)
+	assert.Equal(t, 1, out.ToolCallsRecorded)
+}
+
+func TestBuildSaveOutputNoBaseURL(t *testing.T) {
+	tk := New(Config{Name: "test", S3Bucket: "bucket"})
+	out := tk.buildSaveOutput("abc123", portal.Provenance{})
+
+	assert.Empty(t, out.PortalURL)
+	assert.False(t, out.ProvenanceCaptured)
+}
+
 func TestExtensionForContentType(t *testing.T) {
 	tests := []struct {
 		ct  string
@@ -444,7 +464,7 @@ func TestSaveArtifact_S3Error(t *testing.T) {
 
 func TestSaveArtifact_NoContext(t *testing.T) {
 	store := newInMemoryAssetStore()
-	tk := New(Config{Name: "test", AssetStore: store, S3Bucket: "bucket"})
+	tk := New(Config{Name: "test", AssetStore: store, S3Client: &mockS3Client{}, S3Bucket: "bucket"})
 
 	// Call without PlatformContext — should default to anonymous.
 	input := saveArtifactInput{
@@ -456,6 +476,25 @@ func TestSaveArtifact_NoContext(t *testing.T) {
 	assert.False(t, result.IsError)
 }
 
+func TestSaveArtifact_NilS3Client(t *testing.T) {
+	tk := New(Config{Name: "test", AssetStore: newInMemoryAssetStore(), S3Client: nil, S3Bucket: "bucket"})
+
+	ctx := middleware.WithPlatformContext(context.Background(), &middleware.PlatformContext{
+		UserID: "user1", SessionID: "sess1",
+	})
+
+	input := saveArtifactInput{
+		Name: "Test", Content: "<div/>", ContentType: "text/html",
+	}
+
+	result, _, err := tk.handleSaveArtifact(ctx, nil, input)
+	require.NoError(t, err)
+	assert.True(t, result.IsError)
+	tc, ok := result.Content[0].(*mcp.TextContent) //nolint:errcheck // test assertion
+	require.True(t, ok)
+	assert.Contains(t, tc.Text, "content storage not configured")
+}
+
 func TestManageArtifact_DeleteWrongOwner(t *testing.T) {
 	store := newInMemoryAssetStore()
 	_ = store.Insert(context.Background(), portal.Asset{
@@ -591,7 +630,7 @@ func (s *errorAssetStore) Update(_ context.Context, _ string, _ portal.AssetUpda
 func TestSaveArtifact_StoreInsertError(t *testing.T) {
 	store := &errorAssetStore{insertErr: notFoundError{}}
 	store.assets = make(map[string]portal.Asset)
-	tk := New(Config{Name: "test", AssetStore: store, S3Bucket: "bucket"})
+	tk := New(Config{Name: "test", AssetStore: store, S3Client: &mockS3Client{}, S3Bucket: "bucket"})
 
 	ctx := middleware.WithPlatformContext(context.Background(), &middleware.PlatformContext{
 		UserID: "user1", SessionID: "sess1",
@@ -647,6 +686,27 @@ func TestManageArtifact_UpdateStoreError(t *testing.T) {
 	assert.Contains(t, tc.Text, "failed to update asset")
 }
 
+func TestManageArtifact_UpdateNilS3Client(t *testing.T) {
+	store := newInMemoryAssetStore()
+	_ = store.Insert(context.Background(), portal.Asset{
+		ID: "a1", OwnerID: "user1", Name: "Test", ContentType: "text/html",
+		Tags: []string{}, Provenance: portal.Provenance{},
+	})
+
+	tk := New(Config{Name: "test", AssetStore: store, S3Client: nil, S3Bucket: "bucket"})
+
+	ctx := middleware.WithPlatformContext(context.Background(), &middleware.PlatformContext{UserID: "user1"})
+
+	result, _, err := tk.handleManageArtifact(ctx, nil, manageArtifactInput{
+		Action: "update", AssetID: "a1", Content: "<div>Updated</div>",
+	})
+	require.NoError(t, err)
+	assert.True(t, result.IsError)
+	tc, ok := result.Content[0].(*mcp.TextContent) //nolint:errcheck // test assertion
+	require.True(t, ok)
+	assert.Contains(t, tc.Text, "content storage not configured")
+}
+
 func TestManageArtifact_UpdateWithContentError(t *testing.T) {
 	store := newInMemoryAssetStore()
 	_ = store.Insert(context.Background(), portal.Asset{
