search

cisagov / gophish-tools

Helpful tools for interacting with a GoPhish phishing instance
Creative Commons Zero v1.0 Universal
42 stars 6 forks source link

Remove appended campaign id appended to new campaign name #44

Closed nickviola closed 3 years ago

nickviola commented 3 years ago

Remove appended CX id to campaign name when creating new campaign. We are changing the way we parse exports and will reference by the format "_levelX" instead of CX identifiers. This gives us the ability to correlate levels of assessments with campaigns in GoPhish exports.

🗣 Description

This removes the old structure of appending CX to the campaign name. We no longer need this format and will be referencing by level_id (1-6).

💭 Motivation and context

We no longer need this format and will be referencing by level_id (1-6).

🧪 Testing

To test and validate that the name format change was implemented correctly, I did the following:

  1. Build a new image that includes the changes in this branch and tag it for reference by running: docker build -t gophish-tools-issue-43 .

  2. Run the following to override the default image and reference the newly tagged image with the changes: export GOPHISH_TOOLS_IMAGE="gophish-tools-issue-43" && eval "$(docker run gophish-tools-issue-43)"

  3. The alias should be setup to drop into a bash instance on the gophish-tools container. You can run the following to access the container: gophish-tools-bash

  4. . In the contaner bash instance, create a new assessment as you normally would by running the alias: pca-wizard

  5. Input Campaign data and import into gophish. The campaign name should now exclude the appended _C1 that was originally there.

I also confirmed that the campaign went through all phases correctly sending email and generating click output as expected with the new name format.

✅ Checklist

jsf9k commented 3 years ago

You should remove the unchecked checkbox under "Checklist" in the PR description, unless you intend to add tests fro this change.

Also, I didn't see any documentation updated here, so I suggest removing the "All relevant repo and/or project documentation..." line as well (unless you intend to add it).

nickviola commented 3 years ago

You should remove the unchecked checkbox under "Checklist" in the PR description, unless you intend to add tests fro this change.

Also, I didn't see any documentation updated here, so I suggest removing the "All relevant repo and/or project documentation..." line as well (unless you intend to add it).

Thanks for catching that. I meant to go back and remove them, but my memory failed me. 😆

I've updated the checklist! Thanks for looking it over!

nickviola commented 3 years ago

This looks good to me. 👍

In the "Testing" section of your PR description, I'd like to see this written as "Here's what I did and how I confirmed that everything is correct", rather than what you currently have which comes across more like "Here's what I plan to test" with no indication of successful testing.

👍 Agreed. That provides more clarity. I've updated it to include more details on how it was tested. Thanks for looking it over!