[#1611]: SecKit update + tests. #1635
Conversation
|
""" WalkthroughA new configuration setting was added to disable the 'upgrade-req' option within the Content Security Policy (CSP) settings for CI and LOCAL environments. Corresponding test cases were updated to check for this setting. Additionally, a comment was corrected to accurately reference CSP. A FAQ entry was also added to address a PHPUnit pipeline failure related to directory differences. Furthermore, extensive new test cases were added for Acquia and Lagoon hosting environments to validate environment detection and configuration settings. Changes
Sequence Diagram(s)sequenceDiagram
participant EnvTest as EnvironmentSettingsTest
participant EnvLoader as Environment Loader
participant Config as Configuration
participant SecKit as SecKit Module
EnvTest->>EnvLoader: Set environment variables (CI, LOCAL, Acquia, Lagoon)
EnvLoader->>Config: Load environment-specific settings
Config->>SecKit: Disable CSP 'checkbox' and 'upgrade-req' for CI/LOCAL
EnvTest->>Config: Assert expected configuration values per environment
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🔇 Additional comments (13)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/behat/bootstrap/FeatureContext.php(2 hunks)tests/behat/features/seckit.feature(1 hunks)tests/phpunit/Drupal/EnvironmentSettingsTest.php(2 hunks)web/sites/default/includes/modules/settings.seckit.php(1 hunks)
🧰 Additional context used
🪛 PHPStan (2.0.3)
tests/behat/bootstrap/FeatureContext.php
92-92: PHPDoc tag @SuppressWarnings has invalid value ((PHPMD.StaticAccess)): Unexpected token ".StaticAccess)", expected ')' at offset 171 on line 7
(phpDoc.parseError)
97-97: No error to ignore is reported on line 97.
(ignore.unmatchedLine)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-common
🔇 Additional comments (7)
web/sites/default/includes/modules/settings.seckit.php (1)
11-13: Good changes to fix comment and align CSP settings.The comment has been corrected from "SCP" to "CSP" for accuracy, and a new configuration setting has been added to explicitly disable the 'upgrade-req' option in CI and LOCAL environments. This aligns with the existing practice of disabling the CSP checkbox in these environments.
tests/phpunit/Drupal/EnvironmentSettingsTest.php (2)
345-347: Correct test coverage for the local environment settings.Appropriate test assertion added for the new 'upgrade-req' CSP configuration setting in LOCAL environment.
390-391: Good test coverage for CI environment settings.Test assertion added for the new 'upgrade-req' CSP configuration in CI environment. Note that the order of assertions differs slightly between LOCAL and CI environments (in LOCAL, 'upgrade-req' appears after 'checkbox', while in CI it appears before), but this doesn't affect functionality.
tests/behat/features/seckit.feature (2)
1-21: Well-structured UI test for SecKit configuration.This scenario effectively tests the SecKit configuration UI by:
- Logging in as administrator
- Navigating to the SecKit configuration page
- Enabling CSP and specific options
- Verifying settings persistence
The test correctly uses the new checkbox handling step definitions from FeatureContext to ensure idempotent interactions.
22-25: Good header verification test.This scenario correctly verifies that the "upgrade-insecure-requests" directive is not present in the Content-Security-Policy header, aligning with the configuration changes made in settings.seckit.php.
tests/behat/bootstrap/FeatureContext.php (2)
10-40: Good trait additions and imports.Appropriate imports have been added for the Behat hooks and new traits (BlockTrait, JsTrait, ResponseTrait) have been included to support the SecKit feature tests.
99-135: Excellent checkbox handling step definitions.The new step definitions for conditionally checking or unchecking checkboxes are well-implemented with proper error handling. These methods:
- Only toggle checkboxes if needed (avoiding unnecessary actions)
- Provide clear error messages when checkboxes aren't found
- Support the idempotent UI testing approach in the SecKit feature
This is a good pattern that could be reused in other UI tests.
| /** | ||
| * Original SecKit configuration. | ||
| * | ||
| * @var array<string, mixed> | ||
| */ | ||
| protected $originalSeckitConfig; | ||
|
|
||
| /** | ||
| * Save original SecKit configuration before running SecKit tests. | ||
| * | ||
| * @BeforeScenario @seckit | ||
| */ | ||
| public function saveSeckitConfigBeforeScenario(BeforeScenarioScope $scope): void { | ||
| if ($this->getDrupalParameter('drupal')['drupal_root']) { | ||
| // Only run this if we have a @api tag or can bootstrap Drupal. | ||
| $tags = $scope->getScenario()->getTags(); | ||
| if (in_array('api', $tags)) { | ||
| // Save current SecKit configuration. | ||
| $this->originalSeckitConfig = $this->getConfigFactory() | ||
| ->getEditable('seckit.settings') | ||
| ->get(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Restore original SecKit configuration after running SecKit tests. | ||
| * | ||
| * @AfterScenario @seckit | ||
| */ | ||
| public function restoreSeckitConfigAfterScenario(AfterScenarioScope $scope): void { | ||
| if ($this->getDrupalParameter('drupal')['drupal_root'] && !empty($this->originalSeckitConfig)) { | ||
| // Only run this if we have a @api tag or can bootstrap Drupal. | ||
| $tags = $scope->getScenario()->getTags(); | ||
| if (in_array('api', $tags)) { | ||
| // Restore the original configuration. | ||
| $this->getConfigFactory() | ||
| ->getEditable('seckit.settings') | ||
| ->setData($this->originalSeckitConfig) | ||
| ->save(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the config factory service. | ||
| * | ||
| * @return \Drupal\Core\Config\ConfigFactoryInterface | ||
| * The config factory service. | ||
| * | ||
| * @SuppressWarnings(PHPMD.StaticAccess) | ||
| */ | ||
| protected function getConfigFactory() { | ||
| // We need to use the static Drupal call here as we're in a Behat context. | ||
| // @phpstan-ignore-next-line | ||
| return Drupal::configFactory(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-implemented configuration management hooks.
Excellent implementation of hooks to save and restore SecKit configuration before and after scenarios. This ensures test isolation and prevents side effects between test runs.
The getConfigFactory() helper method appropriately accesses Drupal's config factory.
The @SuppressWarnings annotation on line 92 has a syntax issue according to static analysis. Consider fixing the format:
- @SuppressWarnings(PHPMD.StaticAccess)
+ @SuppressWarnings("PHPMD.StaticAccess")Also, line 97 has an unnecessary PHPStan ignore annotation that could be removed since no error is being reported:
- // @phpstan-ignore-next-line
+ // Access Drupal static service📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Original SecKit configuration. | |
| * | |
| * @var array<string, mixed> | |
| */ | |
| protected $originalSeckitConfig; | |
| /** | |
| * Save original SecKit configuration before running SecKit tests. | |
| * | |
| * @BeforeScenario @seckit | |
| */ | |
| public function saveSeckitConfigBeforeScenario(BeforeScenarioScope $scope): void { | |
| if ($this->getDrupalParameter('drupal')['drupal_root']) { | |
| // Only run this if we have a @api tag or can bootstrap Drupal. | |
| $tags = $scope->getScenario()->getTags(); | |
| if (in_array('api', $tags)) { | |
| // Save current SecKit configuration. | |
| $this->originalSeckitConfig = $this->getConfigFactory() | |
| ->getEditable('seckit.settings') | |
| ->get(); | |
| } | |
| } | |
| } | |
| /** | |
| * Restore original SecKit configuration after running SecKit tests. | |
| * | |
| * @AfterScenario @seckit | |
| */ | |
| public function restoreSeckitConfigAfterScenario(AfterScenarioScope $scope): void { | |
| if ($this->getDrupalParameter('drupal')['drupal_root'] && !empty($this->originalSeckitConfig)) { | |
| // Only run this if we have a @api tag or can bootstrap Drupal. | |
| $tags = $scope->getScenario()->getTags(); | |
| if (in_array('api', $tags)) { | |
| // Restore the original configuration. | |
| $this->getConfigFactory() | |
| ->getEditable('seckit.settings') | |
| ->setData($this->originalSeckitConfig) | |
| ->save(); | |
| } | |
| } | |
| } | |
| /** | |
| * Gets the config factory service. | |
| * | |
| * @return \Drupal\Core\Config\ConfigFactoryInterface | |
| * The config factory service. | |
| * | |
| * @SuppressWarnings(PHPMD.StaticAccess) | |
| */ | |
| protected function getConfigFactory() { | |
| // We need to use the static Drupal call here as we're in a Behat context. | |
| // @phpstan-ignore-next-line | |
| return Drupal::configFactory(); | |
| } | |
| /** | |
| * Gets the config factory service. | |
| * | |
| * @return \Drupal\Core\Config\ConfigFactoryInterface | |
| * The config factory service. | |
| * | |
| * @SuppressWarnings("PHPMD.StaticAccess") | |
| */ | |
| protected function getConfigFactory() { | |
| // We need to use the static Drupal call here as we're in a Behat context. | |
| // Access Drupal static service | |
| return Drupal::configFactory(); | |
| } |
🧰 Tools
🪛 PHPStan (2.0.3)
92-92: PHPDoc tag @SuppressWarnings has invalid value ((PHPMD.StaticAccess)): Unexpected token ".StaticAccess)", expected ')' at offset 171 on line 7
(phpDoc.parseError)
97-97: No error to ignore is reported on line 97.
(ignore.unmatchedLine)
github-project-automation
bot
moved this from Release queue
to To do for current release
in Vortex
skipper-vp
force-pushed
the
feature/1611-fix-seckit
branch
from
f0ca308 to
abc94a9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1635 +/- ##
========================================
Coverage 71.56% 71.56%
========================================
Files 76 76
Lines 4308 4308
========================================
Hits 3083 3083
Misses 1225 1225 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
skipper-vp
force-pushed
the
feature/1611-fix-seckit
branch
from
abc94a9 to
e687f47
Compare
skipper-vp
force-pushed
the
feature/1611-fix-seckit
branch
from
e687f47 to
034278f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
.vortex/installer/tests/Fixtures/install/hosting_acquia/tests/phpunit/Drupal/EnvironmentSettingsTest.php (2)
5-48: 🧹 Nitpick (assertive)Great coverage, but consider consolidating duplicated environment cases
The expanded data-provider thoroughly enumerates Acquia edge-cases – nice!
However, identical cases now exist in three different test suites (core, baseline, Acquia). If these collections ever diverge, false-negatives will creep in.Suggestion:
• Extract a sharedEnvironmentProviderTrait(or similar) that yields the combinations once, and let each suiteuseit.
• Alternatively, move the provider to an abstract parent class.This keeps the source of truth single and reduces test-maintenance effort.
60-137: 🧹 Nitpick (assertive)Large, verbatim test methods could be generated instead
testEnvironmentAcquiaDynamic/Dev/Stage/Prodreplicate the same assertion pattern with only a handful of differing variables. You can:
- Parameterise the differing env-vars & expected values and drive one generic method via
@dataProvider,- Or create a helper like
assertAcquiaEnvironment(array $vars, string $expected, array $indicatorColours).This will cut ~150 lines, improve readability, and make future additions trivial.
.vortex/installer/tests/Fixtures/install/hosting_lagoon/tests/phpunit/Drupal/EnvironmentSettingsTest.php (1)
5-185: 🧹 Nitpick (assertive)Exhaustive Lagoon matrix is valuable, but maintenance-heavy
The sheer number of combinations ensures safety, yet the list is already lengthy and could grow. Two ideas to keep it manageable:
- Generate cases programmatically in a provider (
forloops over branch names & env-types) instead of hand-listing.- Split into focused providers: “production-branch checks”, “regex-branch checks”, etc., so a future reader quickly grasps intent.
This reduces cognitive load and risk of inconsistent updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (8)
.vortex/README.md(1 hunks).vortex/installer/tests/Fixtures/install/_baseline/tests/phpunit/Drupal/EnvironmentSettingsTest.php(2 hunks).vortex/installer/tests/Fixtures/install/_baseline/web/sites/default/includes/modules/settings.seckit.php(1 hunks).vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/sites/default/includes/modules/settings.seckit.php(1 hunks).vortex/installer/tests/Fixtures/install/hosting_acquia/tests/phpunit/Drupal/EnvironmentSettingsTest.php(1 hunks).vortex/installer/tests/Fixtures/install/hosting_lagoon/tests/phpunit/Drupal/EnvironmentSettingsTest.php(1 hunks)tests/phpunit/Drupal/EnvironmentSettingsTest.php(2 hunks)web/sites/default/includes/modules/settings.seckit.php(1 hunks)
🔇 Additional comments (9)
.vortex/README.md (1)
5-8: Good addition of troubleshooting guidance.This FAQ entry provides clear, actionable guidance for resolving a common pipeline failure. The instructions are concise and helpful for developers encountering PHPUnit directory differences errors.
.vortex/installer/tests/Fixtures/install/_baseline/web/sites/default/includes/modules/settings.seckit.php (2)
11-11: Corrected comment terminology from SCP to CSP.The comment now correctly refers to Content Security Policy (CSP) instead of SCP, providing accurate documentation.
13-13: Good security enhancement: disabled CSP upgrade-req directive.Adding this configuration ensures that both the main CSP enforcement and the upgrade-insecure-requests directive are properly disabled in non-HTTPS environments (CI and LOCAL). This prevents potential issues with mixed content warnings and blocked resources.
web/sites/default/includes/modules/settings.seckit.php (2)
11-11: Corrected comment terminology from SCP to CSP.The comment now correctly refers to Content Security Policy (CSP) instead of SCP, providing accurate documentation.
13-13: Good security enhancement: disabled CSP upgrade-req directive.Adding this configuration ensures that both the main CSP enforcement and the upgrade-insecure-requests directive are properly disabled in non-HTTPS environments (CI and LOCAL). This prevents potential issues with mixed content warnings and blocked resources.
.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/sites/default/includes/modules/settings.seckit.php (2)
11-11: Corrected comment terminology from SCP to CSP.The comment now correctly refers to Content Security Policy (CSP) instead of SCP, providing accurate documentation.
13-13: Good security enhancement: disabled CSP upgrade-req directive.Adding this configuration ensures that both the main CSP enforcement and the upgrade-insecure-requests directive are properly disabled in non-HTTPS environments (CI and LOCAL). This prevents potential issues with mixed content warnings and blocked resources.
tests/phpunit/Drupal/EnvironmentSettingsTest.php (1)
345-347: Addition looks correct & follows existing conventionsThe new assertion for
['upgrade-req'] => FALSEis alphabetically after['checkbox'], matching the file-header guidance and mirroring the change in the realsettings.seckit.php. 🟢.vortex/installer/tests/Fixtures/install/_baseline/tests/phpunit/Drupal/EnvironmentSettingsTest.php (1)
118-120: Change acknowledgedThe extra CSP key is added in the proper alphabetical spot (
checkbox→upgrade-req). ✅
.../installer/tests/Fixtures/install/_baseline/tests/phpunit/Drupal/EnvironmentSettingsTest.php
Show resolved
Hide resolved
skipper-vp
force-pushed
the
feature/1611-fix-seckit
branch
from
034278f to
f503a1d
Compare
skipper-vp
added
the
Needs review
github-project-automation
bot
moved this from To do for current release
to Release queue
in Vortex
#1611
SecKit update + tests.
Summary by CodeRabbit
ahoy update-fixtures.