Email Notification on Share Workflow - Issue - 734 #818

TejasRGitHub · 2023-10-18T17:38:32Z

Feature or Bugfix

Feature

Detail

Whenever a share request is created and transitions from states ( approved, revoked, etc ) a notification is created. This notification is displayed on the bell icon on the UI .

We want such a similar notification to be sent to the dataset owner, requester, etc via email

Please take a look at Github Issue 734 For more details - #734

Relates

Email Notification for Actions on Share Requests #734

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)? No
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization? No
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features? No
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users? Yes
- Have you used the least-privilege principle? How? --> **Permission granted for SES:sendEmail to Lambda on resources - (Ses identity and configuration set ) , Also created KMS and SNS for SES setup to handle email bounces . Used least privleged and restricted access on both whenever required. **

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…into v2m1m0 Merging Chnages to fork

… AWS Team

…into v2m1m0 Merged incoming changes

…into v2m1m0 Updated SES stack files

backend/api_handler.py

backend/dataall/base/services/IdentityProvider.py

backend/dataall/core/notifications/services/email_notification_service.py

backend/dataall/core/notifications/services/email_notification_provider.py

backend/dataall/modules/dataset_sharing/services/share_item_service.py

deploy/stacks/backend_stack.py

backend/dataall/modules/dataset_sharing/services/share_notification_service.py

dlpzx

Thanks for the PR! There are some comments related to the overall structure and some minor things on naming conventions.

…into v2m1m0 Merged Changed from Open source

TejasRGitHub · 2023-10-19T23:24:17Z

Thanks for the PR! There are some comments related to the overall structure and some minor things on naming conventions.

Hi @dlpzx . Thanks for the in-depth review. I have updated the code to reflect the changed asked in the comments. After the changes , I have carried out one round of sanity tests on my local dev environment.

dlpzx · 2023-10-20T11:28:36Z

Hi @TejasRGitHub I have reviewed your changes, looking good! I will approve and test together with #790

deploy/stacks/ses_stack.py

dlpzx · 2023-10-23T09:35:28Z

deploy/stacks/ses_stack.py

+            id,
+            envname='dev',
+            resource_prefix='dataall',
+            custom_domain=None,


@TejasRGitHub, why don't we pass the email id here? Aren't we going to use it in the EmailIdentity creation as identity parameter?

Hi @dlpzx , the email id is not used to create an email identity. We are only using the email id as a sender email id. The way currently SES stack is working , is that it is verifying the custom_domain and not the email id. When the custom_domain is verified any email id on that domain can be used to send an email. Also, verifying an email id could be possible in case the user has access to the inbox of that email id. Apart from that if an email id is verified then the user also has to make entries for SPF and DKIM into their DNS server (https://docs.aws.amazon.com/ses/latest/dg/email-authentication-methods.html) for the recipient to receive the email properly ( without it being sent to spam)

dlpzx · 2023-10-23T09:36:36Z

deploy/stacks/ses_stack.py

+
+        self.sns.apply_removal_policy(RemovalPolicy.DESTROY)
+
+        hosted_zone = route53.HostedZone.from_hosted_zone_attributes(


If we use email_id instead of custom_domain, and for some reason the custom domain is not there then this piece of code will make the stack fail. We need to add a conditional block and in case it is not there use the email_id in identity below

I fixed it in this commit: ee2cc20 let me know if you agree with the changes

Currently, only the custom_domain is used to verify with SES. My intent was not to verify the email address as identity as, that would mean that the user has to verify the email address and also make couple of entires in their DNS server ( for DKIM and SPF ).

For example, if the user wants to use an email ids like, data-all-notifications@some-non-aws-domain.com. Then in this case the user has to verify that the email id belongs to him by clicking on the verification email and also authenticate the email address with DKIM and SPF, for which the user needs to have access to the DNS server in someway

@dlpzx

### Feature or Bugfix - Feature - Bugfix - Refactoring ### Detail #### Features * Limit pivot role S3 permissions by @dlpzx in #780 * Limit pivot role KMS permissions by @dlpzx in #830 * Add configurable session timeout to IDP by @manjulaK in #786 * Allow to submit a share when you are both an approver and a requester by @zsaltys in #793 * Redirect upon creating a share request by @zsaltys in #799 * Handle Pre-filtering of tables by @anushka-singh in #811 * Email Notification on Share Workflow - Issue - 734 by @TejasRGitHub in #818 * Refactor notifications from core to modules by @dlpzx in #822 * Add frontend and backend feature flags by @zsaltys in #817 * Make hosted_zone_id optional by @lorchda in #812 #### Fixes * Add Additional Error Messages for KMS Key lookup on imported dataset by @noah-paige in #748 * Handle Environment Import of IAM service roles by @noah-paige in #749 * Build Compliant Names for Opensearch Resources by @noah-paige in #750 * Update Lambda runtime by @nikpodsh in #782 * Ensure valid environments for share request and other objects creation by @dlpzx in #781 * Fix shell true semgrep by @dlpzx in #760 * Add condition when there are no public subnets by @lorchda in #794 * Remove unused variable by @zsaltys in #815 * Check other share exists before clean up by @noah-paige in #769 ### Relates - v2.1.0 minor release ## New Contributors * @manjulaK made their first contribution in #786 * @zsaltys made their first contribution in #793 * @anushka-singh made their first contribution in #811 * @TejasRGitHub made their first contribution in #818 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: jaidisido <jaidisido@gmail.com> Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com> Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com> Co-authored-by: MK <manjula_kasturi@hotmail.com> Co-authored-by: Zilvinas Saltys <zilvinas.saltys@yahooinc.com> Co-authored-by: Daniel Lorch <98748454+lorchda@users.noreply.github.com> Co-authored-by: Anushka Singh <anushka.singh@yahooinc.com> Co-authored-by: trajopadhye <tejas.rajopadhye@yahooinc.com>

TejasRGitHub and others added 6 commits October 18, 2023 12:04

Github Issue 734 - Email notification code change

62edd7d

Merge branch 'v2m1m0' of https://github.com/TejasRGitHub/aws-dataall …

c618c91

…into v2m1m0 Merging Chnages to fork

Github Issue - 734 - Added Integration Tests and Changes suggested by…

37c7eec

… AWS Team

Merge branch 'v2m1m0' of https://github.com/TejasRGitHub/aws-dataall …

1dfff70

…into v2m1m0 Merged incoming changes

Github Issue - 734 - Updated missing from SES Stack

0d5bf5a

Merge branch 'v2m1m0' of https://github.com/TejasRGitHub/aws-dataall …

dc628e9

…into v2m1m0 Updated SES stack files

dlpzx self-requested a review October 19, 2023 06:26