Closed Bug 1520918 Opened 7 years ago Closed 6 years ago

Notification blacklists should be user-editable

Categories

(Taskcluster :: Services, enhancement)

Product:
Taskcluster
Component:
Services
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: ojaswin25111998, Mentored)

Details

We have a blacklist for notifications, but it's a service configuration..

We could, instead, store that information in a table and make it editable via the UI, covered by some administrative scope.

Later, we could potentially track bounces automatically and automatically blacklist addresses that bounce too much, but let's leave that to a followup.

The code for this is in services/notify in https://github.com/taskcluster/taskcluster. You can see that there's a "blacklist" in config.yml in that application already. A few of the pieces of completing this project:

  • create an Azure table (or database table) to contain blacklisted notification destinations [1]
  • make API methods to modify that table (list, get, add, update, delete)
  • make UI changes that use those API methods to manage the data
  • make the notification service read from the table to determine if a destination is blacklisted, before sending the notification

[1] Let's not limit this to emails -- any kind of notification should be possible to blacklist

Hi, I'll take this up :)

Assignee: nobody → ojaswin25111998

Hey i was wondering how should the table be? Just one property for the address?

That makes sense -- or a column for the notification type (irc, email, pulse) and a column for the destination. The format of that destination column may need to be JSON, to hold the irc value.

I started working on the azure table. The PR can be found here https://github.com/taskcluster/taskcluster/pull/127

  • make API methods to modify that table (list, get, add, update, delete)

should i user the 'fast-azure-storage' library for this?

Also, the methods go in the same data.js as methods of the entity's prototype, am I right?

Also, the methods go in the same data.js as methods of the entity's prototype, am I right?

Okay, so I looked around some more. I think I had the wrong idea and will have to use the APIBuilder similar to azure.js in services/auth. Please correct me if I'm wrong :)

Sounds like you're close -- a better example might be the APIBuilder in services/notify/src/api.js.

I pushed a new commit with the APIs and schemas. I'll wait for your review before proceeding further. I have some questions though:-

  1. For the method to fetch a single notification, should I use the post method since I can't use the GET as address is JSON

  2. How should I go about testing my APIs. Right now I have no idea if they'll work or not :p

  1. Maybe we should establish some unique id's for blacklist item entries, rather than keying by the contents. We use slugid for unique id's elsewhere in Taskcluster. Then

    const blacklistId = slugid.nice()
await notify.createBlacklistItem(blacklistId, {notificationType: 'irc-user', notificationAddress: 'dustin'});
const bl = await notify.getBlacklistItem(blacklistId);
assert(bl.notificationType === 'irc-user');
await notifiy.removeBlacklistItem(blacklistId);

    Also, now that I look at the schemas you've developed, all four notification types (pulse, email, irc-user, irc-channel) have a string as their "address". I remembered incorrectly (I should have checked) that the irc methods specified both a server and a name. So we could generalize the table and the API responses to contain only those two fields (type, address). That would simplify the schemas and APIs quite a bit!

  2. You can find some examples of tests in the other services. The secrets service is a good example -- it only has one table and has list, get, set, and remove methods on that table. Its api_test.js calls withSecret which sets up a fake Azure table that is fast and doesn't require you to have access to Azure. Spend a bit of time working that out and show me what you come up with, at which time I can help fill in the details -- there are a lot of parts to set up for tests and I don't want you to spend too much time on it.

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #10)

  1. Maybe we should establish some unique id's for blacklist item entries, rather than keying by the contents. We use slugid for unique id's elsewhere in Taskcluster. Then

    const blacklistId = slugid.nice()
await notify.createBlacklistItem(blacklistId, {notificationType: 'irc-user', notificationAddress: 'dustin'});
const bl = await notify.getBlacklistItem(blacklistId);
assert(bl.notificationType === 'irc-user');
await notifiy.removeBlacklistItem(blacklistId);

    Also, now that I look at the schemas you've developed, all four notification types (pulse, email, irc-user, irc-channel) have a string as their "address". I remembered incorrectly (I should have checked) that the irc methods specified both a server and a name. So we could generalize the table and the API responses to contain only those two fields (type, address). That would simplify the schemas and APIs quite a bit!

If the address itself is unique we can directly use it as a query param for retrieval rather than generating a unique slugid. Won't the slugId just add an extra step of passing the generated id back to the user?

  1. You can find some examples of tests in the other services. The secrets service is a good example -- it only has one table and has list, get, set, and remove methods on that table. Its api_test.js calls withSecret which sets up a fake Azure table that is fast and doesn't require you to have access to Azure. Spend a bit of time working that out and show me what you come up with, at which time I can help fill in the details -- there are a lot of parts to set up for tests and I don't want you to spend too much time on it.

Alright, I'll look around :)

That's true -- there's no need for a blacklistId if the address is always just a string. So I agree, let's do it that way.

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #1)

The code for this is in services/notify in https://github.com/taskcluster/taskcluster. You can see that there's a "blacklist" in config.yml in that application already. A few of the pieces of completing this project:

  • create an Azure table (or database table) to contain blacklisted notification destinations [1]
  • make API methods to modify that table (list, get, add, update, delete)
  • make UI changes that use those API methods to manage the data
  • make the notification service read from the table to determine if a destination is blacklisted, before sending the notification

[1] Let's not limit this to emails -- any kind of notification should be possible to blacklist

Hey, so will the UI changes be in taskcluster-web?

Indeed they will!

And any pointers on how i should go about doing that?

Will we create a new page in the side bar for it?

I think so. Hassan will be back on Monday to offer more guidance. Maybe you could make a quick sketch of what you think it might look like, for him to evaluate?

Right, almost forgot the weekend had started haha. I'll get to work then!

Alright, so I didn't get much time over the weekend as I was busy traveling but I started looking into this today. I believe we will need to write a graphql query in the web-server as well?

Also, for the blacklist, if we add a sidebar item maybe that would be a bit out of place, because as I see it, the blacklist would be used by a select few people and won't be relevant to everybody. Maybe we can make a SideBarGroup named settings and add the blacklist there or maybe we can add it somewhere else. I would love your thoughts on it, Dustin and Hassan

Component: Platform and Services → Services

Okay so Brian helped me shift the addresses from current config file to the denylist. I have also added the queries to the web-server whose PR can be found below. I believe now the only work left is to implement the UI. I am not sure where to add the blacklist in the UI as I mentioned in comment 18. Hassan can you help me a bit here :)

Flags: needinfo?(helfi92)

Awesome work on the web-server! Regarding the UI, feel free to create a new view "Blacklist" under the "Authorization" section. I created https://github.com/taskcluster/taskcluster/issues/282. Let's continue the discussion there :)

Flags: needinfo?(helfi92)

https://taskcluster-web.netlify.com/notify/denylist

Now that I think about it, is this view OK to display for everyone? I'm not sure these email addresses should be available for everyone to see.

Flags: needinfo?(dustin)

It's a good question. I think it's OK because people will wonder if their messages are being blocked by the denylist, and in general the notifications are from tasks and other public things. But I have my doubts, too.

We could also add protections to the API method and only list denylist items that the user has permission to manage. We used to do that with secrets, and it made the UI a bit confusing (it was hard to check that a secret name was correct, for example). But that's a different situation since secret names are not PII.

Let's play it safe and open a new bug to add protections to the list endpoint such that it only returns permitted entries, and adjust the UI to make it clear that things might be omitted.

Flags: needinfo?(dustin)

Created https://bugzilla.mozilla.org/show_bug.cgi?id=1542212. Ojaswin, let me know if you want me to assign it to you if you're interested in continuing your work on that :)

I would love to! Will comment on the bug

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.