From 7e23284305ebed601f9a7e661459e0b6de50b88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sheila=20Ba=C3=B1ez?= Date: Tue, 10 Sep 2019 15:42:46 +1200 Subject: [PATCH 1/3] MMS-104: Add lockout session logic --- .../PartialSubmissionController.php | 33 ++++++- src/controllers/PartialUserFormController.php | 94 ++++++++++++++----- .../UserDefinedFormControllerExtension.php | 2 +- src/models/PartialFormSubmission.php | 14 ++- tests/unit/PartialFieldSubmissionTest.php | 1 - tests/unit/PartialFileFieldSubmissionTest.php | 1 - .../unit/PartialSubmissionControllerTest.php | 24 +++++ tests/unit/PartialUserFormControllerTest.php | 8 +- .../PartialUserFormVerifyControllerTest.php | 1 - 9 files changed, 146 insertions(+), 32 deletions(-) diff --git a/src/controllers/PartialSubmissionController.php b/src/controllers/PartialSubmissionController.php index e46fe8e..f1875fd 100644 --- a/src/controllers/PartialSubmissionController.php +++ b/src/controllers/PartialSubmissionController.php @@ -12,7 +12,9 @@ use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; +use SilverStripe\Control\Session; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\ValidationException; use SilverStripe\UserForms\Model\EditableFormField; @@ -80,7 +82,8 @@ public function savePartialSubmission(HTTPRequest $request) } // Refresh session ID - $request->getSession()->set(self::SESSION_KEY, $submissionID); + static::reloadSession($request->getSession(), $submissionID); + foreach ($postVars as $field => $value) { /** @var EditableFormField $editableField */ $editableField = $this->createOrUpdateSubmission([ @@ -106,6 +109,34 @@ public function savePartialSubmission(HTTPRequest $request) return new HTTPResponse($return, ($return ? 201 : 400)); } + /** + * Reload session for partial submissions + * @param Session $session + * @param int $sessionID Partial form submission ID + * @throws ValidationException + */ + public static function reloadSession($session, $sessionID) + { + $partial = PartialFormSubmission::get()->byID($sessionID); + if (!$partial) { + return; + } + + $session->set(self::SESSION_KEY, $partial->ID); + + $now = new \DateTime(DBDatetime::now()->getValue()); + $now->add(new \DateInterval('PT30M')); + + $phpSessionID = session_id(); + if (!$phpSessionID) { + return; + } + + $partial->LockedOutUntil = $now->format('Y-m-d H:i:s'); + $partial->PHPSessionID = $phpSessionID; + $partial->write(); + } + /** * @param $formData * @return DataObject|EditableFormField diff --git a/src/controllers/PartialUserFormController.php b/src/controllers/PartialUserFormController.php index cd43a8d..529fc75 100644 --- a/src/controllers/PartialUserFormController.php +++ b/src/controllers/PartialUserFormController.php @@ -5,12 +5,14 @@ use Exception; use Firesphere\PartialUserforms\Models\PartialFormSubmission; use Page; +use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; use Firesphere\PartialUserforms\Forms\PasswordForm; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware; +use SilverStripe\Dev\Debug; use SilverStripe\Forms\Form; use SilverStripe\Forms\HiddenField; use SilverStripe\ORM\DataObject; @@ -63,15 +65,23 @@ public function partial(HTTPRequest $request) // or another submission has started $sessionID = $request->getSession()->get(PartialSubmissionController::SESSION_KEY); if (!$sessionID || $sessionID !== $partial->ID) { - $request->getSession()->set(PartialSubmissionController::SESSION_KEY, $partial->ID); + PartialSubmissionController::reloadSession($request->getSession(), $partial->ID); } + // Verify user if ($controller->PasswordProtected && $request->getSession()->get(PasswordForm::PASSWORD_SESSION_KEY) !== $partial->ID ) { return $this->redirect('verify'); } + // Lock form + if ($this->isLockedOut()) { + // TODO: MMS-115 + Debug::dump('This form is currently being used by someone else. Please try again in 30 minutes.'); + // Redirect to overview page $this->redirect($this->Link('overview')); + } + $form = $controller->Form(); $form->loadDataFrom($partial->getFields()); $this->populateData($form, $partial); @@ -103,6 +113,47 @@ public function partial(HTTPRequest $request) ])->renderWith([static::class, Page::class]); } + /** + * A little abstraction to be more readable + * + * @param HTTPRequest $request + * @return PartialFormSubmission|void + * @throws HTTPResponse_Exception + */ + public function validateToken($request) + { + // Ensure this URL doesn't get picked up by HTTP caches + HTTPCacheControlMiddleware::singleton()->disableCache(); + + $key = $request->param('Key'); + $token = $request->param('Token'); + if (!$key || !$token) { + return $this->httpError(404); + } + + /** @var PartialFormSubmission $partial */ + $partial = PartialFormSubmission::validateKeyToken($key, $token); + if ($partial === false) { + return $this->httpError(404); + } + + return $partial; + } + + /** + * Get the partial link + * @param string $action + * @return string + */ + public function Link($action = null) + { + return Controller::join_links( + Director::absoluteBaseURL(), + 'partial', + $action + ); + } + /** * Add partial submission and set the uploaded filenames as right title of the file fields * @@ -142,29 +193,30 @@ protected function populateData($form, $partial) } /** - * A little abstraction to be more readable - * - * @param HTTPRequest $request - * @return PartialFormSubmission|void - * @throws HTTPResponse_Exception + * Checks whether this form is currently being used by someone else + * @return bool + * @throws \SilverStripe\ORM\ValidationException */ - public function validateToken($request) + protected function isLockedOut() { - // Ensure this URL doesn't get picked up by HTTP caches - HTTPCacheControlMiddleware::singleton()->disableCache(); - - $key = $request->param('Key'); - $token = $request->param('Token'); - if (!$key || !$token) { - return $this->httpError(404); - } - - /** @var PartialFormSubmission $partial */ - $partial = PartialFormSubmission::validateKeyToken($key, $token); - if ($partial === false) { - return $this->httpError(404); + $session = $this->getRequest()->getSession(); + $submissionID = $session->get(PartialSubmissionController::SESSION_KEY); + $partial = PartialFormSubmission::get()->byID($submissionID); + $phpSessionID = session_id(); + + // If invalid sessions or if the last session was from the same user or that the recent session has expired + if ( + !$submissionID || + !$partial || + !$partial->PHPSessionID || + $phpSessionID === $partial->PHPSessionID || + $partial->dbObject('LockedOutUntil')->InPast() + ) { + PartialSubmissionController::reloadSession($session, $partial->ID); + return false; } - return $partial; + // Lockout when there's an ongoing session + return $phpSessionID !== $partial->PHPSessionID && $partial->dbObject('LockedOutUntil')->InFuture(); } } diff --git a/src/extensions/UserDefinedFormControllerExtension.php b/src/extensions/UserDefinedFormControllerExtension.php index ae20c12..ed91b4f 100644 --- a/src/extensions/UserDefinedFormControllerExtension.php +++ b/src/extensions/UserDefinedFormControllerExtension.php @@ -96,6 +96,6 @@ protected function createPartialSubmission() } // Refresh session on start - $page->getRequest()->getSession()->set(PartialSubmissionController::SESSION_KEY, $submissionID); + PartialSubmissionController::reloadSession($page->getRequest()->getSession(), $submissionID); } } diff --git a/src/models/PartialFormSubmission.php b/src/models/PartialFormSubmission.php index ea4d6c5..ebd646b 100644 --- a/src/models/PartialFormSubmission.php +++ b/src/models/PartialFormSubmission.php @@ -46,10 +46,12 @@ class PartialFormSubmission extends SubmittedForm * @var array */ private static $db = [ - 'IsSend' => 'Boolean(false)', - 'TokenSalt' => 'Varchar(16)', - 'Token' => 'Varchar(16)', - 'Password' => 'Varchar(64)', + 'IsSend' => 'Boolean(false)', + 'TokenSalt' => 'Varchar(16)', + 'Token' => 'Varchar(16)', + 'Password' => 'Varchar(64)', + 'LockedOutUntil' => 'Datetime', + 'PHPSessionID' => 'Varchar(128)' ]; /** @@ -124,7 +126,9 @@ public function getCMSFields() 'UserDefinedFormID', 'Submitter', 'PartialUploads', - 'Password' + 'Password', + 'LockedOutUntil', + 'PHPSessionID' ]); $partialFields = $this->PartialFields(); diff --git a/tests/unit/PartialFieldSubmissionTest.php b/tests/unit/PartialFieldSubmissionTest.php index 72363a3..d80bf78 100644 --- a/tests/unit/PartialFieldSubmissionTest.php +++ b/tests/unit/PartialFieldSubmissionTest.php @@ -6,7 +6,6 @@ use Firesphere\PartialUserforms\Models\PartialFormSubmission; use SilverStripe\Dev\SapphireTest; use SilverStripe\Security\DefaultAdminService; -use SilverStripe\Security\Member; use SilverStripe\Security\Security; use SilverStripe\UserForms\Model\UserDefinedForm; diff --git a/tests/unit/PartialFileFieldSubmissionTest.php b/tests/unit/PartialFileFieldSubmissionTest.php index 3545be6..2f4c9bb 100644 --- a/tests/unit/PartialFileFieldSubmissionTest.php +++ b/tests/unit/PartialFileFieldSubmissionTest.php @@ -4,7 +4,6 @@ use Firesphere\PartialUserforms\Models\PartialFileFieldSubmission; use Firesphere\PartialUserforms\Models\PartialFormSubmission; -use SilverStripe\Assets\File; use SilverStripe\Dev\SapphireTest; use SilverStripe\Security\DefaultAdminService; use SilverStripe\Security\Security; diff --git a/tests/unit/PartialSubmissionControllerTest.php b/tests/unit/PartialSubmissionControllerTest.php index a711fcc..8f7e0fd 100644 --- a/tests/unit/PartialSubmissionControllerTest.php +++ b/tests/unit/PartialSubmissionControllerTest.php @@ -10,6 +10,7 @@ use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\FunctionalTest; use SilverStripe\ORM\DataList; +use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\UserForms\Model\EditableFormField\EditableFileField; use SilverStripe\UserForms\Model\UserDefinedForm; @@ -212,6 +213,29 @@ public function testSaveDataWithExpiredSession() $this->assertEquals(404, $response->getStatusCode()); } + public function testReloadSession() + { + $sessionID = $this->session()->get(PartialSubmissionController::SESSION_KEY); + $this->assertNotEmpty($sessionID); + + $partial = PartialFormSubmission::get()->byID($sessionID); + $this->assertNull($partial->LockedOutUntil); + $this->assertNull($partial->PHPSessionID); + + $this->session()->clear(PartialSubmissionController::SESSION_KEY); + $sessionID = $this->session()->get(PartialSubmissionController::SESSION_KEY); + $this->assertNull($sessionID); + + // New session + session_id('petrichor'); + DBDatetime::set_mock_now('2019-02-15 10:00:00'); + $id = $this->savePartial(['PartialID' => 1]); // save submission reloads the session + $partial = PartialFormSubmission::get()->byID($id); + + $this->assertEquals('2019-02-15 10:30:00', $partial->LockedOutUntil); + $this->assertEquals('petrichor', $partial->PHPSessionID); + } + /** * Helper function for saving partial submission * @param array $values diff --git a/tests/unit/PartialUserFormControllerTest.php b/tests/unit/PartialUserFormControllerTest.php index fac2ff5..8a10d30 100644 --- a/tests/unit/PartialUserFormControllerTest.php +++ b/tests/unit/PartialUserFormControllerTest.php @@ -2,7 +2,6 @@ namespace Firesphere\PartialUserforms\Tests; -use Firesphere\PartialUserforms\Forms\PasswordForm; use Firesphere\PartialUserforms\Models\PartialFormSubmission; use SilverStripe\Assets\File; use SilverStripe\Dev\FunctionalTest; @@ -97,6 +96,13 @@ public function testPasswordProtectedPartial() $this->assertContains($formOpeningTag, $result->getBody()); } + /** + * TODO: Add test when MMS-115 is complete + */ + public function testIsLockedOut() + { + } + public function setUp() { parent::setUp(); diff --git a/tests/unit/PartialUserFormVerifyControllerTest.php b/tests/unit/PartialUserFormVerifyControllerTest.php index ab686e9..d0b0280 100644 --- a/tests/unit/PartialUserFormVerifyControllerTest.php +++ b/tests/unit/PartialUserFormVerifyControllerTest.php @@ -9,7 +9,6 @@ use SilverStripe\Control\NullHTTPRequest; use SilverStripe\Control\Session; use SilverStripe\Dev\FunctionalTest; -use SilverStripe\Dev\SapphireTest; class PartialUserFormVerifyControllerTest extends FunctionalTest { From 78127cd4ef3759bb33fe10e491eac3540d3e5b7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sheila=20Ba=C3=B1ez?= Date: Tue, 10 Sep 2019 17:18:47 +1200 Subject: [PATCH 2/3] MMS-104: Remove unnecessary code --- src/controllers/PartialUserFormController.php | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/controllers/PartialUserFormController.php b/src/controllers/PartialUserFormController.php index 529fc75..b8f924e 100644 --- a/src/controllers/PartialUserFormController.php +++ b/src/controllers/PartialUserFormController.php @@ -79,7 +79,7 @@ public function partial(HTTPRequest $request) if ($this->isLockedOut()) { // TODO: MMS-115 Debug::dump('This form is currently being used by someone else. Please try again in 30 minutes.'); - // Redirect to overview page $this->redirect($this->Link('overview')); + // Redirect to overview page } $form = $controller->Form(); @@ -140,20 +140,6 @@ public function validateToken($request) return $partial; } - /** - * Get the partial link - * @param string $action - * @return string - */ - public function Link($action = null) - { - return Controller::join_links( - Director::absoluteBaseURL(), - 'partial', - $action - ); - } - /** * Add partial submission and set the uploaded filenames as right title of the file fields * From f016fa7067cf6d76aa6470d21e18fd71f6aea74e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sheila=20Ba=C3=B1ez?= Date: Tue, 10 Sep 2019 18:23:42 +1200 Subject: [PATCH 3/3] MMS-104: Fix destroy session error --- tests/unit/PartialSubmissionControllerTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/PartialSubmissionControllerTest.php b/tests/unit/PartialSubmissionControllerTest.php index 8f7e0fd..ebceb12 100644 --- a/tests/unit/PartialSubmissionControllerTest.php +++ b/tests/unit/PartialSubmissionControllerTest.php @@ -234,6 +234,9 @@ public function testReloadSession() $this->assertEquals('2019-02-15 10:30:00', $partial->LockedOutUntil); $this->assertEquals('petrichor', $partial->PHPSessionID); + + // Set the session back to empty string to prevent destroying uninitialized session + session_id(''); } /**