Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,11 @@ follow [Semantic Versioning](https://semver.org). Ongoing work is collected unde
succeeded and returned an empty list. The callers who hit this were the ones with no reason to
name a project in the first place — a REST API key, which is confined to a single project, and
integrations driving `/api/*` directly — so a service could ask for its traces, agents, suites,
runs or evaluators and be told, with a perfectly successful response, that there were none. An
unfiltered request is now answered with the caller's own projects, and a caller who belongs to
several gets all of them as one correctly paged list. The web app was never affected, because it
always names the current project.
runs, evaluators or optimization proposals and be told, with a perfectly successful response, that
there were none. An unfiltered request is now answered with the caller's own projects, and a
caller who belongs to several gets all of them as one correctly paged list. This now also covers
the two lists that were still left out: individual test runs and optimization proposals. The web
app was never affected, because it always names the current project.

- **The Agent Playground no longer asks for an agent that is gone.** The playground remembers which
agent you had selected, and it kept asking the server for that agent even after it had been
Expand Down
108 changes: 106 additions & 2 deletions Proxytrace.Api.Tests/ProposalsControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,106 @@ public async Task Get_ExistingProposal_ReturnsDto()
dto.AgentId.Should().Be(proposal.Agent.Id);
}

[TestMethod]
public async Task GetAll_AsNonAdminWithoutFilter_ReturnsOwnProjectsProposalsOnly()
{
// #482: an unfiltered list from a non-admin used to short-circuit to an empty list, so a
// REST API key — confined to one project, and with no reason to send a filter — was told its
// own project had no proposals.
IServiceProvider services = GetServices();
var mine = await SeedProposalInNewProjectAsync(services, "mine");
await SeedProposalInNewProjectAsync(services, "theirs");

var controller = ResolveController(services, ScopedGuard(mine.Agent.Project.Id));
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Should().ContainSingle().Which.Id.Should().Be(mine.Id);
}

[TestMethod]
public async Task GetAll_AsNonAdminInSeveralProjectsWithoutFilter_ReturnsTheUnion()
{
IServiceProvider services = GetServices();
var first = await SeedProposalInNewProjectAsync(services, "first");
var second = await SeedProposalInNewProjectAsync(services, "second");
await SeedProposalInNewProjectAsync(services, "outsider");

var controller = ResolveController(
services, ScopedGuard(first.Agent.Project.Id, second.Agent.Project.Id));
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Select(p => p.Id).Should().BeEquivalentTo([first.Id, second.Id]);
}

[TestMethod]
public async Task GetAll_AsNonAdminWithoutAccessibleProjects_ReturnsEmpty()
{
IServiceProvider services = GetServices();
await SeedProposalInNewProjectAsync(services, "theirs");

var controller = ResolveController(services, ScopedGuard());
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Should().BeEmpty();
}

[TestMethod]
public async Task GetAll_AsNonAdminFilteredByInaccessibleProject_ReturnsEmpty()
{
IServiceProvider services = GetServices();
var mine = await SeedProposalInNewProjectAsync(services, "mine");
var theirs = await SeedProposalInNewProjectAsync(services, "theirs");

var controller = ResolveController(services, ScopedGuard(mine.Agent.Project.Id));
var result = await controller.GetAll(projectId: theirs.Agent.Project.Id, cancellationToken: CancellationToken);

result.Should().BeEmpty();
}

// A proposal whose agent lives in a freshly created project, so the project id is unique to it
// and can be handed to ScopedGuard.
private async Task<IOptimizationProposal> SeedProposalInNewProjectAsync(
IServiceProvider services, string name)
{
var endpoint = await services.GetRequiredService<IDomainEntityGenerator<IModelEndpoint>>()
.GetOrCreateAsync(CancellationToken);
var project = await services.GetRequiredService<Proxytrace.Domain.Project.IProjectRepository>().AddAsync(
services.GetRequiredService<Proxytrace.Domain.Project.IProject.CreateNew>()(
$"P-{name}-{Guid.NewGuid():N}", endpoint, []),
CancellationToken);

var agent = await services.GetRequiredService<IAgentRepository>().AddAsync(
services.GetRequiredService<IAgent.CreateNew>()(
$"A-{name}",
services.GetRequiredService<Proxytrace.Domain.Prompt.IPromptTemplate.Create>()(
$"T-{name}", "You are a test agent."),
[],
endpoint,
project,
services.GetRequiredService<Proxytrace.Domain.Inference.IModelParameters.Create>()(
null, null, null, null, null)),
CancellationToken);

var abRun = await services.GetRequiredService<IDomainEntityGenerator<Domain.TestRun.ITestRun>>()
.CreateAsync(CancellationToken);
return await services.GetRequiredService<IOptimizationProposalRepository>().AddAsync(
services.GetRequiredService<ISystemPromptProposal.CreateNew>()(
agent, Priority.Medium, "r", $"proposed-{name}", null, null, [], abRun),
CancellationToken);
}

// A non-admin scoped to a specific set of projects: the scope set is non-null (not admin) and
// contains exactly those projects. No arguments means a member of nothing.
private static Proxytrace.Api.Auth.IProjectAccessGuard ScopedGuard(params Guid[] projectIds)
{
var guard = Substitute.For<Proxytrace.Api.Auth.IProjectAccessGuard>();
guard.CanAccessProjectAsync(Arg.Any<Guid>(), Arg.Any<CancellationToken>())
.Returns(ci => projectIds.Contains(ci.Arg<Guid>()));
guard.GetAccessibleProjectIdsAsync(Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyCollection<Guid>?>(projectIds));
return guard;
}

private async Task<IOptimizationProposal> SeedSystemPromptProposalAsync(
IServiceProvider services, string proposedPrompt = "proposed")
{
Expand All @@ -324,7 +424,11 @@ private async Task<IOptimizationProposal> SeedSystemPromptProposalAsync(
CancellationToken);
}

private static ProposalsController ResolveController(IServiceProvider services) => new(
private static ProposalsController ResolveController(IServiceProvider services)
=> ResolveController(services, services.GetRequiredService<Proxytrace.Api.Auth.IProjectAccessGuard>());

private static ProposalsController ResolveController(
IServiceProvider services, Proxytrace.Api.Auth.IProjectAccessGuard guard) => new(
services.GetRequiredService<IOptimizationProposalRepository>(),
services.GetRequiredService<IModelSwitchProposal.CreateNew>(),
services.GetRequiredService<ISystemPromptProposal.CreateNew>(),
Expand All @@ -340,5 +444,5 @@ private async Task<IOptimizationProposal> SeedSystemPromptProposalAsync(
services.GetRequiredService<OptimizationProposalDtoMapper>(),
services.GetRequiredService<IProposalBroadcaster>(),
Microsoft.Extensions.Logging.Abstractions.NullLogger<Proxytrace.Domain.AuditLog.Audit>.Instance,
services.GetRequiredService<Proxytrace.Api.Auth.IProjectAccessGuard>());
guard);
}
108 changes: 106 additions & 2 deletions Proxytrace.Api.Tests/TestRunsControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@
using Proxytrace.Api.Controllers;
using Proxytrace.Api.Dto.TestRuns;
using Proxytrace.Application.Streaming;
using NSubstitute;
using Proxytrace.Domain;
using Proxytrace.Domain.Agent;
using Proxytrace.Domain.Inference;
using Proxytrace.Domain.ModelEndpoint;
using Proxytrace.Domain.Project;
using Proxytrace.Domain.Prompt;
using Proxytrace.Domain.TestRun;
using Proxytrace.Domain.TestRunGroup;
using Proxytrace.Domain.TestSuite;
using Proxytrace.Testing;

namespace Proxytrace.Api.Tests;
Expand Down Expand Up @@ -160,11 +167,108 @@ public async Task Stream_UnknownRun_Returns404()
controller.Response.StatusCode.Should().Be(404);
}

private static TestRunsController ResolveController(IServiceProvider services) => new(
[TestMethod]
public async Task GetAll_AsNonAdminWithoutAgentFilter_ReturnsOwnProjectsRunsOnly()
{
// #482: an unfiltered list from a non-admin used to short-circuit to an empty page, so a
// REST API key — confined to one project, and with no reason to send an agent filter — was
// told its own project had no runs.
IServiceProvider services = GetServices();
var mine = await SeedRunInNewProjectAsync(services, "mine");
await SeedRunInNewProjectAsync(services, "theirs");

var controller = ResolveController(services, ScopedGuard(mine.Group.Suite.Agent.Project.Id));
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Items.Should().ContainSingle().Which.Id.Should().Be(mine.Id);
result.Total.Should().Be(1);
}

[TestMethod]
public async Task GetAll_AsNonAdminInSeveralProjectsWithoutAgentFilter_ReturnsTheUnion()
{
IServiceProvider services = GetServices();
var first = await SeedRunInNewProjectAsync(services, "first");
var second = await SeedRunInNewProjectAsync(services, "second");
await SeedRunInNewProjectAsync(services, "outsider");

// A member of two projects: the page must be computed over the union of both, not one of
// them and not the whole instance.
var controller = ResolveController(
services,
ScopedGuard(first.Group.Suite.Agent.Project.Id, second.Group.Suite.Agent.Project.Id));
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Items.Select(i => i.Id).Should().BeEquivalentTo([first.Id, second.Id]);
result.Total.Should().Be(2);
}

[TestMethod]
public async Task GetAll_AsNonAdminWithoutAccessibleProjects_ReturnsEmpty()
{
IServiceProvider services = GetServices();
await SeedRunInNewProjectAsync(services, "theirs");

var controller = ResolveController(services, ScopedGuard());
var result = await controller.GetAll(cancellationToken: CancellationToken);

result.Items.Should().BeEmpty();
result.Total.Should().Be(0);
}

// A run whose whole chain (project → agent → suite → group → run) is freshly created, so the
// project id is unique to it and can be handed to ScopedGuard.
private async Task<ITestRun> SeedRunInNewProjectAsync(IServiceProvider services, string name)
{
var endpoint = await services.GetRequiredService<IDomainEntityGenerator<IModelEndpoint>>()
.GetOrCreateAsync(CancellationToken);
var project = await services.GetRequiredService<IProjectRepository>().AddAsync(
services.GetRequiredService<IProject.CreateNew>()($"P-{name}-{Guid.NewGuid():N}", endpoint, []),
CancellationToken);

var agent = await services.GetRequiredService<IAgentRepository>().AddAsync(
services.GetRequiredService<IAgent.CreateNew>()(
$"A-{name}",
services.GetRequiredService<IPromptTemplate.Create>()($"T-{name}", "You are a test agent."),
[],
endpoint,
project,
services.GetRequiredService<IModelParameters.Create>()(null, null, null, null, null)),
CancellationToken);

var suite = await services.GetRequiredService<IRepository<ITestSuite>>().AddAsync(
services.GetRequiredService<ITestSuite.CreateNew>()($"S-{name}", agent, [], []),
CancellationToken);
var group = await services.GetRequiredService<IRepository<ITestRunGroup>>().AddAsync(
services.GetRequiredService<ITestRunGroup.CreateNew>()(suite, isSystemRun: false, null, sampleCount: 1),
CancellationToken);

return await services.GetRequiredService<ITestRunRepository>().AddAsync(
services.GetRequiredService<ITestRun.CreateNew>()(group, endpoint, sampleIndex: 0),
CancellationToken);
}

// A non-admin scoped to a specific set of projects: the scope set is non-null (not admin) and
// contains exactly those projects. No arguments means a member of nothing.
private static Proxytrace.Api.Auth.IProjectAccessGuard ScopedGuard(params Guid[] projectIds)
{
var guard = Substitute.For<Proxytrace.Api.Auth.IProjectAccessGuard>();
guard.CanAccessProjectAsync(Arg.Any<Guid>(), Arg.Any<CancellationToken>())
.Returns(ci => projectIds.Contains(ci.Arg<Guid>()));
guard.GetAccessibleProjectIdsAsync(Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyCollection<Guid>?>(projectIds));
return guard;
}

private static TestRunsController ResolveController(IServiceProvider services)
=> ResolveController(services, services.GetRequiredService<Proxytrace.Api.Auth.IProjectAccessGuard>());

private static TestRunsController ResolveController(
IServiceProvider services, Proxytrace.Api.Auth.IProjectAccessGuard guard) => new(
services.GetRequiredService<ITestRunRepository>(),
services.GetRequiredService<IAgentRepository>(),
services.GetRequiredService<ITestResultBroadcaster>(),
services.GetRequiredService<TestRunDtoMapper>(),
services.GetRequiredService<Proxytrace.Api.Auth.IProjectAccessGuard>(),
guard,
NullLogger<Audit>.Instance);
}
34 changes: 18 additions & 16 deletions Proxytrace.Api/Controllers/ProposalsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,24 @@ public ProposalsController(
this.accessGuard = accessGuard;
}

// Resolve the effective owning project of a list query and verify access. Admins
// (accessible == null) pass for any scope. Non-admins must scope to a project they belong to —
// directly via projectId or via the agent's project — otherwise the query returns nothing rather
// than leaking other tenants' rows.
private async Task<bool> CanListAsync(Guid? agentId, Guid? projectId, CancellationToken cancellationToken)
// The projects this list request may read: null for an admin, the caller's own projects when no
// narrower filter is given (#482), and [] when a named agent/project is out of reach.
private async Task<IReadOnlyCollection<Guid>?> ListScopeAsync(
Guid? agentId,
Guid? projectId,
CancellationToken cancellationToken)
{
var accessible = await accessGuard.GetAccessibleProjectIdsAsync(cancellationToken);
if (accessible is null)
return true;
if (projectId is { } pid)
return accessible.Contains(pid);
var scope = await accessGuard.ResolveListScopeAsync(projectId, cancellationToken);
if (scope is null || scope.IsEmpty())
return scope;

if (agentId is { } aid)
{
var agent = await agents.FindAsync(aid, cancellationToken);
return agent is not null && accessible.Contains(agent.Project.Id);
return agent is not null && scope.Contains(agent.Project.Id) ? scope : [];
}
return false;

return scope;
}

[HttpGet]
Expand All @@ -103,16 +104,17 @@ public async Task<IReadOnlyList<OptimizationProposalDto>> GetAll(
[FromQuery] Guid? projectId = null,
CancellationToken cancellationToken = default)
{
if (!await CanListAsync(agentId, projectId, cancellationToken))
var scope = await ListScopeAsync(agentId, projectId, cancellationToken);
if (scope.IsEmpty())
return [];

IReadOnlyList<IOptimizationProposal> proposals;
if (agentId.HasValue)
proposals = await repository.GetByAgentAsync(agentId.Value, cancellationToken);
else if (projectId.HasValue)
proposals = await repository.GetByProjectAsync(projectId.Value, cancellationToken);
else
else if (scope is null)
proposals = await repository.GetAllAsync(cancellationToken);
else
proposals = await repository.GetByProjectsAsync(scope, cancellationToken);

return proposals.Select(mapper.ToDto).ToList();
}
Expand Down
11 changes: 8 additions & 3 deletions Proxytrace.Api/Controllers/TestRunsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,16 @@ public async Task<PagedResult<TestRunDto>> GetAll(
return pagedByAgent.Map(mapper.ToDto);
}

// No agent filter enumerates across all tenants — admins only; non-admins get nothing.
if (await accessGuard.GetAccessibleProjectIdsAsync(cancellationToken) is not null)
// No agent filter: the caller's own reach is the scope — every project for an admin (null),
// their memberships otherwise, so an unfiltered request answers with that caller's runs
// instead of an empty page (#482).
var scope = await accessGuard.ResolveListScopeAsync(requestedProjectId: null, cancellationToken);
if (scope.IsEmpty())
return new PagedResult<TestRunDto>([], 0, page, pageSize);

var paged = await repository.GetAllPagedAsync(page, pageSize, includeSystem, cancellationToken);
var paged = scope is null
? await repository.GetAllPagedAsync(page, pageSize, includeSystem, cancellationToken)
: await repository.GetByProjectsPagedAsync(scope, page, pageSize, includeSystem, cancellationToken);
return paged.Map(mapper.ToDto);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ Task<IReadOnlyList<IOptimizationProposal>> GetByProjectAsync(
Guid projectId,
CancellationToken cancellationToken = default);

/// <summary>
/// Proposals across several projects as one list, newest first. Backs an unfiltered list request
/// from a caller who may read more than one project (#482), so the set is applied in SQL rather
/// than by concatenating per-project reads.
/// </summary>
Task<IReadOnlyList<IOptimizationProposal>> GetByProjectsAsync(
IReadOnlyCollection<Guid> projectIds,
CancellationToken cancellationToken = default);

/// <summary>
/// Returns the most-recently-updated proposal for the given agent with the specified
/// <see cref="IOptimizationProposal.ContentHash"/>, or null if none exists.
Expand Down
13 changes: 13 additions & 0 deletions Proxytrace.Domain/TestRun/ITestRunRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,17 @@ Task<PagedResult<ITestRun>> GetAllPagedAsync(
int pageSize,
bool includeSystem = false,
CancellationToken cancellationToken = default);

/// <summary>
/// Runs across several projects, paged as one list, newest first. Backs an unfiltered list
/// request from a caller who may read more than one project: the page has to be computed over
/// the union, so merging per-project pages afterwards would not produce a correct page (#482).
/// <paramref name="includeSystem"/> behaves as in <see cref="GetAllPagedAsync"/>.
/// </summary>
Task<PagedResult<ITestRun>> GetByProjectsPagedAsync(
IReadOnlyCollection<Guid> projectIds,
int page,
int pageSize,
bool includeSystem = false,
CancellationToken cancellationToken = default);
}
Loading
Loading