From e919233b3182d55b1d257df3b8ec369d702fff97 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 27 Nov 2017 17:53:58 -0800 Subject: [PATCH] Don't show personalized menu items until users establish a full session Summary: Depends on D18792. Fixes T13024. Fixes T89198. Currently, when users are logging in initially (for example, need to enter MFA) we show more menu items than we should. Notably, we may show some personalized/private account details, like the number of unread notifications (probably not relevant) or a user's saved queries (possibly sensitive). At best these are misleading (they won't work yet) and there's an outside possibility they leak a little bit of private data. Instead, nuke everything except "Log Out" when users have partial sessions. Test Plan: Hit a partial session (MFA required, email verification required) and looked at the menu. Only saw "Log Out". {F5297713} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13024 Differential Revision: https://secure.phabricator.com/D18793 --- .../PhabricatorFileDataController.php | 4 ++ .../PeopleMainMenuBarExtension.php | 62 ++++++++++--------- .../menu/PhabricatorMainMenuBarExtension.php | 14 +++++ .../page/menu/PhabricatorMainMenuView.php | 58 +++++++++++++++-- 4 files changed, 106 insertions(+), 32 deletions(-) diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index da438730cd..98f2cdbd51 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -10,6 +10,10 @@ final class PhabricatorFileDataController extends PhabricatorFileController { return false; } + public function shouldAllowPartialSessions() { + return true; + } + public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); $this->phid = $request->getURIData('phid'); diff --git a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php index cd1cffa7a2..01f1ba7cc1 100644 --- a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php +++ b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php @@ -9,6 +9,10 @@ final class PeopleMainMenuBarExtension return $viewer->isLoggedIn(); } + public function shouldAllowPartialSessions() { + return true; + } + public function getExtensionOrder() { return 1200; } @@ -65,42 +69,44 @@ final class PeopleMainMenuBarExtension $view = id(new PhabricatorActionListView()) ->setViewer($viewer); - $view->addAction( - id(new PhabricatorActionView()) - ->appendChild($user_view)); + if ($this->getIsFullSession()) { + $view->addAction( + id(new PhabricatorActionView()) + ->appendChild($user_view)); - $view->addAction( - id(new PhabricatorActionView()) - ->setType(PhabricatorActionView::TYPE_DIVIDER)); + $view->addAction( + id(new PhabricatorActionView()) + ->setType(PhabricatorActionView::TYPE_DIVIDER)); - $view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Profile')) - ->setHref('/p/'.$viewer->getUsername().'/')); + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Profile')) + ->setHref('/p/'.$viewer->getUsername().'/')); - $view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Settings')) - ->setHref('/settings/user/'.$viewer->getUsername().'/')); + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Settings')) + ->setHref('/settings/user/'.$viewer->getUsername().'/')); - $view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Manage')) - ->setHref('/people/manage/'.$viewer->getID().'/')); + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Manage')) + ->setHref('/people/manage/'.$viewer->getID().'/')); - if ($application) { - $help_links = $application->getHelpMenuItems($viewer); - if ($help_links) { - foreach ($help_links as $link) { - $view->addAction($link); + if ($application) { + $help_links = $application->getHelpMenuItems($viewer); + if ($help_links) { + foreach ($help_links as $link) { + $view->addAction($link); + } } } - } - $view->addAction( - id(new PhabricatorActionView()) - ->addSigil('logout-item') - ->setType(PhabricatorActionView::TYPE_DIVIDER)); + $view->addAction( + id(new PhabricatorActionView()) + ->addSigil('logout-item') + ->setType(PhabricatorActionView::TYPE_DIVIDER)); + } $view->addAction( id(new PhabricatorActionView()) diff --git a/src/view/page/menu/PhabricatorMainMenuBarExtension.php b/src/view/page/menu/PhabricatorMainMenuBarExtension.php index fede2fdb85..d893cccac9 100644 --- a/src/view/page/menu/PhabricatorMainMenuBarExtension.php +++ b/src/view/page/menu/PhabricatorMainMenuBarExtension.php @@ -5,6 +5,7 @@ abstract class PhabricatorMainMenuBarExtension extends Phobject { private $viewer; private $application; private $controller; + private $isFullSession; public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -33,6 +34,15 @@ abstract class PhabricatorMainMenuBarExtension extends Phobject { return $this->controller; } + public function setIsFullSession($is_full_session) { + $this->isFullSession = $is_full_session; + return $this; + } + + public function getIsFullSession() { + return $this->isFullSession; + } + final public function getExtensionKey() { return $this->getPhobjectClassConstant('MAINMENUBARKEY'); } @@ -41,6 +51,10 @@ abstract class PhabricatorMainMenuBarExtension extends Phobject { return true; } + public function shouldAllowPartialSessions() { + return false; + } + public function isExtensionEnabledForViewer(PhabricatorUser $viewer) { if (!$viewer->isLoggedIn()) { return false; diff --git a/src/view/page/menu/PhabricatorMainMenuView.php b/src/view/page/menu/PhabricatorMainMenuView.php index f9e4032d87..ba4bda41ea 100644 --- a/src/view/page/menu/PhabricatorMainMenuView.php +++ b/src/view/page/menu/PhabricatorMainMenuView.php @@ -46,7 +46,9 @@ final class PhabricatorMainMenuView extends AphrontView { $app_button = ''; $aural = null; - if ($viewer->isLoggedIn() && $viewer->isUserActivated()) { + $is_full = $this->isFullSession($viewer); + + if ($is_full) { list($menu, $dropdowns, $aural) = $this->renderNotificationMenu(); if (array_filter($menu)) { $alerts[] = $menu; @@ -54,14 +56,18 @@ final class PhabricatorMainMenuView extends AphrontView { $menu_bar = array_merge($menu_bar, $dropdowns); $app_button = $this->renderApplicationMenuButton(); $search_button = $this->renderSearchMenuButton($header_id); - } else { + } else if (!$viewer->isLoggedIn()) { $app_button = $this->renderApplicationMenuButton(); if (PhabricatorEnv::getEnvConfig('policy.allow-public')) { $search_button = $this->renderSearchMenuButton($header_id); } } - $search_menu = $this->renderPhabricatorSearchMenu(); + if ($search_button) { + $search_menu = $this->renderPhabricatorSearchMenu(); + } else { + $search_menu = null; + } if ($alerts) { $alerts = javelin_tag( @@ -84,7 +90,9 @@ final class PhabricatorMainMenuView extends AphrontView { $extensions = PhabricatorMainMenuBarExtension::getAllEnabledExtensions(); foreach ($extensions as $extension) { - $extension->setViewer($viewer); + $extension + ->setViewer($viewer) + ->setIsFullSession($is_full); $controller = $this->getController(); if ($controller) { @@ -96,6 +104,14 @@ final class PhabricatorMainMenuView extends AphrontView { } } + if (!$is_full) { + foreach ($extensions as $key => $extension) { + if (!$extension->shouldAllowPartialSessions()) { + unset($extensions[$key]); + } + } + } + foreach ($extensions as $key => $extension) { if (!$extension->isExtensionEnabledForViewer($extension->getViewer())) { unset($extensions[$key]); @@ -677,4 +693,38 @@ final class PhabricatorMainMenuView extends AphrontView { ); } + private function isFullSession(PhabricatorUser $viewer) { + if (!$viewer->isLoggedIn()) { + return false; + } + + if (!$viewer->isUserActivated()) { + return false; + } + + if (!$viewer->hasSession()) { + return false; + } + + $session = $viewer->getSession(); + if ($session->getIsPartial()) { + return false; + } + + if (!$session->getSignedLegalpadDocuments()) { + return false; + } + + $mfa_key = 'security.require-multi-factor-auth'; + $need_mfa = PhabricatorEnv::getEnvConfig($mfa_key); + if ($need_mfa) { + $have_mfa = $viewer->getIsEnrolledInMultiFactor(); + if (!$have_mfa) { + return false; + } + } + + return true; + } + }