Feature/272 manual step api implementation #275

lucamrgs · 2024-12-04T15:53:59Z

No description provided.

github-actions · 2024-12-04T15:54:58Z

Sigrid maintainability feedback

✅ You wrote maintainable code and achieved your objective of 3.8 stars

Show details

Sigrid compared your code against the baseline of 2025-01-16.

👍 What went well?

You fixed or improved 0 refactoring candidates.

👎 What could be better?

Unfortunately, 21 refactoring candidates were introduced or got worse.

Risk	System property	Location
🔴	Duplication (Introduced)	pkg/models/cacao/cacao.go (lines 282-294) pkg/models/decoder/decoder.go (lines 50-62)
🔴	Duplication (Introduced)	pkg/core/capability/fin/controller/controller.go (lines 104-110) pkg/core/capability/fin/controller/controller.go (lines 132-138)
🟠	Unit Size (Worsened)	internal/controller/controller.go initializeCore(app *gin.Engine)
🟠	Unit Size (Introduced)	pkg/api/manual/manual_api.go ManualHandler.PostContinue(g *gin.Context)
🟡	Unit Size (Introduced)	pkg/api/manual/manual_api.go ManualHandler.parseCommandInfoToResponse(commandInfo manual.CommandInfo)
🟡	Unit Size (Introduced)	pkg/api/manual/manual_api.go ManualHandler.GetPendingCommands(g *gin.Context)
🟡	Unit Size (Introduced)	pkg/api/manual/manual_api.go ManualHandler.parseManualResponseToInteractionResponse(response api.ManualOutArgsUpdatePayload)
🟡	Unit Size (Introduced)	pkg/api/manual/manual_api.go ManualHandler.GetPendingCommand(g *gin.Context)
⚫️		+ 13 more

📚 Remaining technical debt

9 refactoring candidates didn't get better or worse, but are still present in the code you touched.

View this system in Sigrid** to explore your technical debt

⭐️ Sigrid ratings

System property	System on 2025-01-16	Before changes	New/changed code
Volume	5.4	N/A	N/A
Duplication	4.4	5.1	4.9
Unit Size	2.6	2.8	2.5
Unit Complexity	3.0	2.4	2.9
Unit Interfacing	3.1	5.1	5.3
Module Coupling	3.8	5.5	5.5
Component Independence	0.5	N/A	N/A
Component Entanglement	2.5	N/A	N/A
Maintainability	3.2	4.0	4.1

💬 Did you find this feedback helpful?

We would like to know your thoughts to make Sigrid better.
Your username will remain confidential throughout the process.

✅ Yes, these findings are useful
🔸 The findings are false positives
🔹 These findings are not important to me

View this system in Sigrid

lucamrgs · 2024-12-04T16:11:39Z

Documentation needs to be adapted to these changes

lucamrgs · 2024-12-20T11:16:36Z

Thanks @MaartendeKruijf for rebasing in the middle of my edits on the draft pull request

lucamrgs · 2024-12-20T16:00:34Z

I think we need to fix a couple things trough the way that out args would be passed down to the manual command capability

lucamrgs · 2024-12-23T13:17:17Z

Next on todo is connect manual capability / interaction objects to initialisation controller, and test the APIs

MaartendeKruijf · 2025-01-10T09:16:15Z

docs/content/en/docs/core-components/api-manual.md

@@ -153,8 +154,8 @@ None
 404/Not found with payload:
 General error

-#### POST `/manual/continue`
-Respond to manual command pending in SOARCA, if out_args are defined they must be filled in and returned in the payload body. Only value is required in the response of the variable. You can however return the entire object. Of the object does not match the original out_arg the call we be considered as failed.
+#### PATCH `/manual/<execution-id>/<step-id>`


Why is this changed to PATCH and has a different route

MaartendeKruijf · 2025-01-16T12:18:05Z

docs/content/en/docs/core-components/api-manual.md

@@ -45,7 +46,7 @@ None
 |step_id            |UUID                   |string             |The id of the step executed by the execution
 |description        |description of the step|string             |The description from the workflow step
 |command            |command                |string             |The command for the agent either command 
-|command_is_base64  |true \| false          |bool               |Indicate the command is in base 64


Revert this to the bool

MaartendeKruijf · 2025-01-16T12:18:26Z

docs/content/en/docs/core-components/api-manual.md

@@ -45,7 +46,7 @@ None
 |step_id            |UUID                   |string             |The id of the step executed by the execution
 |description        |description of the step|string             |The description from the workflow step
 |command            |command                |string             |The command for the agent either command 
-|command_is_base64  |true \| false          |bool               |Indicate the command is in base 64


Revert this to the bool

MaartendeKruijf · 2025-01-16T12:19:35Z

docs/content/en/docs/core-components/api-manual.md

-|response_status    |enum                   |string             |Can be either `success` or `failed`
-|response_out_args  |cacao variables        |dictionary         |Map of [cacao variables](https://docs.oasis-open.org/cacao/security-playbooks/v2.0/cs01/security-playbooks-v2.0-cs01.html#_Toc152256555) handled in the step out args with current values and definitions
-
+|response_status    |true / false           |string             |`true` indicates successfull fulfilment of the manual request. `false` indicates failed satisfaction of request


MaartendeKruijf · 2025-01-16T12:25:22Z

docs/content/en/docs/core-components/modules.md

+control "ThirdPartyManualIntegration" as 3ptool
+
+
+manual -> interaction : Queue(command, capabilityChannel)


maybe move the native implementation sequence to be the first

MaartendeKruijf · 2025-01-16T12:55:42Z

pkg/core/capability/manual/interaction/interaction.go

+
+	metadata, err := manualController.makeExecutionMetadataFromPayload(result)
+	if err != nil {
+		return http.StatusBadRequest, err


remove the http details from here check for it in the api

I added some custom errors in the manual model, and now checking for those types on the API side. Let me know what you think about it this way.

MaartendeKruijf · 2025-01-16T12:58:45Z

pkg/core/capability/manual/interaction/interaction.go

+		// then warn if any value outside "value" has changed
+		if pending, ok := pendingEntry.CommandData.OutVariables[varName]; ok {
+			if variable.Constant != pending.Constant {
+				log.Warningf("provided out arg %s is attempting to change 'Constant' property", varName)


also include into the message the attempted action is not effectuated

MaartendeKruijf · 2025-01-16T13:08:16Z

pkg/core/capability/manual/interaction/interaction.go

+			// Check register for pending manual command
+			metadata, err := manualController.makeExecutionMetadataFromPayload(result.Payload)
+			if err != nil {
+				log.Error(err)
+				manualComms.Channel <- manual.InteractionResponse{
+					ResponseError: err,
+					Payload:       cacao.Variables{},
+				}
+				return
+			}
+			// Remove interaction from pending ones
+			err = manualController.removeInteractionFromPending(metadata)
+			if err != nil {
+				// If it was not there, was already resolved
+				log.Warning(err)
+				// Captured if channel not yet closed
+				log.Warning("manual command not found among pending ones. should be already resolved")
+				manualComms.Channel <- manual.InteractionResponse{
+					ResponseError: err,
+					Payload:       cacao.Variables{},
+				}
+				return
+			}
+
+			// Copy result and conversion back to interactionResponse format
+			returnedVars := manualController.copyOutArgsToVars(result.Payload.ResponseOutArgs)
+
+			interactionResponse := manual.InteractionResponse{
+				ResponseError: result.ResponseError,
+				Payload:       returnedVars,
+			}
+
+			manualComms.Channel <- interactionResponse
+			return


Move to separate function

MaartendeKruijf · 2025-01-16T13:12:02Z

pkg/core/capability/manual/interaction/interaction.go

+	for {
+		select {
+		case <-manualComms.TimeoutContext.Done():
+			log.Info("context canceled due to response or timeout. exiting goroutine")


call removeInteractionFromPending

MaartendeKruijf · 2025-01-16T14:30:51Z

pkg/models/manual/manual.go

+// ################################################################################
+// Data structures for native SOARCA manual command handling
+// ################################################################################
+
+// Object stored in interaction storage and provided back from the API


Move the api models to the api folder

MaartendeKruijf · 2025-01-16T14:50:03Z

docs/content/en/docs/core-components/modules.md

+3ptool --> interaction : integrationChannel <- InteractionIntegrationResponse
+interaction --> manual : capabilityChannel <- InteractionResponse


Merge intergration and interaction channel into one

…face

lucamrgs linked an issue Dec 4, 2024 that may be closed by this pull request

manual step api implementation #272

Open

MaartendeKruijf force-pushed the feature/272-manual-step-api-implementation branch 2 times, most recently from b077d25 to bc0953a Compare December 11, 2024 16:03

MaartendeKruijf force-pushed the feature/272-manual-step-api-implementation branch from a2429cf to b84ef03 Compare December 20, 2024 10:31

lucamrgs marked this pull request as ready for review December 23, 2024 13:14

lucamrgs requested review from MaartendeKruijf and RabbITCybErSeC December 23, 2024 13:14

lucamrgs marked this pull request as draft December 23, 2024 13:16

lucamrgs marked this pull request as ready for review January 10, 2025 08:58

MaartendeKruijf requested changes Jan 16, 2025

View reviewed changes

lucamrgs marked this pull request as draft January 17, 2025 16:29

lucamrgs and others added 13 commits January 20, 2025 14:50

restructured capability controllers

13f8528

fix test import

04f02bb

preset manual api components

0a39691

update documentation to match data structs

175bc9e

define first interfaces for manual interaction mechanisms

3be94ec

fix lint

2481f88

fix lint

49aa0a2

update manual docs

9bf58b7

update architecture and related docs

65bd405

update manual flows description

a6c55cb

Updates according to our discussion M&L

ba5d7c7

progress setup manual api mechanisms

c9af7e1

more setup progress and changes to types

18d3d09

lucamrgs added 19 commits January 20, 2025 14:50

add manual api calls testing

478ebd8

update documentation and terminology to latest code status

ac783e3

update tests

995813c

update tests

eb4cb9e

revert to postcontinue instead of patch on ids

0297f28

update documentation with more explicit info

308e263

change manual models but tests are now broken

e418cdd

fix types in manual api but tests still broken

3d1e02d

fix tests after models change

9335811

revert API base64 and responsestatus types

4ec59da

change interaction object warnings on unauthorized arg properties edits

c1b2b2b

refactor interaction to simpler architecture

3463d27

started to fix tests again but more to do

26cc378

fix tests

e03fed4

fix lint

e09a7cd

onelined manualhandler init in manual api

84bddc9

fix lint

d1bf8b2

improve clarity get command info interaction functions

d404d3c

implement error typing and api check for manual interaction storage i…

5a4f8af

…face

lucamrgs force-pushed the feature/272-manual-step-api-implementation branch from d7fafe1 to 5a4f8af Compare January 20, 2025 13:50

slight improvement unit interface

800c527

lucamrgs marked this pull request as ready for review January 20, 2025 14:01

lucamrgs requested a review from MaartendeKruijf January 20, 2025 14:02

lucamrgs added 7 commits January 20, 2025 15:09

increase timeouts in testqueueexitontimeout so git tests dont fail maybe

6a49912

loop on entries to check testqueueexitontimeout should fix test

14383f0

fix test

8bc5d10

correct manual documentation and playbook example

699577b

improve manual command flow diagram

d249b56

add timeout in test for sync in async test execution

f1e70ac

reduce time sleep values in tests

19562e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/272 manual step api implementation #275

Feature/272 manual step api implementation #275

lucamrgs commented Dec 4, 2024

github-actions bot commented Dec 4, 2024 •

edited

Loading

👍 What went well?

👎 What could be better?

📚 Remaining technical debt

⭐️ Sigrid ratings

💬 Did you find this feedback helpful?

lucamrgs commented Dec 4, 2024

lucamrgs commented Dec 20, 2024

lucamrgs commented Dec 20, 2024

lucamrgs commented Dec 23, 2024

MaartendeKruijf Jan 10, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

lucamrgs Jan 20, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

MaartendeKruijf Jan 16, 2025

		control "ThirdPartyManualIntegration" as 3ptool


		manual -> interaction : Queue(command, capabilityChannel)

		3ptool --> interaction : integrationChannel <- InteractionIntegrationResponse
		interaction --> manual : capabilityChannel <- InteractionResponse

Feature/272 manual step api implementation #275

Are you sure you want to change the base?

Feature/272 manual step api implementation #275

Conversation

lucamrgs commented Dec 4, 2024

github-actions bot commented Dec 4, 2024 • edited Loading

Sigrid maintainability feedback

👍 What went well?

👎 What could be better?

📚 Remaining technical debt

⭐️ Sigrid ratings

💬 Did you find this feedback helpful?

lucamrgs commented Dec 4, 2024

lucamrgs commented Dec 20, 2024

lucamrgs commented Dec 20, 2024

lucamrgs commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024 •

edited

Loading