[#2198] Added support for ACME challenge in Shield. #2254
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request adds support for Let's Encrypt ACME HTTP-01 challenge path passthrough in Drupal Shield, controlled via the DRUPAL_SHIELD_ALLOW_ACME_CHALLENGE environment variable, with corresponding comprehensive test coverage for various ACME configuration scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Fix all issues with AI agents
In `@tests/phpunit/Drupal/SwitchableSettingsTest.php`:
- Around line 546-563: Add a PROD environment test case to the ACME passthrough
data set in SwitchableSettingsTest similar to the DEV/LOCAL entries: add an
array entry using self::ENVIRONMENT_PROD with DRUPAL_SHIELD_USER,
DRUPAL_SHIELD_PASS and DRUPAL_SHIELD_ALLOW_ACME_CHALLENGE=1 as the environment
input, and assert the expected 'shield.settings' output contains credentials
(['shield' => ['user' => 'drupal_shield_user','pass' => 'drupal_shield_pass']])
plus 'method' => 0 and 'paths' => '/.well-known/acme-challenge/*' but omit
'shield_enable' (since in PROD it is UI-managed); place this new case alongside
the existing ACME challenge entries so it gets exercised by the same data
provider used by the test methods.
Comment on lines
+546
to
+563
|
|
||
| // ACME challenge passthrough tests. | ||
| [ | ||
| self::ENVIRONMENT_DEV, | ||
| [ | ||
| 'DRUPAL_SHIELD_USER' => 'drupal_shield_user', | ||
| 'DRUPAL_SHIELD_PASS' => 'drupal_shield_pass', | ||
| 'DRUPAL_SHIELD_ALLOW_ACME_CHALLENGE' => 1, | ||
| ], | ||
| [ | ||
| 'shield.settings' => [ | ||
| 'shield_enable' => TRUE, | ||
| 'credentials' => ['shield' => ['user' => 'drupal_shield_user', 'pass' => 'drupal_shield_pass']], | ||
| 'method' => 0, | ||
| 'paths' => '/.well-known/acme-challenge/*', | ||
| ], | ||
| ], | ||
| ], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a PROD environment test case for ACME passthrough.
The current tests cover DEV and LOCAL environments. Adding a PROD test case would verify ACME passthrough works correctly in production where shield_enable is typically managed through the UI rather than environment variables.
💡 Suggested test case for PROD environment
[
self::ENVIRONMENT_PROD,
[
'DRUPAL_SHIELD_USER' => 'drupal_shield_user',
'DRUPAL_SHIELD_PASS' => 'drupal_shield_pass',
'DRUPAL_SHIELD_ALLOW_ACME_CHALLENGE' => 1,
],
[
'shield.settings' => [
'credentials' => ['shield' => ['user' => 'drupal_shield_user', 'pass' => 'drupal_shield_pass']],
'method' => 0,
'paths' => '/.well-known/acme-challenge/*',
],
],
],🤖 Prompt for AI Agents
In `@tests/phpunit/Drupal/SwitchableSettingsTest.php` around lines 546 - 563, Add
a PROD environment test case to the ACME passthrough data set in
SwitchableSettingsTest similar to the DEV/LOCAL entries: add an array entry
using self::ENVIRONMENT_PROD with DRUPAL_SHIELD_USER, DRUPAL_SHIELD_PASS and
DRUPAL_SHIELD_ALLOW_ACME_CHALLENGE=1 as the environment input, and assert the
expected 'shield.settings' output contains credentials (['shield' => ['user' =>
'drupal_shield_user','pass' => 'drupal_shield_pass']]) plus 'method' => 0 and
'paths' => '/.well-known/acme-challenge/*' but omit 'shield_enable' (since in
PROD it is UI-managed); place this new case alongside the existing ACME
challenge entries so it gets exercised by the same data provider used by the
test methods.
Member
Author
|
|
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2254 +/- ##
==========================================
- Coverage 76.85% 76.22% -0.63%
==========================================
Files 112 105 -7
Lines 5846 5687 -159
Branches 44 0 -44
==========================================
- Hits 4493 4335 -158
+ Misses 1353 1352 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2198
Summary by CodeRabbit
Release Notes
New Features
Tests