Bug 255985

Summary: Add Herald:Webhooks permission to user account (or freebsd devs) for IRC/Discord/other review notifications
Product: Services Reporter: Kubilay Kocak <koobs>
Component: Code ReviewAssignee: Phabric Admin <phabric-admin>
Status: Open ---    
Severity: Affects Some People CC: lcook, lwhsu
Priority: --- Keywords: feature, needs-qa
Version: unspecified   
Hardware: Any   
OS: Any   

Description Kubilay Kocak freebsd_committer freebsd_triage 2021-05-19 03:47:54 UTC
We have a system that takes events (via webhooks) from various sytems, including github (src, ports, doc) and notifies various destinations (irc, discord, etc) about these events. 

We'd like to add reviews to these, which requires Heralrd:Webhook [1] permissions, which dont appear to be available to freebsd committers by default.

This request is to add webhook permissions to my koobs account, or better, to enable the permission for freebsd committers/developer group, as there is value for event notifications in many forms

[1] https://secure.phabricator.com/book/phabricator/article/webhooks/
Comment 1 Li-Wen Hsu freebsd_committer 2021-05-19 07:12:57 UTC
Can you tell us more about that system? It sounds useful and we would like to know more to have a better support of it.

Also we would like to check more about the webhook's security and management model.  Enabling a special permission to a personal account or all developer too early has some concerts like increasing system load, outgoing traffic, and would cause trouble to users when we need to revert if we find it doesn't work or has serious issue.

I suggest let's start with a headless test account or other mechanism at the staging env https://reviews-dev.freebsd.org/ , to verify the work flow and data flow.  And because of the phabricator data model, it's very hard to check the object settings owned by a user, is it possible for you to share the planned configure of Heralrd:webbook?
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2021-05-19 07:40:00 UTC
(In reply to Li-Wen Hsu from comment #1)

To simplify things, consider this then a request to add webhooks permissions just to my account rather than enabling the feature for anyone.

At this stage we're only interested in newly created reviews events. I'm not familiar with what options are available, but the Phabricator documentation mentions writing Herald rules, which can already be used within Phabricator.

There's nothing additional to know know about the system besides it being like every other webhook endpoint that, that takes events (POST'ed with json or urlencoded payloads) and does things with them, such as post messages to IRC or Discord, or elsewhere.

I see no need to gate this on development or staging testing, as per above, herald rules are already in enabled, in use in the production system and available for people to use as various subsystem queries.

If however this is a non-negotiable for whatever reason, please provide all of the details required to do the testing.

I tried logging into the url provided, but received an email verification notice, and did not receive an email verification.
Comment 3 Li-Wen Hsu freebsd_committer 2021-05-19 08:11:20 UTC
(In reply to Kubilay Kocak from comment #2)
As both of don't know the side-effect of enabling this, it's better to do proper QA.  Currently Herald is mostly used for adding reviewers and subscribers.

The Heralrd:webbook is not enabled by the phabric-admin in the beginning because it's letting phabric do the external communications, and may leak the information we concern or post to the destination we suppose not want (which may look like reviews.freebsd.org is attacking others). Of course we can relex this restriction, but let's try to not affect the production system at this stage.

The steps:

- Please provide the detail Herald rule you want to create.  If you can do that in the production system but cannot let it take effect, that's OK because we can still review the settings. If that special webhook permission needs to be enabled first, let's check how to do so in the staging env.  Note that we even don't know if it is possible to just to enable that permission to a single account, nor the side-effect of other accounts.

- I won't object to enable this to an individual at the test stage, but I strongly suggest when moving it to permanent, we need to let more people can adjust the configure to avoid single-point-of-failure issue. The phabricator security model makes things very hard to touch the setting owned by an user, even the system admin.
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2021-05-20 00:03:19 UTC
(In reply to Li-Wen Hsu from comment #3)

I cant know the rule to create because I don't know whats available or how they are configured. If phabric admin can add the relevent permissions I can look and detail that.

Please organise the relevent test environment. Per last comment I can't login to the staging/test environment using existing credentials and no email was generated to verify email.