fix(api): guard system playlists/groups + validate preview draft (#153 review)
Hardening from adversarial review of the #153 playlist API: - PUT /api/playlists/{id}: guard IsSystem in the controller after the existence pre-check -> 422, so a system (generated) playlist can no longer be renamed/wiped. ReplacePlaylistItems is never sent for it. - PUT /api/playlists/groups/{id}: add controller existence pre-check (404 for missing, mirroring DeleteGroup) plus an IsSystem 422 guard; RenamePlaylistGroupHandler also gains a system guard (defense-in-depth for the Blazor path). Missing/system are now distinct outcomes despite LanguageExtensions.Apply collapsing NotFoundError to a plain BaseError. - POST /api/playlists/preview: validate each draft item at the controller boundary (the id required for its collection type must be present) -> 422 before the shared PreviewPlaylistPlayoutHandler runs, preventing a NRE/500 in the playout builder. Logic lives in ReplacePlaylistRequest so it stays parallel with ReplacePlaylistItemsHandler's PUT-path check. Tests: controller cases for system-playlist PUT, system-group PUT, missing-group 404, and invalid-preview 422 (each asserting the handler is not invoked); handler tests for RenamePlaylistGroup system/missing/success. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -55,6 +55,27 @@ public class PlaylistController(IMediator mediator) : ControllerBase
|
||||
[Required] [FromBody] UpdatePlaylistGroupRequest request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
// Existence pre-check (404) mirrors DeleteGroup. Required controller-side because the
|
||||
// handler's NotFoundError is collapsed to a plain BaseError by LanguageExtensions.Apply
|
||||
// (error.Join()), so a missing group would otherwise map to 422 instead of 404.
|
||||
List<PlaylistGroupViewModel> groups = await mediator.Send(new GetAllPlaylistGroups(), cancellationToken);
|
||||
Option<PlaylistGroupViewModel> maybeGroup = groups.Find(g => g.Id == id);
|
||||
if (maybeGroup.IsNone)
|
||||
{
|
||||
return ApiResults.NotFoundProblem();
|
||||
}
|
||||
|
||||
// System (generated) groups must not be renamed. The RenamePlaylistGroupHandler also
|
||||
// enforces this (defense-in-depth for the Blazor path); catching it here keeps the API's
|
||||
// 422 independent of the handler's error-collapsing.
|
||||
foreach (PlaylistGroupViewModel group in maybeGroup)
|
||||
{
|
||||
if (group.IsSystem)
|
||||
{
|
||||
return BaseError.New("Cannot rename system playlist group").ToErrorResult();
|
||||
}
|
||||
}
|
||||
|
||||
Either<BaseError, PlaylistGroupViewModel> result =
|
||||
await mediator.Send(request.ToCommand(id), cancellationToken);
|
||||
return result.Match(
|
||||
@@ -162,6 +183,17 @@ public class PlaylistController(IMediator mediator) : ControllerBase
|
||||
return ApiResults.NotFoundProblem();
|
||||
}
|
||||
|
||||
// System (generated) playlists must not be renamed or have their items replaced.
|
||||
// Mirrors the DeletePlaylist system-guard (422); done controller-side so item
|
||||
// replacement never reaches ReplacePlaylistItemsHandler for a generated playlist.
|
||||
foreach (PlaylistViewModel playlist in maybePlaylist)
|
||||
{
|
||||
if (playlist.IsSystem)
|
||||
{
|
||||
return BaseError.New("Cannot modify system (generated) playlist").ToErrorResult();
|
||||
}
|
||||
}
|
||||
|
||||
Either<BaseError, List<PlaylistItemViewModel>> result =
|
||||
await mediator.Send(request.ToCommand(id), cancellationToken);
|
||||
return result.Match(
|
||||
@@ -201,6 +233,18 @@ public class PlaylistController(IMediator mediator) : ControllerBase
|
||||
[Required] [FromBody] ReplacePlaylistRequest request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
// The preview path does not run the handler's CollectionTypesMustBeValid check
|
||||
// (PreviewPlaylistPlayoutHandler is shared with Blazor and left unchanged), so
|
||||
// validate the draft at the controller boundary to avoid a 500 in the playout
|
||||
// builder on an item missing the id required for its collection type.
|
||||
List<int> invalidItems = request.ItemsMissingRequiredId();
|
||||
if (invalidItems.Count > 0)
|
||||
{
|
||||
return BaseError
|
||||
.New($"Invalid playlist item(s) at index(es): {string.Join(", ", invalidItems)}")
|
||||
.ToErrorResult();
|
||||
}
|
||||
|
||||
Either<BaseError, List<PlayoutItemPreviewViewModel>> result =
|
||||
await mediator.Send(new PreviewPlaylistPlayout(request.ToReplaceCommand()), cancellationToken);
|
||||
return result.Match(
|
||||
|
||||
@@ -30,6 +30,28 @@ public record PlaylistItemRequest(
|
||||
Count,
|
||||
PlayAll,
|
||||
IncludeInProgramGuide);
|
||||
|
||||
// Mirrors ReplacePlaylistItemsHandler.CollectionTypeMustBeValid: the id required for the
|
||||
// item's collection type must be present. Kept in sync with that handler so the PUT and
|
||||
// preview endpoints enforce the same rule (the handler operates on the command type in the
|
||||
// Application layer and cannot reference this request DTO, hence the parallel logic).
|
||||
public bool HasRequiredId() =>
|
||||
CollectionType switch
|
||||
{
|
||||
CollectionType.Collection => CollectionId is not null,
|
||||
CollectionType.MultiCollection => MultiCollectionId is not null,
|
||||
CollectionType.SmartCollection => SmartCollectionId is not null,
|
||||
CollectionType.TelevisionShow
|
||||
or CollectionType.TelevisionSeason
|
||||
or CollectionType.Artist
|
||||
or CollectionType.Movie
|
||||
or CollectionType.Episode
|
||||
or CollectionType.MusicVideo
|
||||
or CollectionType.OtherVideo
|
||||
or CollectionType.Song
|
||||
or CollectionType.Image => MediaItemId is not null,
|
||||
_ => false
|
||||
};
|
||||
}
|
||||
|
||||
public record ReplacePlaylistRequest(string? Name, List<PlaylistItemRequest>? Items)
|
||||
@@ -41,6 +63,22 @@ public record ReplacePlaylistRequest(string? Name, List<PlaylistItemRequest>? It
|
||||
public ReplacePlaylistItems ToReplaceCommand() =>
|
||||
new(0, Name ?? string.Empty, BuildItems());
|
||||
|
||||
// Array indexes of draft items missing the id required for their collection type.
|
||||
public List<int> ItemsMissingRequiredId()
|
||||
{
|
||||
List<PlaylistItemRequest> items = Items ?? new List<PlaylistItemRequest>();
|
||||
var invalid = new List<int>();
|
||||
for (int i = 0; i < items.Count; i++)
|
||||
{
|
||||
if (!items[i].HasRequiredId())
|
||||
{
|
||||
invalid.Add(i);
|
||||
}
|
||||
}
|
||||
|
||||
return invalid;
|
||||
}
|
||||
|
||||
private List<ReplacePlaylistItem> BuildItems() =>
|
||||
(Items ?? new List<PlaylistItemRequest>())
|
||||
.Select((item, index) => item.ToReplaceCommand(index))
|
||||
|
||||
Reference in New Issue
Block a user