From 07d8a98325bab06a4b5594a181f4875990000f13 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 11:53:02 -0400
Subject: [PATCH 01/20] Add first `groups_external` test and implement first
 two steps

---
 application/behat.yml                         |  4 +
 .../bootstrap/GroupsExternalContext.php       | 83 +++++++++++++++++++
 application/features/groups-external.feature  | 18 ++++
 3 files changed, 105 insertions(+)
 create mode 100644 application/features/bootstrap/GroupsExternalContext.php
 create mode 100644 application/features/groups-external.feature

diff --git a/application/behat.yml b/application/behat.yml
index 5d93e463..5a988363 100644
--- a/application/behat.yml
+++ b/application/behat.yml
@@ -19,6 +19,10 @@ default:
             paths:
                 - "features/email.feature"
             contexts: [ Sil\SilIdBroker\Behat\Context\EmailContext ]
+        groups_external_features:
+            paths:
+                - "features/groups-external.feature"
+            contexts: [ Sil\SilIdBroker\Behat\Context\GroupsExternalContext ]
         hibp_unit_tests_features:
             paths:
                 - "features/hibp-unit-tests.feature"
diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php
new file mode 100644
index 00000000..a7825091
--- /dev/null
+++ b/application/features/bootstrap/GroupsExternalContext.php
@@ -0,0 +1,83 @@
+<?php
+
+namespace Sil\SilIdBroker\Behat\Context;
+
+use Behat\Behat\Tester\Exception\PendingException;
+use common\models\User;
+use Webmozart\Assert\Assert;
+
+class GroupsExternalContext extends YiiContext
+{
+    private string $userEmailAddress = 'john_smith@example.org';
+    private User $user;
+
+    /**
+     * @Given a user exists
+     */
+    public function aUserExists()
+    {
+        $user = User::findByEmail($this->userEmailAddress);
+        if ($user === null) {
+            $user = $this->createTestUser();
+        }
+        $this->user = $user;
+    }
+
+    private function createTestUser(): User
+    {
+        $user = new User([
+            'email' => $this->userEmailAddress,
+            'employee_id' => '11111',
+            'first_name' => 'John',
+            'last_name' => 'Smith',
+            'username' => 'john_smith',
+        ]);
+        $user->scenario = User::SCENARIO_NEW_USER;
+        
+        $createdNewUser = $user->save();
+        Assert::true($createdNewUser, sprintf(
+            'Failed to create test user: %s',
+            join("\n", $user->getFirstErrors())
+        ));
+        return $user;
+    }
+
+    /**
+     * @Given that user's list of groups is :commaSeparatedGroups
+     */
+    public function thatUsersListOfGroupsIs($commaSeparatedGroups)
+    {
+        $this->user->groups = $commaSeparatedGroups;
+        $this->user->scenario = User::SCENARIO_UPDATE_USER;
+
+        $savedChanges = $this->user->save();
+        Assert::true($savedChanges, sprintf(
+            'Failed to save changes to test user: %s',
+            join("\n", $this->user->getFirstErrors())
+        ));
+    }
+
+    /**
+     * @Given that user's list of external groups is :arg1
+     */
+    public function thatUsersListOfExternalGroupsIs($arg1)
+    {
+        throw new PendingException();
+    }
+
+    /**
+     * @When I sign in as that user
+     */
+    public function iSignInAsThatUser()
+    {
+        throw new PendingException();
+    }
+
+    /**
+     * @Then the members list will be :arg1
+     */
+    public function theMembersListWillBe($arg1)
+    {
+        throw new PendingException();
+    }
+}
diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature
new file mode 100644
index 00000000..1f6a7d36
--- /dev/null
+++ b/application/features/groups-external.feature
@@ -0,0 +1,18 @@
+Feature: Incorporating custom (external) groups in a User's `members` list
+
+  # Scenarios that belong here in ID Broker:
+
+  Scenario: Include external groups in a User's `members` list
+    Given a user exists
+      And that user's list of groups is "one,two"
+      And that user's list of external groups is "app-three,app-four"
+    When I sign in as that user
+    Then the members list will be "one,two,app-three,app-four"
+
+#  Scenario: Update a user's `groups_external` list, given a group prefix and list of groups
+
+#  # 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

From 42b3213a06ac66665811b172fc8aeb5144c330bb Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 11:55:42 -0400
Subject: [PATCH 02/20] Implement (failing) test step for setting
 `groups_external` value

---
 .../bootstrap/GroupsExternalContext.php         | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php
index a7825091..3a47623e 100644
--- a/application/features/bootstrap/GroupsExternalContext.php
+++ b/application/features/bootstrap/GroupsExternalContext.php
@@ -33,7 +33,7 @@ private function createTestUser(): User
             'username' => 'john_smith',
         ]);
         $user->scenario = User::SCENARIO_NEW_USER;
-        
+
         $createdNewUser = $user->save();
         Assert::true($createdNewUser, sprintf(
             'Failed to create test user: %s',
@@ -52,17 +52,24 @@ public function thatUsersListOfGroupsIs($commaSeparatedGroups)
 
         $savedChanges = $this->user->save();
         Assert::true($savedChanges, sprintf(
-            'Failed to save changes to test user: %s',
+            'Failed to set list of `groups` on test user: %s',
             join("\n", $this->user->getFirstErrors())
         ));
     }
 
     /**
-     * @Given that user's list of external groups is :arg1
+     * @Given that user's list of external groups is :commaSeparatedExternalGroups
      */
-    public function thatUsersListOfExternalGroupsIs($arg1)
+    public function thatUsersListOfExternalGroupsIs($commaSeparatedExternalGroups)
     {
-        throw new PendingException();
+        $this->user->groups_external = $commaSeparatedExternalGroups;
+        $this->user->scenario = User::SCENARIO_UPDATE_USER;
+
+        $savedChanges = $this->user->save();
+        Assert::true($savedChanges, sprintf(
+            'Failed to set list of `groups_external` on test user: %s',
+            join("\n", $this->user->getFirstErrors())
+        ));
     }
 
     /**

From 6c6c14a962b841916b80c81af278fa7205df1b14 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:36:17 -0400
Subject: [PATCH 03/20] Use migration to add `user.groups_external` field to
 database

---
 ...5757_add_groups_external_field_to_user.php | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 application/console/migrations/m240813_155757_add_groups_external_field_to_user.php

diff --git a/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
new file mode 100644
index 00000000..531feeef
--- /dev/null
+++ b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
@@ -0,0 +1,19 @@
+<?php
+
+use yii\db\Migration;
+
+/**
+ * Class m240813_155757_add_groups_external_field_to_user
+ */
+class m240813_155757_add_groups_external_field_to_user extends Migration
+{
+    public function safeUp()
+    {
+        $this->addColumn('{{user}}', 'groups_external', 'string NOT NULL AFTER `groups`');
+    }
+
+    public function safeDown()
+    {
+        $this->dropColumn('{{user}}', 'groups_external');
+    }
+}

From 41702f2242fef73a98ed96e647d835e97dc34985 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:40:58 -0400
Subject: [PATCH 04/20] Regenerate the base models (excluding `groups_external`
 changes)

---
 application/common/models/EmailLogBase.php         |  4 ++--
 application/common/models/InviteBase.php           |  4 ++--
 application/common/models/MethodBase.php           |  4 ++--
 application/common/models/MfaBackupcodeBase.php    |  4 ++--
 application/common/models/MfaBase.php              | 10 +++++-----
 application/common/models/MfaFailedAttemptBase.php |  4 ++--
 application/common/models/MfaWebauthnBase.php      |  4 ++--
 application/common/models/PasswordBase.php         |  2 +-
 application/common/models/UserBase.php             | 12 ++++++------
 9 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/application/common/models/EmailLogBase.php b/application/common/models/EmailLogBase.php
index fcbc7ef8..c7a8ede9 100644
--- a/application/common/models/EmailLogBase.php
+++ b/application/common/models/EmailLogBase.php
@@ -34,7 +34,7 @@ public function rules()
             [['user_id'], 'integer'],
             [['message_type'], 'string'],
             [['sent_utc'], 'safe'],
-            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::className(), 'targetAttribute' => ['user_id' => 'id']],
+            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::class, 'targetAttribute' => ['user_id' => 'id']],
         ];
     }
 
@@ -58,6 +58,6 @@ public function attributeLabels()
      */
     public function getUser()
     {
-        return $this->hasOne(User::className(), ['id' => 'user_id']);
+        return $this->hasOne(User::class, ['id' => 'user_id']);
     }
 }
diff --git a/application/common/models/InviteBase.php b/application/common/models/InviteBase.php
index c7e41429..78e526ca 100644
--- a/application/common/models/InviteBase.php
+++ b/application/common/models/InviteBase.php
@@ -35,7 +35,7 @@ public function rules()
             [['user_id'], 'integer'],
             [['created_utc', 'expires_on'], 'safe'],
             [['uuid'], 'string', 'max' => 36],
-            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::className(), 'targetAttribute' => ['user_id' => 'id']],
+            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::class, 'targetAttribute' => ['user_id' => 'id']],
         ];
     }
 
@@ -60,6 +60,6 @@ public function attributeLabels()
      */
     public function getUser()
     {
-        return $this->hasOne(User::className(), ['id' => 'user_id']);
+        return $this->hasOne(User::class, ['id' => 'user_id']);
     }
 }
diff --git a/application/common/models/MethodBase.php b/application/common/models/MethodBase.php
index 4dc6ab73..899dbf87 100644
--- a/application/common/models/MethodBase.php
+++ b/application/common/models/MethodBase.php
@@ -44,7 +44,7 @@ public function rules()
             [['uid'], 'unique'],
             [['user_id', 'value'], 'unique', 'targetAttribute' => ['user_id', 'value']],
             [['verification_code'], 'unique'],
-            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::className(), 'targetAttribute' => ['user_id' => 'id']],
+            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::class, 'targetAttribute' => ['user_id' => 'id']],
         ];
     }
 
@@ -73,6 +73,6 @@ public function attributeLabels()
      */
     public function getUser()
     {
-        return $this->hasOne(User::className(), ['id' => 'user_id']);
+        return $this->hasOne(User::class, ['id' => 'user_id']);
     }
 }
diff --git a/application/common/models/MfaBackupcodeBase.php b/application/common/models/MfaBackupcodeBase.php
index 663899aa..0e5cd909 100644
--- a/application/common/models/MfaBackupcodeBase.php
+++ b/application/common/models/MfaBackupcodeBase.php
@@ -35,7 +35,7 @@ public function rules()
             [['mfa_id'], 'integer'],
             [['created_utc', 'expires_utc'], 'safe'],
             [['value'], 'string', 'max' => 255],
-            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::className(), 'targetAttribute' => ['mfa_id' => 'id']],
+            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::class, 'targetAttribute' => ['mfa_id' => 'id']],
         ];
     }
 
@@ -60,6 +60,6 @@ public function attributeLabels()
      */
     public function getMfa()
     {
-        return $this->hasOne(Mfa::className(), ['id' => 'mfa_id']);
+        return $this->hasOne(Mfa::class, ['id' => 'mfa_id']);
     }
 }
diff --git a/application/common/models/MfaBase.php b/application/common/models/MfaBase.php
index 46a1f351..fe9b8004 100644
--- a/application/common/models/MfaBase.php
+++ b/application/common/models/MfaBase.php
@@ -44,7 +44,7 @@ public function rules()
             [['created_utc', 'last_used_utc'], 'safe'],
             [['external_uuid', 'label'], 'string', 'max' => 64],
             [['key_handle_hash'], 'string', 'max' => 255],
-            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::className(), 'targetAttribute' => ['user_id' => 'id']],
+            [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::class, 'targetAttribute' => ['user_id' => 'id']],
         ];
     }
 
@@ -73,7 +73,7 @@ public function attributeLabels()
      */
     public function getMfaBackupcodes()
     {
-        return $this->hasMany(MfaBackupcode::className(), ['mfa_id' => 'id']);
+        return $this->hasMany(MfaBackupcode::class, ['mfa_id' => 'id']);
     }
 
     /**
@@ -83,7 +83,7 @@ public function getMfaBackupcodes()
      */
     public function getMfaFailedAttempts()
     {
-        return $this->hasMany(MfaFailedAttempt::className(), ['mfa_id' => 'id']);
+        return $this->hasMany(MfaFailedAttempt::class, ['mfa_id' => 'id']);
     }
 
     /**
@@ -93,7 +93,7 @@ public function getMfaFailedAttempts()
      */
     public function getMfaWebauthns()
     {
-        return $this->hasMany(MfaWebauthn::className(), ['mfa_id' => 'id']);
+        return $this->hasMany(MfaWebauthn::class, ['mfa_id' => 'id']);
     }
 
     /**
@@ -103,6 +103,6 @@ public function getMfaWebauthns()
      */
     public function getUser()
     {
-        return $this->hasOne(User::className(), ['id' => 'user_id']);
+        return $this->hasOne(User::class, ['id' => 'user_id']);
     }
 }
diff --git a/application/common/models/MfaFailedAttemptBase.php b/application/common/models/MfaFailedAttemptBase.php
index ce212aaf..12155d53 100644
--- a/application/common/models/MfaFailedAttemptBase.php
+++ b/application/common/models/MfaFailedAttemptBase.php
@@ -32,7 +32,7 @@ public function rules()
             [['mfa_id', 'at_utc'], 'required'],
             [['mfa_id'], 'integer'],
             [['at_utc'], 'safe'],
-            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::className(), 'targetAttribute' => ['mfa_id' => 'id']],
+            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::class, 'targetAttribute' => ['mfa_id' => 'id']],
         ];
     }
 
@@ -55,6 +55,6 @@ public function attributeLabels()
      */
     public function getMfa()
     {
-        return $this->hasOne(Mfa::className(), ['id' => 'mfa_id']);
+        return $this->hasOne(Mfa::class, ['id' => 'mfa_id']);
     }
 }
diff --git a/application/common/models/MfaWebauthnBase.php b/application/common/models/MfaWebauthnBase.php
index 1f1c14fa..afd78408 100644
--- a/application/common/models/MfaWebauthnBase.php
+++ b/application/common/models/MfaWebauthnBase.php
@@ -37,7 +37,7 @@ public function rules()
             [['created_utc', 'last_used_utc'], 'safe'],
             [['key_handle_hash'], 'string', 'max' => 255],
             [['label'], 'string', 'max' => 64],
-            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::className(), 'targetAttribute' => ['mfa_id' => 'id']],
+            [['mfa_id'], 'exist', 'skipOnError' => true, 'targetClass' => Mfa::class, 'targetAttribute' => ['mfa_id' => 'id']],
         ];
     }
 
@@ -63,6 +63,6 @@ public function attributeLabels()
      */
     public function getMfa()
     {
-        return $this->hasOne(Mfa::className(), ['id' => 'mfa_id']);
+        return $this->hasOne(Mfa::class, ['id' => 'mfa_id']);
     }
 }
diff --git a/application/common/models/PasswordBase.php b/application/common/models/PasswordBase.php
index a93c36dc..a63f359a 100644
--- a/application/common/models/PasswordBase.php
+++ b/application/common/models/PasswordBase.php
@@ -66,6 +66,6 @@ public function attributeLabels()
      */
     public function getUsers()
     {
-        return $this->hasMany(User::className(), ['current_password_id' => 'id']);
+        return $this->hasMany(User::class, ['current_password_id' => 'id']);
     }
 }
diff --git a/application/common/models/UserBase.php b/application/common/models/UserBase.php
index 245d396c..217dd5e0 100644
--- a/application/common/models/UserBase.php
+++ b/application/common/models/UserBase.php
@@ -64,7 +64,7 @@ public function rules()
             [['employee_id'], 'unique'],
             [['username'], 'unique'],
             [['email'], 'unique'],
-            [['current_password_id'], 'exist', 'skipOnError' => true, 'targetClass' => Password::className(), 'targetAttribute' => ['current_password_id' => 'id']],
+            [['current_password_id'], 'exist', 'skipOnError' => true, 'targetClass' => Password::class, 'targetAttribute' => ['current_password_id' => 'id']],
         ];
     }
 
@@ -109,7 +109,7 @@ public function attributeLabels()
      */
     public function getCurrentPassword()
     {
-        return $this->hasOne(Password::className(), ['id' => 'current_password_id']);
+        return $this->hasOne(Password::class, ['id' => 'current_password_id']);
     }
 
     /**
@@ -119,7 +119,7 @@ public function getCurrentPassword()
      */
     public function getEmailLogs()
     {
-        return $this->hasMany(EmailLog::className(), ['user_id' => 'id']);
+        return $this->hasMany(EmailLog::class, ['user_id' => 'id']);
     }
 
     /**
@@ -129,7 +129,7 @@ public function getEmailLogs()
      */
     public function getInvites()
     {
-        return $this->hasMany(Invite::className(), ['user_id' => 'id']);
+        return $this->hasMany(Invite::class, ['user_id' => 'id']);
     }
 
     /**
@@ -139,7 +139,7 @@ public function getInvites()
      */
     public function getMethods()
     {
-        return $this->hasMany(Method::className(), ['user_id' => 'id']);
+        return $this->hasMany(Method::class, ['user_id' => 'id']);
     }
 
     /**
@@ -149,6 +149,6 @@ public function getMethods()
      */
     public function getMfas()
     {
-        return $this->hasMany(Mfa::className(), ['user_id' => 'id']);
+        return $this->hasMany(Mfa::class, ['user_id' => 'id']);
     }
 }

From 6aa3d7a45ef6331912247c546266d6bafa314a49 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:41:19 -0400
Subject: [PATCH 05/20] Regenerate UserBase model to include `groups_external`
 field

---
 application/common/models/UserBase.php | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/application/common/models/UserBase.php b/application/common/models/UserBase.php
index 217dd5e0..8c554cf6 100644
--- a/application/common/models/UserBase.php
+++ b/application/common/models/UserBase.php
@@ -26,6 +26,7 @@
  * @property string|null $manager_email
  * @property string $hide
  * @property string|null $groups
+ * @property string $groups_external
  * @property string|null $personal_email
  * @property string|null $expires_on
  * @property string $nag_for_mfa_after
@@ -55,12 +56,12 @@ public static function tableName()
     public function rules()
     {
         return [
-            [['uuid', 'employee_id', 'first_name', 'last_name', 'username', 'active', 'locked', 'last_changed_utc', 'last_synced_utc', 'review_profile_after', 'nag_for_mfa_after', 'nag_for_method_after'], 'required'],
+            [['uuid', 'employee_id', 'first_name', 'last_name', 'username', 'active', 'locked', 'last_changed_utc', 'last_synced_utc', 'review_profile_after', 'groups_external', 'nag_for_mfa_after', 'nag_for_method_after'], 'required'],
             [['current_password_id'], 'integer'],
             [['active', 'locked', 'require_mfa', 'hide'], 'string'],
             [['last_changed_utc', 'last_synced_utc', 'review_profile_after', 'last_login_utc', 'expires_on', 'nag_for_mfa_after', 'nag_for_method_after', 'created_utc', 'deactivated_utc'], 'safe'],
             [['uuid'], 'string', 'max' => 64],
-            [['employee_id', 'first_name', 'last_name', 'display_name', 'username', 'email', 'manager_email', 'groups', 'personal_email'], 'string', 'max' => 255],
+            [['employee_id', 'first_name', 'last_name', 'display_name', 'username', 'email', 'manager_email', 'groups', 'groups_external', 'personal_email'], 'string', 'max' => 255],
             [['employee_id'], 'unique'],
             [['username'], 'unique'],
             [['email'], 'unique'],
@@ -93,6 +94,7 @@ public function attributeLabels()
             'manager_email' => Yii::t('app', 'Manager Email'),
             'hide' => Yii::t('app', 'Hide'),
             'groups' => Yii::t('app', 'Groups'),
+            'groups_external' => Yii::t('app', 'Groups External'),
             'personal_email' => Yii::t('app', 'Personal Email'),
             'expires_on' => Yii::t('app', 'Expires On'),
             'nag_for_mfa_after' => Yii::t('app', 'Nag For Mfa After'),

From bd0b0b1f1dd0b55aa4271b3ddeb45006d1c5a7e7 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:47:30 -0400
Subject: [PATCH 06/20] Set `user.groups_external`'s default value to be an
 empty string

---
 .../m240813_155757_add_groups_external_field_to_user.php        | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
index 531feeef..dc8fd18a 100644
--- a/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
+++ b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
@@ -9,7 +9,7 @@ class m240813_155757_add_groups_external_field_to_user extends Migration
 {
     public function safeUp()
     {
-        $this->addColumn('{{user}}', 'groups_external', 'string NOT NULL AFTER `groups`');
+        $this->addColumn('{{user}}', 'groups_external', 'string NOT NULL DEFAULT "" AFTER `groups`');
     }
 
     public function safeDown()

From db30f68d824db9c41633bd296716d0da560bd963 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:49:48 -0400
Subject: [PATCH 07/20] Switch to fluent syntax (not DB-specific) for defining
 the new field

---
 .../m240813_155757_add_groups_external_field_to_user.php    | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
index dc8fd18a..84543bc9 100644
--- a/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
+++ b/application/console/migrations/m240813_155757_add_groups_external_field_to_user.php
@@ -9,7 +9,11 @@ class m240813_155757_add_groups_external_field_to_user extends Migration
 {
     public function safeUp()
     {
-        $this->addColumn('{{user}}', 'groups_external', 'string NOT NULL DEFAULT "" AFTER `groups`');
+        $this->addColumn(
+            '{{user}}',
+            'groups_external',
+            $this->string()->notNull()->defaultValue('')->after('groups')
+        );
     }
 
     public function safeDown()

From 3f47bb3ca83a22efc45b670eed9a59453af1a7e2 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 15:50:51 -0400
Subject: [PATCH 08/20] Regenerate UserBase model now that `groups_external`
 has a default value

---
 application/common/models/UserBase.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/application/common/models/UserBase.php b/application/common/models/UserBase.php
index 8c554cf6..e0200c7d 100644
--- a/application/common/models/UserBase.php
+++ b/application/common/models/UserBase.php
@@ -56,7 +56,7 @@ public static function tableName()
     public function rules()
     {
         return [
-            [['uuid', 'employee_id', 'first_name', 'last_name', 'username', 'active', 'locked', 'last_changed_utc', 'last_synced_utc', 'review_profile_after', 'groups_external', 'nag_for_mfa_after', 'nag_for_method_after'], 'required'],
+            [['uuid', 'employee_id', 'first_name', 'last_name', 'username', 'active', 'locked', 'last_changed_utc', 'last_synced_utc', 'review_profile_after', 'nag_for_mfa_after', 'nag_for_method_after'], 'required'],
             [['current_password_id'], 'integer'],
             [['active', 'locked', 'require_mfa', 'hide'], 'string'],
             [['last_changed_utc', 'last_synced_utc', 'review_profile_after', 'last_login_utc', 'expires_on', 'nag_for_mfa_after', 'nag_for_method_after', 'created_utc', 'deactivated_utc'], 'safe'],

From ffb26745274463c50feccd35f9b03a42d7c4aa2a Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Tue, 13 Aug 2024 16:29:11 -0400
Subject: [PATCH 09/20] Get new test successfully signing in as the test user

---
 application/features/groups-external.feature | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature
index 1f6a7d36..9391cd75 100644
--- a/application/features/groups-external.feature
+++ b/application/features/groups-external.feature
@@ -1,5 +1,8 @@
 Feature: Incorporating custom (external) groups in a User's `members` list
 
+  Background:
+    Given the requester is authorized
+
   # Scenarios that belong here in ID Broker:
 
   Scenario: Include external groups in a User's `members` list

From bb5f0009742af5bbec980c8d12c00f90f97515f8 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Wed, 14 Aug 2024 15:08:19 -0400
Subject: [PATCH 10/20] Add phpMyAdmin container for testdb

---
 docker-compose.yml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/docker-compose.yml b/docker-compose.yml
index c7e14658..fb10cd4b 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -211,6 +211,19 @@ services:
             PMA_USER: user
             PMA_PASSWORD: pass
 
+    phpmyadmintest:
+        image: phpmyadmin:5
+        ports:
+            - "51142:80"
+        depends_on:
+            - testdb
+        env_file:
+            - ./local.env
+        environment:
+            PMA_HOST: testdb
+            PMA_USER: appfortests
+            PMA_PASSWORD: appfortests
+
     raml2html:
         image: mattjtodd/raml2html
         platform: linux/amd64

From 7dbad6e295792a65a6d036539a81f775c6bb3b82 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Wed, 14 Aug 2024 15:09:41 -0400
Subject: [PATCH 11/20] Improve groups_external test, implement next (failing)
 step

---
 .../bootstrap/GroupsExternalContext.php       | 66 +++++++++++++++----
 application/features/groups-external.feature  |  7 +-
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php
index 3a47623e..be295f09 100644
--- a/application/features/bootstrap/GroupsExternalContext.php
+++ b/application/features/bootstrap/GroupsExternalContext.php
@@ -2,28 +2,40 @@
 
 namespace Sil\SilIdBroker\Behat\Context;
 
-use Behat\Behat\Tester\Exception\PendingException;
+use Behat\Gherkin\Node\TableNode;
 use common\models\User;
+use FeatureContext;
 use Webmozart\Assert\Assert;
 
-class GroupsExternalContext extends YiiContext
+class GroupsExternalContext extends FeatureContext
 {
-    private string $userEmailAddress = 'john_smith@example.org';
     private User $user;
+    private string $userEmailAddress = 'john_smith@example.org';
+    private string $userPassword = 'dummy-password-#1';
 
     /**
      * @Given a user exists
      */
     public function aUserExists()
+    {
+        $this->deleteThatTestUser();
+        $this->createTestUser();
+        $this->setThatUsersPassword($this->userPassword);
+    }
+
+    private function deleteThatTestUser()
     {
         $user = User::findByEmail($this->userEmailAddress);
-        if ($user === null) {
-            $user = $this->createTestUser();
+        if ($user !== null) {
+            $didDeleteUser = $user->delete();
+            Assert::notFalse($didDeleteUser, sprintf(
+                'Failed to delete existing test user: %s',
+                join("\n", $user->getFirstErrors())
+            ));
         }
-        $this->user = $user;
     }
 
-    private function createTestUser(): User
+    private function createTestUser()
     {
         $user = new User([
             'email' => $this->userEmailAddress,
@@ -39,7 +51,20 @@ private function createTestUser(): User
             'Failed to create test user: %s',
             join("\n", $user->getFirstErrors())
         ));
-        return $user;
+        $user->refresh();
+
+        $this->user = $user;
+    }
+
+    private function setThatUsersPassword(string $password)
+    {
+        $this->user->scenario = User::SCENARIO_UPDATE_PASSWORD;
+        $this->user->password = $password;
+
+        Assert::true($this->user->save(), sprintf(
+            "Failed to set the test user's password: %s",
+            join("\n", $this->user->getFirstErrors())
+        ));
     }
 
     /**
@@ -77,14 +102,31 @@ public function thatUsersListOfExternalGroupsIs($commaSeparatedExternalGroups)
      */
     public function iSignInAsThatUser()
     {
-        throw new PendingException();
+        $dataForTableNode = [
+            ['property', 'value'],
+            ['username', $this->user->username],
+            ['password', $this->userPassword],
+        ];
+        $this->iProvideTheFollowingValidData(new TableNode($dataForTableNode));
+        $this->iRequestTheResourceBe('/authentication', 'created');
+        $this->theResponseStatusCodeShouldBe(200);
     }
 
     /**
-     * @Then the members list will be :arg1
+     * @Then the member list will include the following groups:
      */
-    public function theMembersListWillBe($arg1)
+    public function theMemberListWillIncludeTheFollowingGroups(TableNode $table)
     {
-        throw new PendingException();
+        $memberList = $this->getResponseProperty('member');
+        Assert::notEmpty($memberList);
+
+        foreach ($table as $row) {
+            $group = $row['group'];
+            Assert::inArray($group, $memberList, sprintf(
+                'Expected to find group %s, but only found %s.',
+                $group,
+                join(', ', $memberList)
+            ));
+        }
     }
 }
diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature
index 9391cd75..05f69e12 100644
--- a/application/features/groups-external.feature
+++ b/application/features/groups-external.feature
@@ -10,7 +10,12 @@ Feature: Incorporating custom (external) groups in a User's `members` list
       And that user's list of groups is "one,two"
       And that user's list of external groups is "app-three,app-four"
     When I sign in as that user
-    Then the members list will be "one,two,app-three,app-four"
+    Then the member list will include the following groups:
+      | group     |
+      | one       |
+      | two       |
+      | app-three |
+      | app-four  |
 
 #  Scenario: Update a user's `groups_external` list, given a group prefix and list of groups
 

From 54fd8bae66a33de5f93fb2c02435f90b04500ead Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Wed, 14 Aug 2024 15:27:21 -0400
Subject: [PATCH 12/20] Make groups_external test failure message more
 informative

---
 application/features/bootstrap/GroupsExternalContext.php | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php
index be295f09..3286cb07 100644
--- a/application/features/bootstrap/GroupsExternalContext.php
+++ b/application/features/bootstrap/GroupsExternalContext.php
@@ -123,9 +123,10 @@ public function theMemberListWillIncludeTheFollowingGroups(TableNode $table)
         foreach ($table as $row) {
             $group = $row['group'];
             Assert::inArray($group, $memberList, sprintf(
-                'Expected to find group %s, but only found %s.',
+                'Expected to find group %s, but only found %s. User: %s',
                 $group,
-                join(', ', $memberList)
+                join(', ', $memberList),
+                json_encode($this->user->attributes, JSON_PRETTY_PRINT)
             ));
         }
     }

From 9fcaa428bc862f64613d4d780686eccfa49d8711 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Wed, 14 Aug 2024 15:28:00 -0400
Subject: [PATCH 13/20] Include groups_external values in a User's `member`
 list during login

---
 application/common/models/User.php | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/application/common/models/User.php b/application/common/models/User.php
index ebe550b9..d690c0ea 100644
--- a/application/common/models/User.php
+++ b/application/common/models/User.php
@@ -571,7 +571,15 @@ public function fields(): array
             'member' => function (self $model) {
                 if (!empty($model->groups)) {
                     $member = explode(',', $model->groups);
+                } else {
+                    $member = [];
+                }
+
+                $externalGroups = explode(',', $model->groups_external);
+                foreach ($externalGroups as $externalGroup) {
+                    $member[] = $externalGroup;
                 }
+
                 $member[] = \Yii::$app->params['idpName'];
                 return $member;
             },

From 0d43d78cabfaae72c7ec3469f060a9651e94952a Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Wed, 14 Aug 2024 15:54:32 -0400
Subject: [PATCH 14/20] Ensure empty `groups` and empty `groups_external`
 fields work properly

---
 application/features/groups-external.feature | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature
index 05f69e12..ac26a81e 100644
--- a/application/features/groups-external.feature
+++ b/application/features/groups-external.feature
@@ -17,6 +17,26 @@ Feature: Incorporating custom (external) groups in a User's `members` list
       | app-three |
       | app-four  |
 
+  Scenario: Gracefully handle an empty list of groups in a User's `members` list
+    Given a user exists
+      And that user's list of groups is ""
+      And that user's list of external groups is "app-three,app-four"
+    When I sign in as that user
+    Then the member list will include the following groups:
+      | group     |
+      | app-three |
+      | app-four  |
+
+  Scenario: Gracefully handle an empty list of external groups in a User's `members` list
+    Given a user exists
+      And that user's list of groups is "one,two"
+      And that user's list of external groups is ""
+    When I sign in as that user
+    Then the member list will include the following groups:
+      | group     |
+      | one       |
+      | two       |
+
 #  Scenario: Update a user's `groups_external` list, given a group prefix and list of groups
 
 #  # Scenarios that belong in the new "groups_external" sync:

From 690289e9f746e163b9511e70695a365712b38ae1 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:24:45 -0400
Subject: [PATCH 15/20] Customize the label for the new `groups_external` field

---
 application/common/models/User.php | 1 +
 1 file changed, 1 insertion(+)

diff --git a/application/common/models/User.php b/application/common/models/User.php
index d690c0ea..835e5bd8 100644
--- a/application/common/models/User.php
+++ b/application/common/models/User.php
@@ -977,6 +977,7 @@ public function attributeLabels()
         $labels['last_synced_utc'] = Yii::t('app', 'Last Synced (UTC)');
         $labels['created_utc'] = Yii::t('app', 'Created (UTC)');
         $labels['deactivated_utc'] = Yii::t('app', 'Deactivated (UTC)');
+        $labels['groups_external'] = Yii::t('app', 'Groups (External)');
 
         return $labels;
     }

From 4f6e2113b915a12d792852ee46fb21fd4b7c6ad6 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:43:13 -0400
Subject: [PATCH 16/20] Stop running CI/CD tests as soon as one of them fails

---
 application/run-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/application/run-tests.sh b/application/run-tests.sh
index b6acfbc5..a5cbf5c3 100755
--- a/application/run-tests.sh
+++ b/application/run-tests.sh
@@ -43,7 +43,7 @@ apachectl start
 rc=$?; if [[ $rc != 0 ]]; then exit $rc; fi
 
 # Run the feature tests
-./vendor/bin/behat --strict
+./vendor/bin/behat --strict --stop-on-failure
 
 # If they failed, exit.
 rc=$?; if [[ $rc != 0 ]]; then exit $rc; fi

From ca0c7a22a4c9aed3830ea00449ab76c75853e2b6 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:54:53 -0400
Subject: [PATCH 17/20] Fix groups_external test to detect an empty string in
 the `members` list

---
 application/features/groups-external.feature | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/application/features/groups-external.feature b/application/features/groups-external.feature
index ac26a81e..f93e6232 100644
--- a/application/features/groups-external.feature
+++ b/application/features/groups-external.feature
@@ -10,32 +10,35 @@ Feature: Incorporating custom (external) groups in a User's `members` list
       And that user's list of groups is "one,two"
       And that user's list of external groups is "app-three,app-four"
     When I sign in as that user
-    Then the member list will include the following groups:
-      | group     |
+    Then the response should contain a member array with only these elements:
+      | element   |
       | one       |
       | two       |
       | app-three |
       | app-four  |
+      | {idpName} |
 
   Scenario: Gracefully handle an empty list of groups in a User's `members` list
     Given a user exists
       And that user's list of groups is ""
       And that user's list of external groups is "app-three,app-four"
     When I sign in as that user
-    Then the member list will include the following groups:
-      | group     |
+    Then the response should contain a member array with only these elements:
+      | element   |
       | app-three |
       | app-four  |
+      | {idpName} |
 
   Scenario: Gracefully handle an empty list of external groups in a User's `members` list
     Given a user exists
       And that user's list of groups is "one,two"
       And that user's list of external groups is ""
     When I sign in as that user
-    Then the member list will include the following groups:
-      | group     |
+    Then the response should contain a member array with only these elements:
+      | element   |
       | one       |
       | two       |
+      | {idpName} |
 
 #  Scenario: Update a user's `groups_external` list, given a group prefix and list of groups
 

From 931f4db779415a8e6b41e57ce7b231a7133eeed7 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:55:08 -0400
Subject: [PATCH 18/20] Include the array contents in the failed-test output

---
 application/features/bootstrap/FeatureContext.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/application/features/bootstrap/FeatureContext.php b/application/features/bootstrap/FeatureContext.php
index 45e74eac..b4beece0 100644
--- a/application/features/bootstrap/FeatureContext.php
+++ b/application/features/bootstrap/FeatureContext.php
@@ -838,7 +838,7 @@ public function theResponseShouldContainAMemberArrayWithOnlyTheseElements($key,
             if ($want == '{idpName}') {
                 $want = \Yii::$app->params['idpName'];
             }
-            Assert::true(in_array($want, $property), '"' . $want . '" not in array');
+            Assert::true(in_array($want, $property), '"' . $want . '" not in array: ' . json_encode($property));
             $n++;
         }
 

From b608e9f1bcad30af686e44f58e00fe810c8ab642 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:55:18 -0400
Subject: [PATCH 19/20] Remove now-unused new test step

---
 .../bootstrap/GroupsExternalContext.php       | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/application/features/bootstrap/GroupsExternalContext.php b/application/features/bootstrap/GroupsExternalContext.php
index 3286cb07..a39c4851 100644
--- a/application/features/bootstrap/GroupsExternalContext.php
+++ b/application/features/bootstrap/GroupsExternalContext.php
@@ -111,23 +111,4 @@ public function iSignInAsThatUser()
         $this->iRequestTheResourceBe('/authentication', 'created');
         $this->theResponseStatusCodeShouldBe(200);
     }
-
-    /**
-     * @Then the member list will include the following groups:
-     */
-    public function theMemberListWillIncludeTheFollowingGroups(TableNode $table)
-    {
-        $memberList = $this->getResponseProperty('member');
-        Assert::notEmpty($memberList);
-
-        foreach ($table as $row) {
-            $group = $row['group'];
-            Assert::inArray($group, $memberList, sprintf(
-                'Expected to find group %s, but only found %s. User: %s',
-                $group,
-                join(', ', $memberList),
-                json_encode($this->user->attributes, JSON_PRETTY_PRINT)
-            ));
-        }
-    }
 }

From 6ce06ac33ad51ea054191532f7b999dcb4916422 Mon Sep 17 00:00:00 2001
From: Matt H <matt_henderson@sil.org>
Date: Mon, 19 Aug 2024 14:57:12 -0400
Subject: [PATCH 20/20] Avoid adding empty string to `members` when
 `groups_external` is empty

PHP's `explode()` returns an array with an empty string if the given
string is empty, but we do not want to add that to the `members` list.
---
 application/common/models/User.php | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/application/common/models/User.php b/application/common/models/User.php
index 835e5bd8..27e63ea8 100644
--- a/application/common/models/User.php
+++ b/application/common/models/User.php
@@ -577,7 +577,9 @@ public function fields(): array
 
                 $externalGroups = explode(',', $model->groups_external);
                 foreach ($externalGroups as $externalGroup) {
-                    $member[] = $externalGroup;
+                    if (!empty($externalGroup)) {
+                        $member[] = $externalGroup;
+                    }
                 }
 
                 $member[] = \Yii::$app->params['idpName'];