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

Fix Email Reciever Field in Notification Hook #946

Merged
merged 12 commits into from
Jan 28, 2022
Merged

Conversation

J12934
Copy link
Member

@J12934 J12934 commented Jan 27, 2022

Description

The Email Notifier of the Notification Hook currently behaves differently as described in the Docs.
The docs indicate that channel.endPoint is supposed to be email recipient for the notification but actually has to be the name of a env var which then contains the email address.
This is a feature was added for the Slack & Microsoft Teams Notifier as these use webhook urls containing sensitive tokens.

I've updated the implementation to behave like the documentation.

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • Make sure npm test runs for the whole project.
  • Make codeclimate checks happy

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
This is the behaviour which is acutally documented and makes more sense for the email notification hook

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
@J12934 J12934 added the bug Bugs label Jan 27, 2022
@J12934 J12934 self-assigned this Jan 27, 2022
@J12934 J12934 added this to Backlog in secureCodeBox v3 via automation Jan 27, 2022
@J12934 J12934 moved this from Backlog to To Review in secureCodeBox v3 Jan 27, 2022
Copy link
Member

@malexmave malexmave left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. I have a small comment on a typo. Also, since you mentioned the documentation: It is currently a bit inconsistent. At the top, it says "The general configuration of a notification looks something like this" and then shows an example that loads the env from a secret. However, for the email hook, it then loads the env directly from the file, with the username and password for the email account visible. Maybe we can take this opportunity to also fix that small part of the docs? (Reworking the rest of the docs for the hook is tracked in https://github.com/secureCodeBox/documentation/issues/182 and does not need to be addressed in this PR.)

hooks/notification/hook/Notifiers/EMailNotifier.test.ts Outdated Show resolved Hide resolved
J12934 and others added 4 commits January 28, 2022 10:37
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
Signed-off-by: GitHub Actions <securecodebox@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
hooks/notification/.helm-docs.gotmpl Outdated Show resolved Hide resolved
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
@J12934 J12934 requested a review from malexmave January 28, 2022 09:48
J12934 and others added 2 commits January 28, 2022 09:48
Signed-off-by: GitHub Actions <securecodebox@iteratec.com>
Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>
secureCodeBox v3 automation moved this from To Review to Reviewer approved Jan 28, 2022
@J12934 J12934 merged commit 69abc9b into main Jan 28, 2022
secureCodeBox v3 automation moved this from Reviewer approved to Done Jan 28, 2022
@J12934 J12934 deleted the fix/email-notifications branch January 28, 2022 10:40
@J12934 J12934 moved this from Done to counter in secureCodeBox v3 Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants