saschaeggi → credited paul121 → .
Including patch with latest changes for 6.0.0-beta6
Marking Needs Review since I have refactored some of the logic here, would love a review of this. Acknowledge that we could still use tests
In reviewing this issue again I believe there is another issue regarding client_credentials grant with public clients.
A "confidential" consumer with a field value consumer.confidential === TRUEthat is configured *without* a secret (or an empty secret string) is not validated and could be authenticated. This is a problem. The spec says "the client credentials grant type MUST only be used by confidential clients" - the spec does not care nor know about the concept of a consumer.confidential field, solely that if a client secret is not configured (or has an empty string, per the spec) then client_credentials grant cannot be used.
I've refactored the logic in this validate function to explicitly check for this case with a "public" client + client_credentials grant. I think this makes things simpler and easier to follow. I've added a kernel test to cover this case. Using the Gitlab CI "test only changes" you can see this test fails on current code: https://git.drupalcode.org/issue/simple_oauth-3322325/-/jobs/1539305#L110
I would also want test coverage to ensure that in case the client secret is set to empty the client can not authenticate with a non-empty secret (something I think the current code allows if I look at it). In the case a client provides a secret but is configured not to use one, they're still using an incorrect secret and should be denied.
Good catch @kingdutch. These changes should cover this case as well. With my changes we only return TRUE in one place for a fully correct public client w/o any client secret provided. The final check does a password check only when the stored and provided client secret exist, else returns FALSE.
Unit tests, so that this does not regress again.
Agreed @rudolfbyker, this ClientRepository::validateClient logic would best be covered in a unit test. Unfortunately I don't have much experience writing unit tests and don't have time at the moment.
A way to clear the secret in the UI when editing the consumer. Currently, if you leave the field empty, it just means "don't update".
Agreed this is needed as well. I'm not sure the best approach to do this from the UI. Perhaps we could discuss and agree on an approach before starting work? Personally I'd like to see the UI changes happen in a separate issue. We've been dealing with this issue and patching in FarmOS for a while and would love to just get the validateClient logic in place.
Made a new 1.1.1 release with this
That caching makes sense. Thanks for sharing. Will keep this in mind for future cases. Thanks again @wotnak!
Thanks @wotnak! In the past we've solved this issue of hiding local tasks by making the associated route have an access check result/response of 4xx. I've done this in farmOS core and some contrib modules. With your implementation I worry that this might entirely remove the task definition? Unless they are cached for each entity/page? And even if they are, it might just be easier to have a single task that is always visible and use access checking to determine if the task is shown?
.. In fact, it looks like I had already implemented my approach in this module via a Route subscriber: https://git.drupalcode.org/project/farm_project_plan/-/blob/1.x/src/Rout...
BUT _entity_bundles is deprecated and must have been removed from Drupal 10, now we should use bundle
https://www.drupal.org/node/3155569 →
I think that would solve it. What do you think of this approach?
Fixed via 📌 Uninstall v1 migrations Fixed
Here is a simple fix. Attaching a patch we can use in farmOS.
Although I feel like there must be a more elegant solution because we are adding various margin-top, only to remove some of it from higher up in the .content-header. But it's tricky because the top margin is added to .help, .region-content and .region-highlighted. https://git.drupalcode.org/project/gin/-/blob/8.x-3.x/styles/base/_layer...
To to properly test a more thorough change I think we would need to consider all combinations of pages with/without local tasks, help text, and status messages, all across different screen sizes.
Attaching screenshot examples
I've implemented a first version of this: https://git.drupalcode.org/project/farm_jd/-/commit/eb3785b9c704a7625474...
When checking for new field operations we also enqueue the field operation's shape file to be generated (this is an async operation, can take up to 30 minutes). Most of the time this file should be ready once the field operation is actually imported as a log.
Right now it is only requesting the Point - EachSection shapefile from JD. It also uses the farmOS configured units. For more info see here: https://developer.deere.com/dev-docs/field-operations#/fieldOps/{operationId}/get
Some ideas for future:
- Have a dedicated page that lets you sync shape files "on demand" - this could allow specifying a specific granularity of shape file you want to sync (Point/Polygon, Section, Sensor, Hertz). It could also help remedy issues when a shape file is not able to be synced when the log is created.
- Make auto-syncing of shapefiles entirely configurable. This could also include configuration for the type of shapefile.
- Parse the equipment out from the shapefile. This should be possible, but looks challenging.
Done!
Pretty simple changes. We only use the product vs products when populating Field Operation change lists (so we have a simple name of products/varieties). Update here: https://git.drupalcode.org/project/farm_jd/-/commit/daf671d2400e9b20acd2...
When actually creating logs, we get product information from the Measurements API. I've updated this to use ProductTotal measurements nested under the new ApplicationProductTotal key/list: https://git.drupalcode.org/project/farm_jd/-/commit/74fbc17954ec7717bf1b...
This is specified in the varietyTotals - added here: https://git.drupalcode.org/project/farm_jd/-/commit/daf671d2400e9b20acd2...
What if we abstracted BOTH of these hooks (hook_entity_base_field_info() and hook_farm_entity_bundle_field_info()) into a single farmOS EntityFields event, and recommended that modules use that instead of hooks.
This makes sense. I think we would still want separate events to distinguish between base and bundle fields (modules should know which they are providing?) - but yes, we could likely have a single EntityFields event class that could be re-used for both use-cases. And the nice benefit being modules could encapsulate all of this logic into their own, single Event Subscriber class.
Awesome.
I also like referring to this resource: https://gist.github.com/bojanz/c5fcf5cef22406096588
TLDR;
Choosing the right solution:
Are you allowing modules to react to something that has already happened, or alter a structure? Use events. Do you need to allow your class to have multiple instances created by the user? Use plugins. Otherwise, use tagged services.
Most of the time I think we'll want events. But a few of these hooks that are just collecting provided definitions and not altering (like dashboard panes & groups) could potentially use tagged services. A nice write up on them here: https://drupalsun.com/lakshminp/2016/08/17/tagged-services-drupal-8-0
No worries, I'll close that one. I won't have time to work on tests anytime soon.
Attaching a patch we can use in farmOS.
I've opened a new issue in Drupal core: 🐛 Datetime_timestamp widget does not use default field value Active
Thanks for the *fast* response @acbramley! I've opened a new issue 🐛 Datetime_timestamp widget does not use default field value Active
Aha. With some help from @wotnak I've found a slightly easier to way to adopt content forms in farmOS. We can register the node_edit_form template ourselves as a little hack with just the following:
/**
* Implements hook_theme().
*/
function farm_ui_theme_theme($existing, $type, $theme, $path) {
return [
'node_edit_form' => [
'render element' => 'form',
],
];
}
Once we implement hook_gin_content_form_routes() the content form starts working great.
There is just one issue, this warning is thrown in the log messages the first time you load an entity form after clearing the cache:
User warning: The following theme is missing from the file system: node in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of /var/www/html/web/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php)
Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of /var/www/html/web/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php)
This is caused by the implicit (and unnecessary) dependency on the node module as described above. Removing the claro/node-form library solves this issue. I'm attaching a patch for this single change so that we can use it in farmOS, but it should not be considered a complete fix for this issue.
Unfortunately this has caused a breaking change starting with Drupal 10.2. The log entity form no longer sets the default value for the timestamp field and because the field is required, a value must be entered in order to submit the form. If the field was optional then I believe
Previously the datetime_timestamp widget would always use default value set for the field at $items[$delta]->value but now this widget only uses the default value if the parent entity (the log) is not new. This was changed in
🐛
Remove sample date from date field error message and title attribute
Fixed
: https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...
Just re-opening this issue to flag this... we may want to open a new issue in Log or contribute to Drupal core to fix. I left a comment in the relevant issue for now: https://www.drupal.org/project/drupal/issues/2791693#comment-15399165 🐛 Remove sample date from date field error message and title attribute Fixed
I believe an unintended change was introduced with this issue. If a timestamp field provided a default value or default value callback, that field-level default value is no longer used as the default value in the entity form when creating a new entity. There is a check in TimestampDatetimeWidget::formElement() to only use the field item delta value if the entity is "not new": https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...
A timestamp field with a default value might be a common use-case when the field is required (the user must provide a value) but the application would like to preset a default value to the current time for convenience. For an example see
📌
Make timestamp required
Fixed
paul121 → created an issue.
LGTM
And a re-roll for 10.2.x. There was a change to the TaxonomyTermPagerTest modules 8d2f88bf since 10.2.0 that conflicts. Having trouble generating the interdiff for this.
Re-roll of the patch in #80 for 10.2.0. The TokenReplaceTest file has been removed.
hestenet → credited paul121 → .
Done. I added a new JDSync service, Drush commands and cron job. It also keeps track of last updated times and only syncs via cron at max 1/hour.
Fixed. I also cleaned up some of the module config, there were inconsistencies between how we used config and state.
Fixed a bug with the search operator. Thought we could use CONTAINS but that is only supported for the Entity Query API.
@Gaurav-gupta you created a separate issue for changes related to the D10 support and I merged your commits from there: Please see #4: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... →
Thank you!
Thanks Mo. I made a few follow up commits to make the page_size work via the query parameters. There was a bug if you set the page size to 100 and then moved to page #3, the page size would go back to 50.
Attaching screenshot of the Examples module package. This at least somewhat of a convention for migrate modules.
Attaching screenshot of what this looks like. The scopes field is available below the 3rd party/confidential fields, but before Redirect URIs.
paul121 → created an issue.
That is an interesting challenge. Automating this might be hard to do within Simple OAuth. This patch appears to change the redirect_uri for all consumers, but I don't think that is a solution. In my experience the redirect URI is often a URL external to the Drupal host (for integrations with other services) so you would never want to change this value even in non-prod environments. Maybe you are using this for a headless app on the same domain?
Maybe there could be a boolean on the consumer to indicate that the redirect URI is the Drupal host, in which case you could conditionally use the logic in this patch.
Patch for 6.0.0-beta5
This issue still exists in v6. Attaching a patch with same solution as above. Also have a MR ready once the Drupal gitlab instance is back up.
Actually, this will help us now with v6! More info and implementation in this PR: https://github.com/farmOS/farmOS/pull/743
I should note that those tools and scripts won't support both farmOS 1.x and 2.x long term,
... and now 3.x and Simple OAuth v6 is in the mix. In v6 consumers can no-longer automatically grant "user access" to a token. This is because all scopes must now be created separate of roles, either as an OAuth2 Scope config entity, or be defined in a static scopes file. There is no guarantee that scopes have been created for each role that exists in the system (although I think this would be a nice convenience thing for simple OAuth to offer).
Ultimately I think it is a good that API integrations be more specific in requesting which scopes they require and not blanket granting all user access. This will require some work in the short term for existing apps/integrations to upgrade and make sure the relevant scopes exist on the server(s) they are connecting to. But in the long term this new implementation of OAuth2 scopes should make things easier because we can provide a default set of scopes that are based on permissions instead of roles, and applications can be more confident that these general purpose scopes they are requesting will exist (eg: "asset:view", "asset:create")
I did not add additional tests with this MR but all existing tests are passing. Including a patch for use in the meantime.
Update IS to clarify issue with "Admin" roles
Related issues for revoking refresh tokens on profile update which I think would solve this problem: 💬 [PP-1] Revoke refresh tokens Needs work and 📌 Auth revoke on profile update Needs work which has MRs for both 5.2 and 6.
Thanks @BMO for adding tests here.. if you can add tests to the other issues that might help move things along?
@kenorb I believe this is the playlist that used to be referenced there: https://www.youtube.com/watch?v=rTcC0maPLSA&list=PLZOQ_ZMpYrZtqy5-o7KoDh...
The specifics of how to use simple_oauth may be a little dated, especially for v6 (no password grant, implicit grant, scopes have changed...) But still good info on OAuth2 more generally. If a maintainer can correct the link I think this playlist is still better than nothing.
This LGTM. Trying to think where else this change could cause issues but not sure. Glad this doesn't break JSON:API!
Thank you @NicolasGraph! I mentioned this error here but it still got committed: https://www.drupal.org/project/gin/issues/3394085#comment-15280197 🐛 Fatal error when enabling the new experimental sidebar navigation Fixed
This is the solution I was thinking as well. You went a step further and correct the sidebar items to only be links when necessary. Works great.