Skip to content

Add validation to create evidence form #1115

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.

Sign up for GitHub

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

Open
jkennedyvz wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −0
Open

Add validation to create evidence form #1115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f67716
Add validation to create evidence form
jkennedyvz Jun 1, 2024
dce160e
Implement backend validation for evidence upload
jkennedyvz Jun 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions backend/services/evidence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ func CreateEvidence(ctx context.Context, db *database.Connection, contentStore c
logging.Log(ctx, "msg", "Unable to run workers", "error", err.Error())
}

// Validation logic for description and content fields
if i.Description == "" {
return nil, backend.WrapError("Description cannot be empty.", backend.BadInputErr(nil, "Description cannot be empty."))

@JoelAtDeluxe JoelAtDeluxe Jun 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You don't need to use WrapError here. You can just use the BadInputErr
  2. You're doing this too low in the function. Two things to think about: First, you should be failing as soon as you know the operation won't work. Second, this needs to be done before it's written into the database (see lines 134 to 152 above).

}
if i.ContentType != "event" && i.Content == nil {
return nil, backend.WrapError("Evidence content cannot be empty.", backend.BadInputErr(nil, "Evidence content cannot be empty."))
}

return &dtos.Evidence{
UUID: evidenceUUID,
Description: i.Description,
Expand Down
8 changes: 8 additions & 0 deletions backend/services/evidence_metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ func CreateEvidenceMetadata(ctx context.Context, db *database.Connection, i Edit
return backend.WrapError("Unwilling to create evidence metadata", backend.UnauthorizedWriteErr(err))
}

if i.Source == "" || i.Body == "" {

@JoelAtDeluxe JoelAtDeluxe Jun 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to you if this is the way you want to do that, but generally you'd want to do these checks individually, as it'll provide more of an indication to the caller how to fix the issue.

You also forgot to add a similar check in updateEvidence. I'd need to check, but I suspect you could still create empty evidence if you wanted by creating evidence with source and body, and then editing the evidence to remove the source and body.

@JoelAtDeluxe JoelAtDeluxe Jun 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also look at strings.TrimSpace

return backend.WrapError("Source and Body cannot be empty.", backend.MissingValueErr("Source/Body"))
}

_, err = db.Insert("evidence_metadata", map[string]interface{}{
"evidence_id": evidence.ID,
"source": i.Source,
Expand Down Expand Up @@ -95,6 +99,10 @@ func UpsertEvidenceMetadata(ctx context.Context, db *database.Connection, i Upse
return backend.WrapError("Unwilling to edit evidence metadata", backend.UnauthorizedWriteErr(err))
}

if i.Source == "" || i.Body == "" {
return backend.WrapError("Source and Body cannot be empty.", backend.MissingValueErr("Source/Body"))
}

err = db.WithTx(ctx, func(tx *database.Transactable) {
var metadata []models.EvidenceMetadata
tx.Select(&metadata, sq.Select("*").From("evidence_metadata").Where(sq.Eq{
Expand Down
17 changes: 17 additions & 0 deletions frontend/src/pages/operation_show/evidence_modals/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@ export const CreateEvidenceModal = (props: {
const [selectedCBValue, setSelectedCBValue] = React.useState<string>(evidenceTypeOptions[0].value)
const getSelectedOption = () => evidenceTypeOptions.filter(opt => opt.value === selectedCBValue)[0]

// Validation logic for description and content fields
const validateForm = () => {
const errors = []
if (descriptionField.value.trim() === "") {
errors.push("Description cannot be empty.")
}
if (getSelectedOption().value !== 'event' && binaryBlobField.value === null && codeblockField.value.code.trim() === "") {
errors.push("Evidence content cannot be empty.")
}
return errors
}
Comment on lines +94 to +103

@JoelAtDeluxe JoelAtDeluxe Jun 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to run this, but docker seems to be misconfigured on this box. But, I think this will be okay. I don't think we have a lot of validation rules elsewhere to pull from, but I'll take a look to see if we solved this in a different way elsewhere.


const formComponentProps = useForm({
fields: [descriptionField, binaryBlobField, adjustedAtField],
onSuccess: () => { props.onCreated(); props.onRequestClose() },
Expand All @@ -106,6 +118,11 @@ export const CreateEvidenceModal = (props: {
data = { type: 'event' }
}

const formErrors = validateForm()
if (formErrors.length > 0) {
return Promise.reject(new Error(formErrors.join("\n")))
}

return createEvidence({
adjustedAt: adjustedAtField.value,
operationSlug: props.operationSlug,
Expand Down