When dealing with multiple consumers that have set the same default user, this will result in unexpected access behaviour. The current cache context is based on roles, this made sense in simple_oauth: 5.2 where scopes are roles, but in 6.0 the scopes are a separate entity which reference to permissions or roles.
We should introduce the new access policy (from Drupal 10.3) and custom cache context for scopes. This way the cache keys will be unique by user and requested scopes. Leveraging the access policy will also give the community possibilities to alter or add their own policies.
Drupal 10.2 is EOL, so we can increase the minimum Drupal core requirement to 10.3 and don't have to consider BC.
Workaround that can be used is to set a different default user (with unique roles) per consumer.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !172
Comments
Comment #2
bojan_dev commentedComment #4
bojan_dev commentedComment #5
bojan_dev commentedComment #6
bojan_dev commentedCan someone please review MR !172?
Comment #7
berenddeboer commentedI would appreciate this being merged in if planned. Massive change, so any merge request would be so much easier if we had this :-)
Comment #8
bojan_dev commentedIt’s indeed a massive change, which is why I need a review from other developers before merging my MR.
So please, review away!
Comment #9
kingdutchThanks for the effort Bojan! Really nice to see Simple OAuth move to the new system in core 🎉
Comment #12
bojan_dev commentedI accidentally merged this into 6.0.x. This has now been corrected and reverted. The feature is available on release in 6.1.0.
@kingdutch Thank you for the constructive feedback!
Comment #14
m.stentaFirst, let me say: Thank you! I think this change makes sense, and I'm glad it was done! We are extraordinarily grateful for all of the work that goes into maintaining this module.
Can I ask why? Why did you want to avoid a major version bump for this?
This is a significant refactor that IMO warranted a v7, if only to provide downstream dependents an opportunity to test and update accordingly.
This caused our automated tests to start failing in farmOS. I don't know why yet, but I suspect it is due to the fact that we have our own custom access policy logic and custom scope granularity. We use a version constraint of
^6.0fordrupal/simple_oauth, but will have to start pinning to6.0.9until we can spend the time to dig in and understand what's broken. We wouldn't have to pin if the major version was bumped.It may be that this change did not "technically" require a major version bump, according to the semantic versioning specification. I can't say until I look closer. But it certainly seems big enough to warrant a major bump, if only out of an abundance of caution and courtesy to downstream.
Comment #15
bojan_dev commentedThe primary reason for avoiding a major version increment is the additional maintenance overhead it entails. As noted in the MR, there are no breaking changes (BC's), and it is assumed that existing implementations of permission handling will remain unaffected.
This update also addresses a bug introduced in version 6.0.7 as part of a security update (see: #3558777: Token Auth User results in "The 'restful get YOURPERMISSION' permission is required."). By delivering this fix as part of a minor release, it can be integrated and adopted by the community more rapidly, without the disruption associated with a major version upgrade.
Comment #17
m.stentaI finally had some time to investigate what broke in farmOS.
It was the addition of
ScopeGranularityInterface::getPermissions(), and the related refactoring that now requires this method.This is a breaking change because downstream implementations of
ScopeGranularityInterfacewere not made aware of the new required method. Any downstream code that provides their own custom scope granularity may potentially break simply by runningcomposer update(upgrading from 6.0.9 to 6.1.0). Saving this change for a major version update would have avoided this.In our case, we have a custom scope granularity plugin that extends from
\Drupal\simple_oauth\Plugin\ScopeGranularity\Role. The only method we are implementing in our child class isScopeGranularityInterface::hasPermission(). So, when the new logic asks our scope granularity for a list of permissions, it ends up falling back on the parent class'sgetPermissions()logic.If we had not been extending the parent
Rolemethod, then we would have experienced a fatal error, becauseScopeGranularityBasedoes not implementgetPermissions()either.It's probably too late to undo this, and I want to reiterate that I think these are good and sensible improvements, AND our users specifically were not critically impacted by this because we caught it early and pinned to 6.0.9, but IMO we should be more careful about major changes in minor version releases moving forward.
Comment #18
bojan_dev commentedThe assumption was that there were no BC breaks, but it turns out one was introduced, which was not intended. You are absolutely right that we should be more strict with changes like this. Given that this resulted in a BC break, it would be appropriate to add a change record so this is clearly documented. Thank you for calling this out, I will take greater care to avoid this in the future.
Comment #19
m.stentaThanks @bojan_dev! Appreciate all you do for this project!