working with Threat dragon 2 #123
Conversation
vishalguptahmh
force-pushed
the
supportForTMV2
branch
3 times, most recently
from
694d9d7
to
6f6736f
Compare
apps/server/src/endpoints.ts
Outdated
| } | ||
|
|
||
| case ModelType.THREAT_DRAGON_V2: { | ||
| // TODO: validation |
There was a problem hiding this comment.
Does Threat Dragon provide a lib for validation?
There was a problem hiding this comment.
There is a json schema. We could do validation based on that. Personally, I would prefer to convert that to a zod schema, though, to force the TypeScript types and the schema to be in sync.
| logEvent('vishal printing body'); | ||
| logEvent(`printing body: ${body.model}`); | ||
| var model2 = JSON.parse(body.model as string) as ThreatDragonModel2; | ||
| var model = mapModel2toOldModel(model2); |
There was a problem hiding this comment.
Do I understand it correctly: The Input is a V2 model, but internally it gets mapped to a V1 model? This would mean that the model that can be downloaded is also V1? For the users this might be every unintuitive...
There was a problem hiding this comment.
I tend to agree. I know it's probably a decent amount of work, but I would prefer to actually support V2 properly, which likely means completely switching out the rendering logic of the diagrams. We might also have to deal with the fact that a threat dragon model can contain multiple diagrams. I'm not sure how this is handled right now (if it is at all).
But maybe this could be an interim solution, to unblock users who want to load a v2 model into EoP, until we have a proper solution?
There was a problem hiding this comment.
Ok. But if we use this as an interim solution, we should be transparent and rename the download button. Maybe add "(Threat Dragon V1)" tiny underneath the "Download Threats"?
There was a problem hiding this comment.
Yes @ChristophNiehoff i have mapped v2 model to v1 as workaround.
There is new diagram in V2 (box) which is handled. I am working on proper solution on v2.
| @@ -511,6 +512,47 @@ class Create extends React.Component<CreateProps, CreateState> { | |||
| to try it out. | |||
| </FormText> | |||
| </FormGroup> | |||
| <FormGroup> | |||
There was a problem hiding this comment.
Does one need a dedicated form group for V2? Or could one validate the uploaded model and set a ModelType according to that?
|
@vishalguptahmh Thank you very much for your work! This highly appreciated! However, I wrote down a few comments. Especially the internal conversion of V2 to V1 is something we would need to discuss. Thanks a lot! |
vishalguptahmh
force-pushed
the
supportForTMV2
branch
from
6f6736f
to
d2e344b
Compare
Signed-off-by: Vishal Gupta <[email protected]>
vishalguptahmh
force-pushed
the
supportForTMV2
branch
from
d2e344b
to
6f06bef
Compare
There was a problem hiding this comment.
@vishalguptahmh Thank you very much for your changes!
Unfortunately, I could not get this running with the simple example model. Could you please have a look at this and add test cases to ensure a reliable support for v2?
| matchID, | ||
| JSON.parse(body.model as string) as ThreatDragonModel, | ||
| ); | ||
| logEvent(`printing body: ${body.model}`); |
There was a problem hiding this comment.
This log statement should be removed. The model can potentially contain sensitive data that MUST NOT end up in the logs.
| @@ -127,3 +129,190 @@ export function logEvent(message: string): void { | |||
| export function isModelType(value: string): value is ModelType { | |||
| return Object.values<string>(ModelType).includes(value); | |||
| } | |||
|
|
|||
| export function mapModel2toOldModel(model2: ThreatDragonModel2): ThreatDragonModel { | |||
There was a problem hiding this comment.
When I start the application in a docker setup, download the V2 example model and try to start a game, I get an internal server error originating from this function:
threats-server | TypeError: Cannot read properties of undefined (reading 'description')
threats-server | at mapModel2toOldModel (/app/packages/shared/dist/utils/utils.js:106:41)
threats-server | at /app/apps/server/build/endpoints.js:57:62
threats-server | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
threats-server | at async cors (/app/node_modules/@koa/cors/index.js:109:16) TypeError: Cannot read properties of undefined (reading 'description')
threats-server | at mapModel2toOldModel (/app/packages/shared/dist/utils/utils.js:106:41)
threats-server | at /app/apps/server/build/endpoints.js:57:62
threats-server | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
threats-server | at async cors (/app/node_modules/@koa/cors/index.js:109:16)
threats-server |
threats-server | InternalServerError: Internal Server Error
threats-server | at Object.throw (/app/node_modules/koa/lib/context.js:97:11)
threats-server | at /app/apps/server/build/endpoints.js:102:18
threats-server | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
threats-server | at async cors (/app/node_modules/@koa/cors/index.js:109:16)
This is probably a strong indicator that tests are missing for this function.
No description provided.