@epriestley Can you take a look at P1195 for sanity checking purposes and let me know if this is a valid way to do it?

A herald action might be more ideal but the only major issue we have so far with the extension is that we have to deploy the listener class and we don't have any way to ensure that phabricator loads the extension. Consequently, @chase.mp is concerned that tasks could still be submitted to the security project without the extension loaded and they would silently end up publicly visible.

Extensions: For events, you can subclass PhabricatorAutoEventListener and drop files in src/extensions/, although I think you have enough stuff in general that it's worthwhile to build a real external library. You could maybe make sure the extension is loaded by having a restart script like this:

• Stop httpd.
• Bring up iptables, blocking external connections from port 80.
• Update/restart daemons/etc.
• Start httpd.
• Make a request to some URI you define in your extension, like /wmf/is-wmf-extension-loaded/. If this fails or does not return the expected response, die with an error and leave iptables up.
• If you get a 200 and a good response, bring down iptables and accept connections.

This is complicated, but should prevent mistakes and can be handled by whatever deployment automation stuff you use. This is similar to the method Facebook uses to push servers. You can look at PhabricatorStatusController for an example of a status controller (you can access /status/ to see the results). You can look at T5051 for a skeletal extension which adds routes, etc.

In general, you can subclass PhabricatorAutoEventListener to automatically install the listener when the event class is loaded, without needing to worry about the value of separate config.

Approach: I think there are three reasonable approaches:

• Events (as P1195).
  • Pros: you've already written it.
  • Cons: this is less-supported by the upstream in the future, might go away completely, you'll have to load an extension, you won't get transactions in the transaction log, moderate flexibility, database can end up in inconsistent state, moving things out of "security" requires editing the policy too.
• Herald custom action: this is new (D8784) and not documented yet (T5194) but has a brighter future in the upstream. You would subclass HeraldCustomAction, then write a rule like "When a Manipest task is updated, if [always] take these actions [adjust policies]". Then your "adjust policies" action would emit a transaction which changes statuses.
  • Pros: more-supported by the upstream in the future, shows a transaction in the transaction log.
  • Cons: new, maybe a bit unstable, you'll have to load an extension, moderate flexibility, database can end up in an inconsistent state, moving things out of "security" requires multiple changes.
• Patch the policy filter directly.
  • Pros: more flexible, maybe simplest.
  • Cons: local fork, very magical, no transaction record.

The policy filter patch would look something like this (I haven't tested or run this code):

diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index f2b84d6..63bcef6 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -128,7 +128,11 @@ final class PhabricatorPolicyFilter {
       $this->userProjects[$viewer_phid] = array();
     }
 
-    $need_projects = array();
+    // PHID of the "security" project.
+    $security_phid = 'PHID-PROJ-SECURITY';
+
+    // Always load membership in the "Security" project.
+    $need_projects = array($security_phid);
     $need_policies = array();
     foreach ($objects as $key => $object) {
       $object_capabilities = $object->getCapabilities();
@@ -205,6 +209,19 @@ final class PhabricatorPolicyFilter {
       $filtered[$key] = $object;
     }
 
+    // Filter out any "Security" tasks if the user isn't a member of the
+    // "Security" project.
+    foreach ($filtered as $key => $object) {
+      if ($object instanceof ManiphestTask) {
+        $project_phids = $object->getProjectPHIDs();
+        if (in_array($security_phid, $project_phids)) {
+          if (empty($this->userProjects[$viewer_phid][$security_phid])) {
+            unset($filtered[$key]);
+          }
+        }
+      }
+    }
+
     return $filtered;
   }

Basically: after filtering objects, do another hard-coded filter. This makes the "Security" project mean that the filtering rules should be applied, rather than requiring a second-degree interaction. However, it's also more magical, since the UI won't reflect any of this.

I lean slightly toward the Herald or "local patch" approaches, but I think the tradeoffs aren't too different, all considered. I'm also not sure the event always fires right now (e.g., email or conduit edits might not fire it?).

Event: The only feedback I have on the event handler is that the policies you want to set are probably $project->getPHID() (which means "members of project X". The policies you're currently setting ($project->getViewPolicy(), for example) might be different.

In particular, if you want members of the public to be able to tag things with "Security", you need to make it visible to "All Users" or "Public". As written, the code will then set the task visibility to "All Users" (or public). If you use $project->getPHID() instead, it will set task visibility to "Members of project Security", which is probably what you want.

Thanks Evan for the thorough response.

The herald custom action sounds like what I was looking for. I think I will attempt to get it going as a custom action and I may be able to help with T5194 as a result, though really, @epriestley's comment above is decently good documentation on it's own.

Ported to a custom herald action: P1204