-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/204 add variables payload to trigger by UUID #206
Feature/204 add variables payload to trigger by UUID #206
Conversation
routes/trigger/trigger_api.go
Outdated
jsonData, err := io.ReadAll(context.Request.Body) | ||
if err != nil { | ||
log.Trace("Playbook trigger has failed to decode request body") | ||
error.SendErrorResponse(context, http.StatusBadRequest, "Failed to decode request body", "POST /tirgger/playbook/"+id, "") |
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.
error.SendErrorResponse(context, http.StatusBadRequest, "Failed to decode request body", "POST /tirgger/playbook/"+id, "") | |
error.SendErrorResponse(context, http.StatusBadRequest, "Failed to decode request body", "POST /trigger/playbook/"+id, "") |
ffa60d5
to
5bdbf5c
Compare
routes/trigger/trigger_api.go
Outdated
} | ||
|
||
if playbook.PlaybookVariables == nil { | ||
playbook.PlaybookVariables = variables |
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.
Does this initialise a nil playbookVariables object? In that case, we might want to initialise an empty playbookVariables object instead, as per recent merge request - right?
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.
I stand corrected, variables, is assigned. All good.
routes/trigger/trigger_api.go
Outdated
} | ||
|
||
if playbook.PlaybookVariables == nil { | ||
playbook.PlaybookVariables = variables |
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.
I stand corrected, variables, is assigned. All good.
routes/trigger/trigger_api.go
Outdated
@@ -43,6 +45,22 @@ func New(controller decomposer_controller.IController, database database.IContro | |||
return &instance | |||
} | |||
|
|||
func MergeVariablesInPlaybook(playbook *cacao.Playbook, body []byte) { |
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.
We should add a function that checks that all injected variables from the trigger API payload actually exist in the playbook variables, otherwise send back a bad request and not execute further
routes/trigger/trigger_api.go
Outdated
@@ -43,6 +46,41 @@ func New(controller decomposer_controller.IController, database database.IContro | |||
return &instance | |||
} | |||
|
|||
func MergeVariablesInPlaybook(playbook *cacao.Playbook, body []byte) (bool, string) { |
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.
Return error instead of string
routes/trigger/trigger_api.go
Outdated
} | ||
// TODO: Exists, must not overwrite the external field | ||
// Impossible to implement currently or it would break execution: new vars values are initialized to external: false, which means on injection | ||
// they change the value of the variable currently there |
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.
Just add the value and description to the playbook.PlayboookVariables...
// @Param id path string true "playbook ID" | ||
// @Success 200 {object} api.Execution | ||
// @failure 400 {object} api.Error | ||
// @Param id path string true "playbook ID" |
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.
execute swag fmt
routes/trigger/trigger_api.go
Outdated
// TODO: to change in documentation too. The variable must be provided as a valid { key: cacao.Variable} object, | ||
// and the check on mathcing playbook variables must happen on the key. | ||
// in the current code, we are wrongly assuming that the "name" of the variable, and the "key" in the playbook, match | ||
payload_variables := cacao.NewVariables() |
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.
playbookVariables
payload_variables := cacao.NewVariables() | |
playbookVariables := cacao.NewVariables() |
routes/trigger/trigger_api.go
Outdated
} | ||
|
||
// Check payload-injected variables are valid set for playbook variables | ||
for k, payload_var := range payload_variables { |
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.
for k, payload_var := range payload_variables { | |
for name, variable := range payloadVariables { |
routes/trigger/trigger_api.go
Outdated
} | ||
|
||
fmt.Println(playbook) |
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.
use log or remove
assert.Equal(t, expected_message_not_in_pb, result_not_in_pb["message"].(string)) | ||
|
||
// ################################################################### Var is of wrong type | ||
var_wrong_type := cacao.Variable{ |
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.
Make this a separate test
assert.Equal(t, 400, recorder_wrong_type.Code) | ||
assert.Equal(t, expected_message_wrong_type, result_wrong_type["message"].(string)) | ||
|
||
// ################################################################### Playbook var is not external |
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.
Separate test
assert.Equal(t, 400, recorder_wrong_type.Code) | ||
|
||
// Assertions | ||
var result_wrong_type map[string]interface{} |
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.
are we using lowercase _?
mock_decomposer.AssertExpectations(t) | ||
} | ||
|
||
func TestExecutionOfPlaybookByIdWithPayloadInvalidVariables(t *testing.T) { |
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.
name is a bit long no?
34da841
to
8ea7d52
Compare
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.
General remark:
Maybe we should also change the naming in the test. It uses underscores right now.
routes/trigger/trigger_api.go
Outdated
id := context.Param("id") | ||
|
||
db := trigger.database.GetDatabaseInstance() | ||
playbook, err := db.Read(id) | ||
if err != nil { | ||
log.Error("failed to load playbook") | ||
error.SendErrorResponse(context, http.StatusBadRequest, | ||
soarca_http_error.SendErrorResponse(context, http.StatusBadRequest, |
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.
change soarca_http_error
to something like SoarcaHttpError
if context.Request.Body != nil { | ||
jsonData, err := io.ReadAll(context.Request.Body) | ||
if err != nil { | ||
log.Trace("Playbook trigger has failed to decode request body") |
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.
maybe to has failed to read request body
routes/trigger/trigger_api.go
Outdated
"Failed to marshall json on server side", | ||
"POST /trigger/playbook", "") | ||
return | ||
} | ||
// playbook := cacao.Decode(jsonData) | ||
playbook := decoder.DecodeValidate(jsonData) | ||
if playbook == nil { | ||
error.SendErrorResponse(context, http.StatusBadRequest, | ||
soarca_http_error.SendErrorResponse(context, http.StatusBadRequest, |
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.
change name no _ _ in name
routes/trigger/trigger_api.go
Outdated
case executionsDetail := <-trigger.ExecutionsChannel: | ||
playbookId := executionsDetail.PlaybookId | ||
executionId := executionsDetail.ExecutionId | ||
if playbookId == playbook.ID { | ||
msg := gin.H{ |
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.
maybe we should make a model for this?
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.
for the msg
Approved by JP and addressed issues
No description provided.