I had a quick peek and let me first say that it is great work what you did here @tstoeckler. Unfortunately I jammed with work, so an extensive review I can't give in the short term. Any dev's that what to speed up the process, feel free to review.
Looks good!
I agree with @kingdutch points, we shouldn't change the default behavior for invalidating the tokens and I rather not introduce a new major version on the 5.x. For 6.0.x on the other hand we are still in beta release and there is more active development, we could introduce a new token invalidation behavior there. The question is if this (token invalidation) change benefits a larger target group, I believe it does, so I would propose making the token invalidation configurable via config settings per case (password change, account blocked, roles change).
I guess in the README.md.
Closing due to lack of activity.
Closing due to lack of activity.
Closing due to lack of activity.
Closing due to lack of activity.
Nice catch!
Could be related to:
🐛
"Reference" must be defined in MODULE_NAME.field_type_categories.yml in assert() (line 183 of /var/www/html/web/core/lib/Drupal/Core/Field/FieldTypePluginManager.php)
Needs review
.
In any case if you can't reproduce it on 6.0.x, there is nothing to fix.
Looks good to me, TY!
bojan_dev → changed the visibility of the branch 3484623-the-autocomplete-routeendpoint to hidden.
bojan_dev → created an issue.
Nice work!
TY!
Great work @manojsaha! TY for contributing.
bojan_dev → made their first commit to this issue’s fork.
Authenticated request are by default not cachable (coupled or decoupled). You could try enabling "Dynamic Page Cache" module, it caches pages minus the personalized parts (both anonymous & authenticated). If you want to leverage a reverse-proxy you could take a look on https://www.drupal.org/project/adv_varnish → .
TY for contributing,
TY!
Done
Remaining task (for 5.2.x): copy the Kernel tests from MR !66 to !64.
Looks good, TY!
Looks good, TY @idebr!
See comments from bradjones1: #6, #8 and #23.
I'm pretty jammed with work, so anyone able to write tests will be welcome.
I'm happy to merge it, but the MR's are still missing test coverage, so feel free to contribute.
bojan_dev → created an issue.
bojan_dev → created an issue.
The project page and README.md does mention that it uses the PHP library "OAuth 2.0 Server". Also installing modules via composer is the recommended way, the following quote covers it:
The recommended way of adding a module is with Composer, especially for modules that have dependencies. Several modules will just not work if you only add the zip / tar.gz file.
Source: https://www.drupal.org/docs/extending-drupal/installing-modules#s-severa... →
https://git.drupalcode.org/project/simple_oauth/-/merge_requests/99.diff applies on 6.0.x.
Added setting/kill switch as suggested, please review.
bojan_dev → made their first commit to this issue’s fork.
Found related issue:
✨
Allow to index empty fields
Active
I will try to build the kill switch and contribute in the other issue.
I think to properly check whether the row might have the property in question, you will always need to load the entity
I rather find a solution to not load entities, this makes even more sense when using Search API Solr, where you actually want to only use Solr instead of DB queries.
I also don’t quite understand your use case, to be honest. If your view only lists that single field, why include nodes that don’t have it at all? Why not add a “associated reference field: not empty” filter to the view?
For example imagine a specific CT that has tags or a category and you want to display the taxonomy label it in the search results. I don't want to add that filter because that would mean I won't get other CT's in the search. You need to see it as search results that for one or more CT's has more info to show.
bojan_dev → created an issue.
This was already resolved in 6.0.x, thanks for the MR.
This will be resolved in 📌 Auth revoke on profile update Needs review
@TrevorBradley tnx for the headsup and @ankitv18 tnx for the MR!
TY!
TY!
Yup, you are right, my bad, I overlooked the fact you can just subscribe on the event, add/alter conditions and make use of the setInvalidateAccessTokens method on the event.
Need to check why the tests are failing, after that we can finally merge this.
Looking at the different use case, I think we should make the implementation in the EntityUpdateHookHandler more flexible, so that the conditions for revoking the tokens can be altered with more ease. The EntityUpdateHookHandler is a final class, so it's not extendible. Making it regular/abstract class and introducing a interface would make the service more flexible and guided. We could even introduce an alterHook to simplify the alterations like e0ipso said in
#55
📌
Auth revoke on profile update
Needs review
.
Looks good, TY!
Will create new release, after consumers module released D11 support.
Nice work, TY!
Bug should be reported on the consumers module.
Looks good to me, TY!
For the role granularity, it does make sense, TY for the fix.
Missing update hooks from 5.2 have been added.
bojan_dev → made their first commit to this issue’s fork.
Simple OAuth is based on the "thephpleague/oauth2-server" library. The feature request there is similar to yours.
Thanks you for the MR. I have added a commit to maintain support for JWT 4.
When we move to league/oauth2-server:9.0 we can drop the JWT 4, this way we are consistent with the league lib.
bojan_dev → made their first commit to this issue’s fork.
Tnx for the patch and the bump :)
Resolved in 5.2.x + 6.0.x.
I don't think the description is the right property to use, it's purpose is to describe the scope, which can be lengthy. It would be more fitting to introduce a new property e.g. label.
I do believe it would be an improvement to make the granularity pluggable, I can imagine simple_oauth would be able to serve more use cases this way. The naming (granularity) is also a valid point. I would go for it.
Unit tests would be a must for this to land.
Looks good, TY!
bojan_dev → made their first commit to this issue’s fork.
Looks good to me, TY!
bojan_dev → made their first commit to this issue’s fork.
Scopes are now config entities and they can be mapped to roles, so you have the possibility to use scopes as roles like simple_oauth:5.2 does and you could even automate the scope creation process when roles are being created by implementing event subscribers/hook_entity_update.
To map a scope to a role via the UI:
- Add dynamic scope: "/admin/config/people/simple_oauth/oauth2_scope/dynamic/add"
- Set granularity to "Role"
- Select a role
If I understand you correctly, you want to authorize without specifying a scope? This is currently not possible, but there is a feature request which would solve your issue:
🐛
Allow default scopes to be set regardless of grant type
Needs work
This would make it possible to set default scopes from the consumer for the authorization grant type, this way you could omit the scopes when authorizing.
Unfortunately drupal.org is filtering the URL query parameters, as workaround I have updated the demo link to the first video.
Thanks @paul121, this is a nice improvement.
FYI: I have removed the Oauth2ScopeProviderInterface::getFlattenPermissionTree method (which is redundant now), updated the tests and fixed some coding standard/deprecations errors.
bojan_dev → made their first commit to this issue’s fork.
The issue is reported on version 8.x-4.x, which is end of life since 2022 February 28. I would recommend to update to 5.2.x or 6.0.x.
This slipped through #3374084, will be releasing a patch release for this.
bojan_dev → made their first commit to this issue’s fork.
Nice feature!
I added support for changed and created field types, and also fixed some deprecation/coding standards.
TY @mglaman, for 6.0.x this is not applicable.
Merged, TY!
bojan_dev → made their first commit to this issue’s fork.
I made a few changes to support the default scopes in all grant types. Remaining tasks:
- Check if we still need the default scope logic in the ScopeRepository::finalizeScopes.
- Add tests for all grant types.
bojan_dev → made their first commit to this issue’s fork.
I agree, merged ;)
bojan_dev → made their first commit to this issue’s fork.
Looks good to me, TY!
There is a edge case were this issue is still occuring in an initial state, but this can be resolved when the editor:attached event lands in core, see:
✨
Trigger event when Text Editor is attached
Needs work
We can then do:
$(document).on('editor:attached', function () {
if (Drupal.CKEditor5Instances) {
Drupal.CKEditor5Instances.forEach(function (editor) {
editor.editing.view.document.on('change:isFocused', (event, data, isFocused) => {
if (isFocused) {
drupalSettings.tokenFocusedField = false;
drupalSettings.tokenFocusedCkeditor5 = editor;
}
});
})
}
});
bojan_dev → created an issue.
Released 5.2.4 and 6.0.0-beta5, thanks for contributing!
bojan_dev → made their first commit to this issue’s fork.
Rerolled #7 + added some coding standards fixes
I was getting some errors, e.g: a translation already exists for the specified language, duplicate entry. I resolved it by adding additional condition to make it more specific.
I'm using patch #122 in combination with the paragraphs_asymmetric_translation_widgets module. The client doesn't want any UI changes while having async paragraphs, so thats why I'm using the "stable" widget. But like I tried to explain in #136, there is a issue when initially translating the paragraphs, it doesn't set the host langcode on the paragraphs, while it does when you just add new paragraphs. There is a clear difference how paragraphs are being stored in "Paragraphs Legacy Asymmetric" vs "Paragraphs (stable)", while I assume it should be the same.
While trying to solve the above issue, I'm trying to get my head around patch #122:
This chunk makes sense for async paragraphs, but it doesn't get here when I translate the node and reuse the existing paragraphs (see #136 HTR). I moved this chunk lower in the code where it gets triggered when reusing paragraphs and creating new ones.
// Set the new paragraph language if the host entity is translatable.
if ($host->isTranslatable()) {
$langcode_key = $paragraphs_entity->getEntityType()->getKey('langcode');
$paragraphs_entity->set($langcode_key, $form_state->get('langcode'));
}
The problem now is that the functional tests fail, in my new patch #140 I have added an additional condition to check on "allowReferenceChanges", so that this logic only applies when dealing with async paragraphs.
#122 has one case not covered: when adding a new translation to a node and reusing the same paragraphs, this will result in an issue with functionality that relies on the langcode of the associated field, for example: linkit with “Linkit URL converter” filter enabled, will generate URL aliases in a wrong language.
HTR:
- Add paragraphs with long_text fields.
- Make the paragraphs and underlying fields not translatable.
- Setup entity revision reference field on a node and make it translatable.
- Create a node with paragraphs.
- Add a translation to the node and just save.
- Look in the database, look up the associated paragraph in “paragraphs_item_field_data” table.
- You will see two rows with the same id and revision_id but with different langcode.
- Also the associated long text field in the DB, will give back a wrong langcode.
I updated patch #122, by moving the logic where the langcode is being set when the host entity is translatable, to a condition which covers both cases (adding new paragraphs or reusing them from translations).
After some retesting, I figured out it had todo with core.extensions in my DB, had to write a hook_update to disable ckeditor before running drush cim. My bad, I will close the issue.
bojan_dev → created an issue.