Thank you for the review, @avivey !

I'm having trouble landing this, I keep getting 403 errors. I suspect it's a local configuration issue, though...

In D25035#1051, @speck wrote:

I'm having trouble landing this, I keep getting 403 errors. I suspect it's a local configuration issue, though...

All that should be required to land is being in Blessed Committers I think, which you are a member of

I did confirm that is the case. After some troubleshooting, I think my local Git install is not aware of my ssh key.

Real quick before landing -- should this change be made here in PhabricatorUser or would it be sufficient in PhabricatorPeopleProfileController? Placing it here affects the profile at the data model source which would likely cause the same blurb-scrub in any other location it might render, but it might also cause problems in areas which need to access the profile data for other reasons other than rendering, e.g. if a profile gets copied/cloned in memory then this might result in losing the profile data altogether. Updating only PhabricatoPeopleProfileController to call cleanupProfile() instead of within PhabricatorUser would only scrub it at the time it's being rendered (to the profile page at least).

In D25035#1059, @speck wrote:

Real quick before landing -- should this change be made here in PhabricatorUser or would it be sufficient in PhabricatorPeopleProfileController? Placing it here affects the profile at the data model source which would likely cause the same blurb-scrub in any other location it might render, but it might also cause problems in areas which need to access the profile data for other reasons other than rendering, e.g. if a profile gets copied/cloned in memory then this might result in losing the profile data altogether. Updating only PhabricatoPeopleProfileController to call cleanupProfile() instead of within PhabricatorUser would only scrub it at the time it's being rendered (to the profile page at least).

Good question! I went with the PhabricatorUser route because it's entirely possible for the PhabricatorUserProfile object $this->profile to be null for a large part of the lifecycle of these page. I could clean the profile in the PhabricatorPeopleProfileController::buildProfileHeader() because that's where the profile is loaded... Would you rather that?

fwiw, this is how I handled it in the wikimedia fork:

commit 8282af06e8948f7d3a9c197f10ea99b005e5c252                                                                        
Refs: <release/2020-05-14/1>                                                                                           
Author:     Mukunda Modell <mmodell@wikimedia.org>                                                                     
AuthorDate: Fri May 1 04:53:08 2020 -0500                                                                              
Commit:     Mukunda Modell <mmodell@wikimedia.org>                                                                     
CommitDate: Fri May 1 04:53:08 2020 -0500                                                                              

    Don't display user 'blurb' for disabled accounts.

    Bug: T250946
---                                                                                                                    
 src/applications/people/customfield/PhabricatorUserBlurbField.php | 5 +++++                                           
 1 file changed, 5 insertions(+)

diff --git a/src/applications/people/customfield/PhabricatorUserBlurbField.php b/src/applications/people/customfield/PhabricatorUserBlurbField.php                                                                                           
index 222061ad0444..e0c1946619aa 100644                                                                                
--- a/src/applications/people/customfield/PhabricatorUserBlurbField.php                                                
+++ b/src/applications/people/customfield/PhabricatorUserBlurbField.php                                                
@@ -83,6 +83,11 @@ final class PhabricatorUserBlurbField                                                               
   }

   public function renderPropertyViewValue(array $handles) {
+    // WMF Hack: Don't display profile of disabled users. See T250946                                                 
+    if ($this->getObject()->getIsDisabled()) {                                                                        
+      return pht('This account has been disabled.');                                                                  
+    }                                                                                                                 
+                                                                                                                      
     $blurb = $this->getObject()->loadUserProfile()->getBlurb();
     if (!strlen($blurb)) {
       return null;

Thanks again. Generally super-useful.

Corner case: after this change, it's not possible anymore to set a description to a disabled user. Like "disabled because bla bla". Or: "bot migrated to this different account: xzy". Probably, disabling an user should create an "expand transaction", but we should not cleanup everytime.

(I cannot see T15074 - was it about spam? I would like to create a wishlist Sub-task if possible)

(It's about spam, but it doesn't have anything explicitly sensitive. I've made it public now)