-
Notifications
You must be signed in to change notification settings - Fork 10
Unify mission move across backend CLI and UI with project migration support #2124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 6 commits
a1ac96d
fcbbd69
0b0ba94
4972d6f
b1a7209
968e2cc
42dc87e
7fb32d0
01b3610
99374ac
26d1967
1d99bf6
9fbc0fb
b86af82
bd488a7
06238ae
7dec044
2cd2dc6
4f4f741
7aa5b2b
a056685
bb2c3f1
3529d9e
fd2a843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,8 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DeleteProjectGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DeleteTagGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LoggedInUserGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MigrateMissionByBodyGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MigrateProjectByBodyGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MoveFilesGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MoveMissionToProjectGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ReadActionGuard, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -143,6 +145,19 @@ export function CanDeleteProject() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function CanMigrateProjectByBody() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return applyDecorators( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SetMetadata('CanMigrateProjectByBody', true), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UseGuards(MigrateProjectByBodyGuard), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ApiResponse({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: 401, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: UnauthorizedExceptionDto, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'User does not have permissions to migrate between the specified projects.', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+147
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ApiResponse({ status: 401, ... })
// 403 is never declaredThe parallel decorator
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/src/endpoints/auth/roles.decorator.ts
Line: 148-159
Comment:
`CanMigrateProjectByBody` decorator is missing HTTP 403 documentation.
`MigrateProjectByBodyGuard.canActivate` can return `false` when `canAccessProject` evaluates to `false` for either the source or target project, which NestJS translates to a **403 Forbidden** response. However, the decorator only documents the 401 case:
```ts
ApiResponse({ status: 401, ... })
// 403 is never declared
```
The parallel decorator `CanMigrateMissionByBody` (lines 213-229) correctly documents both 401 and 403. Clients and generated SDK consumers relying on the Swagger spec won't know to handle a 403 response on `POST /projects/migrate`.
```suggestion
export function CanMigrateProjectByBody() {
return applyDecorators(
SetMetadata('CanMigrateProjectByBody', true),
UseGuards(MigrateProjectByBodyGuard),
ApiResponse({
status: 401,
type: UnauthorizedExceptionDto,
description:
'User does not have permissions to migrate between the specified projects.',
}),
ApiResponse({
status: 403,
type: ForbiddenException,
description:
'User does not have sufficient access rights on the source or target project.',
}),
);
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function CanCreate() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return applyDecorators( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SetMetadata('CanCreate', true), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -195,6 +210,19 @@ export function CanMoveMission() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function CanMigrateMissionByBody() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return applyDecorators( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SetMetadata('CanMigrateMissionByBody', true), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UseGuards(MigrateMissionByBodyGuard), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ApiResponse({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: 403, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: ForbiddenException, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'User does not have permissions to migrate the specified mission.', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}But
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/src/endpoints/auth/roles.decorator.ts
Line: 213-224
Comment:
`CanMigrateMissionByBody` missing HTTP 401 documentation
`MigrateMissionByBodyGuard.canActivate` throws `UnauthorizedException` (HTTP 401) when an API key is detected:
```ts
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}
```
But `CanMigrateMissionByBody` only documents status 403, not 401. This is inconsistent with the parallel `CanMigrateProjectByBody` decorator (line 148), which correctly documents 401. Clients or generated SDK consumers relying on the Swagger spec won't know to handle a 401 response.
```suggestion
export function CanMigrateMissionByBody() {
return applyDecorators(
SetMetadata('CanMigrateMissionByBody', true),
UseGuards(MigrateMissionByBodyGuard),
ApiResponse({
status: 401,
type: UnauthorizedExceptionDto,
description:
'User does not have permissions to migrate the specified mission.',
}),
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function CanReadFile() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return applyDecorators( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SetMetadata('CanReadFile', true), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ import { | |
| import { | ||
| CreateMission, | ||
| FlatMissionDto, | ||
| MigrateMissionDto, | ||
| MigrateMissionResponseDto, | ||
| MinimumMissionsDto, | ||
| MissionsDto, | ||
| MissionWithFilesDto, | ||
|
|
@@ -21,6 +23,7 @@ import { ParameterUuid as ParameterUID } from '../../validation/parameter-decora | |
| import { | ||
| CanCreateInProjectByBody, | ||
| CanDeleteMission, | ||
| CanMigrateMissionByBody, | ||
| CanMoveMission, | ||
| CanReadMission, | ||
| CanWriteMissionByBody, | ||
|
|
@@ -172,6 +175,27 @@ export class MissionController { | |
| return this.missionService.moveMission(missionUUID, projectUUID); | ||
| } | ||
|
|
||
| @Post('migrate') | ||
| @CanMigrateMissionByBody() | ||
| @ApiOkResponse({ | ||
| description: 'Mission migrated to another project', | ||
| type: MigrateMissionResponseDto, | ||
| }) | ||
| async migrateMission( | ||
| @Body() dto: MigrateMissionDto, | ||
| ): Promise<MigrateMissionResponseDto> { | ||
| await this.missionService.migrateMission( | ||
| dto.missionUUID, | ||
| dto.targetProjectUUID, | ||
| dto.newName, | ||
| ); | ||
| return { | ||
| success: true, | ||
| missionUUID: dto.missionUUID, | ||
| targetProjectUUID: dto.targetProjectUUID, | ||
| }; | ||
|
Comment on lines
+177
to
+187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The service returns both return {
success: true,
movedMissionCount: result.movedMissionCount,
targetProjectUUID: dto.targetProjectUUID,
// movedMissionUUIDs: result.movedMissionUUIDs ← never forwarded
};
Add the field to both the DTO and the controller response: // move-missions.dto.ts
export class MoveMissionsResponseDto {
@ApiProperty()
@IsBoolean()
success!: boolean;
@ApiProperty()
movedMissionCount!: number;
@ApiProperty({ type: [String] })
@IsArray()
@IsUUID('4', { each: true })
movedMissionUUIDs!: string[];
@ApiProperty()
targetProjectUUID!: string;
}// controller
return {
success: true,
movedMissionCount: result.movedMissionCount,
movedMissionUUIDs: result.movedMissionUUIDs,
targetProjectUUID: dto.targetProjectUUID,
};Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/src/endpoints/mission/mission.controller.ts
Line: 176-185
Comment:
**`movedMissionUUIDs` silently dropped from response**
The service returns both `movedMissionCount` and `movedMissionUUIDs`, but the controller discards the UUIDs:
```ts
return {
success: true,
movedMissionCount: result.movedMissionCount,
targetProjectUUID: dto.targetProjectUUID,
// movedMissionUUIDs: result.movedMissionUUIDs ← never forwarded
};
```
`MoveMissionsResponseDto` doesn't declare the field either, so callers (CLI, frontend) cannot programmatically know which UUIDs were actually moved in a bulk operation. The parallel `MigrateProjectResponseDto` **does** expose `movedMissionUUIDs`, making this an API inconsistency.
Add the field to both the DTO and the controller response:
```ts
// move-missions.dto.ts
export class MoveMissionsResponseDto {
@ApiProperty()
@IsBoolean()
success!: boolean;
@ApiProperty()
movedMissionCount!: number;
@ApiProperty({ type: [String] })
@IsArray()
@IsUUID('4', { each: true })
movedMissionUUIDs!: string[];
@ApiProperty()
targetProjectUUID!: string;
}
```
```ts
// controller
return {
success: true,
movedMissionCount: result.movedMissionCount,
movedMissionUUIDs: result.movedMissionUUIDs,
targetProjectUUID: dto.targetProjectUUID,
};
```
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
|
|
||
| @Delete(':uuid') | ||
| @CanDeleteMission() | ||
| @OutputDto(null) // TODO: type API response | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent validation order between project and mission migration guards
MigrateProjectByBodyGuardchecks API key authorization before validating the request body UUIDs:But
MigrateMissionByBodyGuard(same PR) validates body first, then checks API key:This causes an API-key caller with invalid/missing UUIDs to receive:
For consistency and better security posture (avoid leaking "this endpoint blocks API keys" before validation), validate request body fields before auth shortcuts. Recommend reordering to match
MigrateMissionByBodyGuardpattern: body validation → apiKey check → permission checks.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks — good catch on consistency. I aligned
MigrateProjectByBodyGuardwith the mission migration guard in42dc87eb: body UUID presence/format is validated first, then API-key rejection is applied.