Merge pull request #358 from silinternational/feature/IDP-1183-sync-g…

…oogle-sheet-to-external-groups

[IDP-1183, partial] Revise "external groups" API endpoints

.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`

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": "[email protected]",
-        "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.

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"

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([

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())
         ));
     }
 

application/features/bootstrap/GroupsExternalSyncContext.php

@@ -0,0 +1,116 @@
+<?php
+
+namespace Sil\SilIdBroker\Behat\Context;
+
+use Behat\Gherkin\Node\TableNode;
+use common\models\User;
+use Webmozart\Assert\Assert;
+
+class GroupsExternalSyncContext extends GroupsExternalContext
+{
+    /**
+     * The lists of external groups for use by these tests. Example:
+     * ```
+     * [
+     *     'wiki' => [
+     *         '[email protected]' => 'wiki-one,wiki-two',
+     *     ],
+     * ]
+     * ```
+     * @var array<string,string[]>
+     */
+    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;
+        }
+    }
+}