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

Email Notification for Actions on Share Requests #734

Closed
TejasRGitHub opened this issue Sep 6, 2023 · 7 comments
Closed

Email Notification for Actions on Share Requests #734

TejasRGitHub opened this issue Sep 6, 2023 · 7 comments
Labels
effort: medium priority: medium status: not-picked-yet At the moment we have not picked this item. Anyone can pick it up type: newfeature New feature request
Milestone

Comments

@TejasRGitHub
Copy link
Contributor

TejasRGitHub commented Sep 6, 2023

Is your idea related to a problem? Please describe.
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

Describe the solution you'd like
An email is sent on creation and on any transition of the share request to the dataset owner, requester, etc

A solution consisting of using SNS topic - for which dataset owner, requester of the dataset are subscribed - where any notification which is sent to the topic gets delivered to the email addresses could be one potential way to implement this.

Further, a way to filter recipient email list where email about the share request are sent to only specific email ids ( from the environment teams ) would be nice to have.

P.S. Don't attach files. Please, prefer add code snippets directly in the message body.

@anmolsgandhi anmolsgandhi added type: newfeature New feature request priority: medium status: not-picked-yet At the moment we have not picked this item. Anyone can pick it up labels Sep 7, 2023
@dlpzx
Copy link
Contributor

dlpzx commented Sep 7, 2023

Hi @TejasRGitHub , thanks for opening an issue this is a great feature idea.

First, we need to define the actions that are going to be notified:

  • share workflow actions - what you describe on approved, revoked...
  • dataset updates - in addition, do we want requesters to get notified when a dataset gets new tables (for example)

Secondly, and this is more complex, we need to decide on the subscription architecture. Presumably we'll be using AWS SNS to get the notifications from the ECS share task + GraphQL API calls. I can brainstorm different alternatives, let me know if this is aligned with what you have in mind:

  • One SNS topics in data.all infrastructure account
  • One SNS topic per environment (in data.all environment account)
  • One SNS topics per team (in data.all environment account)

One SNS topics in data.all infrastructure account

PROS:

  • easy to send notifications, all go to the same topic

CONS:

  • end-users have not access to the topic, cannot add subscribers.
  • Service quota limitations, at first sight it does not look like we are going to hit them in the near future, but with a single-thread architecture we should always review just in case
  • Needs implementation of SNS filter policies to segregate by all teams

One SNS topic per environment

PROS:

  • users have access to the topic
  • more scalable
  • Already part of environment stack --> subscriptions enabled!

CONS:

  • Needs some implementation of SNS filter policies to segregate by teams in the environment
  • From ECS and GraphQL Lambda we need to select the correct SNS topic

One SNS topics per team

Very similar to the previous

PROS:

  • We can isolate the SNS topic and allow only the team members access to it.
  • Also no filtering is needed, subscriptions are pretty easy
  • it can be re-used for any team-notifications that should be received by the whole team

CONS:

  • more infra in the environment stack
  • From ECS and GraphQL Lambda we need to select the correct SNS topic

@dlpzx
Copy link
Contributor

dlpzx commented Sep 7, 2023

@noah-paige I have not used SNS that much, so feel free to add any thoughts

@noah-paige
Copy link
Contributor

Other considerations to note:

  • Is the email notification on a team level or on a individual user email level (singular owner of the share and/or dataset)? Can this be configurable?
  • Any ways to handle pending subscriptions?

@anmolsgandhi anmolsgandhi added this to the v2.1.0 milestone Sep 8, 2023
@TejasRGitHub
Copy link
Contributor Author

Hi @dlpzx , Thanks for the ideas about implementation.

I think I would be leaning towards using SNS topic per environment , in which , any share workflow which is created a notification will be sent to each SNS topic and delivered to both the producer and the consumer team. Here though we would like to have filtering to send it to only a set of people in a team OR only certain teams in that environment. But as per the discussion, it seems that is a user experience concern in which if a user belongs to multiple environment then the same user would have to verify his email for each SNS topics subscription. It would be great to know your thoughts on it and see if there is a work around this.

I am thinking on another alternative to sending email via SES which would look like, Share Workflow -> Create Task for Email notification -> Put in Worker.queue ( sqs.queue) -> When the worker.process is called , appropriate handler will be called . Here for the handler another ECS service specifically for sending email notification can be made which will use SES.

Hi @noah-paige ,

Email notification should be sent to the teams ( for e.g. when a share is created, email notification can be sent to all the teams( cognito groups ) in requester's environment and on the approvers side , email can be sent to dataset owners team and stewards team but a way to configure to send the email only to the user requesting for dataset in that team and only to the dataset owners would be nice to have.

@TejasRGitHub
Copy link
Contributor Author

TejasRGitHub commented Sep 28, 2023

Hi @noah-paige , @dlpzx ,

Here is the design and few other details for this enhancement feature

The actions made on the UI for any share workflow will trigger GraphQL Lambda. This lambda ( api-handler ) will processes the request and create a Task containing information about the email notification ( e.g. Email Subject, Email Message, Username/email Id of the user, datasetOwnerteam, datasetStewardsTeam & requesterUserTeam)

This Task will be sent to the SQS ( Queue ) which will trigger the aws_handler lambda. The lambda handler's associated service will be responsible for fetching email ids of the dataset owner team, stewards team ( create a SET of those email Ids to avoid duplicates), and get the email id of the requester / requester team members and call the SES email api to send email. The emailIds for both the requester and the approvers team will be fetched from the cognito user pools.

The SES setup on AWS accounts will happen through SesStack which will be initialized during the pipeline phase. This will setup the identity ( domain ) used for sending email on SES and also setup monitoring for email bounces, complaints & delivery issues. For now, the domain which is specified in the cdk.json 's DeploymentEnvironment will be used and will be verified. The SES Stack will initiate and verify the domain the domain with route53 if it exists there. SesStack will also create a SNS topic which will be used to capture email bounces , complaints & delivery delays. ( Note - Subscriptions on this SNS topic will have to be created separately from AWS Console )

Configs Introduced

  1. Config to enable/ disable email notification along with enable/disable group notification on the requester side.
"share_notifications": [
   {
       "email": {
           "active": true,
           "features": {
               "group_notifications": true
           }
       }
   }
]
  1. Sender's Email Id / From Email Id - The sender’s email address will be added in the cdk.json ( in the DeploymentEnvironments ). This email id will then be added as an environment variable in the aws_handler lambda which will be used to send email to recipients

Permission Granted as a part of Email notification Change

  1. aws_handler permission - 'ses:SendEmail' will be added in the aws_handler's policy for the SES Verified Entity

@dlpzx
Copy link
Contributor

dlpzx commented Oct 11, 2023

Hi @TejasRGitHub, @noah-paige , @anmolsgandhi I created the issue #785 that describes one preliminary task needed for @TejasRGitHub to contribute this feature back to the open-source repo

dlpzx pushed a commit that referenced this issue Oct 20, 2023
### 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
- #734


### 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)? 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.

Co-authored-by: trajopadhye <tejas.rajopadhye@yahooinc.com>
dlpzx added a commit that referenced this issue Oct 26, 2023
### Feature or Bugfix
- Refactoring

### Detail
As a rule of thumb, we encourage customization of `modules` while
changes in `core` should be avoided when possible. `notifications` is a
component initially in core which is only used by `dataset_sharing`. To
facilitate customization of the `notifications` module and also to
clearly see its dependencies we have:

- Moved `notifications` code from core to modules as it is a reusable
component that is not needed by any core component.
- Moved dataset_sharing references inside dataset_sharing module and
left `notifications` independent from any other module (done mostly in
#734, so credits to @TejasRGitHub)
- Added depends_on in the dataset_sharing module to load notifications
if the data_sharing module is imported.
- Modified frontend navigation bar to make it conditional of the
notifications module
- Added migration script to modify the notification type column
- Fix tests from #734, some references on the payload of the
notification tasks were wrong
- Small fixes to SES stack: added account in KMS policy and email_id as
input

### [WIP] Testing
Local testing
- [ ] loading of notifications with datasets enabled
- [ ] ...

AWS testing
- [ ] CICD pipeline succeds

### Other remarks
Not for this PR, but as a general note, we should clean up deprecated
ECS tasks

### Relates
- #785 
- #734 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

`N/A` just refactoring


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
noah-paige pushed a commit that referenced this issue Nov 3, 2023
### Feature or Bugfix
- Feature ( Documentation ) 

### Detail

When enabling email notification on share workflow, an SES identity is
created on AWS SES. This PR contains documentation about the manual
steps that need to be taken towards setting up SES and also additional
steps to setup monitoring.

### Relates
- #734
- #785

### 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)? N/A
  - 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? N/A
- 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? N/A
  - 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? N/A
  - 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.
@dlpzx
Copy link
Contributor

dlpzx commented Nov 8, 2023

Merged and released with v2.1.0 🚀

@dlpzx dlpzx closed this as completed Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium priority: medium status: not-picked-yet At the moment we have not picked this item. Anyone can pick it up type: newfeature New feature request
Projects
None yet
Development

No branches or pull requests

4 participants