From 9adbb49fc9c134539fb8311612ccc6f67637e6d8 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Tue, 29 Mar 2011 16:20:12 -0700 Subject: [PATCH] Consolidate some precondition checks for repeats, fix a couple bits in the repeat command. Notice::saveNew() now does these checks directly when making a repeat: * make sure the original is valid and existing * stop you from repeating your own message * stop you from repeating something you've previously repeated * prevent repeats of any non-public messages * explicit inScope() check to make sure you can read the original too (just in case there's a funky extension at play that changes scoping rules) These error conditions throw exceptions, which the caller either uses as an error message or passes on up the stack, without having to duplicate the checks in each i/o channel. --- actions/apistatusesretweet.php | 36 ------------------------------- actions/repeat.php | 29 ------------------------- classes/Notice.php | 33 +++++++++++++++++++++++++--- lib/command.php | 39 +++++----------------------------- 4 files changed, 35 insertions(+), 102 deletions(-) diff --git a/actions/apistatusesretweet.php b/actions/apistatusesretweet.php index 2bc9092ba6..4832da1823 100644 --- a/actions/apistatusesretweet.php +++ b/actions/apistatusesretweet.php @@ -78,42 +78,6 @@ class ApiStatusesRetweetAction extends ApiAuthAction $this->user = $this->auth_user; - if ($this->user->id == $this->original->profile_id) { - // TRANS: Client error displayed trying to repeat an own notice through the API. - $this->clientError(_('Cannot repeat your own notice.'), - 400, $this->format); - return false; - } - - // Is it OK to repeat that notice (general enough scope)? - - if ($this->original->scope != Notice::SITE_SCOPE && - $this->original->scope != Notice::PUBLIC_SCOPE) { - $this->clientError(_('You may not repeat a private notice.'), - 403, - $this->format); - return false; - } - - $profile = $this->user->getProfile(); - - // Can the profile actually see that notice? - - if (!$this->original->inScope($profile)) { - $this->clientError(_('No access to that notice.'), - 403, - $this->format); - return false; - } - - if ($profile->hasRepeated($id)) { - // TRANS: Client error displayed trying to re-repeat a notice through the API. - $this->clientError(_('Already repeated that notice.'), - 400, $this->format); - return false; - } - - return true; } diff --git a/actions/repeat.php b/actions/repeat.php index 4201a4ce95..44c57456fa 100644 --- a/actions/repeat.php +++ b/actions/repeat.php @@ -73,20 +73,6 @@ class RepeatAction extends Action return false; } - // Is it OK to repeat that notice (general enough scope)? - - if ($this->notice->scope != Notice::SITE_SCOPE && - $this->notice->scope != Notice::PUBLIC_SCOPE) { - $this->clientError(_('You may not repeat a private notice.'), - 403); - } - - if ($this->user->id == $this->notice->profile_id) { - // TRANS: Client error displayed when trying to repeat an own notice. - $this->clientError(_('You cannot repeat your own notice.')); - return false; - } - $token = $this->trimmed('token-'.$id); if (empty($token) || $token != common_session_token()) { @@ -94,21 +80,6 @@ class RepeatAction extends Action return false; } - $profile = $this->user->getProfile(); - - // Can the profile actually see that notice? - - if (!$this->notice->inScope($profile)) { - $this->clientError(_('No access to that notice.'), 403); - } - - - if ($profile->hasRepeated($id)) { - // TRANS: Client error displayed when trying to repeat an already repeated notice. - $this->clientError(_('You already repeated that notice.')); - return false; - } - return true; } diff --git a/classes/Notice.php b/classes/Notice.php index b5eafb0ffa..81ccd8d74f 100644 --- a/classes/Notice.php +++ b/classes/Notice.php @@ -350,13 +350,31 @@ class Notice extends Memcached_DataObject $repeat = Notice::staticGet('id', $repeat_of); - if (!empty($repeat) && - $repeat->scope != Notice::SITE_SCOPE && + if (empty($repeat)) { + throw new ClientException(_('Cannot repeat; original notice is missing or deleted.')); + } + + if ($profile->id == $repeat->profile_id) { + // TRANS: Client error displayed when trying to repeat an own notice. + throw new ClientException(_('You cannot repeat your own notice.')); + } + + if ($repeat->scope != Notice::SITE_SCOPE && $repeat->scope != Notice::PUBLIC_SCOPE) { + // TRANS: Client error displayed when trying to repeat a non-public notice. throw new ClientException(_('Cannot repeat a private notice.'), 403); } - // XXX: Check for access...? + if (!$repeat->inScope($profile)) { + // The generic checks above should cover this, but let's be sure! + // TRANS: Client error displayed when trying to repeat a notice you cannot access. + throw new ClientException(_('Cannot repeat a notice you cannot read.'), 403); + } + + if ($profile->hasRepeated($repeat->id)) { + // TRANS: Client error displayed when trying to repeat an already repeated notice. + throw new ClientException(_('You already repeated that notice.')); + } $notice->repeat_of = $repeat_of; } else { @@ -1567,6 +1585,15 @@ class Notice extends Memcached_DataObject return $location; } + /** + * Convenience function for posting a repeat of an existing message. + * + * @param int $repeater_id: profile ID of user doing the repeat + * @param string $source: posting source key, eg 'web', 'api', etc + * @return Notice + * + * @throws Exception on failure or permission problems + */ function repeat($repeater_id, $source) { $author = Profile::staticGet('id', $this->profile_id); diff --git a/lib/command.php b/lib/command.php index 35d0702684..aaad577761 100644 --- a/lib/command.php +++ b/lib/command.php @@ -537,44 +537,15 @@ class RepeatCommand extends Command { $notice = $this->getNotice($this->other); - if($this->user->id == $notice->profile_id) - { - // TRANS: Error text shown when trying to repeat an own notice. - $channel->error($this->user, _('Cannot repeat your own notice.')); - return; - } - - // Is it OK to repeat that notice (general enough scope)? - - if ($notice->scope != Notice::SITE_SCOPE && - $notice->scope != Notice::PUBLIC_SCOPE) { - $channel->error($this->user, _('You may not repeat a private notice.')); - } - - $profile = $this->user->getProfile(); - - // Can the profile actually see that notice? - - if (!$notice->inScope($profile)) { - $channel->error($this->user, _('You have no access to that notice.')); - } - - if ($profile->hasRepeated($notice->id)) { - // TRANS: Error text shown when trying to repeat an notice that was already repeated by the user. - $channel->error($this->user, _('Already repeated that notice.')); - return; - } - - $repeat = $notice->repeat($this->user->id, $channel->source); - - if ($repeat) { + try { + $repeat = $notice->repeat($this->user->id, $channel->source()); + $recipient = $notice->getProfile(); // TRANS: Message given having repeated a notice from another user. // TRANS: %s is the name of the user for which the notice was repeated. $channel->output($this->user, sprintf(_('Notice from %s repeated.'), $recipient->nickname)); - } else { - // TRANS: Error text shown when repeating a notice fails with an unknown reason. - $channel->error($this->user, _('Error repeating notice.')); + } catch (Exception $e) { + $channel->error($this->user, $e->getMessage()); } } }