From f65c3c3a6a0280aa5b0b0ec0e9a66e8ef1097fbb Mon Sep 17 00:00:00 2001 From: Michael Wilson Date: Thu, 19 Sep 2024 16:41:47 +0930 Subject: [PATCH 1/6] add actionUpdateLastLogin --- application/common/models/Authentication.php | 8 ++------ application/common/models/User.php | 9 +++++++-- application/features/authentication.feature | 3 +-- .../features/bootstrap/FeatureContext.php | 8 ++++---- application/features/user.feature | 13 +++++++++++- application/frontend/config/main.php | 13 ++++++------ .../frontend/controllers/UserController.php | 20 +++++++++++++++++++ 7 files changed, 53 insertions(+), 21 deletions(-) diff --git a/application/common/models/Authentication.php b/application/common/models/Authentication.php index 7b3ee98f..3e03fb01 100644 --- a/application/common/models/Authentication.php +++ b/application/common/models/Authentication.php @@ -2,8 +2,6 @@ namespace common\models; -use common\components\Emailer; -use common\helpers\MySqlDateTime; use yii\web\HttpException; /** @@ -91,7 +89,7 @@ protected function authenticateByInvite($invite) /** * Run User validation rules. If all rules pass, $this->authenticatedUser will be a * clone of the User, and the User record in the database will be updated with new - * login and reminder dates. + * reminder dates. * * @param User $user */ @@ -100,15 +98,13 @@ protected function validateUser(User $user) if ($user->validate()) { $this->authenticatedUser = clone $user; - $user->last_login_utc = MySqlDateTime::now(); - $user->updateProfileReviewDates(); $user->checkAndProcessHIBP(); if (!$user->save()) { \Yii::error([ - 'action' => 'save last_login_utc and nag dates for user after authentication', + 'action' => 'save nag dates for user after authentication', 'status' => 'error', 'message' => $user->getFirstErrors(), ]); diff --git a/application/common/models/User.php b/application/common/models/User.php index 29ab7587..8ebafcee 100644 --- a/application/common/models/User.php +++ b/application/common/models/User.php @@ -4,7 +4,6 @@ use Br33f\Ga4\MeasurementProtocol\Dto\Event\BaseEvent; use Closure; -use common\components\Emailer; use common\components\HIBP; use common\components\Sheets; use common\helpers\MySqlDateTime; @@ -13,7 +12,6 @@ use Ramsey\Uuid\Uuid; use Sil\EmailService\Client\EmailServiceClientException; use Sil\PhpArrayDotNotation\DotNotation; -use TheIconic\Tracking\GoogleAnalytics\Analytics; use Yii; use yii\behaviors\AttributeBehavior; use yii\data\ActiveDataProvider; @@ -802,6 +800,13 @@ public function save($runValidation = true, $attributeNames = null) return parent::save($runValidation, $attributeNames); } + + public function updateLastLogin() + { + $this->last_login_utc = MySqlDateTime::now(); + + return $this->save(true, ['last_login_utc']); + } /** * @param bool $isNewUser diff --git a/application/features/authentication.feature b/application/features/authentication.feature index 7b1ac5b7..376e727b 100644 --- a/application/features/authentication.feature +++ b/application/features/authentication.feature @@ -51,8 +51,7 @@ Feature: Authentication | active | yes | | locked | no | And a record still exists with an employee_id of "123" -# TODO: check that last_login_utc was updated and current -# And none of the data has changed +# TODO: check that none of the data has changed Scenario: Attempt to authenticate an unknown user Given I provide the following valid data: diff --git a/application/features/bootstrap/FeatureContext.php b/application/features/bootstrap/FeatureContext.php index b4beece0..87ed8f8f 100644 --- a/application/features/bootstrap/FeatureContext.php +++ b/application/features/bootstrap/FeatureContext.php @@ -452,9 +452,9 @@ public function shouldBeStoredAsNowUTC($property) $minAcceptable = $expectedNow - self::ACCEPTABLE_DELTA_IN_SECONDS; $maxAcceptable = $expectedNow + self::ACCEPTABLE_DELTA_IN_SECONDS; - $storedNow = strtotime($this->userFromDb->$property); + $storedNow = $this->userFromDb->$property ? strtotime($this->userFromDb->$property) : 0; - Assert::range($storedNow, $minAcceptable, $maxAcceptable); + Assert::range($storedNow, $minAcceptable, $maxAcceptable, "Stored time $storedNow is not within acceptable range of $minAcceptable to $maxAcceptable"); } /** @@ -490,8 +490,8 @@ public function theOnlyPropertyToChangeShouldBe($property) $previous = $this->userFromDbBefore->$name; $current = $this->userFromDb->$name; - $name === $property ? Assert::notEq($current, $previous) - : Assert::eq($current, $previous); + $name === $property ? Assert::notEq($current, $previous, "$property is equal. Previous: $previous, Current: $current") + : Assert::eq($current, $previous,"$property is not equal. Previous: $previous, Current: $current"); } } diff --git a/application/features/user.feature b/application/features/user.feature index fd84d02e..df0f2f4d 100644 --- a/application/features/user.feature +++ b/application/features/user.feature @@ -135,6 +135,17 @@ Feature: User | hide | yes | | groups | mensa,hackers | + Scenario: update the last_login_utc of an existing user + Given the requester is authorized + And I add a user with a employee_id of 123 + When I request "/user/123/update-last-login" be updated + #And a record exists with an employee_id of "123" + Then the response status code should be 200 + And last_login_utc should be stored as now UTC + And a record exists with a employee_id of "123" + And the only property to change should be last_login_utc + #* @Then /^a record exists with (?:a|an) (.*) of "?([^"]*)"?$/ + Scenario: Deactivate an existing user Given a record does not exist with an employee_id of "123" And the requester is authorized @@ -771,4 +782,4 @@ Feature: User Examples: | active | isOrIsNot | | no | is NOT | - | yes | is | \ No newline at end of file + | yes | is | diff --git a/application/frontend/config/main.php b/application/frontend/config/main.php index 13818255..b10e8bc1 100644 --- a/application/frontend/config/main.php +++ b/application/frontend/config/main.php @@ -45,12 +45,13 @@ /* * User routes */ - 'GET user' => 'user/index', - 'GET user/' => 'user/view', - 'POST user' => 'user/create', - 'PUT user/' => 'user/update', - 'PUT user//password' => 'user/update-password', - 'PUT user//password/assess' => 'user/assess-password', + 'GET user' => 'user/index', + 'GET user/' => 'user/view', + 'POST user' => 'user/create', + 'PUT user/' => 'user/update', + 'PUT user//update-last-login' => 'user/update-last-login', + 'PUT user//password' => 'user/update-password', + 'PUT user//password/assess' => 'user/assess-password', /* * Authentication routes diff --git a/application/frontend/controllers/UserController.php b/application/frontend/controllers/UserController.php index 696a4808..645fe8fd 100644 --- a/application/frontend/controllers/UserController.php +++ b/application/frontend/controllers/UserController.php @@ -75,6 +75,26 @@ public function actionUpdate(string $employeeId) return $user; } + public function actionUpdateLastLogin(string $employeeId) + { + $user = User::findOne(condition: ['employee_id' => $employeeId]); + + if ($user === null) { + Yii::$app->response->statusCode = 404; + + return null; + } + + $user->scenario = User::SCENARIO_UPDATE_USER; + + if (!$user->updateLastLogin()) { + Yii::$app->response->statusCode = 500; + return null; + } + + return $user; + } + public function actionUpdatePassword(string $employeeId) { $user = User::findOne(['employee_id' => $employeeId]); From 3592033854e121922c84fd43fca43e8103683b86 Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Thu, 19 Sep 2024 15:29:39 +0800 Subject: [PATCH 2/6] small fix for the last_login_utc test --- application/features/user.feature | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/application/features/user.feature b/application/features/user.feature index df0f2f4d..2f177193 100644 --- a/application/features/user.feature +++ b/application/features/user.feature @@ -137,13 +137,13 @@ Feature: User Scenario: update the last_login_utc of an existing user Given the requester is authorized - And I add a user with a employee_id of 123 - When I request "/user/123/update-last-login" be updated - #And a record exists with an employee_id of "123" - Then the response status code should be 200 - And last_login_utc should be stored as now UTC - And a record exists with a employee_id of "123" - And the only property to change should be last_login_utc + And I add a user with a employee_id of "123" + And a record exists with an employee_id of "123" + When I request "/user/123/update-last-login" be updated + Then the response status code should be "200" + And a record exists with an employee_id of "123" + And last_login_utc should be stored as now UTC + And the only property to change should be last_login_utc #* @Then /^a record exists with (?:a|an) (.*) of "?([^"]*)"?$/ Scenario: Deactivate an existing user From 57216e950460cb47c4903f2b08247630d91d2742 Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Thu, 19 Sep 2024 15:34:20 +0800 Subject: [PATCH 3/6] fix PSR2 --- application/common/models/User.php | 2 +- application/features/bootstrap/FeatureContext.php | 2 +- application/frontend/controllers/UserController.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/application/common/models/User.php b/application/common/models/User.php index 8ebafcee..e2b99b17 100644 --- a/application/common/models/User.php +++ b/application/common/models/User.php @@ -800,7 +800,7 @@ public function save($runValidation = true, $attributeNames = null) return parent::save($runValidation, $attributeNames); } - + public function updateLastLogin() { $this->last_login_utc = MySqlDateTime::now(); diff --git a/application/features/bootstrap/FeatureContext.php b/application/features/bootstrap/FeatureContext.php index 87ed8f8f..e9ad002e 100644 --- a/application/features/bootstrap/FeatureContext.php +++ b/application/features/bootstrap/FeatureContext.php @@ -491,7 +491,7 @@ public function theOnlyPropertyToChangeShouldBe($property) $current = $this->userFromDb->$name; $name === $property ? Assert::notEq($current, $previous, "$property is equal. Previous: $previous, Current: $current") - : Assert::eq($current, $previous,"$property is not equal. Previous: $previous, Current: $current"); + : Assert::eq($current, $previous, "$property is not equal. Previous: $previous, Current: $current"); } } diff --git a/application/frontend/controllers/UserController.php b/application/frontend/controllers/UserController.php index 645fe8fd..2b1bb03a 100644 --- a/application/frontend/controllers/UserController.php +++ b/application/frontend/controllers/UserController.php @@ -84,7 +84,7 @@ public function actionUpdateLastLogin(string $employeeId) return null; } - + $user->scenario = User::SCENARIO_UPDATE_USER; if (!$user->updateLastLogin()) { From fd5ca9ea9452aface50fcefd68e66d1d4f2fbe73 Mon Sep 17 00:00:00 2001 From: Michael Wilson Date: Thu, 19 Sep 2024 17:24:55 +0930 Subject: [PATCH 4/6] update api.raml --- api.raml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/api.raml b/api.raml index 83ce571d..6189b61c 100644 --- a/api.raml +++ b/api.raml @@ -451,6 +451,23 @@ types: description: A server-side error occurred. body: type: Error + /update-last-login: + put: + description: Update the last login timestamp of a user based on their employee ID. + responses: + 200: + description: Successfully updated the last login time. + body: + application/json: + example: | + { + "employee_id": "f6bf51f2-4ccc-4d85-8f75-02132c67af27", + "last_login_utc": "2024-09-18T12:34:56Z" + } + 404: + description: Not found. The user with the specified `employeeId` was not found. + 500: + description: Server error. Failed to update the last login time. /mfa: get: description: Get a list of configured MFA devices for user From 7e32ba1cf9fd4dd7a2323e0a88dc86bfe794f7bc Mon Sep 17 00:00:00 2001 From: Michael Wilson Date: Thu, 19 Sep 2024 17:46:40 +0930 Subject: [PATCH 5/6] remove scenario and only return employee_id and last_login_utc --- application/common/models/User.php | 2 +- application/frontend/controllers/UserController.php | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/application/common/models/User.php b/application/common/models/User.php index e2b99b17..7f8285e1 100644 --- a/application/common/models/User.php +++ b/application/common/models/User.php @@ -805,7 +805,7 @@ public function updateLastLogin() { $this->last_login_utc = MySqlDateTime::now(); - return $this->save(true, ['last_login_utc']); + return $this->save(false, ['last_login_utc']); } /** diff --git a/application/frontend/controllers/UserController.php b/application/frontend/controllers/UserController.php index 2b1bb03a..7dd4e5a9 100644 --- a/application/frontend/controllers/UserController.php +++ b/application/frontend/controllers/UserController.php @@ -85,14 +85,12 @@ public function actionUpdateLastLogin(string $employeeId) return null; } - $user->scenario = User::SCENARIO_UPDATE_USER; - if (!$user->updateLastLogin()) { Yii::$app->response->statusCode = 500; return null; } - return $user; + return ['employee_id' => $user->employee_id, 'last_login_utc' => $user->last_login_utc]; } public function actionUpdatePassword(string $employeeId) From 0b4703284bf49245ecf7e313397d3e74868b1369 Mon Sep 17 00:00:00 2001 From: Michael Wilson <70765247+hobbitronics@users.noreply.github.com> Date: Thu, 19 Sep 2024 17:48:21 +0930 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: briskt <3172830+briskt@users.noreply.github.com> --- api.raml | 14 ++++++++------ application/features/user.feature | 3 +-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/api.raml b/api.raml index 6189b61c..cdcde181 100644 --- a/api.raml +++ b/api.raml @@ -458,12 +458,14 @@ types: 200: description: Successfully updated the last login time. body: - application/json: - example: | - { - "employee_id": "f6bf51f2-4ccc-4d85-8f75-02132c67af27", - "last_login_utc": "2024-09-18T12:34:56Z" - } + properties: + employee_id: string + last_login_utc: string + example: | + { + "employee_id": "12345", + "last_login_utc": "2024-09-18T12:34:56Z" + } 404: description: Not found. The user with the specified `employeeId` was not found. 500: diff --git a/application/features/user.feature b/application/features/user.feature index 2f177193..8db1293f 100644 --- a/application/features/user.feature +++ b/application/features/user.feature @@ -137,14 +137,13 @@ Feature: User Scenario: update the last_login_utc of an existing user Given the requester is authorized - And I add a user with a employee_id of "123" + And I add a user with an employee_id of "123" And a record exists with an employee_id of "123" When I request "/user/123/update-last-login" be updated Then the response status code should be "200" And a record exists with an employee_id of "123" And last_login_utc should be stored as now UTC And the only property to change should be last_login_utc - #* @Then /^a record exists with (?:a|an) (.*) of "?([^"]*)"?$/ Scenario: Deactivate an existing user Given a record does not exist with an employee_id of "123"