Skip to content
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

Support setting a custom JSON schema for copied boefjes #3344

Merged
merged 12 commits into from
Aug 21, 2024

Conversation

Donnype
Copy link
Contributor

@Donnype Donnype commented Aug 12, 2024

Changes

This PR adds a "schema" field to the boefje database models so that we can also change the schema for copied (or new) boefjes. This was still lacking in the current APIs.

Issue link

Closes #3052

Demo

image

QA notes

Again this is a migration mostly so: check out main, enable dns-records, check out this branch and verify that dns-records is still enabled:

git checkout main
# enable dns-records, change "test" to your own organisation id
curl -X PATCH -H 'Content-Type: application/json' --data-raw '{"enabled": true}' 'http://localhost:8003/v1/organisations/test/plugins/dns-records'
git checkout feature/add-json-schema-for-copied-boefjes
make kat
curl 'http://localhost:8003/v1/organisations/test/plugins/dns-records' | jq
# Expected: "enabled": true and a new "schema" field 

Edit: Verify that the migration ran successfully. The output of the command below should be 5be152459a7b

docker compose exec -u postgres postgres /bin/bash -c 'psql -U $POSTGRES_USER -d katalogus -c "SELECT * FROM public.alembic_version;"'

Code Checklist

  • All the commits in this PR are properly PGP-signed and verified.
  • This PR only contains functionality relevant to the issue.
  • I have written unit tests for the changes or fixes I made.
  • I have checked the documentation and made changes where necessary.
  • I have performed a self-review of my code and refactored it to the best of my abilities.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@Donnype Donnype requested a review from a team as a code owner August 12, 2024 10:36
@Donnype Donnype changed the title Feature/add json schema for copied boefjes Support setting a custom JSON schema for copied boefjes Aug 12, 2024
@Donnype Donnype self-assigned this Aug 12, 2024
@dekkers
Copy link
Contributor

dekkers commented Aug 12, 2024

Looks good to me, can be moved to QA if no one else has any code review feedback.

@stephanie0x00
Copy link
Contributor

stephanie0x00 commented Aug 12, 2024

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

Unsure yet.

What doesn't work:

The migration breaks things. There are some SQL errors in the log files, logs are shared with @Donnype
One of the commands from the QA notes curl -X PATCH -H 'Content-Type: application/json' --data-raw '{"enabled": true}' 'http://localhost:8003/v1/organisations/test/plugins/dns-records' doesn't give output. It's unclear whether or not this is intentional.

Bug or feature?:

Unsure yet.

@stephanie0x00 stephanie0x00 added the 😸 Review/QA feedback Review/QA feedback provided label Aug 12, 2024
@Donnype
Copy link
Contributor Author

Donnype commented Aug 13, 2024

I haven't been able to find logs of the migration itself, so perhaps that wasn't triggered somehow. We should perhaps rerun the flow and verify the migration was triggered. Also there should indeed be no response body returned from the curl command. TBC!

@madelondohmen
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset. --> I did a "make kat" like in the QA notes
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

I've only tested the QA notes.

  • docker compose exec -u postgres postgres /bin/bash -c 'psql -U $POSTGRES_USER -d katalogus -c "SELECT * FROM public.alembic_version;"' gives back 5be152459a7b
  • Checked out to checkout feature/add-json-schema-for-copied-boefjes, did "make kat" and curl 'http://localhost:8003/v1/organisations/test/plugins/dns-records' shows "enabled=true" and contains a "schema"

What doesn't work:

N/A

@stephanie0x00
Copy link
Contributor

stephanie0x00 commented Aug 21, 2024

Alright ready for merge with some team work 💪 Thanks @madelondohmen !

The rest seems to work as expected.

@underdarknl underdarknl merged commit 47866c0 into main Aug 21, 2024
9 checks passed
@underdarknl underdarknl deleted the feature/add-json-schema-for-copied-boefjes branch August 21, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😸 Review/QA feedback Review/QA feedback provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle json schema's for copied boefjes
5 participants