DGI9-610: Prevent page_cache caching where an ip cache context is relevant.#46
DGI9-610: Prevent page_cache caching where an ip cache context is relevant.#46nchiasson-dgi merged 5 commits intomainfrom
ip cache context is relevant.#46Conversation
…r access control.
WalkthroughAdds a page-cache response policy service and class that denies caching when IP-related embargo cache contexts are present; updates several method signatures to accept nullable types and refactors a controller to read Request from the action parameter instead of from an injected property. Changes
Sequence DiagramsequenceDiagram
participant Req as Request
participant PC as Page Cache System
participant Policy as DenyIpDependentResponse
participant AR as Access Result
Req->>PC: Incoming request + response
PC->>Policy: check(response, request)
Policy->>Req: Read access result from request
alt access result exists
Policy->>AR: ensure AR is RefinableCacheableDependencyInterface
alt AR is cacheable
Policy->>Policy: collect cache contexts
alt has ip.embargo_range or ip
Policy-->>PC: Return "DENY"
PC-->>Req: Do not cache
else
Policy-->>PC: Return NULL
PC-->>Req: Normal caching flow
end
else
Policy-->>PC: Return NULL (non-cacheable)
PC-->>Req: Normal caching flow
end
else
Policy-->>PC: Return NULL (no access result)
PC-->>Req: Normal caching flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Should allow for some better documentation on the value(s).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php (1)
59-68: Fix type mismatch: nullable parameter passed to non-nullable constructor.Line 59 accepts a nullable
?MigrationInterface $migration = NULL, but line 65 passes it to the constructor, which expects a non-nullableMigrationInterface $migration(line 42). This will cause a type error if$migrationisNULL.Apply this diff to make the constructor parameter nullable:
public function __construct( array $configuration, $plugin_id, $plugin_definition, - MigrationInterface $migration, + ?MigrationInterface $migration, EntityTypeManagerInterface $entity_type_manager, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
embargo.services.yml(1 hunks)modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php(1 hunks)src/Controller/IpRangeAccessExemptionController.php(1 hunks)src/PageCache/DenyIpDependentResponse.php(1 hunks)src/Plugin/search_api/processor/EmbargoJoinProcessor.php(1 hunks)src/Plugin/search_api/processor/EmbargoProcessor.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
src/Plugin/search_api/processor/EmbargoProcessor.php (1)
getPropertyDefinitions(91-103)
src/Plugin/search_api/processor/EmbargoProcessor.php (1)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
getPropertyDefinitions(107-132)
src/PageCache/DenyIpDependentResponse.php (1)
src/Controller/IpRangeAccessExemptionController.php (1)
response(46-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.2
- GitHub Check: PHPUnit / Drupal 10.5 | PHP 8.4
- GitHub Check: PHPUnit / Drupal 10.5 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.4 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.4 | PHP 8.4
🔇 Additional comments (3)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
107-107: LGTM: Explicit nullable type hint improves clarity.The addition of the explicit nullable type hint (
?DatasourceInterface) improves code clarity and aligns with modern PHP type safety practices. The method correctly handles NULL by returning an empty array.src/Plugin/search_api/processor/EmbargoProcessor.php (1)
91-91: LGTM: Explicit nullable type hint improves clarity.The addition of the explicit nullable type hint (
?DatasourceInterface) improves code clarity and aligns with modern PHP type safety practices, matching the pattern used inEmbargoJoinProcessor.src/PageCache/DenyIpDependentResponse.php (1)
34-57: LGTM: Correct implementation of IP-dependent page cache bypass.The logic correctly implements the PR objective by:
- Extracting the access result from the request
- Checking if it's cacheable
- Denying page caching when IP-related cache contexts (
ip.embargo_rangeorip) are presentThis properly prevents anonymous page caching when IP embargoes are in effect, addressing the core requirement.
Controllers support having it passed automatically, so let's leverage it.
2 participants
Summary by CodeRabbit
New Features
Chores