Hardening checks for applicable embargoes#44
Conversation
WalkthroughA conditional was added to the NodeInterface branch of getApplicableEmbargoes in src/EmbargoStorageTrait.php to only process non-new nodes (isNew() === FALSE). Embargo resolution for MediaInterface/FileInterface remains unchanged. No public signatures were modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EmbargoStorageTrait
participant Storage as EntityStorage
participant LUT as Lookup/Join
Caller->>EmbargoStorageTrait: getApplicableEmbargoes(entity)
alt entity is NodeInterface
alt entity.isNew() === FALSE
EmbargoStorageTrait->>Storage: load by embargoed_node (node id)
Storage-->>EmbargoStorageTrait: embargo list (possibly empty)
else entity.isNew() === TRUE
note over EmbargoStorageTrait: Skip node embargo lookup
end
else entity is MediaInterface/FileInterface
EmbargoStorageTrait->>LUT: map to embargoed_node via LUT join
LUT-->>EmbargoStorageTrait: related IDs (possibly none)
EmbargoStorageTrait->>Storage: loadMultiple(IDs)
Storage-->>EmbargoStorageTrait: embargo list (possibly empty)
end
EmbargoStorageTrait-->>Caller: applicable embargoes (array)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (1)
src/EmbargoStorageTrait.php (1)
52-61: Extend the “new entity” skip to Media/File for consistency with PR intentThe PR states “Do not check for applicable embargoes if a new object is being added.” Currently, Media/File embargo lookups will still run for unsaved entities (with a NULL id). While it likely returns an empty result set, it still performs an unnecessary join and query and diverges from the stated objective.
Recommend guarding Media/File with the same isNew() check:
- elseif ($entity instanceof MediaInterface || $entity instanceof FileInterface) { + elseif (($entity instanceof MediaInterface || $entity instanceof FileInterface) && $entity->isNew() === FALSE) { $query = $this->database->select('embargo', 'e') ->fields('e', ['id']) ->distinct(); $lut_alias = $query->join(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = e.embargoed_node'); $key = $entity instanceof MediaInterface ? 'mid' : 'fid'; $query->condition("{$lut_alias}.{$key}", $entity->id()); $ids = $query->execute()->fetchCol(); return $this->loadMultiple($ids); }
- Keeps behavior consistent across entity types.
- Avoids a LUT join with a NULL id on create flows.
Can you confirm whether the PR intent is to skip embargo checks for all new entities (Node/Media/File), not just Nodes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/EmbargoStorageTrait.php(1 hunks)
⏰ 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). (4)
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.2
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.2
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.3
🔇 Additional comments (1)
src/EmbargoStorageTrait.php (1)
48-51: Good guard: skip embargo lookup for unsaved nodesAdding
&& $entity->isNew() === FALSEprevents lookups with a NULL nid and avoids unnecessary DB work. This aligns with the PR objective.
|
Closing this PR, will raise a ticket to fix this. More context: The error is triggered from: where it's going over all request attributes and is not just constrained to objects that should be checked. Probably needs to be fixed here: embargo/src/Access/EmbargoAccessCheck.php Lines 56 to 94 in a481d32 where we add embargo entities as parameters to routes and only check those. |
adam-vessey
left a comment
adam-vessey
left a comment
There was a problem hiding this comment.
Could still explode if we were hitting on a new media or file.
That said something being allowed/trying to perform an access check on something that has not yet been ingested feels like something else going wrong elsewhere. Bit of discussion, seems to be something off in IpRangeRedirect being insufficiently specific... should possibly make things more specific, constrain what we consider based on the _access_result attribute from the request, when/if present?
2 participants
Bug
On enabling https://www.drupal.org/project/mercury_editor the node/add page whitescreens.
Stacktrace:
The website encountered an unexpected error. Try again later. Drupal\Core\Database\InvalidQueryException: Query condition 'embargo.embargoed_node IN ()' cannot be empty. in Drupal\Core\Database\Query\Condition->condition() (line 108 of core/lib/Drupal/Core/Database/Query/Condition.php).Do not check for applicable embargoes if a new object is being added.
Summary by CodeRabbit