diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index bcbbe5de..d5576fb6 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -26,7 +26,7 @@ --- ### Feature PR Checklist -- [ ] Documentation (README, local.env.dist, etc.) +- [ ] Documentation (README, local.env.dist, api.raml, etc.) - [ ] Tests created or updated - [ ] Run `make composershow` - [ ] Run `make psr2` diff --git a/api.raml b/api.raml index ec99df8c..83ce571d 100644 --- a/api.raml +++ b/api.raml @@ -44,24 +44,6 @@ types: "code": 0, "status": 400 } - GroupsExternalUpdate: - type: object - properties: - email_address: string - app_prefix: - type: string - description: > - The app-specific prefix, without the trailing hyphen. - groups: - type: string[] - description: > - The desired list of groups, including the app-prefix. - example: | - { - "email_address": "john_smith@example.com", - "app_prefix": "myapp", - "groups": ["myapp-users", "myapp-managers"] - } UserResponse: description: > Information on user record. Password is not included if a password @@ -436,25 +418,6 @@ types: description: A server-side error occurred. body: type: Error - /external-groups: - put: - description: > - Update a user's list of external groups for a given app-prefix to be - exactly the given list, leaving external groups with other prefixes - unchanged. - body: - type: GroupsExternalUpdate - responses: - 204: - description: > - The user's external groups for that app-prefix were updated. - 404: - description: > - No such user found. - 422: - description: The given data does not satisfy some validation rule. - body: - type: Error /{employee_id}: get: description: Get information about a specific user. diff --git a/application/behat.yml b/application/behat.yml index 9b3cf8de..8c5ceee4 100644 --- a/application/behat.yml +++ b/application/behat.yml @@ -23,10 +23,10 @@ default: paths: - "features/groups-external.feature" contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalContext ] - groups_external_updates_features: + groups_external_sync_features: paths: - - "features/groups-external-updates.feature" - contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalUpdatesContext ] + - "features/groups-external-sync.feature" + contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalSyncContext ] hibp_unit_tests_features: paths: - "features/hibp-unit-tests.feature" diff --git a/application/common/models/User.php b/application/common/models/User.php index 3f8c5760..5507f89c 100644 --- a/application/common/models/User.php +++ b/application/common/models/User.php @@ -641,6 +641,28 @@ public function getNagState() return $this->nagState->getState(); } + private static function listUsersWithExternalGroupWith($appPrefix): array + { + $appPrefixWithHyphen = $appPrefix . '-'; + + /** @var User[] $users */ + $users = User::find()->where( + ['like', 'groups_external', $appPrefixWithHyphen] + )->all(); + + $emailAddresses = []; + foreach ($users as $user) { + $externalGroups = explode(',', $user->groups_external); + foreach ($externalGroups as $externalGroup) { + if (str_starts_with($externalGroup, $appPrefixWithHyphen)) { + $emailAddresses[] = $user->email; + break; + } + } + } + return $emailAddresses; + } + public function loadMfaData(string $rpOrigin = '') { $verifiedMfaOptions = $this->getVerifiedMfaOptions($rpOrigin); @@ -1003,8 +1025,61 @@ public function isPromptForMfa(): bool return false; } - public function updateExternalGroups($appPrefix, $appExternalGroups): bool + /** + * Update users' external-groups data using the given external-groups data. + * + * @param string $appPrefix -- Example: "wiki" + * @param array $desiredExternalGroupsByUserEmail -- The authoritative list + * of external groups for the given app-prefix, where each key is a + * User's email address and each value is a comma-delimited string of + * which groups (with that app-prefix) that the user should have. Any + * other Users with external groups starting with the given app-prefix + * will have those external groups removed. Any external groups starting + * with a different prefix will be left unchanged. + * @return string[] -- The resulting error messages. + */ + public static function updateUsersExternalGroups( + string $appPrefix, + array $desiredExternalGroupsByUserEmail + ): array { + $errors = []; + $emailAddressesOfCurrentMatches = self::listUsersWithExternalGroupWith($appPrefix); + + // Indicate that users not in the "desired" list should not have any + // such external groups. + foreach ($emailAddressesOfCurrentMatches as $email) { + if (! array_key_exists($email, $desiredExternalGroupsByUserEmail)) { + $desiredExternalGroupsByUserEmail[$email] = ''; + } + } + + foreach ($desiredExternalGroupsByUserEmail as $email => $groupsForPrefix) { + $user = User::findByEmail($email); + if ($user === null) { + $errors[] = 'No user found for email address ' . json_encode($email); + continue; + } + $successful = $user->updateExternalGroups($appPrefix, $groupsForPrefix); + if (! $successful) { + $errors[] = sprintf( + "Failed to update external groups for %s: \n%s", + $email, + json_encode($user->getFirstErrors(), JSON_PRETTY_PRINT) + ); + } + } + return $errors; + } + + public function updateExternalGroups(string $appPrefix, string $csvAppExternalGroups): bool { + if (empty($csvAppExternalGroups)) { + $appExternalGroups = []; + } else { + $untrimmedAppExternalGroups = explode(',', $csvAppExternalGroups); + $appExternalGroups = array_map('trim', $untrimmedAppExternalGroups); + } + foreach ($appExternalGroups as $appExternalGroup) { if (! str_starts_with($appExternalGroup, $appPrefix . '-')) { $this->addErrors([ diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php index 4b270dca..510f4b5c 100644 --- a/application/features/bootstrap/GroupsExternalContext.php +++ b/application/features/bootstrap/GroupsExternalContext.php @@ -18,52 +18,68 @@ class GroupsExternalContext extends FeatureContext */ public function aUserExists() { - $this->deleteThatTestUser(); - $this->createTestUser(); - $this->setThatUsersPassword($this->userPassword); + $this->deleteTestUser($this->userEmailAddress); + + $this->user = $this->createTestUser( + $this->userEmailAddress, + '11111' + ); + + $this->setTestUsersPassword( + $this->user, + $this->userPassword + ); } - private function deleteThatTestUser() + protected function deleteTestUser(string $emailAddress) { - $user = User::findByEmail($this->userEmailAddress); + $user = User::findByEmail($emailAddress); if ($user !== null) { $didDeleteUser = $user->delete(); Assert::notFalse($didDeleteUser, sprintf( - 'Failed to delete existing test user: %s', + 'Failed to delete existing test user (%s): %s', + $emailAddress, join("\n", $user->getFirstErrors()) )); } } - private function createTestUser() - { + protected function createTestUser( + string $emailAddress, + string $employeeId, + string $externalGroups = '' + ): User { + list($username, ) = explode('@', $emailAddress); + list($lcFirstName, $lcLastName) = explode('_', $username); $user = new User([ - 'email' => $this->userEmailAddress, - 'employee_id' => '11111', - 'first_name' => 'John', - 'last_name' => 'Smith', - 'username' => 'john_smith', + 'email' => $emailAddress, + 'employee_id' => $employeeId, + 'first_name' => ucfirst($lcFirstName), + 'last_name' => ucfirst($lcLastName), + 'username' => $username, + 'groups_external' => $externalGroups, ]); $user->scenario = User::SCENARIO_NEW_USER; $createdNewUser = $user->save(); Assert::true($createdNewUser, sprintf( - 'Failed to create test user: %s', + 'Failed to create test user %s: %s', + json_encode($emailAddress), join("\n", $user->getFirstErrors()) )); $user->refresh(); - - $this->user = $user; + return $user; } - private function setThatUsersPassword(string $password) + private function setTestUsersPassword(User $user, string $password) { - $this->user->scenario = User::SCENARIO_UPDATE_PASSWORD; - $this->user->password = $password; + $user->scenario = User::SCENARIO_UPDATE_PASSWORD; + $user->password = $password; - Assert::true($this->user->save(), sprintf( - "Failed to set the test user's password: %s", - join("\n", $this->user->getFirstErrors()) + Assert::true($user->save(), sprintf( + "Failed to set the %s test user's password: %s", + json_encode($user->email), + join("\n", $user->getFirstErrors()) )); } diff --git a/application/features/bootstrap/GroupsExternalSyncContext.php b/application/features/bootstrap/GroupsExternalSyncContext.php new file mode 100644 index 00000000..06fae6d8 --- /dev/null +++ b/application/features/bootstrap/GroupsExternalSyncContext.php @@ -0,0 +1,116 @@ + [ + * 'john_smith@example.org' => 'wiki-one,wiki-two', + * ], + * ] + * ``` + * @var array + */ + private array $externalGroupsLists = []; + + /** @var string[] */ + private array $syncErrors; + + /** + * @Given the following users exist, with these external groups: + */ + public function theFollowingUsersExistWithTheseExternalGroups(TableNode $table) + { + $dummyEmployeeId = 11110; + foreach ($table as $row) { + $emailAddress = $row['email']; + $employeeId = ++$dummyEmployeeId; + $groups = $row['groups']; + + $this->deleteTestUser($emailAddress); + $this->createTestUser( + $emailAddress, + (string)$employeeId, + $groups + ); + } + } + + /** + * @Given the :appPrefix external groups list is the following: + */ + public function theExternalGroupsListIsTheFollowing(string $appPrefix, TableNode $table) + { + $userGroupsMap = []; + foreach ($table as $row) { + $emailAddress = $row['email']; + $externalGroupsCsv = $row['groups']; + $userGroupsMap[$emailAddress] = $externalGroupsCsv; + } + $this->externalGroupsLists[$appPrefix] = $userGroupsMap; + } + + /** + * @When I sync the list of :appPrefix external groups + */ + public function iSyncTheListOfExternalGroups($appPrefix) + { + $this->syncErrors = User::updateUsersExternalGroups( + $appPrefix, + $this->externalGroupsLists[$appPrefix] + ); + } + + /** + * @Then there should not have been any sync errors + */ + public function thereShouldNotHaveBeenAnySyncErrors() + { + Assert::isEmpty($this->syncErrors, sprintf( + "Unexpected sync error(s): \n%s", + implode("\n", $this->syncErrors) + )); + } + + /** + * @Then the following users should have the following external groups: + */ + public function theFollowingUsersShouldHaveTheFollowingExternalGroups(TableNode $table) + { + foreach ($table as $row) { + $emailAddress = $row['email']; + $expectedExternalGroups = explode(',', $row['groups']); + + $user = User::findByEmail($emailAddress); + Assert::notNull($emailAddress, 'User not found: ' . $emailAddress); + $actualExternalGroups = explode(',', $user->groups_external); + + sort($actualExternalGroups); + sort($expectedExternalGroups); + + Assert::same( + json_encode($actualExternalGroups, JSON_PRETTY_PRINT), + json_encode($expectedExternalGroups, JSON_PRETTY_PRINT) + ); + } + } + + /** + * @Then there should have been a sync error + */ + public function thereShouldHaveBeenASyncError() + { + Assert::notEmpty($this->syncErrors); + foreach ($this->syncErrors as $syncError) { + echo $syncError . PHP_EOL; + } + } +} diff --git a/application/features/bootstrap/GroupsExternalUpdatesContext.php b/application/features/bootstrap/GroupsExternalUpdatesContext.php deleted file mode 100644 index 61018287..00000000 --- a/application/features/bootstrap/GroupsExternalUpdatesContext.php +++ /dev/null @@ -1,37 +0,0 @@ -cleanRequestBody(); - $this->setRequestBody('email_address', $this->getUserEmailAddress()); - $this->setRequestBody('app_prefix', $appPrefix); - $this->setRequestBody('groups', $externalGroups); - - $this->iRequestTheResourceBe('/user/external-groups', 'updated'); - } - - /** - * @Then that user's list of external groups should be :commaSeparatedExternalGroups - */ - public function thatUsersListOfExternalGroupsShouldBe($commaSeparatedExternalGroups) - { - $user = User::findByEmail($this->getUserEmailAddress()); - Assert::same($user->groups_external, $commaSeparatedExternalGroups); - } -} diff --git a/application/features/groups-external-sync.feature b/application/features/groups-external-sync.feature new file mode 100644 index 00000000..dbe39a0c --- /dev/null +++ b/application/features/groups-external-sync.feature @@ -0,0 +1,120 @@ +Feature: Syncing a specific app-prefix of external groups with an external list + + Scenario: Add an external group to a user's list for a particular app + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two | + + Scenario: Change the external group in a user's list for a particular app + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-two | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-two | + + Scenario: Leave a user's external groups for a different app unchanged + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one,map-europe | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-two | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-two,map-europe | + + Scenario: Remove an external group from a user's list for a particular app + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-one | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-one | + + Scenario: Remove all external groups from a user's list for a particular app (no entry in list) + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two,map-asia | + And the "wiki" external groups list is the following: + | email | groups | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | map-asia | + + Scenario: Remove all external groups from a user's list for a particular app (blank entry in list) + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two,map-asia | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | map-asia | + + Scenario: Try to add an external group that does not match the given app-prefix + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-one,map-asia | + When I sync the list of "wiki" external groups + Then there should have been a sync error + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-one | + + Scenario: Properly handle (trim) spaces around external groups + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one | + And the "wiki" external groups list is the following: + | email | groups | + | john_smith@example.org | wiki-one, wiki-two | + When I sync the list of "wiki" external groups + Then there should not have been any sync errors + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two | + + Scenario: Properly handle an empty email address + Given the following users exist, with these external groups: + | email | groups | + | john_smith@example.org | wiki-one | + And the "wiki" external groups list is the following: + | email | groups | + | | wiki-one | + | john_smith@example.org | wiki-one,wiki-two | + When I sync the list of "wiki" external groups + Then there should have been a sync error + And the following users should have the following external groups: + | email | groups | + | john_smith@example.org | wiki-one,wiki-two | + + # Scenario: Send 1 notification email if sync finds group(s) for a user not in this IDP diff --git a/application/features/groups-external-updates.feature b/application/features/groups-external-updates.feature deleted file mode 100644 index c4c358d8..00000000 --- a/application/features/groups-external-updates.feature +++ /dev/null @@ -1,42 +0,0 @@ -Feature: Updating a User's list of external groups - - Background: - Given the requester is authorized - - Scenario: Add an external group to a user's list for a particular app - Given a user exists - And that user's list of external groups is "wiki-users" - When I update that user's list of "wiki" external groups to the following: - | externalGroup | - | wiki-users | - | wiki-managers | - Then the response status code should be 204 - And that user's list of external groups should be "wiki-users,wiki-managers" - - Scenario: Remove an external group from a user's list for a particular app - Given a user exists - And that user's list of external groups is "wiki-users,wiki-managers" - When I update that user's list of "wiki" external groups to the following: - | externalGroup | - | wiki-managers | - Then the response status code should be 204 - And that user's list of external groups should be "wiki-managers" - - Scenario: Leave a user's external groups for a different app unchanged - Given a user exists - And that user's list of external groups is "wiki-users,map-europe" - When I update that user's list of "map" external groups to the following: - | externalGroup | - | map-america | - Then the response status code should be 204 - And that user's list of external groups should be "wiki-users,map-america" - - Scenario: Try to add an external group that does not match the given app-prefix - Given a user exists - And that user's list of external groups is "wiki-users" - When I update that user's list of "wiki" external groups to the following: - | externalGroup | - | map-america | - Then the response status code should be 422 - And the response body should contain "prefix" - And that user's list of external groups should be "wiki-users" diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature index 3b6ef113..d1bd9c71 100644 --- a/application/features/groups-external.feature +++ b/application/features/groups-external.feature @@ -39,9 +39,3 @@ Feature: Incorporating custom (external) groups in a User's `members` list | one | | two | | {idpName} | - -# # Scenarios that belong in the new "groups_external" sync: -# Scenario: Send 1 notification email if sync finds group(s) for a user not in this IDP -# Scenario: Add entries in the synced Google Sheet to the `groups_external` field -# Scenario: Remove entries not in the synced Google Sheet from the `groups_external` field -# Scenario: Only use entries from the synced Google Sheet that specify this IDP diff --git a/application/frontend/config/main.php b/application/frontend/config/main.php index ea56b04e..13818255 100644 --- a/application/frontend/config/main.php +++ b/application/frontend/config/main.php @@ -48,7 +48,6 @@ 'GET user' => 'user/index', 'GET user/' => 'user/view', 'POST user' => 'user/create', - 'PUT user/external-groups' => 'user/update-external-groups', 'PUT user/' => 'user/update', 'PUT user//password' => 'user/update-password', 'PUT user//password/assess' => 'user/assess-password', diff --git a/application/frontend/controllers/UserController.php b/application/frontend/controllers/UserController.php index 6c413c64..696a4808 100644 --- a/application/frontend/controllers/UserController.php +++ b/application/frontend/controllers/UserController.php @@ -75,27 +75,6 @@ public function actionUpdate(string $employeeId) return $user; } - public function actionUpdateExternalGroups() - { - $emailAddress = Yii::$app->request->getBodyParam('email_address'); - $appPrefix = Yii::$app->request->getBodyParam('app_prefix'); - $externalGroups = Yii::$app->request->getBodyParam('groups'); - - $user = User::findByEmail($emailAddress); - if ($user === null) { - Yii::$app->response->statusCode = 404; - return; - } - - if ($user->updateExternalGroups($appPrefix, $externalGroups)) { - Yii::$app->response->statusCode = 204; - return; - } - - $errors = join(', ', $user->getFirstErrors()); - throw new UnprocessableEntityHttpException($errors); - } - public function actionUpdatePassword(string $employeeId) { $user = User::findOne(['employee_id' => $employeeId]);