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

Improve Slack integration to allow notifying private, public, and group channels (fixes #8569) #9046

Closed
wants to merge 2 commits into from

Conversation

tomaszn
Copy link
Contributor

@tomaszn tomaszn commented Nov 23, 2023

This change enables users to set private, public, or group channels as the target for Slack notifications.

It achieves this by removing the slack_user_id intermediary field, which was filled with a response after querying the Slack API with the user's email, and which limited the notification targets to users' private Slack channels. Before removal, the value is copied into slack_username field, overwriting the email address, which is no longer needed (and no longer works).

Detailed explanations are in #8569.

Test results

  • Unit tests pass.
  • SLA breach notifications are delivered successfully when slack_username is set to a channel names, with and without a "#" prefix.
  • Also success when a channel ID is used.

Documentation

No changes in the documentation are needed.
However, I noticed that the Slack administration page looks now differently, so I updated the relevant screen shot in the "Notifications" section.

Checklist

This checklist is for your information.

  • Unit tests pass.
  • Your code is flake8 compliant.
  • Model changes must include the necessary migrations in the dojo/db_migrations folder.

@github-actions github-actions bot added New Migration Adding a new migration file. Take care when merging. docs labels Nov 23, 2023
Copy link

dryrunsecurity bot commented Nov 23, 2023

Contextual Security Analysis

As DryRun Security performs checks, we’ll summarize them here. You can always dive into the detailed results in the section below for checks.

Status DryRun Security Check
Sensitive Functions Analyzer
Configured Sensitive Files Check
Sensitive Files Analyzer

Chat with your AI-powered Security Buddy by typing @dryrunsecurity followed by your question into a comment.
Example: @dryrunsecurity What are common security issues with web application cookies?

Install and configure more repositories at DryRun Security

@tomaszn tomaszn force-pushed the slack-fields branch 2 times, most recently from 1773f30 to f88d1e7 Compare November 29, 2023 00:25
@tomaszn
Copy link
Contributor Author

tomaszn commented Jan 16, 2024

@cneill, @kiblik, please review if possible

@devGregA
Copy link
Contributor

Hi @tomaszn, we reviewed this PR but weren't able to replicate the problems mentioned. Could you please provide additional details on the issues you experienced?

@tomaszn
Copy link
Contributor Author

tomaszn commented Jan 16, 2024

@devGregA, sure. The simplest use case is setting up notifications for #general Slack channel. It is currently not possible, as far as I know.

model_name="usercontactinfo",
name="slack_user_id",
),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

 migrations.RemoveField(
            model_name="usercontactinfo",
            name="slack_username",
        ),
 migrations.RenameField(
            model_name="usercontactinfo",
            old_name="slack_user_id",
            new_name="slack_username",
        ),

Looks easier compare to RunPython
https://docs.djangoproject.com/en/5.0/ref/migration-operations/#renamefield

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks. And then AlterField needs to be moved to the end, to set up verbose_name etc.

Moreover, I should set max_length=81 (# plus 80 characters of the channel name).

@paulOsinski
Copy link
Contributor

paulOsinski commented Jan 31, 2024

Hi @tomaszn , I tested creating and linking the Slack integration and I was able to post two different kinds of notifications:

  • personal notifications to an individual user's Slackbot (which is what the Slack Email Address field is used for in the DefectDojo UI. Not sure what this maps to on the backend).
  • system-wide notifications to a shared channel

I was able to define the Slack Channel for posts on the Slack-app side, when I created and installed it to my workplace.

I updated the documentation with what I found: 750e548


Currently, Dojo does not support any customized or filtered notifications, or posts to multiple slack channels. Is that what you're intending to do by changing the notification targets from Slack Usernames to Slack Channels?

If you wanted to add that feature, I think it would be more appropriate to add a Slack Channel mapping field on a Product, rather than on each user's profile which is where the Slack email address currently exists.

@tomaszn
Copy link
Contributor Author

tomaszn commented Jan 31, 2024

Hello @paulOsinski, thank you for your input here, and for the update to the documentation.

Currently, Dojo does not support any customized or filtered notifications, or posts to multiple slack channels. Is that what you're intending to do by changing the notification targets from Slack Usernames to Slack Channels?

Correct. And by creating "virtual" users and assigning them Slack channels I can customize notifications. Several channels can get notifications about events in independently configured sets of products.

If you wanted to add that feature, I think it would be more appropriate to add a Slack Channel mapping field on a Product, rather than on each user's profile which is where the Slack email address currently exists.

In my case, I need several Slack channels to be informed about various events in the products. So a simple 1:1 mapping would not be enough.

I acknowledge that the current solution is not perfect and serves more as a workaround. A fully-featured notification manager capable of handling mappings and preferences is something we should aim for in the future.

Please bear in mind that this changeset was prepared in a feature freeze situation. The goal had to be obtained with a minimal changes not to disrupt work of others :)

Copy link
Contributor

github-actions bot commented Apr 6, 2024

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

@mtesauro
Copy link
Contributor

Close as stale

@mtesauro mtesauro closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts-detected docs New Migration Adding a new migration file. Take care when merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants