Looks like I could do this - I am a maintainer, but don't technically have "administer maintainers" on there. however, I can do it via my security team role
pwolanin → created an issue.
@daffie - yes, we need to support a lot of authenticated users, but a web application may not behave like a social media site in terms of the kind of data you need to load and show. Still, for our uses we are very much pained by the slowness of loading (and even more) saving entity data en mass. So, entity/field API performance one of our biggest pain points. We also use custom SQL queries at points, so it's not trivial to drop mongoDB in.
More personally in response - it feels ever more intimidating to get involved in core issues and it seems like a lot of good ideas linger with years of discussion. As a (listed) subsystem maintainer I don't feel like I have any special authority or reason to stay involved. Perhaps the core strategy will help define some technical directions that will reduce circular discussions on issues and encourage involvement.
@catch - that sounds like a lot of thing that may not come to pass.
Also - if I'm building a data-oriented web app, I don't think I'd start from Drupal CMS, and I think there needs to still be consideration for all the use cases that want to start from a reasonably fully functional Drupal core.
So, at the very least this issue sounds premature until the Drupal CMS being stable with a good search_api recipe is implemented. At that point, it sounds like the only concern is around user confusion?
Took a look at the 11.x MR. Looks like a simple and sensible fix, so moving back to RTBC.
This seems like a bad idea to me. The functionality of this module seems pretty essential for Drupal core to have a minima feature set.
pwolanin → created an issue.
There are interesting ideas from symfony about what to support. Their style of key rotation doesn't seem great for us where values are likely in the database an prod rotation happens only on the live site.
pwolanin → created an issue.
There is another request for a container parameter. The goal here is just to make it work better for everyone by default in a simple way
@ro-no-lo sounds like you have more of a support request - maybe try Slack?
We are using the JWT path auth in production for accessing private files, so I assure you that it can work.
Not sure I follow. There are similar examples in core that don't have a mapping, and a couple that do
15-
16-views.field.moderation_state_field:
17: type: views.field.field
18- label: 'Moderation state field'
core/modules/comment/config/schema/comment.views.schema.yml
21- label: 'Comment bulk form'
22-
23-views.field.commented_entity:
24: type: views.field.field
25- label: 'Commented entity'
26-
merged, thanks!
pwolanin → made their first commit to this issue’s fork.
Doing a quick test - I am still getting the OOM with that hook removed. Will double check with the specific diff applied.
Not sure why it would be related? As I mentioned above this seems related to a change in the config schema in the 3x series and is being triggered by config translation code.
volkswagenchick → credited pwolanin → .
Node module is also responding to various changes in hooks (e.g. comment update) and calling node_reindex_node_search()
This feels like a similar case, so I think the approach in the MR is reasonable given the limitations of the system.
xjm → credited pwolanin → .
larowlan → credited pwolanin → .
@drumm could you point to the code involved in project module? Is there some way we could make it possible to pick the release "series" when I'm making a new release?
In the users_jwt module I disallow an expiry more than 24 hours in the future, so I think that would be a reasonable max here also (at least for the form).
I agree with berdir here - pretty much every contrib release is a minor release and the availability of patch releases is a nice to have but not even necessary for most cases.
It seem like a "series" of releases used to be based on an actual git branch?
If I want to have the behavior currently being forced here I'd expect to need to create 2.0.x, 2.1.x, etc branches for each minor.
The fact that I have to specifically flag the old minor as supported and have it show on the project page seems especially weird and terrible. I don't need it to be unsupported unless it has a security vuln, but I certainly don't want it to show as a suggested download.
merged to 2.x and will cherry-pick to 3.x
Please make a 3.x issue to remove the old methods.
thanks alexpott!
not sure why it shows some commits against MR 12 as well as 15
Moved to target 3.x
Good idea - I didn't see this existing issue and started on this in
[#427911]
Can you include the hashed session ID as part of the JWT payload? That can't be used alone to hijack a session, but I worry it's kind of an internal detail that isn't really exposed as part of the session API
Also - digging into this before, it's seemed hard to bootstrap the session code if there is not an expected cookie.
pwolanin → created an issue.
pwolanin → created an issue.
Can you clarify - you got a session cookie after logging? How did you get the JWT?
Looks, good - thanks!
Looks good, thanks for catching those warnings
pwolanin → made their first commit to this issue’s fork.
Adding an alternate method to split out a core module using git filter-repo
@drumm - I should be able to run through this process within a day or two when it's possible
Gave this a quick review - seems like a fine test change to back to RTBC
Gave this quick review - not sure altering the field label is needed, but it's not a big deal
Created 💬 Unreserve or create "book" module in ontrib Active
pwolanin → created an issue.
I just tried to create a module with short name "book" but I was rejected: "This project short name is reserved."
I could be listed as a co-maintainer with smutgave
If people are seeing this for real, please suggest a test case we can add. I'll note the bug is fixed in a different way in the 2.x branch
How about this version?
Looks good, thanks for the cleanup
The site is "multilingual" in as much as we have a custom version of English enabled to use to replace some of the interface text. And yes, the problem is related to processing the config overrides.
We do not have any of those modules (- geofield - entity_reference_revisions - search_api)
According to git bisect:
3e77a0d12a52856316f3e5da004af82e6572be9f is the first bad commit
https://git.drupalcode.org/project/rest_views/-/commit/3e77a0d12a5285631...
That added views.field.field_export to the config schema
Weird - the segfault (need to test OOM) seems to be happening in the setup for the foreach loop in \Drupal\locale\LocaleConfigManager::getTranslatableData():
if ($element instanceof TraversableTypedDataInterface) {
foreach ($element as $key => $property) {
the foreach is going to cause this to get called:
/**
* {@inheritdoc}
*/
#[\ReturnTypeWillChange]
public function getIterator() {
return new \ArrayIterator($this->getElements());
}
We seem then to be into the config schema
The config schema was changed in the 3.x branch here: https://git.drupalcode.org/project/rest_views/-/commit/a986dd394e2116f42...
The segfault happens in \Drupal\locale\LocaleConfigManager::getTranslatableData()
the element triggering it is: display.default.display_options.fields.field_location_related_orgs_export in the view
That looks like:
field_location_related_orgs_export:
id: field_location_related_orgs_export
table: node__field_location_related_orgs
field: field_location_related_orgs_export
relationship: none
group_type: group
admin_label: ''
plugin_id: field_export
label: 'Used by'
That plugin is provided by rest_views
The error seems to happen in
\Drupal\locale\LocaleConfigManager::getTranslatableData()
which is a recursive method
It seems like the problem is locally caused by a view (config) that uses rest_views and also views_streaming_data to handle a csv export of the data.
Trying to reproduce locally, I'm getting a seg fault instead of OOM at exactly the same point :(
Looks like what's happening is drush is trying to process a batch, the callback is locale_config_batch_refresh_name(), and that is trying to update config translations.
That batch job looks like it is set by function locale_system_update() which runs after modules are installed
So, this seems like it would have been happening before. IDK why updating rest_views is causing an OOM or seg fault with this update.
Applying a patch to disable the hook in the module still gives an OOM, so it's probably not that hook just something else happening after modules are enabled or caches cleared
Note that some custom modules depend on the main rest_views module, but none depend on any sub-module of rest_views.
We also install rest_views as a dependency earlier in the process, so it's not newly enabled at the point we see the OOM
It is possible it's happening in some code called from this function, or some other implementation of that hook.
function rest_views_modules_installed($modules):
The OOM happens in CI as various modules are being enabled and I see a log message from one of our custom modules from it's implementation of that hook.
Oddly the OOM is always mentioning the same 2 files/lines:
[2024-02-05T18:51:36.610Z] [info] Executing: /var/www/drupal/vendor/bin/drush batch-process 2 --uri=default --root=/var/www/drupal/web
[2024-02-05T18:51:36.949Z] > [info] Switched to user 1
[2024-02-05T18:51:39.954Z] > PHP Fatal error: Allowed memory size of 1572864000 bytes exhausted (tried to allocate 262144 bytes) in /var/www/drupal/web/core/lib/Drupal/Core/Config/TypedConfigManager.php on line 208
[2024-02-05T18:51:39.954Z] > PHP Fatal error: Allowed memory size of 1572864000 bytes exhausted (tried to allocate 262144 bytes) in /var/www/drupal/vendor/symfony/http-foundation/Response.php on line 226
pwolanin → created an issue.
The logic change looks ok, but you changed the code style to be incorrect:
12 coding standards messages
✗ 12 more than branch result
src/Transcoder/JwtTranscoder.php ✗ 12 more
line 261 Line indented incorrectly; expected 6 spaces, found 8
262 Expected newline after closing brace
263 Line indented incorrectly; expected 6 spaces, found 8
264 Line indented incorrectly; expected 8 spaces, found 12
265 Line indented incorrectly; expected 6 spaces, found 8
265 Expected newline after closing brace
266 Line indented incorrectly; expected 8 spaces, found 12
267 Line indented incorrectly; expected 6 spaces, found 8
271 Line indented incorrectly; expected 2 spaces, found 0
271 Closing brace indented incorrectly; expected 2 spaces, found 0
271 Expected 1 blank line after function; 2 found
274 The closing brace for the class must have an empty line before it
Sounds like we should add typehints in the 3.x branch if we do that any time soon.
Option #1 is maybe fine... you can change the @return param comment even and document that it just returns what was added in setUser() / setAccount() ?
As far as tests... well maybe you are right that there is not much to test. You could have a Kernel test or something that sets a \Drupal\Core\Session\UserSession as the user in a test module event subscriber? Why do you need a non-core implementation?
Looks like a good addition - we should have some tests to show it works
The test module should probably not depend on itself
I think this is duplicate to the D10 update issue fixes? MR shows a merge conflict now
pwolanin → made their first commit to this issue’s fork.
Yes, I don't see why you'd get that kind of error based on the patch.
I want to take one more look. Also, needs to target the 2.x branch
Seems like this is not a bug. You may be interested in this issue to be able to log some info about auth failures:
✨
Create a mechanism to log the decode exception in \Drupal\jwt\Authentication\Provider\JwtAuth::authenticate()
Needs review
Looks like an issue with vendor library and maybe values in your settings.php
Changing the method names seems overkill and likely to break other people's integrations.
If you really want to do that, just add the new methods and call them from the old methods and mark the old ones as deprecated. Maybe we need to do that in any case if you are changing the type hinting.
This should probably have some test coverage added?
This more-or-less copies the code used already in \Drupal\users_jwt\Authentication\Provider\UsersJwtAuth::authenticate()
here's a quick first patch, NR for testing
pwolanin → created an issue.
hestenet → credited pwolanin → .
hestenet → credited pwolanin → .