Needs review, and feedback welcomed on the wording.
("Please" is removed from the message since the screenshot is taken.)
Leaving 🐛 EntityReferenceSelection handlers does not consider entity view/view label operations Active here as a reference that not everything is perfect in the entity selection handlers either, or precisely probably everything works as expected but better documentation is needed about which type access check should be applied where and how...
It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the $entity->access() calls remains a necessity even if they break pagination or may lead to performance issues.
For historical reference, we had the following discussion in the security issue with mainly @berdir and @larowlan - please add your correction to my summary if necessary:
Originally, I have started the security thread with these three questions after opened 🐛 Fix filtering on Node's Authored by field (and on other entity reference fields) Active :
- Should entity references respect either of these entity operation accesses?
- What is the intended purpose of the "view label" entity operation if not for controlling label visibility?
- How should this interact with node access, given that
hook_node_access()can override permissions granted byhook_node_access_grants()(Proof: https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.0.0/tests/src/Kernel/NodeAccessTest.php?ref_type=tags#L94-106)?
TL;DR
- Entity Reference Selection handlers should perform query level access checks and avoid listing entities that a given user does not have access to. This can be achieved through (entity) query alters or via custom selection plugins, see
\Drupal\user\Plugin\EntityReferenceSelection\UserSelection::buildEntityQuery()and\Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery() $entity->access()performs access check on a single entity. When performed on a list of entities, it breaks pagination and could lead to performance issues.- The "view label" operation was introduced as a subset of the "view" entity operation. The primary use case in core is being able to view user display names without having access to their profile. There is some overlap with field access, but a label does not have to be a field.
In addition: node/entity access is inconsistent. List and entity level access checking in Drupal core is complex, with several open issues addressing these concerns. Developers should be aware of how to use them properly.
Potential action items
During a security team triage meeting (@greggles, @mcdruid, @drumm, @larowlan), the following suggestions were made:
- Add additional documentation to
hook_entity_access()andhook_entity_access_alter()hook documentation, noting the gotchas around selection plugins. - Expand the documentation to explain that
hook_entity_reference_selection_alter()can be used to modify the class for a given entity-type and add custom logic as needed.
These documentation improvements would help developers, especially those new to Drupal, understand the potential pitfalls related to access controls.
Additional documentation suggestions
Extend the documentation on \Drupal\Core\Entity\EntityAccessControlHandler::$viewLabelOperation to properly explain that "view label" is a subset of the "view" operation. Clarify that even if it grants access to a field/property on an entity or sometimes a dynamically generated value (e.g., $user->getDisplayName()), it is not equivalent to field access.
Explain that when a user has "view" operation access to an entity, they also have "view label" access. However, when they only have "view label" access, they literally only have access to the entity label and nothing more. This also means that they may not have access to the entity's canonical URL (see The "Label" entity reference field formatter now restricts links for inaccessible destinations → ).
mxr576 → created an issue.
Could this be caused by #2741937: Removed paragraph entities are not deleted from database → -> ✨ [META] Remove obsolete composite entities from existing storage (garbage collection) Active ?
Thank you, too!
Is this eligible for backporting?
That makes sense. If this change is introduced in a new minor version, couldn’t we also provide an update hook to remove the unnecessary status filter from affected Views? Or would that only be feasible in a new Drupal major release? (The idea came when I checked code examples for the requirement checks and found all these update hooks in \Drupal\views\ViewsConfigUpdater.)
If the plan is to introduce this in 10.4.x and 11.1.x (which I’d love to see happen after all these iterations), then these versions could include the requirements warning, while the subsequent minor versions could handle the removal of redundant filters via an update hook.
Correct role label - this is what currently visible on GCP
Awesome improvements indeed! Maybe announcing the new API worth a change record?
Hi
What is your ETA for releasing a Drupal 11 compatible version from this module?
There was no conflict, so moving back to RTBC.
❯ git fetch origin
remote: Enumerating objects: 7322, done.
remote: Counting objects: 100% (5502/5502), done.
remote: Compressing objects: 100% (1877/1877), done.
remote: Total 2830 (delta 1874), reused 1628 (delta 854), pack-reused 0 (from 0)
Receiving objects: 100% (2830/2830), 719.19 KiB | 10.27 MiB/s, done.
Resolving deltas: 100% (1874/1874), completed with 976 local objects.
From https://git.drupalcode.org/project/drupal
018087b1ff7..42de5b1df41 10.4.x -> origin/10.4.x
0cf4a33c149..60a21fa9287 10.5.x -> origin/10.5.x
d3b4dce5a03..ff743d3ce22 11.1.x -> origin/11.1.x
6dd918c9360..93ec7a93b73 11.x -> origin/11.x
* [new tag] 10.4.4 -> 10.4.4
* [new tag] 11.1.4 -> 11.1.4
❯ git merge origin/11.x
Auto-merging core/modules/node/src/NodeViewsData.php
Merge made by the 'ort' strategy.
I am still catching up with this thread, but...
Example with cache context "entity_status:node|1"
IIRC @kristiaanvandeneynde you have suggested against doing something similar in #2628870-69: Access result caching per user(.permissions) does not check for correct user → -- and fun fact, the code that I copied the comment is available in a public repo since then: https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.x...
Fix the implementation of node_access_rebuild()'s batch mode to register only one batch set even if the function is called multiple times.
This is what I currently see in the MR.
As an alternative solution - if it turns out that there can be scenarios when running the rebuild process multiple times....
What would be those scenarios realistically? I cannot think of any at this moment... ideally there would be only one node access rebuild, triggered at the very at of the deploy process, only triggered when it is necessary.
Would be nice if you had a readme with examples in there. It's quite a lot of code to read through. Either way, the goal here is to add to AccessResult so that we can slowly move from one approach to another. Call it a band-aid.
+1, example usages and docs will come, I was just excited to have this finally published and shared with internally/externally.
The real question is indeed whether we need to completely rethink AccessResult as a whole, but this issue is not for that.
I think we should merge changes in this issue that already improves the situation and think about how to slowly kill and replace AccessResult in Drupal 14/15.
Test suite failed for unrelated reasons.
There were 2 failures:
1) Drupal\Tests\single_content_sync\Functional\SingleContentSyncImportTest::testSingleImport
Unable to install modules: module 'single_content_sync' is incompatible with this version of Drupal core.
/builds/issue/single_content_sync-3510655/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:505
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:555
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
/builds/issue/single_content_sync-3510655/tests/src/Functional/SingleContentSyncBrowserTestBase.php:43
2) Drupal\Tests\single_content_sync\Functional\SingleContentSyncImportUITest::testSingleImportUi
Unable to install modules: module 'single_content_sync' is incompatible with this version of Drupal core.
/builds/issue/single_content_sync-3510655/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:505
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:555
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
/builds/issue/single_content_sync-3510655/tests/src/Functional/SingleContentSyncBrowserTestBase.php:43
FAILURES!Raised an MR againts 2.0.x but the fix also applies to 1.4.x and should be merged to that branch too.
The outcome after the fix is applied.
This issue inspired me to start working on something I had planned for a long time - a generic Binary and Trinary logic implementation for Drupal with built-in cacheability and lazy evaluation. The first version is now available here: https://git.drupalcode.org/project/stdlib/-/tree/1.x/src/Logic?ref_type=...
My motivation was simple: I found myself writing too much code where I either had to use `AccessResult` to propagate a binary/trinary value *with cacheability* or resorted to constructs like this:
* @return array{result: bool, cacheability: \Drupal\Core\Cache\CacheableDependencyInterface}
* The boolean result with cacheability.
This new approach aims to streamline such logic while maintaining Drupal’s caching best practices and also incorporates DX tweaks and improvements for the sake of higher cache hit rates like what this issue is about. Feedback and contributions are welcome!
Thanks @longwave for the suggestions and @prabha1997 applying them. I have also bumped versions on related CR-s.
Should be ready to be merged, right? :)
Also repeating my comment from the other thread, could be useful here, should not be lost.
Adding #1144644: Enable specifying the collation when creating a database table → because comment 1 explains to potential root cause perfectly (@catch called my attention to it on Slack).
@nico.b told me that my 🐛 User names uniqueness is no longer accent-sensitive Active is actually is a duplicate of this one - I do not know how I did not find this one back at the time, but thanks for intel Nico.
In that issue I had a very similar fix that is being proposed here and because I saw test failures on MR for other database engines than MariaDB and MYSQL, I have also triggered test runs on those db engines here and they failed too. So the proposed fix actually needs work:
core/tests/Drupal/KernelTests/Core/Validation/UniqueValuesConstraintValidatorTest.php
Drupal\KernelTests\Core\Validation\UniqueValuesConstraintValidatorTest::testUsernameUniqueValidation
Failed asserting that actual size 0 matches expected size 1.
core/tests/Drupal/KernelTests/Core/Validation/UniqueValuesConstraintValidatorTest.php:359
(source: https://git.drupalcode.org/issue/drupal-3456964/-/pipelines/433653/test_...)
Yes, this is a duplicate of 🐛 Unhandled exception when trying to register a duplicate username with equality mismatch between php and database layer Active . Thanks for letting me know!
Let's continue in that ticket.
Increasing to major just like the issue that introduced this problem, because user registration is blocked by this.
Just referenced this issue on 🐛 User names uniqueness is no longer accent-sensitive Active because 14 years later I guess we are still suffering the consequence of general_ci collation described in comment #1.
Adding #1144644: Enable specifying the collation when creating a database table → because comment 1 explains to potential root cause perfectly (@catch called my attention to it on Slack).
It seems the fix that was merged caused an unexpected side-effect, referencing the new issue in case anybody from the old crew would like to jump on the new one as well: 🐛 User names uniqueness is no longer accent-sensitive Active .
Since this is not my expert area, I have asked one of the AI-s how to fix the \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValueValidator::caseInsensitiveArrayIntersect() implementation:
* \Normalizer::normalize($value, \Normalizer::FORM_NFKD) was suggested but that adds a dependecy on intl: https://www.php.net/manual/it/class.normalizer.php - could not make it work on my ddev local, but did not invested too much time either
* https://packagist.org/packages/wikimedia/utfnormal which introduces a new package dependency, haven't checked
* iconv("UTF-8", "ASCII//TRANSLIT", $text) as a quick solution, currently used in MR.
and this is just one way to address the problem, I am unsure at this moment if the idea behind the \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValueValidator::caseInsensitiveArrayIntersect() should be reconsidered after this finding.
FTR, originally we thought this is a collision issue but actually it is not, because the db returned the duplicates.
The root of the issue lies in the database collation, which is set to utf8mb4_general_ci. This collation is both case-insensitive and accent-insensitive, meaning that "René" and "Rene" are treated as the same username. As a result, when the user attempted to register with the username "René," the site encountered an unexpected error because "Rene" already existed in the database.
(Original Slack thread: https://drupal.slack.com/archives/C079NQPQUEN/p1739955491188989)
mxr576 → created an issue. See original summary → .
Wow!
Tbh, I am a bit hesitanr about introducing a new entity operation, although I am fully understanding why this is feels a compelling solutions. Why am I hesitant? Because I have been involved several private conversations about what *view label" is and what not; and I am still not convinced that the majority of devs aware of it.
Would "view linked label" become a generic entity operation or username only? Should it be handled properly by all logic in core/contrib?
What I can also add that I have an extensive test coverage for username access in my module with a test controller that could be also useful foe debugging.
https://git.drupalcode.org/project/view_usernames/-/blob/1.x/tests/modul...
Moving this to needs work since test coverage is missing.
Pipeline on the MR is currently in a failed state, could be due to transient errors but since the build log has expired, I am going to rebase and see what happens.
updated IS
mxr576 → created an issue.
if there is missing entity access check
in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities() - because it seems it only relies on query level access checking...
Indeed, that is the reason.
Chris arrived here sooner than myself :) I was also syncing with my CISO @akosh about your questions,.
So we believe access to the username should be denied by default and only gracefully opened up on a need-to-know basis.
Where would the username of a non-author being ever exposed to non-admins?
Yes, JSON:API as a default answer in Drupal as we explain that in our blog post with @akosh.
Isn't it important to e.g. disclose the author name of e.g. blog posts? I see lot's of cases where the missing author name on blog posts is an issue when someone else wants to give credit when quoting them.
Yes, in certain cases, such as blog posts, displaying the author's name can be useful, especially to enable proper attribution and give credit. However, this functionality should be configurable to suit different use cases and privacy considerations. Additionally, any configuration related to author name visibility must be consistent across all interfaces, including user interfaces (UIs) and APIs, to ensure clarity and alignment with permissions.
View Usernames Node Author → module addresses this challenge effectively. It works alongside View Usernames → , which modifies Drupal Core's behavior to a deny-by-default approach for username visibility. This extension explicitly ensures that when you author content on the site, your username becomes publicly available for attribution purposes, aligning with the principle of controlled, use-case-specific access.
Same thing may apply to many other scenarios, where attribution may be relevant and the username not being perceived as PII.
When attribution is necessary and usernames are used for that purpose, it is important to recognize that usernames are considered personal data if they can identify an individual, even if they are not strictly classified as PII in all contexts. As with the previous point, ensuring configurability and maintaining consistency across all interfaces (UIs and APIs) remains critical to appropriately handle such scenarios.
And if we move on with this, as the user_privacy_cms recipe is fairly simple, would you mind if we installed and configured the 2 extra modules directly from one of the existing recipes rather than adding yet 2 more dependencies?
Sure, it’s relatively small for now, and since we don’t yet know how it might evolve, it could make sense to install and configure the View Usernames modules directly without adding a dependency on the recipe.
Hm, reminder to me to check if there is missing entity access check
in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities() - because it seems it only relies on query level access checking... - and that could be a reason why the "Author" autocomplete works interesting in node edit form when View Usernames and friends are enabled.
The MR was still mergeable, but merged 11.x to keep the MR up to date.
Configure entity fields by marking those with personal data as sensitive
Would be a real invention! Usernames should be marked as sensitive by default. See: 🌱 [policy] Treat username enumerations as security bugs that require Security Advisories Active
... and now these modules are also available in Recipes.
https://www.drupal.org/project/user_privacy_core →
https://www.drupal.org/project/user_privacy_cms →
Good catch and regular issue, although the fix leads to the infamous N+1 problem of ORMs
I would rather recommend implementing something like I did in an other module, load multiple with a maximum amount:
https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.0...
(tricks like this is necessary until a stable solution lands in Core for #2577417)
Related Slack thread: https://drupal.slack.com/archives/C1BMUQ9U6/p1735897088083149
Also attached the textual representation created via Slack dump.
AI summary:
Participants discuss the problems, including:
1. **State Service Issues**: It has become problematic to access the `state` service during container build time due to caching mechanisms and service dependencies.
2. **Race Conditions and Alternatives**: Mention of related issues like race conditions in development, alternative storage options like key-value storage, and container parameters for performance and minimal complexity.
3. **Testing and Failures**: Observations that the update system and session storage break when `state` is used in compiler passes. Failed tests highlight specific issues triggered by `state` usage.
4. **Recommendations and Challenges**: Advice is given to use key-value storage cautiously and ensure cache clearance. However, achieving this in compiler passes presents challenges due to potential outdated information being cached.
5. **Codebase Observations**: A null return from `$this->requestStack->getCurrentRequest()` was identified as a potential unhandled issue in Drupal core, exacerbated by using `state` in compiler passes.
6. **Discussion on Feasibility**: Some participants recommend changing direction entirely due to the complexities and risks involved.
Could you please also opt-in tokenuuid to SA coverage?
This project is not covered by Drupal’s security advisory policy.
mxr576 → created an issue.
Removing the tag, since this has not been merged before 10.4.0 got released. :-(
🐛 The content overview Views view filters out unpublished content Active is still on RTBC, but hopefully the final-final-final stab was made on it minutes ago.
A related Slack thread: https://drupal.slack.com/archives/C079NQPQUEN/p1733181222760849
The MR still applies.
Thanks all, especially @mxr576 for sticking with this one.
:beers:
A backport to 10.3.x or 11.0.x would also require a backport of ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review which as a task isn't eligible for backport, so marking this as fixed.
Does this highlight an edge case in the backporting policy? Here’s the dilemma:
This fix cannot be backported to 11.0.x due to its dependency on a non-backportable issue. As a result, the upgrade path from 10.4.x/10.5.x to 11.0.x could lead to unexpected and unforeseeable feature or bugfix losses unless projects skip directly to 11.1.x.
10.4.x backport is ready in the related MR.
🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work can be only backported to 11.0.x is this also gets bacported.
It seems ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review has not been backported to 11.0.x yet, so that should be backported first.
Can we solve this by implementing a \Drupal\symfony_mailer\Processor\EmailProcessorInterface and enabling the bypasser in build() and deactivating it in postSend()?
Exciting news! Thanks for everyone who got involved, this is an important step to fix the dependent issue.
Thank you to everyone following this issue and to those who join the conversation in the future!
We’ve just published a blog post that explains why we believe this topic is critically important. The post also serves as a release announcement for two contributed modules we developed in-house to address this challenge:
These modules do not require any API changes in Drupal core but aim to provide immediate, practical solutions. Ideally, a comprehensive solution would involve robust new APIs for managing personally identifiable information (PII), including but not limited to usernames.
We hope these contributed modules can help move the discussion forward and pave the way for the necessary enhancements in core.
Green again! Kept it on RTBC...
Rebased again, due to random test failure fixed in upstream: https://www.drupal.org/project/drupal/issues/3478621#comment-15875919 📌 Add a filecache-backed attribute parser Active
Looks good so far, we only have those changes in the MR that we should be, no changes on ResourceTestBase that was fixed in the spin-off issue.
❯ git rebase origin/11.x
Auto-merging core/.phpstan-baseline.php
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
error: could not apply 07987b49a5... Cherry-picked "Improve Dynamic Page Cache header assertions in JSON:API tests"
I am going to use git merge origin/11.x instead of rebase in a hope that we do not need to restart the review process here.
Now that ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review is in 11.x, this needs a rebase because the MR does not apply anymore, because it had a cherry picked version. Maybe I just drop the cherry picked commit and see what happens...
+ langcode:
+ type: string
+ label: Language
Do we need to update existing plugin instances in an update hook due to this config schema change? Will the stored config schema remain valid?
Ack, thanks for the clarification.
Moving back to needs work since the "needs test" tag is on the issue and I haven't seen any test coverage in MR#3891 so far.
isNew() can be tricked, it most likely does not help PHPStan to narrow down the actual type of $node->id(), but I agree, let's follow the Drupal Way here -- funny, I wanted to use that originally, but I ditched the idea for some reasons, I hope not because tests started to fail :)
public function isNew() {
return !empty($this->enforceIsNew) || !$this->id();
}Yes they were random failures, restart make them disappear.
Test fails seem to be random ones, so if you can ping @bbrala or @larowlan for a final approval then it's RTBC in my book.
+1
No longer a Starshot blocker now that WASM is out.
For other curious ppl like /me who would like to what this means and why: 📌 Remove the WebAssembly trial Active
Thanks for the additional feedbacks, I have pushed back on some with answers and accepted one. Back to review