- Issue created by @kristiaanvandeneynde
- last update
over 1 year ago 29,881 pass - @kristiaanvandeneynde opened merge request.
- Status changed to Needs review
over 1 year ago 3:03pm 25 July 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Some things worth mentioning:
- I needed a fast memory cache and the one freely available continuously serializes/unserializes its data whenever its used. As permissions are asked for often during a request, I decided to use the MemoryCache class also used by jsonapi. The only downside is that that class has no factory like all the other ones and so I added it here. I'll ping @catch and let him decide what to do with this (or guide me on a good alternative)
- I've made the "drupal" scope the default in the applies() check, but in the CalculatedPermissions* classes you still need to pass "drupal" as the scope and identifier. Should we also somehow make these default to "drupal" to make it easier for people to use in combination with core? If so, suggestions welcome.
- I'll convert the unit tests I had in Flexible Permissions ASAP and add them here, but I've run out of time for the day.
- last update
over 1 year ago 29,881 pass - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,881 pass, 4 fail - last update
over 1 year ago 29,889 pass, 2 fail - last update
over 1 year ago 29,889 pass, 2 fail - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Last test is choking on
$cache_static->set(Argument::cetera())->willReturn(NULL);
as the set method has a void return type. There is a workaround like this:$cache_static->set(Argument::cetera())->will(function(){});
as per https://github.com/phpspec/prophecy/issues/280.However, newer versions of prophecy shouldn't have this issue as it got fixed here: https://github.com/phpspec/prophecy/pull/398
Added a commit that removes any mention of willReturn and can add in the will workaround if they go red. Hopefully, we are on a recent enough prophecy version and do not need the workaround.
- last update
over 1 year ago Custom Commands Failed - 🇺🇸United States partdigital
@kristiaanvandeneynde
Any thoughts on an API like this being applicable with access policy module? https://www.drupal.org/project/access_policy →
This provides a powerful UI for allowing site builders to define their own access policies. It currently works on Drupal 9 and 10 but I'd be open to releasing version 2.0 to use this API if you feel it's a good fit with this API.
- last update
over 1 year ago 29,889 pass, 1 fail - last update
over 1 year ago 29,910 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Two more things I want to try:
- Change AccessPolicyChain into AccessPolicyProcessor and remove the notion of it being chainable. Also do not implement AccessPolicyInterface anymore
- Change CalculatedPermissionsItem to default to Drupal scope and identifier by turning order of parameters into:
- permissions
- isAdmin (optional)
- scope (optional)
- identifier (optional)
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Re #6 I've looked into your module and the whole idea of building access policies using a UI is incompatible with this API-first approach. The goal here is to build an API that both core and contrib can use and then allow contrib to build any UI on top of that.
Your module comes with plugins that make assumptions about the types of access policies, limiting what can be done with it unless you introduce more plugins. This work comes with tagged services that translate an access policy into a set of permissions that can then be handed out on a particular level: core, Group, Domain, Commerce Stores, ...
Secondly, evaluating access policies during runtime is unsafe because we'd still be caching things with user.permissions even though your permissions may vary based on what access policies apply during your current and next request. The API presented here prevents that by introducing a one-time build phase and then caching your processed permissions using cache contexts. This way, it's still safe to use the user.permissions cache context because we know that your access does not change between requests unless you no longer match your policies' criteria (e.g. you lose a user role).
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,910 pass - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,910 pass - 🇺🇸United States partdigital
re #8
Just to clarify, I’m not suggesting moving the access policy module into core. I’m wondering if I could use your new API as part of the contrib module?
In regards to your other comments, did you get a chance to try out the access policy module? It can be difficult to interpret how it works just by looking at the code. For example, you can make as many access policies as you like. The Access policy type plugin merely defines which operations you want to control. The access policies themselves are config entities.
Secondly, evaluating access policies during runtime is unsafe because we'd still be caching things with user.permissions even though your permissions may vary based on what access policies apply during your current and next request.
The access policy module uses a combination of permissions and access rules to control access. So any user.permission cache context still applies. If the user has permission then it will evaluate the access rules via hook_entity_access() which uses the user cache context. It sounds like, for this reason, it might not be viable to use your API in access policy?
However, I don’t want to hijack this issue, so I’d happy to open another issue or even hop on a brief call.
Thanks, and nice work! Looking forward to seeing this get in!
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
I will add documentation and an example on how to use this, so that should help in evaluating whether you could leverage it in your module.
- 🇫🇮Finland lauriii Finland
Tagging for a framework manager review. I think it would be helpful for the review to have the API documented on a high level in the issue summary / draft CR.
- 🇬🇧United Kingdom catch
Yeah I'm missing an issue summary here or in the meta issue. Left a note about memory cache.
- last update
over 1 year ago 29,937 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks for the reviews so far!
Updated the IS to document the new API a bit.
- last update
over 1 year ago 29,938 pass - last update
over 1 year ago 29,978 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
As the commit says: I made the scope and identifier 'drupal' the default across the board now. So core code should look cleaner. Should still go green and I consider this the final version, aside from obvious discussions regarding naming etc.
But functionally this is feature complete.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Actually working on one minor change: Add the account as a parameter to the alter function. Will post an update with that functionality soon.
- last update
over 1 year ago 29,979 pass - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,979 pass - last update
over 1 year ago 29,984 pass - last update
over 1 year ago 29,984 pass - last update
over 1 year ago 29,997 pass - last update
about 1 year ago 30,084 pass - Status changed to RTBC
about 1 year ago 7:31pm 30 August 2023 - 🇺🇸United States smustgrave
Tried my best with this one. Read the issue summary a few times, kudos btw for all the detail, everything looks okay.
Seems it's been through some review as all the threads appear to be resolved.
Going to move this one to the committer bucket?
Not sure if the change record will be needed for this ticket or one of the related tickets?
- last update
about 1 year ago 30,126 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks so much for the review! Updated the IS a bit and will write a change record when I have more time.
- last update
about 1 year ago 30,161 pass - last update
about 1 year ago 30,149 pass, 2 fail - last update
about 1 year ago 30,162 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Added a CR. It mainly copies the IS from this issue, but I've changed the language and intro a bit so that we can also have a CR for the implementation issue that merely focuses on the changes to the permission checker and the 2 core access policies.
- Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
about 1 year ago Waiting for branch to pass - last update
about 1 year ago 30,172 pass - last update
about 1 year ago 30,174 pass - last update
about 1 year ago 30,180 pass - last update
about 1 year ago 30,187 pass - last update
about 1 year ago 30,173 pass, 3 fail - last update
about 1 year ago 30,194 pass - last update
about 1 year ago 30,194 pass - last update
about 1 year ago 30,231 pass - 🇳🇿New Zealand quietone
I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.
The change record has been added but has not been reviewed. That still needs to be done. I added a remaining tasks section to the IS that include this. I did skim the change record and I think that as new API there should be a guide at Drupal APIs → that can contain pages as needed.
I did not review the code.
Leaving at RTBC because this still needs framework manager review.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Suggested documentation is linked from the meta issue: 📌 Document the new access policy API Needs review
- last update
about 1 year ago 30,389 pass - last update
about 1 year ago 30,386 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks for the review @mmxr576!
My main takeaway is that we could improve the DX surrounding the exception and cache , but I would like to wait for core maintainers to sign off before I go through the trouble (as I'm on the DA's budget here).
Your other suggestions I would mark as personal taste and I tried to stick as close to what core already does as possible (hard-coding cache tag, not mentioning Drupal or Core in a base class, ...)
- last update
about 1 year ago 30,386 pass - last update
about 1 year ago 30,387 pass - last update
about 1 year ago 30,386 pass - last update
about 1 year ago 30,397 pass - 🇭🇺Hungary mxr576 Hungary
The AccessPolicyCacheWrapper also raised another question, do we want to mark certain code as @internal? JSONAPI was and still is very strict about its programming APIs... Could other components do that, like this one?
So to reduce complexity in AccessPolicyProcessor, AccessPolicyCacheWrapper could be a viable answer, but we could be uncertain about marking that as a public API at day 1.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
I'll try to go over your comments on Gitlab and see which ones I can reply to. I again want to iterate that I can't currently spend time on your suggestions regarding the exceptions and additional custom cache layer as I'm on a budget and I don't want to use it up by getting sidetracked without core committers voicing support for these topics.
Having said that, marker interfaces are kind of an anti-pattern as they use empty contracts (interfaces) to project meaning. I'd rather not go down that route as we're better off writing dedicated exceptions for each scenario then; something I also wanted to avoid by simply having a more generic exception.
- last update
about 1 year ago Custom Commands Failed - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
I ended up reviewing all but one comment as, while I was looking at them, it became clear that we should not use a custom cache nor prefix our base classes with Drupal. The only thing that I left up for discussion is what we should do with the exceptions, but as I stated above I am not a fan of marker interfaces so I'll leave that one up for core maintainers to decide.
Also removing needs change record tag as we have a draft ready.
- last update
about 1 year ago 30,404 pass - 🇭🇺Hungary mxr576 Hungary
Having said that, marker interfaces are kind of an anti-pattern as they use empty contracts (interfaces) to project meaning.
I had the exact same mindset years ago and tbh I am still bit on a fence becomes sometimes they prove to be useful... there are design patterns that makes time necessary for example, e.g. Command Bus.
Thanks for reviewing my comments. I definitely do not want to be the cause any over budget type of issues. The quality of this MR is already very high so IMO this can be also merged as is.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Yeah and thanks for the reviews, I really appreciate it.
- last update
about 1 year ago 30,402 pass - last update
about 1 year ago 30,407 pass - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
about 1 year ago Not currently mergeable. - last update
about 1 year ago 30,417 pass - last update
about 1 year ago 30,419 pass - last update
about 1 year ago 30,422 pass - last update
about 1 year ago 30,438 pass - last update
about 1 year ago 30,442 pass - last update
about 1 year ago 30,442 pass - last update
about 1 year ago 30,451 pass - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Left a review, great work @kristiaanvandeneynde
- last update
about 1 year ago 30,453 pass - last update
about 1 year ago 30,436 pass, 3 fail - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Re #29, thanks @larowlan. I've tried to solve as many things as possible and added some explanations left and right. I've not marked things as resolved when I felt you might still have feedback on my answers.
- last update
about 1 year ago Build Successful - last update
about 1 year ago 30,454 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
@larowlan I had to change your code from:
return array_reduce($this->items, fn($carry, $scope_items) => [...$carry, ...$scope_items], []);
to:
return array_reduce($this->items, fn($carry, $scope_items) => [...$carry, ...array_values($scope_items)], []);
As the original would keep keys and keys can potentially be strings here, leading to overriding of elements rather than adding them all to one big array. The latter takes care of that problem by calling array_values() first.
Also, not entirely sold on the readability of the above versus:
$item_sets = []; foreach ($this->items as $scope_items) { $item_sets[] = array_values($scope_items); } return array_merge(...$item_sets);
- 🇳🇱Netherlands kingdutch
I have no knowledge of this issue but stumbled upon the code snippet in #31 and have previously played with similar code. It's not just readability. The closure and overhead of multiple copy/spread is also an order of magnitude slower and has different memory characteristics for large arrays.
Using a sufficiently large items array this can be shown with a small sandbox. I tried upping the iterations to 10.000 but the 3v4l sandbox rand out of memory for the array_reduce example.
$iterations = 1000; $start = microtime(true); for ($i=0 ; $i<$iterations ; $i++) { $reduced = array_reduce($items, fn($carry, $scope_items) => [...$carry, ...array_values($scope_items)], []); } $split = microtime(true); for ($i=0 ; $i<$iterations ; $i++) { $item_sets = []; foreach ($items as $scope_items) { $item_sets[] = array_values($scope_items); } $looped = array_merge(...$item_sets); } $end = microtime(TRUE); echo "Reduce: " . ($split-$start)/$iterations . " sec\n"; echo "Merge: " . ($end-$split)/$iterations . " sec\n";
Reduce: 0.0002404670715332 sec Merge: 1.0043859481812E-5 sec
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks for chiming in! I'll change it to the foreach then.
- last update
about 1 year ago 30,454 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Important topic brought up in the sibling issue: #3376846-21: Implement the new access policy API →
Summarized quote:
However, this means we allow unsaved user's permissions to be calculated and the question is whether that is something we should allow. ... Should we update AccessPolicyProcessor to throw an exception or something if $account->id() is NULL?
- last update
about 1 year ago 30,454 pass - last update
about 1 year ago 30,461 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
One more discussion I'd like to have before the names are set in stone. We currently have:
- Drupal\Core\Session\(Refinable)CalculatedPermissions(Interface) -> Stores all permissions from calculatePermissions and alterPermissions
- Drupal\Core\Session\CalculatedPermissionsItem(Interface) -> A single value object within the above, matching a specific scope-identifier pair
- calculatePermissions and alterPermissions methods on access policies
If you don't mind having short names, we could also rename these to:
- Drupal\Core\Session\(Refinable)Permissions(Interface)
- Drupal\Core\Session\PermissionsItem(Interface)
- buildPermissions and alterPermissions methods, as the docs talk about build phase and alter phase
The shorter class names are a bit less verbose, but at the same time less explicit than the original ones. So this comes down to preference.
- last update
about 1 year ago 30,461 pass - last update
about 1 year ago 30,462 pass - last update
about 1 year ago 30,458 pass, 2 fail - last update
about 1 year ago 30,462 pass - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Personally I prefer the more verbose names but I don't feel that strongly either way
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Yeah me too, it's just I wanted to proactively bring up rather than have people ask about it after the fact :)
- last update
about 1 year ago 30,466 pass - last update
about 1 year ago 30,483 pass, 1 fail 32:31 30:58 Running- last update
about 1 year ago 30,511 pass - last update
about 1 year ago 30,514 pass - last update
about 1 year ago 30,514 pass - last update
about 1 year ago 30,521 pass - 🇭🇺Hungary mxr576 Hungary
Let me second what @Kingdutch wrote in #32
The closure and overhead of multiple copy/spread is also an order of magnitude slower and has different memory characteristics for large arrays.
I also bumped into this problem and it is actually a known and "won't fix" PHP bug tracked under https://github.com/php/php-src/issues/8283
but there is also a potential workaround mentioned in the thread: https://github.com/php/php-src/issues/8283#issuecomment-1084143580 - 🇬🇧United Kingdom catch
Couple of minor comments on the MR, I'm happy with the account switching logic now, very little code in the wild actually checks permissions for not-the-current-user and we've minimised any effects to only when that happens.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Thanks for the review! I dropped the constructor docs and moved the user ID check outside of the loop (good suggestion). I've also updated the inline docs regarding the account switcher to mention that we only switch when the accounts don't match.
In the implementation issue I was able to drop a few more constructor docs.
- 🇬🇧United Kingdom catch
There is one unresolved thread (that I can actually see, MR says there's 4) that is still open, but for me it's not blocking. I am pretty happy at this point but leaving the FM tag on a bit longer in case @larowlan wants to take a final look.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
The only unresolved thread I can still see is @larowlan's proposed refactoring of early returning cache hits, which I implemented. I merely kept that one unresolved for Lee to flag as resolved as I left a comment there:
I adjusted the code, but we now have some minor code duplication: When we retrieve from the DB cache, we run the same code to set it to the static cache as we do at the very end. Namely: wrapping it in an immutable object and adding the same comment to explain why we do so.
Aside from that all threads show up as resolved on my end.
- last update
about 1 year ago 30,540 pass - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Removing the tag as @catch and I have gone over this a few times
- Issue was unassigned.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Awesome! So is it safe to assume that this is now considered "ready"? Asking so I can give Alex Moreno an update regarding the sponsorship. Also unassigning myself then as there seems to be nothing left to do.
- last update
about 1 year ago 30,547 pass - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I think it should be fine to update Alex
- 🇳🇱Netherlands ndf Amsterdam
Hi kristiaanvandeneynde, RTBC it's finally happening!
What a major accomplishment for you personally and a great gift to the wider Drupal community.
I know you have been working on this for years (2016? 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed , ✨ Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() Needs work ) and you kept on going and find your way.Thanks a lot!
- last update
about 1 year ago Build Successful - Status changed to Needs work
about 1 year ago 8:57pm 15 November 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Gave this one final pass review. Left some minor questions/nits. Fine to self RTBC after addressing/replying
Thanks @kristiaanvandeneynde!Adding issue credits.
- Status changed to RTBC
about 1 year ago 12:47pm 16 November 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Answered all comments. Hopefully the one about VariationCache is satisfactory. I felt it would be a bit overkill to explain how API A works in the context of API B's implementation of A.
- last update
about 1 year ago 30,582 pass - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Also thanks @ndf for your kind words!
- Assigned to kristiaanvandeneynde
- Status changed to Needs work
about 1 year ago 3:12pm 16 November 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Just discussed the newly introduced factory with @catch and until we can come up with a better solution I'm going to go with a manually instantiated VariationCache object instead. This due to the fact that VariationCacheFactory only supports bins that can be instantiated by CacheFactory and MemoryCache should have no factory because it's not a true cache bin.
So we're not adding a factory for MemoryCache and I'll adjust the MR. Probably on Monday as I'm off tomorrow.
Setting to NW, will update, ping @catch for approval and then set back to RTBC.
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 6:06pm 16 November 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Didn't want to keep people waiting 3 days. If @catch approves of the new approach then we can remove the CR I wrote here: https://www.drupal.org/node/3402104 →
Will also merge in the Implement issue to see if things break there or not. They shouldn't, but theory and practice don't always agree.
- 🇬🇧United Kingdom catch
Yes using the memory cache directly and tagging the service as a cache tag invalidator seems better to me.
My concern with tagging it as a cache bin is that cache bins serialize and unserialize objects, whereas MemoryCache specifically gives you back what you put in (as documented in MemoryCacheInterface). If we start mixing and matching the patterns, it could get (and is) very confusing. We have existing issues open like 📌 Audit usages of cache.static Needs work and 📌 [PP-1] Clean up MemoryCache and MemoryBackend naming issues Postponed to try to untangle this.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Tests in pipelines are now failing on seemingly random frontend stuff. I fixed the obvious services.yml file mistakes (that's what you get for trying to get something in before your long weekend), but I think everything is fine now. I pressed the "rerun" button on the failing FE tests.
Also, it's really hard to find what failed in the GitLab UI compared to the old test results we'd see on DO itself. Any tips?
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Ah, all green again. Cya on Monday :D
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Hmm, with the cache factory approach all tests in the follow-up "implement" issue go green. With this new approach without a factory, UserPermissionsTest::testUserPermissionChanges() goes red. So maybe cache tags aren't being cleared the way they should be with this approach?
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Yeah just confirmed that tests go green with:
cache.access_policy_memory: class: Drupal\Core\Cache\MemoryCache\MemoryCache tags: - { name: cache.bin }
But red with:
cache.access_policy_memory: class: Drupal\Core\Cache\MemoryCache\MemoryCache tags: - { name: cache_tags_invalidator }
So there is a difference somewhere. Will have to figure that one out.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Found it!
UiHelperTrait::submitForm() and UiHelperTrait::drupalGet() both call
$this->refreshVariables();
, but if we look into that:protected function refreshVariables() { // Clear the tag cache. \Drupal::service('cache_tags.invalidator')->resetChecksums(); foreach (Cache::getBins() as $backend) { if (is_callable([$backend, 'reset'])) { $backend->reset(); } } \Drupal::service('config.factory')->reset(); \Drupal::service('state')->resetCache(); }
It's clearly only resetting cache.bin tagged services, which is why we are seeing this testfail. If I manually add in this line:
\Drupal::service('cache.access_policy_memory')->reset();
Then the tests go green again.
So it's purely a test fail because refreshVariables() doesn't pick up on cache tag invalidators that also support a reset() method.
- Status changed to RTBC
about 1 year ago 10:05pm 16 November 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
With the above confirmed and @catch's earlier approval we can set this issue back to RTBC and we'll have to work around what I found here in the Implement issue.
-
larowlan →
committed 77638be3 on 11.x
Issue #3376843 by kristiaanvandeneynde, larowlan, catch, mxr576,...
-
larowlan →
committed 77638be3 on 11.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed and pushed to 11.x!
Published change record.
Congratulations @kristiaanvandeneynde on a huge effort to get a major new API into core (in record time no less)
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
To avoid confusion in the future, I deleted the redundant CR for the memory cache factory
- Status changed to Fixed
about 1 year ago 9:02pm 17 November 2023 - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Awesome!
Thanks to everyone who helped review and move this forward.
Automatically closed - issue fixed for 2 weeks with no activity.