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

Notifications: Add support for webhooks #7311

Merged
merged 63 commits into from
Sep 14, 2024

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Dec 21, 2022

We would like to do an implementation similar to this: https://docs.gitlab.com/ee/user/project/integrations/webhooks.html

Events will be published via notifications

The whole functionality is described in the documentation section. This is an image which is rendered by mermaid

image

@github-actions github-actions bot added docker docs integration_tests New Migration Adding a new migration file. Take care when merging. ui labels Dec 21, 2022
@Maffooch
Copy link
Contributor

@kiblik this is looking really interesting so far. The only critique I have thus far is that it appears there is only support for one web hook at a time. I think it would be good to allow for an arbitrary amount of web hooks. Sorta like how the Jira integration works where you can connect an arbitrary amount of Jira servers and push to them according to how they are configured.

@kiblik
Copy link
Contributor Author

kiblik commented Dec 24, 2022

@Maffooch, thank you for the feedback. I can add management of multiple webhook endpoints.

Btw, what do you think about User notifications? Do they make sense to you? They are disabled for MS Teams.

Or the combination of your idea and my question: One endpoint System notification endpoint and each user will be able to define his/her/they own endpoint.

@Maffooch
Copy link
Contributor

I would think adding a web hook url would require a higher permission (I think maintainer would be sufficient) so adding user levels would be a little redundant.

@kiblik kiblik force-pushed the notifications_webhooks branch from 2438ec8 to 061a125 Compare January 9, 2023 22:44
@kiblik kiblik force-pushed the notifications_webhooks branch from fa88b41 to 34404ce Compare January 23, 2023 15:37
@kiblik
Copy link
Contributor Author

kiblik commented Jan 23, 2023

I would think adding a web hook url would require a higher permission (I think maintainer would be sufficient) so adding user levels would be a little redundant.

@Maffooch, we see a possible use case when the user should be able to define his own endpoint.
What about this:

  • add user = ForeignKey(Dojo_User, null=True, blank=True) to Webhook_Endpoints
  • regular user is able to create an endpoint assigned only to himself
  • high-privileged user is able to assign an endpoint to anybody and also to nobody
  • if it will be assigned to the user, user notifications will be processed/send (the user will also define from which products he/she/they would like to receive notifications)
  • if it will not be assigned to the user, system notifications will be processed/send

@github-actions github-actions bot added the apiv2 label Jan 23, 2023
@Maffooch
Copy link
Contributor

I think that sounds reasonable

@Maffooch
Copy link
Contributor

Maffooch commented Feb 9, 2023

Hey @kiblik how are things going on this? Can I be of any help on this PR (or any of your others)?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kiblik
Copy link
Contributor Author

kiblik commented Feb 9, 2023

Hey @kiblik how are things going on this? Can I be of any help on this PR (or any of your others)?

So, my priorities were on the other tasks recently but I had planned to come back to this functionality during the next week.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

<h3 class="has-filters">
Notification Webhook List
<div class="dropdown pull-right">
<button id="show-filters" data-toggle="collapse" data-target="#the-filters" class="btn btn-primary toggle-filters"> <i class="fa-solid fa-filter"></i> <i class="caret"></i> </button>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

<h3 class="has-filters">
Notification Webhook List
<div class="dropdown pull-right">
<button id="show-filters" data-toggle="collapse" data-target="#the-filters" class="btn btn-primary toggle-filters"> <i class="fa-solid fa-filter"></i> <i class="caret"></i> </button>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

<div class="dropdown pull-right">
<button id="show-filters" data-toggle="collapse" data-target="#the-filters" class="btn btn-primary toggle-filters"> <i class="fa-solid fa-filter"></i> <i class="caret"></i> </button>
{% if "dojo.add_notification_webhook"|has_configuration_permission %}
<button class="btn btn-primary dropdown-toggle" type="button" id="dropdownMenu1"
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

<div class="dropdown pull-right">
<button id="show-filters" data-toggle="collapse" data-target="#the-filters" class="btn btn-primary toggle-filters"> <i class="fa-solid fa-filter"></i> <i class="caret"></i> </button>
{% if "dojo.add_notification_webhook"|has_configuration_permission %}
<button class="btn btn-primary dropdown-toggle" type="button" id="dropdownMenu1"
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

@kiblik kiblik force-pushed the notifications_webhooks branch 2 times, most recently from 08c44bd to 0daeeab Compare March 1, 2023 13:01
dojo/models.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kiblik kiblik force-pushed the notifications_webhooks branch from 0daeeab to 4a6c3f1 Compare March 9, 2023 23:39
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kiblik kiblik force-pushed the notifications_webhooks branch from 4a6c3f1 to a695ce1 Compare March 10, 2023 08:38
@kiblik kiblik force-pushed the notifications_webhooks branch from 96b1730 to 3cc64f7 Compare September 12, 2024 17:23
@kiblik
Copy link
Contributor Author

kiblik commented Sep 12, 2024

I fixed the lastest comments from @cneill. I hope this PR is (finally after 1 year, 8 months, 22 days) ready for merge 😄

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉 Thanks for taking care of those final comments

@Maffooch Maffooch merged commit 330462d into DefectDojo:dev Sep 14, 2024
72 of 73 checks passed
@kiblik kiblik deleted the notifications_webhooks branch September 14, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apiv2 docker docs integration_tests New Migration Adding a new migration file. Take care when merging. settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR ui unittests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants