From e019a44e3ea4025d47e9504145024bd3885e8fef Mon Sep 17 00:00:00 2001
From: briskt <3172830+briskt@users.noreply.github.com>
Date: Mon, 27 May 2024 20:57:32 +0800
Subject: [PATCH 1/3] use material module for all tests

---
 actions-services.yml                          |  4 +-
 development/idp-local/config/authsources.php  |  3 ++
 development/idp3-local/config/config.php      |  2 +-
 development/sp-local/config/config.php        |  2 +-
 development/sp2-local/config/config.php       |  2 +-
 development/sp3-local/config/config.php       |  2 +-
 docker-compose.yml                            |  4 +-
 features/Sp1Idp1Sp2Idp2Sp3.feature            |  4 +-
 features/bootstrap/ExpiryContext.php          |  2 +-
 features/bootstrap/MfaContext.php             | 28 +++++-----
 features/bootstrap/ProfileReviewContext.php   | 54 +++++++++++++++++--
 features/bootstrap/SilDiscoContext.php        |  4 +-
 features/profilereview.feature                | 30 ++++++++---
 .../material/mfa/low-on-backup-codes.php      |  2 +-
 modules/mfa/lib/Auth/Process/Mfa.php          |  2 +-
 15 files changed, 107 insertions(+), 38 deletions(-)

diff --git a/actions-services.yml b/actions-services.yml
index abcd117f..07835e8d 100644
--- a/actions-services.yml
+++ b/actions-services.yml
@@ -117,7 +117,7 @@ services:
       PROFILE_URL_FOR_TESTS: "http://pwmanager.local/module.php/core/authenticate.php?as=ssp-hub"
       SECURE_COOKIE: "false"
       SHOW_SAML_ERRORS: "true"
-      THEME_USE: "default"
+      THEME_USE: "material:material"
       MYSQL_HOST: "db"
       MYSQL_DATABASE: "silauth"
       MYSQL_USER: "silauth"
@@ -261,7 +261,7 @@ services:
       SHOW_SAML_ERRORS: "true"
       SAML20_IDP_ENABLE: "false"
       ADMIN_PROTECT_INDEX_PAGE: "false"
-      THEME_USE: default
+      THEME_USE: "material:material"
 
   # the broker and brokerDb containers are used by the silauth module
   broker:
diff --git a/development/idp-local/config/authsources.php b/development/idp-local/config/authsources.php
index b03e4fcb..2c72033e 100644
--- a/development/idp-local/config/authsources.php
+++ b/development/idp-local/config/authsources.php
@@ -72,6 +72,9 @@
             'mail' => ['missing_exp@example.com'],
             'employeeNumber' => ['44444'],
             'cn' => ['MISSING_EXP'],
+            'mfa' => [
+                'prompt' => 'no',
+            ],
         ],
 
         // expirychecker test user whose password expiry is invalid
diff --git a/development/idp3-local/config/config.php b/development/idp3-local/config/config.php
index 656009fa..28a93ce9 100644
--- a/development/idp3-local/config/config.php
+++ b/development/idp3-local/config/config.php
@@ -27,7 +27,7 @@
 $SESSION_COOKIE_LIFETIME = (int)(Env::get('SESSION_COOKIE_LIFETIME', 0));
 $SESSION_REMEMBERME_LIFETIME = (int)(Env::get('SESSION_REMEMBERME_LIFETIME', (14 * 86400))); // 14 days
 $SECURE_COOKIE = Env::get('SECURE_COOKIE', true);
-$THEME_USE = Env::get('THEME_USE', 'default');
+$THEME_USE = Env::get('THEME_USE', 'material:material');
 $MEMCACHE_STORE_EXPIRES = (int)(Env::get('MEMCACHE_STORE_EXPIRES', (36 * 60 * 60))); // 36 hours.
 $SAML20_IDP_ENABLE = Env::get('SAML20_IDP_ENABLE', true);
 $GOOGLE_ENABLE = Env::get('GOOGLE_ENABLE', false);
diff --git a/development/sp-local/config/config.php b/development/sp-local/config/config.php
index 4bd8af87..742d2a61 100644
--- a/development/sp-local/config/config.php
+++ b/development/sp-local/config/config.php
@@ -37,7 +37,7 @@
 $SESSION_COOKIE_LIFETIME = (int)(Env::get('SESSION_COOKIE_LIFETIME', 0));
 $SESSION_REMEMBERME_LIFETIME = (int)(Env::get('SESSION_REMEMBERME_LIFETIME', (14 * 86400))); // 14 days
 $SECURE_COOKIE = Env::get('SECURE_COOKIE', true);
-$THEME_USE = Env::get('THEME_USE', 'default');
+$THEME_USE = Env::get('THEME_USE', 'material:material');
 $SAML20_IDP_ENABLE = Env::get('SAML20_IDP_ENABLE', true);
 $GOOGLE_ENABLE = Env::get('GOOGLE_ENABLE', false);
 
diff --git a/development/sp2-local/config/config.php b/development/sp2-local/config/config.php
index 3c42682a..26239025 100644
--- a/development/sp2-local/config/config.php
+++ b/development/sp2-local/config/config.php
@@ -37,7 +37,7 @@
 $SESSION_COOKIE_LIFETIME = (int)(Env::get('SESSION_COOKIE_LIFETIME', 0));
 $SESSION_REMEMBERME_LIFETIME = (int)(Env::get('SESSION_REMEMBERME_LIFETIME', (14 * 86400))); // 14 days
 $SECURE_COOKIE = Env::get('SECURE_COOKIE', true);
-$THEME_USE = Env::get('THEME_USE', 'default');
+$THEME_USE = Env::get('THEME_USE', 'material:material');
 $SAML20_IDP_ENABLE = Env::get('SAML20_IDP_ENABLE', true);
 $GOOGLE_ENABLE = Env::get('GOOGLE_ENABLE', false);
 
diff --git a/development/sp3-local/config/config.php b/development/sp3-local/config/config.php
index 1686e5a2..74e472af 100644
--- a/development/sp3-local/config/config.php
+++ b/development/sp3-local/config/config.php
@@ -37,7 +37,7 @@
 $SESSION_COOKIE_LIFETIME = (int)(Env::get('SESSION_COOKIE_LIFETIME', 0));
 $SESSION_REMEMBERME_LIFETIME = (int)(Env::get('SESSION_REMEMBERME_LIFETIME', (14 * 86400))); // 14 days
 $SECURE_COOKIE = Env::get('SECURE_COOKIE', true);
-$THEME_USE = Env::get('THEME_USE', 'default');
+$THEME_USE = Env::get('THEME_USE', 'material:material');
 $SAML20_IDP_ENABLE = Env::get('SAML20_IDP_ENABLE', true);
 $GOOGLE_ENABLE = Env::get('GOOGLE_ENABLE', false);
 
diff --git a/docker-compose.yml b/docker-compose.yml
index 267dbfa8..2b3a78a6 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -184,7 +184,7 @@ services:
       PROFILE_URL_FOR_TESTS: "http://pwmanager.local/module.php/core/authenticate.php?as=ssp-hub"
       SECURE_COOKIE: "false"
       SHOW_SAML_ERRORS: "true"
-      THEME_USE: "default"
+      THEME_USE: "material:material"
       SESSION_STORE_TYPE: "sql"
       MYSQL_HOST: "db"
       MYSQL_DATABASE: "silauth"
@@ -372,7 +372,7 @@ services:
       SHOW_SAML_ERRORS: "true"
       SAML20_IDP_ENABLE: "false"
       ADMIN_PROTECT_INDEX_PAGE: "false"
-      THEME_USE: material:material
+      THEME_USE: "material:material"
 
   # the broker and brokerDb containers are used by the silauth module
   broker:
diff --git a/features/Sp1Idp1Sp2Idp2Sp3.feature b/features/Sp1Idp1Sp2Idp2Sp3.feature
index be765055..6498fa23 100644
--- a/features/Sp1Idp1Sp2Idp2Sp3.feature
+++ b/features/Sp1Idp1Sp2Idp2Sp3.feature
@@ -25,9 +25,9 @@ Feature: Ensure I can login to Sp1 through Idp1, must login to Sp2 through Idp2
   Scenario: Logout of IDP1
     Given I have authenticated with IDP1 for SP1
     When I log out of IDP1
-    Then I should see "You have been logged out."
+    Then I should see "You have now been logged out."
 
   Scenario: Logout of IDP2
     Given I have authenticated with IDP2 for SP2
     When I log out of IDP2
-    Then I should see "You have been logged out."
+    Then I should see "You have now been logged out."
diff --git a/features/bootstrap/ExpiryContext.php b/features/bootstrap/ExpiryContext.php
index 749c134b..358e1767 100644
--- a/features/bootstrap/ExpiryContext.php
+++ b/features/bootstrap/ExpiryContext.php
@@ -152,7 +152,7 @@ public function iProvideCredentialsThatHaveNoPasswordExpirationDate()
     public function iShouldSeeAnErrorMessage()
     {
         $page = $this->session->getPage();
-        Assert::assertContains('Unhandled exception', $page->getHtml());
+        Assert::assertContains('An error occurred', $page->getHtml());
     }
 
     /**
diff --git a/features/bootstrap/MfaContext.php b/features/bootstrap/MfaContext.php
index 16dbdaa0..d23d1592 100644
--- a/features/bootstrap/MfaContext.php
+++ b/features/bootstrap/MfaContext.php
@@ -150,7 +150,7 @@ public function iShouldSeeAPromptForABackupCode()
     {
         $page = $this->session->getPage();
         $pageHtml = $page->getHtml();
-        Assert::assertContains('<h2>Printable Backup Code</h2>', $pageHtml);
+        Assert::assertContains('Printable code', $pageHtml);
         Assert::assertContains('Enter code', $pageHtml);
     }
     
@@ -171,7 +171,7 @@ public function iShouldSeeAPromptForATotpCode()
     {
         $page = $this->session->getPage();
         $pageHtml = $page->getHtml();
-        Assert::assertContains('<h2>Smartphone App</h2>', $pageHtml);
+        Assert::assertContains('Smartphone app', $pageHtml);
         Assert::assertContains('Enter 6-digit code', $pageHtml);
     }
 
@@ -191,7 +191,7 @@ public function iProvideCredentialsThatNeedMfaAndHaveUfAvailable()
     public function iShouldSeeAPromptForAWebAuthn()
     {
         $page = $this->session->getPage();
-        Assert::assertContains('<h2>USB Security Key</h2>', $page->getHtml());
+        Assert::assertContains('Security key', $page->getHtml());
     }
 
     protected function submitMfaValue($mfaValue)
@@ -207,8 +207,10 @@ protected function submitMfaValue($mfaValue)
      */
     public function iSubmitACorrectBackupCode()
     {
-        if (! $this->pageContainsElementWithText('h2', 'Printable Backup Code')) {
-            $this->clickLink('backupcode');
+        if (! $this->pageContainsElementWithText('h1', 'Printable code')) {
+            // find image of the backup code option presented in other_mfas.php
+            $printableCodeOption = $this->session->getPage()->find('css', 'img[src=mfa-backupcode\002Esvg]');
+            $printableCodeOption->click();
         }
         $this->submitMfaValue(FakeIdBrokerClient::CORRECT_VALUE);
     }
@@ -344,7 +346,7 @@ public function iShouldSeeAMessageThatIAmRunningLowOnBackupCodes()
     {
         $page = $this->session->getPage();
         Assert::assertContains(
-            'You are almost out of Printable Backup Codes',
+            'Almost out of printable codes',
             $page->getHtml()
         );
     }
@@ -375,7 +377,7 @@ public function iShouldSeeAMessageThatIHaveUsedUpMyBackupCodes()
     {
         $page = $this->session->getPage();
         Assert::assertContains(
-            'You just used your last Printable Backup Code',
+            'Last printable code used',
             $page->getHtml()
         );
     }
@@ -405,7 +407,7 @@ public function iShouldBeToldIOnlyHaveBackupCodesLeft($numRemaining)
     {
         $page = $this->session->getPage();
         Assert::assertContains(
-            'You only have ' . $numRemaining . ' remaining',
+            'You only have ' . $numRemaining . ' more left',
             $page->getHtml()
         );
     }
@@ -417,7 +419,7 @@ public function iShouldBeGivenMoreBackupCodes()
     {
         $page = $this->session->getPage();
         Assert::assertContains(
-            'Here are your new Printable Backup Codes',
+            'New printable codes',
             $page->getContent()
         );
     }
@@ -610,7 +612,7 @@ public function theUserHasAManagerEmail()
     public function iShouldSeeALinkToSendACodeToTheUsersManager()
     {
         $page = $this->session->getPage();
-        Assert::assertContains('Can\'t use any of your 2-Step Verification options', $page->getContent());
+        Assert::assertContains('I need help', $page->getContent());
     }
 
     /**
@@ -637,7 +639,9 @@ public function iShouldNotSeeALinkToSendACodeToTheUsersManager()
      */
     public function iClickTheRequestAssistanceLink()
     {
-        $this->clickLink('Click here');
+        // find image of the recovery contact option presented in prompt_for_mfa_manager.php
+        $printableCodeOption = $this->session->getPage()->find('css', 'img[src=mfa-manager\002Esvg]');
+        $printableCodeOption->click();
     }
 
     /**
@@ -655,7 +659,7 @@ public function iShouldSeeAPromptForAManagerRescueCode()
     {
         $page = $this->session->getPage();
         $pageHtml = $page->getHtml();
-        Assert::assertContains('<h2>Manager Rescue Code</h2>', $pageHtml);
+        Assert::assertContains('Recovery contact help', $pageHtml);
         Assert::assertContains('Enter code', $pageHtml);
     }
 
diff --git a/features/bootstrap/ProfileReviewContext.php b/features/bootstrap/ProfileReviewContext.php
index 9c88f526..917be783 100644
--- a/features/bootstrap/ProfileReviewContext.php
+++ b/features/bootstrap/ProfileReviewContext.php
@@ -77,13 +77,19 @@ public function iProvideCredentialsThatAreDueForAReminder($category, $nagType)
             case 'method_add':
                 $this->password = 'g';
                 break;
-
-            case 'profile_review':
-                $this->password = 'h';
-                break;
         }
     }
 
+    /**
+     * @Given I provide credentials that are due for a profile review
+     */
+    public function iProvideCredentialsThatAreDueForAProfileReview()
+    {
+        // See `development/idp-local/config/authsources.php` for options.
+        $this->username = 'profile_review';
+        $this->password = 'h';
+    }
+
 
     protected function pageContainsElementWithText($cssSelector, $text)
     {
@@ -122,6 +128,14 @@ public function iClickTheUpdateProfileButton()
         $this->submitFormByClickingButtonNamed('update');
     }
 
+    /**
+     * @When I click the :text link
+     */
+    public function iClickTheLink($text)
+    {
+        $this->clickLink($text);
+    }
+
     /**
      * @Then I should end up at the update profile URL
      */
@@ -137,6 +151,29 @@ public function iShouldEndUpAtTheUpdateProfileUrl()
         );
     }
 
+    /**
+     * @Then I should end up at the update profile URL on a new tab
+     */
+    public function iShouldEndUpAtTheUpdateProfileUrlOnANewTab()
+    {
+        $profileUrl = Env::get('PROFILE_URL_FOR_TESTS');
+        Assert::assertNotEmpty($profileUrl, 'No PROFILE_URL_FOR_TESTS provided');
+
+        $windowNames = $this->session->getWindowNames();
+        Assert::assertGreaterThanOrEqual(2, sizeof($windowNames),
+            'Expected to see at least 2 windows opened');
+
+        foreach ($windowNames as $windowName) {
+            $this->session->switchToWindow($windowName);
+            $currentUrl = $this->session->getCurrentUrl();
+            if ($currentUrl == $profileUrl) {
+                return;
+            }
+        }
+
+        Assert::fail('Did NOT end up at the update profile URL');
+    }
+
     /**
      * @Then I should see the message: :message
      */
@@ -155,6 +192,15 @@ public function thereShouldBeAWayToGoUpdateMyProfileNow()
         $this->assertFormContains('name="update"', $page);
     }
 
+    /**
+     * @Then there should be a way to go review my profile now
+     */
+    public function thereShouldBeAWayToGoReviewMyProfileNow()
+    {
+        $page = $this->session->getPage();
+        Assert::assertContains('Some of these need updating', $page->getHtml());
+    }
+
     /**
      * @Given I provide credentials for a user that has used the manager mfa option
      */
diff --git a/features/bootstrap/SilDiscoContext.php b/features/bootstrap/SilDiscoContext.php
index 24781840..b277f256 100644
--- a/features/bootstrap/SilDiscoContext.php
+++ b/features/bootstrap/SilDiscoContext.php
@@ -86,7 +86,7 @@ public function iLogOutOfIdp1()
         $this->iGoToTheSpLoginPage('SP3');
         $this->iClickOnTheTile('IDP 1');
         $this->clickLink('Logout');
-        $this->assertPageContainsText('You have been logged out.');
+        $this->assertPageContainsText('You have now been logged out.');
     }
 
     /**
@@ -96,7 +96,7 @@ public function iLogOutOfIdp2()
     {
         $this->iGoToTheSpLoginPage('SP2');
         $this->clickLink('Logout');
-        $this->assertPageContainsText('You have been logged out.');
+        $this->assertPageContainsText('You have now been logged out.');
     }
 
     /**
diff --git a/features/profilereview.feature b/features/profilereview.feature
index 9c3988d1..be4f84e3 100644
--- a/features/profilereview.feature
+++ b/features/profilereview.feature
@@ -16,10 +16,16 @@ Feature: Prompt to review profile information
     And there should be a way to continue to my intended destination
 
     Examples:
-      | category | nag type | message                          |
-      | mfa      | add      | "2-Step Verification"            |
-      | method   | add      | "alternate email addresses"      |
-      | profile  | review   | "Please take a moment to review" |
+      | category | nag type | message                    |
+      | mfa      | add      | "2-Step Verification"      |
+      | method   | add      | "alternate email address"  |
+
+  Scenario: Present profile review as required by the user profile
+    Given I provide credentials that are due for a profile review
+    When I log in
+    Then I should see the message: "Are these still correct?"
+    And there should be a way to go review my profile now
+    And there should be a way to continue to my intended destination
 
   Scenario Outline: Obeying a reminder
     Given I provide credentials that are due for a <category> <nag type> reminder
@@ -31,7 +37,12 @@ Feature: Prompt to review profile information
       | category | nag type |
       | mfa      | add      |
       | method   | add      |
-      | profile  | review   |
+
+  Scenario: Obeying a profile review reminder
+    Given I provide credentials that are due for a profile review
+    And I have logged in
+    When I click the "Some of these need updating" link
+    Then I should end up at the update profile URL on a new tab
 
   Scenario Outline: Ignoring a reminder
     Given I provide credentials that are due for a <category> <nag type> reminder
@@ -43,10 +54,15 @@ Feature: Prompt to review profile information
       | category | nag type |
       | mfa      | add      |
       | method   | add      |
-      | profile  | review   |
+
+  Scenario: Ignoring a profile review reminder
+    Given I provide credentials that are due for a profile review
+    And I have logged in
+    When I click the remind-me-later button
+    Then I should end up at my intended destination
 
   Scenario: Ensuring that manager mfa data is not displayed to the user
     Given I provide credentials for a user that has used the manager mfa option
     And I have logged in
-    Then I should see the message: "Please take a moment to review"
+    Then I should see the message: "Are these still correct?"
     And I should not see any manager mfa information
diff --git a/modules/material/themes/material/mfa/low-on-backup-codes.php b/modules/material/themes/material/mfa/low-on-backup-codes.php
index 152207dd..b8d75d41 100644
--- a/modules/material/themes/material/mfa/low-on-backup-codes.php
+++ b/modules/material/themes/material/mfa/low-on-backup-codes.php
@@ -29,7 +29,7 @@
 
                 <div class="mdl-card__title center" >
                     <p class="mdl-card__subtitle-text">
-                        <?= $this->t('{material:mfa:running_out_info}', ['{numBackupCodesRemaining}' => (int)$this->data['numBackupCodesRemaining']]) ?>
+                        <?= $this->t('{material:mfa:running_out_info}', ['{numBackupCodesRemaining}' => $this->data['numBackupCodesRemaining']]) ?>
                     </p>
                 </div>
 
diff --git a/modules/mfa/lib/Auth/Process/Mfa.php b/modules/mfa/lib/Auth/Process/Mfa.php
index ecf95aa8..44b475eb 100644
--- a/modules/mfa/lib/Auth/Process/Mfa.php
+++ b/modules/mfa/lib/Auth/Process/Mfa.php
@@ -761,7 +761,7 @@ protected static function redirectToLowOnBackupCodesNag(
         $numBackupCodesRemaining
     ) {
         $state['employeeId'] = $employeeId;
-        $state['numBackupCodesRemaining'] = $numBackupCodesRemaining;
+        $state['numBackupCodesRemaining'] = (string)$numBackupCodesRemaining;
         
         $stateId = State::saveState($state, self::STAGE_SENT_TO_LOW_ON_BACKUP_CODES_NAG);
         $url = Module::getModuleURL('mfa/low-on-backup-codes.php');

From be25acad767401aff3dd53dc886bd515ad022cf3 Mon Sep 17 00:00:00 2001
From: briskt <3172830+briskt@users.noreply.github.com>
Date: Wed, 29 May 2024 18:49:55 +0800
Subject: [PATCH 2/3] update match strings in mfa tests

---
 features/bootstrap/MfaContext.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/features/bootstrap/MfaContext.php b/features/bootstrap/MfaContext.php
index d23d1592..988f1fb6 100644
--- a/features/bootstrap/MfaContext.php
+++ b/features/bootstrap/MfaContext.php
@@ -171,7 +171,7 @@ public function iShouldSeeAPromptForATotpCode()
     {
         $page = $this->session->getPage();
         $pageHtml = $page->getHtml();
-        Assert::assertContains('Smartphone app', $pageHtml);
+        Assert::assertContains('Authenticator app', $pageHtml);
         Assert::assertContains('Enter 6-digit code', $pageHtml);
     }
 
@@ -659,7 +659,7 @@ public function iShouldSeeAPromptForAManagerRescueCode()
     {
         $page = $this->session->getPage();
         $pageHtml = $page->getHtml();
-        Assert::assertContains('Recovery contact help', $pageHtml);
+        Assert::assertContains('Ask Your Recovery Contact for Help', $pageHtml);
         Assert::assertContains('Enter code', $pageHtml);
     }
 

From 24fd66b7785cb7f13ddd65fbde8c12a57eebb13f Mon Sep 17 00:00:00 2001
From: briskt <3172830+briskt@users.noreply.github.com>
Date: Thu, 30 May 2024 10:54:14 +0800
Subject: [PATCH 3/3] clarify comment about the purpose of authsources.php
 [skip ci]

---
 features/bootstrap/ProfileReviewContext.php | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/features/bootstrap/ProfileReviewContext.php b/features/bootstrap/ProfileReviewContext.php
index 917be783..5312b09e 100644
--- a/features/bootstrap/ProfileReviewContext.php
+++ b/features/bootstrap/ProfileReviewContext.php
@@ -57,7 +57,7 @@ protected function submitFormByClickingButtonNamed($buttonName)
      */
     public function iProvideCredentialsThatDoNotNeedReview()
     {
-        // See `development/idp-local/config/authsources.php` for options.
+        // Credentials defined in `development/idp-local/config/authsources.php`
         $this->username = 'no_review';
         $this->password = 'e';
     }
@@ -67,7 +67,7 @@ public function iProvideCredentialsThatDoNotNeedReview()
      */
     public function iProvideCredentialsThatAreDueForAReminder($category, $nagType)
     {
-        // See `development/idp-local/config/authsources.php` for options.
+        // Credentials defined in `development/idp-local/config/authsources.php`
         $this->username = $category . '_' . $nagType;
         switch ($this->username) {
             case 'mfa_add':
@@ -85,7 +85,7 @@ public function iProvideCredentialsThatAreDueForAReminder($category, $nagType)
      */
     public function iProvideCredentialsThatAreDueForAProfileReview()
     {
-        // See `development/idp-local/config/authsources.php` for options.
+        // Credentials defined in `development/idp-local/config/authsources.php`
         $this->username = 'profile_review';
         $this->password = 'h';
     }