From a50b8e39b12ce9b046702894114238b47c872314 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Oct 2012 14:50:04 -0700 Subject: [PATCH] Make posts 1:1 with blogs and implement policy controls Summary: This leaves the UI in a pretty rough state, but implements blog policy controls and queries, and 1:1 relationships between posts and blogs. Needs a bunch more cleanup but seemed like an okayish breaking point in terms of cohesiveness. Posts have these rules: - Drafts are visible only to the author. - Published posts are visible to anyone who can see the blog they appear on. - Posts are only editable by the author. ...so we don't need any special policy UI or state to accommodate these rules. Posts may have no blog if they're grandfathered in or you write a post to a blog and then lose the ability to see the blog. This is the messiest edge case -- specifically: - You write a post to blog A. - You publish the post. - I edit the "Visible To:" for blog A and set it to exclude you. What we do in this case is let you see the post in "My Posts", but you can no longer see the blog and you'll see the post as not being part of a blog. We can maybe give you some UI to let you move it later or something. Test Plan: Hit all (I think?) of the interfaces without issues. Definitely some UI problems still right now. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1373 Differential Revision: https://secure.phabricator.com/D3694 --- resources/sql/patches/phameoneblog.sql | 2 + .../blog/PhameBlogViewController.php | 95 ++----------------- .../post/PhamePostDeleteController.php | 25 ++--- .../post/PhamePostEditController.php | 16 ++-- .../post/PhamePostViewController.php | 36 +++---- .../post/list/PhameAllPostListController.php | 4 +- .../list/PhameBloggerPostListController.php | 7 +- .../post/list/PhameDraftListController.php | 8 +- .../post/list/PhameUserPostListController.php | 7 +- .../phame/query/PhamePostQuery.php | 94 +++++++++--------- src/applications/phame/storage/PhamePost.php | 60 +++++++++++- .../phame/view/PhameBlogDetailView.php | 7 -- .../phame/view/PhameBlogListView.php | 5 +- .../patch/PhabricatorBuiltinPatchList.php | 4 + 14 files changed, 177 insertions(+), 193 deletions(-) create mode 100644 resources/sql/patches/phameoneblog.sql diff --git a/resources/sql/patches/phameoneblog.sql b/resources/sql/patches/phameoneblog.sql new file mode 100644 index 0000000000..4796914d55 --- /dev/null +++ b/resources/sql/patches/phameoneblog.sql @@ -0,0 +1,2 @@ +ALTER TABLE `{$NAMESPACE}_phame`.`phame_post` + ADD `blogPHID` varchar(64) COLLATE utf8_bin; diff --git a/src/applications/phame/controller/blog/PhameBlogViewController.php b/src/applications/phame/controller/blog/PhameBlogViewController.php index dcf89570b5..6be892a40c 100644 --- a/src/applications/phame/controller/blog/PhameBlogViewController.php +++ b/src/applications/phame/controller/blog/PhameBlogViewController.php @@ -19,28 +19,9 @@ /** * @group phame */ -final class PhameBlogViewController - extends PhameController { +final class PhameBlogViewController extends PhameController { private $blogPHID; - private $bloggerPHIDs; - private $postPHIDs; - - private function setPostPHIDs($post_phids) { - $this->postPHIDs = $post_phids; - return $this; - } - private function getPostPHIDs() { - return $this->postPHIDs; - } - - private function setBloggerPHIDs($blogger_phids) { - $this->bloggerPHIDs = $blogger_phids; - return $this; - } - private function getBloggerPHIDs() { - return $this->bloggerPHIDs; - } private function setBlogPHID($blog_phid) { $this->blogPHID = $blog_phid; @@ -54,6 +35,7 @@ final class PhameBlogViewController $filter = 'blog/view/'.$this->getBlogPHID(); return $filter; } + protected function getSideNavExtraBlogFilters() { $filters = array( array('key' => $this->getSideNavFilter(), @@ -67,57 +49,26 @@ final class PhameBlogViewController } public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); - $blog_phid = $this->getBlogPHID(); + $request = $this->getRequest(); + $user = $request->getUser(); $blog = id(new PhameBlogQuery()) ->setViewer($user) - ->withPHIDs(array($blog_phid)) + ->withPHIDs(array($this->getBlogPHID())) ->executeOne(); if (!$blog) { return new Aphront404Response(); } - $this->loadEdges(); - - $blogger_phids = $this->getBloggerPHIDs(); - if ($blogger_phids) { - $bloggers = $this->loadViewerHandles($blogger_phids); - } else { - $bloggers = array(); - } - - $post_phids = $this->getPostPHIDs(); - if ($post_phids) { - $posts = id(new PhamePostQuery()) - ->withPHIDs($post_phids) - ->withVisibility(PhamePost::VISIBILITY_PUBLISHED) - ->execute(); - } else { - $posts = array(); - } - - $notice = array(); - if ($request->getExists('new')) { - $notice = - array( - 'title' => 'Successfully created your blog.', - 'body' => 'Time to write some posts.' - ); - } else if ($request->getExists('edit')) { - $notice = - array( - 'title' => 'Successfully edited your blog.', - 'body' => 'Time to write some posts.' - ); - } + $posts = id(new PhamePostQuery()) + ->setViewer($user) + ->withBlogPHIDs(array($blog->getPHID())) + ->execute(); $skin = $blog->getSkinRenderer(); $skin ->setUser($this->getRequest()->getUser()) - ->setNotice($notice) - ->setBloggers($bloggers) + ->setBloggers($this->loadViewerHandles(mpull($posts, 'getBloggerPHID'))) ->setPosts($posts) ->setBlog($blog) ->setRequestURI($this->getRequest()->getRequestURI()); @@ -135,30 +86,4 @@ final class PhameBlogViewController 'title' => $blog->getName(), )); } - - private function loadEdges() { - - $edge_types = array( - PhabricatorEdgeConfig::TYPE_BLOG_HAS_BLOGGER, - PhabricatorEdgeConfig::TYPE_BLOG_HAS_POST, - ); - $blog_phid = $this->getBlogPHID(); - $phids = array($blog_phid); - - $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($phids) - ->withEdgeTypes($edge_types) - ->execute(); - - $blogger_phids = array_keys( - $edges[$blog_phid][PhabricatorEdgeConfig::TYPE_BLOG_HAS_BLOGGER] - ); - $this->setBloggerPHIDs($blogger_phids); - - $post_phids = array_keys( - $edges[$blog_phid][PhabricatorEdgeConfig::TYPE_BLOG_HAS_POST] - ); - $this->setPostPHIDs($post_phids); - - } } diff --git a/src/applications/phame/controller/post/PhamePostDeleteController.php b/src/applications/phame/controller/post/PhamePostDeleteController.php index 3e0db52cbf..0108756081 100644 --- a/src/applications/phame/controller/post/PhamePostDeleteController.php +++ b/src/applications/phame/controller/post/PhamePostDeleteController.php @@ -40,32 +40,33 @@ extends PhameController { public function processRequest() { $request = $this->getRequest(); $user = $request->getUser(); - $post_phid = $this->getPostPHID(); - $posts = id(new PhamePostQuery()) - ->withPHIDs(array($post_phid)) - ->execute(); - $post = reset($posts); - if (empty($post)) { + + $post = id(new PhamePostQuery()) + ->setViewer($user) + ->withPHIDs(array($this->getPostPHID())) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$post) { return new Aphront404Response(); } - if ($post->getBloggerPHID() != $user->getPHID()) { - return new Aphront403Response(); - } $post_noun = $post->getHumanName(); if ($request->isFormPost()) { $edge_type = PhabricatorEdgeConfig::TYPE_POST_HAS_BLOG; $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array($post_phid)) + ->withSourcePHIDs(array($post->getPHID())) ->withEdgeTypes(array($edge_type)) ->execute(); - $blog_edges = $edges[$post_phid][$edge_type]; + $blog_edges = $edges[$post->getPHID()][$edge_type]; $blog_phids = array_keys($blog_edges); $editor = id(new PhabricatorEdgeEditor()) ->setActor($user); foreach ($blog_phids as $phid) { - $editor->removeEdge($post_phid, $edge_type, $phid); + $editor->removeEdge($post->getPHID(), $edge_type, $phid); } $editor->save(); diff --git a/src/applications/phame/controller/post/PhamePostEditController.php b/src/applications/phame/controller/post/PhamePostEditController.php index 547e2fadf9..5cf23497b6 100644 --- a/src/applications/phame/controller/post/PhamePostEditController.php +++ b/src/applications/phame/controller/post/PhamePostEditController.php @@ -115,16 +115,18 @@ final class PhamePostEditController $errors = array(); if ($this->isPostEdit()) { - $posts = id(new PhamePostQuery()) + $post = id(new PhamePostQuery()) + ->setViewer($user) ->withPHIDs(array($this->getPostPHID())) - ->execute(); - $post = reset($posts); - if (empty($post)) { + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$post) { return new Aphront404Response(); } - if ($post->getBloggerPHID() != $user->getPHID()) { - return new Aphront403Response(); - } + $post_noun = ucfirst($post->getHumanName()); $cancel_uri = $post->getViewURI($user->getUsername()); $submit_button = 'Save Changes'; diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index 085b7ccb8b..0f225d1599 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -19,8 +19,7 @@ /** * @group phame */ -final class PhamePostViewController -extends PhameController { +final class PhamePostViewController extends PhameController { private $postPHID; private $phameTitle; @@ -75,29 +74,21 @@ extends PhameController { public function processRequest() { $request = $this->getRequest(); $user = $request->getUser(); - $post_phid = null; if ($this->getPostPHID()) { - $post_phid = $this->getPostPHID(); - if (!$post_phid) { - return new Aphront404Response(); - } + $post = id(new PhamePostQuery()) + ->setViewer($user) + ->withPHIDs(array($this->getPostPHID())) + ->executeOne(); - $posts = id(new PhamePostQuery()) - ->withPHIDs(array($post_phid)) - ->execute(); - $post = reset($posts); - - if ($post) { - $this->setPhameTitle($post->getPhameTitle()); - $blogger = PhabricatorObjectHandleData::loadOneHandle( - $post->getBloggerPHID(), - $user); - if (!$blogger) { - return new Aphront404Response(); - } + if (!$post) { + return new Aphront404Response(); } + $this->setPhameTitle($post->getPhameTitle()); + $blogger = PhabricatorObjectHandleData::loadOneHandle( + $post->getBloggerPHID(), + $user); } else if ($this->getBloggerName() && $this->getPhameTitle()) { $phame_title = $this->getPhameTitle(); $phame_title = PhabricatorSlug::normalize($phame_title); @@ -114,8 +105,9 @@ extends PhameController { return new Aphront404Response(); } $posts = id(new PhamePostQuery()) - ->withBloggerPHID($blogger->getPHID()) - ->withPhameTitle($phame_title) + ->setViewer($user) + ->withBloggerPHIDs(array($blogger->getPHID())) + ->withPhameTitles(array($phame_title)) ->execute(); $post = reset($posts); diff --git a/src/applications/phame/controller/post/list/PhameAllPostListController.php b/src/applications/phame/controller/post/list/PhameAllPostListController.php index 1f26c70ec2..0d6ce42284 100644 --- a/src/applications/phame/controller/post/list/PhameAllPostListController.php +++ b/src/applications/phame/controller/post/list/PhameAllPostListController.php @@ -76,8 +76,8 @@ final class PhameAllPostListController public function processRequest() { $user = $this->getRequest()->getUser(); - $query = new PhamePostQuery(); - $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $query = id(new PhamePostQuery()) + ->setViewer($user); $this->setPhamePostQuery($query); $this->setActions(array()); diff --git a/src/applications/phame/controller/post/list/PhameBloggerPostListController.php b/src/applications/phame/controller/post/list/PhameBloggerPostListController.php index d6932caa4f..2914de89ce 100644 --- a/src/applications/phame/controller/post/list/PhameBloggerPostListController.php +++ b/src/applications/phame/controller/post/list/PhameBloggerPostListController.php @@ -59,9 +59,10 @@ final class PhameBloggerPostListController } $this->setActions($actions); - $query = new PhamePostQuery(); - $query->withBloggerPHID($blogger_phid); - $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $query = id(new PhamePostQuery()) + ->setViewer($user) + ->withBloggerPHIDs(array($blogger_phid)); + $this->setPhamePostQuery($query); $page_title = 'Posts by '.$this->getBloggerName(); diff --git a/src/applications/phame/controller/post/list/PhameDraftListController.php b/src/applications/phame/controller/post/list/PhameDraftListController.php index 1934e3d788..0dd1d1fb56 100644 --- a/src/applications/phame/controller/post/list/PhameDraftListController.php +++ b/src/applications/phame/controller/post/list/PhameDraftListController.php @@ -51,9 +51,11 @@ final class PhameDraftListController $user = $this->getRequest()->getUser(); $phid = $user->getPHID(); - $query = new PhamePostQuery(); - $query->withBloggerPHID($phid); - $query->withVisibility(PhamePost::VISIBILITY_DRAFT); + $query = id(new PhamePostQuery()) + ->setViewer($user) + ->withBloggerPHIDs(array($phid)) + ->withVisibility(PhamePost::VISIBILITY_DRAFT); + $this->setPhamePostQuery($query); $actions = array('view', 'edit'); diff --git a/src/applications/phame/controller/post/list/PhameUserPostListController.php b/src/applications/phame/controller/post/list/PhameUserPostListController.php index cce488c889..35595f265e 100644 --- a/src/applications/phame/controller/post/list/PhameUserPostListController.php +++ b/src/applications/phame/controller/post/list/PhameUserPostListController.php @@ -47,9 +47,10 @@ final class PhameUserPostListController $user = $this->getRequest()->getUser(); $phid = $user->getPHID(); - $query = new PhamePostQuery(); - $query->withBloggerPHID($phid); - $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $query = id(new PhamePostQuery()) + ->setViewer($user) + ->withBloggerPHIDs(array($phid)); + $this->setPhamePostQuery($query); $actions = array('view', 'edit'); diff --git a/src/applications/phame/query/PhamePostQuery.php b/src/applications/phame/query/PhamePostQuery.php index cfceec15a9..c07a3882e2 100644 --- a/src/applications/phame/query/PhamePostQuery.php +++ b/src/applications/phame/query/PhamePostQuery.php @@ -19,45 +19,40 @@ /** * @group phame */ -final class PhamePostQuery extends PhabricatorOffsetPagedQuery { +final class PhamePostQuery extends PhabricatorCursorPagedPolicyAwareQuery { - private $bloggerPHID; - private $withoutBloggerPHID; - private $phameTitle; + private $blogPHIDs; + private $bloggerPHIDs; + private $phameTitles; private $visibility; private $phids; - /** - * Mutually exlusive with @{method:withoutBloggerPHID}. - * - * @{method:withBloggerPHID} wins because being positive and inclusive is - * cool. - */ - public function withBloggerPHID($blogger_phid) { - $this->bloggerPHID = $blogger_phid; + public function withPHIDs(array $phids) { + $this->phids = $phids; return $this; } - public function withoutBloggerPHID($blogger_phid) { - $this->withoutBloggerPHID = $blogger_phid; + + public function withBloggerPHIDs(array $blogger_phids) { + $this->bloggerPHIDs = $blogger_phids; return $this; } - public function withPhameTitle($phame_title) { - $this->phameTitle = $phame_title; + public function withBlogPHIDs(array $blog_phids) { + $this->blogPHIDs = $blog_phids; return $this; } - public function withVisibility($visibility) { - $this->visibility = $visibility; + public function withPhameTitles(array $phame_titles) { + $this->phameTitles = $phame_titles; return $this; } - public function withPHIDs($phids) { - $this->phids = $phids; + public function withVisibility($visibility) { + $this->visibility = $visibility; return $this; } - public function execute() { + protected function loadPage() { $table = new PhamePost(); $conn_r = $table->establishConnection('r'); @@ -75,54 +70,65 @@ final class PhamePostQuery extends PhabricatorOffsetPagedQuery { $posts = $table->loadAllFromArray($data); + if ($posts) { + // We require these to do visibility checks, so load them unconditionally. + $blog_phids = mpull($posts, 'getBlogPHID'); + $blogs = id(new PhameBlogQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($blog_phids) + ->execute(); + $blogs = mpull($blogs, 'getPHID'); + foreach ($posts as $post) { + if (isset($blogs[$post->getBlogPHID()])) { + $post->setBlog($blogs[$post->getBlogPHID()]); + } + } + } + return $posts; } - private function buildWhereClause($conn_r) { + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); if ($this->phids) { $where[] = qsprintf( $conn_r, - 'phid IN (%Ls)', - $this->phids - ); + 'p.phid IN (%Ls)', + $this->phids); } - if ($this->bloggerPHID) { + if ($this->bloggerPHIDs) { $where[] = qsprintf( $conn_r, - 'bloggerPHID = %s', - $this->bloggerPHID - ); - } else if ($this->withoutBloggerPHID) { + 'p.bloggerPHID IN (%Ls)', + $this->bloggerPHIDs); + } + + if ($this->phameTitles) { $where[] = qsprintf( $conn_r, - 'bloggerPHID != %s', - $this->withoutBloggerPHID - ); + 'p.phameTitle IN (%Ls)', + $this->phameTitles); } - if ($this->phameTitle) { + if ($this->visibility !== null) { $where[] = qsprintf( $conn_r, - 'phameTitle = %s', - $this->phameTitle - ); + 'p.visibility = %d', + $this->visibility); } - if ($this->visibility !== null) { + if ($this->blogPHIDs) { $where[] = qsprintf( $conn_r, - 'visibility = %d', - $this->visibility - ); + 'p.blogPHID in (%Ls)', + $this->blogPHIDs); } + $where[] = $this->buildPagingClause($conn_r); + return $this->formatWhereClause($where); } - private function buildOrderClause($conn_r) { - return 'ORDER BY datePublished DESC, id DESC'; - } } diff --git a/src/applications/phame/storage/PhamePost.php b/src/applications/phame/storage/PhamePost.php index 9abff35967..66eeef1624 100644 --- a/src/applications/phame/storage/PhamePost.php +++ b/src/applications/phame/storage/PhamePost.php @@ -19,7 +19,7 @@ /** * @group phame */ -final class PhamePost extends PhameDAO { +final class PhamePost extends PhameDAO implements PhabricatorPolicyInterface { const VISIBILITY_DRAFT = 0; const VISIBILITY_PUBLISHED = 1; @@ -33,6 +33,18 @@ final class PhamePost extends PhameDAO { protected $visibility; protected $configData; protected $datePublished; + protected $blogPHID; + + private $blog; + + public function setBlog(PhameBlog $blog) { + $this->blog = $blog; + return $this; + } + + public function getBlog() { + return $this->blog; + } public function getViewURI($blogger_name = '') { // go for the pretty uri if we can @@ -44,15 +56,19 @@ final class PhamePost extends PhameDAO { } return $uri; } + public function getEditURI() { return $this->getActionURI('edit'); } + public function getDeleteURI() { return $this->getActionURI('delete'); } + public function getChangeVisibilityURI() { return $this->getActionURI('changevisibility'); } + private function getActionURI($action) { return '/phame/post/'.$action.'/'.$this->getPHID().'/'; } @@ -78,6 +94,7 @@ final class PhamePost extends PhameDAO { } return idx($config_data, 'comments_widget', 'none'); } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -116,4 +133,45 @@ final class PhamePost extends PhameDAO { return $options; } + +/* -( PhabricatorPolicyInterface Implementation )-------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + + public function getPolicy($capability) { + // Draft posts are visible only to the author. Published posts are visible + // to whoever the blog is visible to. + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + if (!$this->isDraft() && $this->getBlog()) { + return $this->getBlog()->getViewPolicy(); + } else { + return PhabricatorPolicies::POLICY_NOONE; + } + break; + case PhabricatorPolicyCapability::CAN_EDIT: + return PhabricatorPolicies::POLICY_NOONE; + } + } + + + public function hasAutomaticCapability($capability, PhabricatorUser $user) { + // A blog post's author can always view it, and is the only user allowed + // to edit it. + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + case PhabricatorPolicyCapability::CAN_EDIT: + return ($user->getPHID() == $this->getBloggerPHID()); + } + } + } diff --git a/src/applications/phame/view/PhameBlogDetailView.php b/src/applications/phame/view/PhameBlogDetailView.php index 6d5bc95109..330ff8b0e1 100644 --- a/src/applications/phame/view/PhameBlogDetailView.php +++ b/src/applications/phame/view/PhameBlogDetailView.php @@ -81,13 +81,6 @@ final class PhameBlogDetailView extends AphrontView { 'class' => 'description' ), $description - ). - phutil_render_tag( - 'div', - array( - 'class' => 'bloggers' - ), - 'Current bloggers: '.$this->getBloggersHTML($bloggers) ) ); diff --git a/src/applications/phame/view/PhameBlogListView.php b/src/applications/phame/view/PhameBlogListView.php index 0e345ccf9f..997aa8618c 100644 --- a/src/applications/phame/view/PhameBlogListView.php +++ b/src/applications/phame/view/PhameBlogListView.php @@ -71,12 +71,9 @@ final class PhameBlogListView extends AphrontView { $item = id(new PhabricatorObjectItemView()) ->setHeader($blog->getName()) ->setHref($blog->getViewURI()) - ->addDetail( - 'Bloggers', - implode(', ', mpull($bloggers, 'renderLink'))) ->addDetail( 'Custom Domain', - $blog->getDomain()); + phutil_escape_html($blog->getDomain())); $can_edit = PhabricatorPolicyFilter::hasCapability( $user, diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index d34b45535c..a739e51ebe 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1008,6 +1008,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('phamepolicy.sql'), ), + 'phameoneblog.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('phameoneblog.sql'), + ), ); } -- 2.51.2