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

On master: new restricted domain list doesn't respect 'Send Now' button #17977

Closed
rlotun opened this issue Sep 20, 2021 · 5 comments · Fixed by #18122 or #18237
Closed

On master: new restricted domain list doesn't respect 'Send Now' button #17977

rlotun opened this issue Sep 20, 2021 · 5 comments · Fixed by #18122 or #18237
Assignees
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Milestone

Comments

@rlotun
Copy link
Contributor

rlotun commented Sep 20, 2021

Describe the bug
With the new restricted domains list, I can still send dashboard subscription emails using Send Now to an email address whose domain is not on the list.

To Reproduce
Steps to reproduce the behavior:

  1. Add a user with an email outside of the domain you want to filter down to - e.g. a persona email address (through Admin)
  2. Go to Admin -> General
  3. Set a domain in the "Approved Domains for Notifications" box (e.g. metabase.com)
  4. Go to a dashboard, click share, and click add an email notification and 'Send Now'.
  5. See email sent to account where it shouldn't.

Expected behavior
It shouldn't sent, and should probably show an error saying 'Domain not allowed, contact administrator' or something to that effect.

This happens on the current master.

@rlotun rlotun added Type:Bug Product defects Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Correctness .Needs Triage labels Sep 20, 2021
@jeff303
Copy link
Contributor

jeff303 commented Sep 29, 2021

Feature added in #17897.

@jeff303 jeff303 self-assigned this Sep 29, 2021
@jeff303 jeff303 linked a pull request Sep 29, 2021 that will close this issue
@jeff303
Copy link
Contributor

jeff303 commented Sep 29, 2021

Added the backend change and test under #18121.

I'm not sure if we'll also want some additional frontend piece to explain what went wrong. With this, the user will just see the button text temporarily change from Send email now to Sending failed (i.e. the current behavior if anything goes wrong with the test send, I suppose).

@camsaul
Copy link
Member

camsaul commented Sep 30, 2021

Ok, not actually fixed yet. #18122 fixed this for Alerts but not for Dashboard Subscriptions -- this uses a different API endpoint

@nemanjaglumac
Copy link
Member

Tested manually for dashboard subscriptions "Send now'.

It shows "Send failed", and it responds with 403. All good.

Do we also want to display the message to the user, like we do when one wants to create a subscription (clicking on the "Done" button)? It feels weird to have it for one case, but not for the other one.

cc @rlotun @brunobergher

nemanjaglumac added a commit that referenced this issue Oct 6, 2021
@nemanjaglumac nemanjaglumac added the .Reproduced Issues reproduced in test (usually Cypress) label Oct 6, 2021
@brunobergher
Copy link
Contributor

💯, we should also communicate something to the user in this context. I suspect ideally the code path is the same, right?

This was referenced May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Projects
None yet
6 participants