1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00

Remove explicit administrative actions from the user activity log

Summary:
Depends on D20669. Ref T13343. Currently, the user activity log includes a number of explicit administrative actions which some administrator (not a normal user or a suspicious remote address) takes. In most/all cases, these changes are present in the user profile transaction log too, and that's //generally// a better place for them (for example, it doesn't get GC'd after a couple months).

Some of these are so old that they have no writers (like DELETE and EDIT). I'd generally like to modernize this a bit so we can reference it in email (see T13343) and I'd like to modularize the event types as part of that -- partly, cleaning this up makes that modularization easier.

There's maybe some hand-wavey argument that administrative vs non-administrative events could be related and might be useful to see in a single log, but I can't recall a time when that was actually true, and we could always build that kind of view later by just merging the two log sources, or by restoring double-writes for some subset of events. In practice, I've used this log mostly to look for obvious red flags when users report authentication difficulty (e.g., many unauthorized login attempts), and removing administrative actions from the log is only helpful in that use case.

Test Plan: Grepped for all the affected constants, no more hits in the codebase.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13343

Differential Revision: https://secure.phabricator.com/D20670
This commit is contained in:
epriestley 2019-07-19 10:57:13 -07:00
parent 2ee5e71029
commit 4fd473e7ed
7 changed files with 1 additions and 109 deletions

View file

@ -74,13 +74,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
throw $ex; throw $ex;
} }
$log = PhabricatorUserLog::initializeNewLog(
$this->requireActor(),
$user->getPHID(),
PhabricatorUserLog::ACTION_CREATE);
$log->setNewValue($email->getAddress());
$log->save();
if ($is_reassign) { if ($is_reassign) {
$log = PhabricatorUserLog::initializeNewLog( $log = PhabricatorUserLog::initializeNewLog(
$this->requireActor(), $this->requireActor(),
@ -100,35 +93,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
} }
/**
* @task edit
*/
public function updateUser(
PhabricatorUser $user,
PhabricatorUserEmail $email = null) {
if (!$user->getID()) {
throw new Exception(pht('User has not been created yet!'));
}
$user->openTransaction();
$user->save();
if ($email) {
$email->save();
}
$log = PhabricatorUserLog::initializeNewLog(
$this->requireActor(),
$user->getPHID(),
PhabricatorUserLog::ACTION_EDIT);
$log->save();
$user->saveTransaction();
return $this;
}
/* -( Editing Roles )------------------------------------------------------ */ /* -( Editing Roles )------------------------------------------------------ */
/** /**
@ -151,18 +115,9 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
return $this; return $this;
} }
$log = PhabricatorUserLog::initializeNewLog(
$actor,
$user->getPHID(),
PhabricatorUserLog::ACTION_SYSTEM_AGENT);
$log->setOldValue($user->getIsSystemAgent());
$log->setNewValue($system_agent);
$user->setIsSystemAgent((int)$system_agent); $user->setIsSystemAgent((int)$system_agent);
$user->save(); $user->save();
$log->save();
$user->endWriteLocking(); $user->endWriteLocking();
$user->saveTransaction(); $user->saveTransaction();
@ -189,18 +144,9 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
return $this; return $this;
} }
$log = PhabricatorUserLog::initializeNewLog(
$actor,
$user->getPHID(),
PhabricatorUserLog::ACTION_MAILING_LIST);
$log->setOldValue($user->getIsMailingList());
$log->setNewValue($mailing_list);
$user->setIsMailingList((int)$mailing_list); $user->setIsMailingList((int)$mailing_list);
$user->save(); $user->save();
$log->save();
$user->endWriteLocking(); $user->endWriteLocking();
$user->saveTransaction(); $user->saveTransaction();

View file

@ -11,16 +11,6 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
const ACTION_LOGIN_LEGALPAD = 'login-legalpad'; const ACTION_LOGIN_LEGALPAD = 'login-legalpad';
const ACTION_RESET_PASSWORD = 'reset-pass'; const ACTION_RESET_PASSWORD = 'reset-pass';
const ACTION_CREATE = 'create';
const ACTION_EDIT = 'edit';
const ACTION_ADMIN = 'admin';
const ACTION_SYSTEM_AGENT = 'system-agent';
const ACTION_MAILING_LIST = 'mailing-list';
const ACTION_DISABLE = 'disable';
const ACTION_APPROVE = 'approve';
const ACTION_DELETE = 'delete';
const ACTION_CONDUIT_CERTIFICATE = 'conduit-cert'; const ACTION_CONDUIT_CERTIFICATE = 'conduit-cert';
const ACTION_CONDUIT_CERTIFICATE_FAILURE = 'conduit-cert-fail'; const ACTION_CONDUIT_CERTIFICATE_FAILURE = 'conduit-cert-fail';
@ -31,7 +21,6 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
const ACTION_EMAIL_REASSIGN = 'email-reassign'; const ACTION_EMAIL_REASSIGN = 'email-reassign';
const ACTION_CHANGE_PASSWORD = 'change-password'; const ACTION_CHANGE_PASSWORD = 'change-password';
const ACTION_CHANGE_USERNAME = 'change-username';
const ACTION_ENTER_HISEC = 'hisec-enter'; const ACTION_ENTER_HISEC = 'hisec-enter';
const ACTION_EXIT_HISEC = 'hisec-exit'; const ACTION_EXIT_HISEC = 'hisec-exit';
@ -59,14 +48,6 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
pht('Login: Signed Required Legalpad Documents'), pht('Login: Signed Required Legalpad Documents'),
self::ACTION_LOGOUT => pht('Logout'), self::ACTION_LOGOUT => pht('Logout'),
self::ACTION_RESET_PASSWORD => pht('Reset Password'), self::ACTION_RESET_PASSWORD => pht('Reset Password'),
self::ACTION_CREATE => pht('Create Account'),
self::ACTION_EDIT => pht('Edit Account'),
self::ACTION_ADMIN => pht('Add/Remove Administrator'),
self::ACTION_SYSTEM_AGENT => pht('Add/Remove System Agent'),
self::ACTION_MAILING_LIST => pht('Add/Remove Mailing List'),
self::ACTION_DISABLE => pht('Enable/Disable'),
self::ACTION_APPROVE => pht('Approve Registration'),
self::ACTION_DELETE => pht('Delete User'),
self::ACTION_CONDUIT_CERTIFICATE self::ACTION_CONDUIT_CERTIFICATE
=> pht('Conduit: Read Certificate'), => pht('Conduit: Read Certificate'),
self::ACTION_CONDUIT_CERTIFICATE_FAILURE self::ACTION_CONDUIT_CERTIFICATE_FAILURE
@ -77,7 +58,6 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
self::ACTION_EMAIL_VERIFY => pht('Email: Verify'), self::ACTION_EMAIL_VERIFY => pht('Email: Verify'),
self::ACTION_EMAIL_REASSIGN => pht('Email: Reassign'), self::ACTION_EMAIL_REASSIGN => pht('Email: Reassign'),
self::ACTION_CHANGE_PASSWORD => pht('Change Password'), self::ACTION_CHANGE_PASSWORD => pht('Change Password'),
self::ACTION_CHANGE_USERNAME => pht('Change Username'),
self::ACTION_ENTER_HISEC => pht('Hisec: Enter'), self::ACTION_ENTER_HISEC => pht('Hisec: Enter'),
self::ACTION_EXIT_HISEC => pht('Hisec: Exit'), self::ACTION_EXIT_HISEC => pht('Hisec: Exit'),
self::ACTION_FAIL_HISEC => pht('Hisec: Failed Attempt'), self::ACTION_FAIL_HISEC => pht('Hisec: Failed Attempt'),

View file

@ -19,10 +19,6 @@ final class PhabricatorUserApproveTransaction
public function applyExternalEffects($object, $value) { public function applyExternalEffects($object, $value) {
$user = $object; $user = $object;
$this->newUserLog(PhabricatorUserLog::ACTION_APPROVE)
->setOldValue((bool)$user->getIsApproved())
->setNewValue((bool)$value)
->save();
$actor = $this->getActor(); $actor = $this->getActor();
$title = pht( $title = pht(

View file

@ -17,13 +17,6 @@ final class PhabricatorUserDisableTransaction
$object->setIsDisabled((int)$value); $object->setIsDisabled((int)$value);
} }
public function applyExternalEffects($object, $value) {
$this->newUserLog(PhabricatorUserLog::ACTION_DISABLE)
->setOldValue((bool)$object->getIsDisabled())
->setNewValue((bool)$value)
->save();
}
public function getTitle() { public function getTitle() {
$new = $this->getNewValue(); $new = $this->getNewValue();
if ($new) { if ($new) {

View file

@ -17,15 +17,6 @@ final class PhabricatorUserEmpowerTransaction
$object->setIsAdmin((int)$value); $object->setIsAdmin((int)$value);
} }
public function applyExternalEffects($object, $value) {
$user = $object;
$this->newUserLog(PhabricatorUserLog::ACTION_ADMIN)
->setOldValue($this->getOldValue())
->setNewValue($value)
->save();
}
public function validateTransactions($object, array $xactions) { public function validateTransactions($object, array $xactions) {
$user = $object; $user = $object;
$actor = $this->getActor(); $actor = $this->getActor();

View file

@ -1,13 +1,4 @@
<?php <?php
abstract class PhabricatorUserTransactionType abstract class PhabricatorUserTransactionType
extends PhabricatorModularTransactionType { extends PhabricatorModularTransactionType {}
protected function newUserLog($action) {
return PhabricatorUserLog::initializeNewLog(
$this->getActor(),
$this->getObject()->getPHID(),
$action);
}
}

View file

@ -24,11 +24,6 @@ final class PhabricatorUserUsernameTransaction
$old_username = $this->getOldValue(); $old_username = $this->getOldValue();
$new_username = $this->getNewValue(); $new_username = $this->getNewValue();
$this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME)
->setOldValue($old_username)
->setNewValue($new_username)
->save();
// The SSH key cache currently includes usernames, so dirty it. See T12554 // The SSH key cache currently includes usernames, so dirty it. See T12554
// for discussion. // for discussion.
PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache(); PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache();