I've looked into the Redis module documentation and then also went through the CrowdSec library code. It looks like the socket connection should work ootb.
So defining this
// New Hetzner redis via UI:
$settings['redis.connection']['host'] = '/run/redis_me/redis.sock'; // Your Redis instance hostname.
$settings['redis.connection']['port'] = 0; // Redis port
as you've done above is according to the readme of the redis module. And that's exactly what the \CrowdSec\RemediationEngine\CacheStorage\Redis class from CrowdSec requires. I tested their code and it works just fine.
The error you're getting is Redis connection failed: No such file or directory according to your IP and that suggests that the given file /run/redis_me/redis.sock doesn't exist.
I'd say, regarding the code, that everything is ready to go, it probably only needs correct settings.
That's looking pretty good. I haven't tested it on a site, but the code is LGTM.
Can you please turn this into an MR?
Also, this raises a question: should we instead update the weight of that library in the Gin theme rather than altering it after the event?
The CrowdSec handler is implemented as middleware in order to keep the overheads low and block early. That's the whole purpose of this, otherwise DOSing a site would still be too simple. The DB should be available at that point, but querying it at that point is against the purpose.
I just meant to not handle that case for the "ignored" IPs in any way through crowdsec. So it should never assume that as "OK" crowdsec just should not handle these requests at all.
That's exactly what I'm referring to. My point is, why should your internal IP cause 4xx responses? Or why should your API requests cause 4xx responses? They shouldn't. And if they don't, the absence of an allow list will not be an issue.
Just realized that the ViewsBulkOperationsBulkForm class is also final but needs to be extended by eca_vbo. I've updated the MR to remove that as well.
The module ECA VBO → is crashing sites because of this change as well.
I guess you could make the constructor final instead of the class, that should also satisfy PHPStan.
Simple question: Why should we assume that it's OK that users coming from your internal IP or the API clients generate requests with 4xx responses?
And even if that did happen accidentally, a drush sql:query "DELETE FROM ban_ip WHERE ip='1.2.3.4" would quickly recover you from that.
@michaellander I was just about to say something similar. However, as @yautja_cetanu mentioned in #6 there is of course that option in the graphical UIs to have only the selected tools being listed. And that approach comes with another benefit: it allows to use the same tool multiple times with different configuration for each. That relates to ✨ Make it possible to create tool instances Active .
But you're right, that's not exclusive to graphical UIs, it can also been achieved in form based configurations.
Yeah, like I mentioned in #10 it needs somebody with a socket based redis installation to find out the proper DSN.
So this is an issue with documented usage of Hetzner Sockets for anyone.
That's kind of surprising, as we host with Hetzner as well, and we installed Redis there with TCP based connections that are only allowed locally. So, sockets are not a must.
@anybody that's an interesting thought. I don't know this module well enough to comment on it, but I trust your assessment. So, maybe there's a way to nudge existing users of perimeter to also switch over?
I've cleaned up settings, sticky action button, and content form determination. As part of that I found 3 cases of contrib modules being covered. That's being removed and issue opened for each of them explaining what they should be doing to prepare for this.
Why not improving the modules as suggested so that automated workflows work without manual intervention by the user?
The setting for the experimental sticky buttons had already been removed. I've now also removed the remaining code that relied on that setting. The action buttons are now always handled such that they appear in the top bar, but with the proven implementation that we had in Gin contrib before. The suggested changes above would be far too disruptive.
The 6.x branch has been created and worked on for the last few months. The 3.x branch will be marked unsupported after DrupalCon Vienna.
And for open issues, we will focus on 6.x and the core version of Gin. Once that's completed, issues in the older branches can be tested again with the merged Gin release. The expectations are that most of the issue will have gone by that time.
The signalling by other modules has now been implemented in the MR of ✨ More signals: encourage other modules to integrate with crowdsec Active and is waiting for a review.
When it comes to displaying a captcha instead of blocking an entire IP, I thought about it again and realized that the CrowdSec module doesn't really to the blocking, it's the ban module. And that's where the captcha display should be added as a new feature, i.e. instead of displaying a message that the IP is blocked, a captcha should be displayed, and if the user successfully solves that, the IP gets removed from the ban list.
As the ban module will be moved to contrib and out of core, there's actually a good chance of getting this addressed.
As for the roadmap of this module's 1.2 release, I'm closing this issue as everything is completed except for the review of the signalling issue.
Closing as I assume there's no follow up. Feel free to re-open if that's not the case.
Closing this as it seems to be caused by another module.
Is there anything else we need to address?
Closing this as explained above. Please re-open if there's a reason that we need to re-evaluate this.
This is now implemented, and the 3 scenarios supported by the module have also been converted to plugins. They can be found in src/Plugin/CrowdsecScenario.
To integrate with this API, other modules can now simply provide a plugin like this:
<?php
declare(strict_types=1);
namespace Drupal\my_module\Plugin\CrowdsecScenario;
use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\crowdsec\Attribute\Scenario;
use Drupal\crowdsec\ScenarioPluginBase;
/**
* Plugin implementation of the SOMETHING scenario.
*/
#[Scenario(
id: 'something',
scenario: 'drupal/something',
label: new TranslatableMarkup('Bans from something'),
description: new TranslatableMarkup('Describe the purpose of this plugin.'),
)]
final class Something extends ScenarioPluginBase {}
To add a signal for this scenario, you only need to call ScenarioPluginManager::getPlugin('something')->addSignal('1.2.3.4', 403, $targetUser);, where 1.2.3.4 is the IP address, 403 the response code, and the optional $targetUser is the user ID if this is a block for a specific user on that IP, or NULL otherwise.
The crowdsec module takes care of buffering signals automatically. However, if a module bans IPs themselves and doesn't want to use the buffering, use the buffer: FALSE attribute for your plugin. In that case, a signal will be pushed upstream directly without buffering.
I'd love to opt in for the modules that we have massive planning ahead of us following the DriesNote:
- eca
- modeler_api
- bpmn_io
- orchestration
I've now started the architecture on this and we should implement this signalling by other parties with plugins, not events. The reason being that implementing a plugin is very easy for the other module maintainers but even more so, it provides us with an important extra benefit:
When collecting bad behaviour by the "Whisper" scenario (4xx and 5xx responses), we have some configuration that determines under what circumstances that behaviour is being signalled upstream:
We do need similar configuration for each additional implementation as well, so that they can be configured separately. E.g. for the captcha module, that could be setup such that only if a user fails with a captcha more than 5 times within 2 minutes, then this should be signalled, and the site owner can also configure if and for how long such IPs should be banned locally.
@rr404 we currently have 3 signal scenarios in the module:
drupal/core-ban: Drupal bans from administrators
drupal/auth-bruteforce: Drupal bans from flood control
drupal/4xx-scan: Drupal bans from whispers
Can we introduce new ones ourselves, like e.g. drupal/captcha-ban: Drupal ban from captches, or do you have to provide them first upstream at the CrowdSec backend level?
Great find and fix, thanks for your contribution @vetchneons
This is being merged.
Yes, ECA UI is enabled by the recipe. But I just realized that we should make modeler_api a dependency of ECA, not just of ECA UI. It is already required by composer.json, but ECA does not yet require it to be enabled. That should be added, and then the container rebuild should also be called early enough, I guess.
When using two fields I was actually following what has been done in similar forms to keep it consistent.
Not sure about that. E.g. if you go into the AI Agent Explorer, there is only a single drop down to select the model, even if there are multiple providers.
Would it not be possible for the same model to be offered by two providers (something similar to ollama) ?
In the AI Agent Explorer, the options are build by the method $this->providerManager->getSimpleProviderModelOptions() and that contains item keys that are derived from a provider-model combination. I think you can simply re-use that and don't even have to develop anything yourself.
We haven't thought about that before.
In which way are DMN Decision Tables different from BPMN and what problem should that solve in Drupal?
Maybe you can provide some use cases for better understanding.
I've tested the latest version of the MR again with the Gin 6.x branch, and all still works as expected. Leaving it at NR as I haven't reviewed the code.
For some reason, some previously removed code in Agent.php:322 came back:
// Load the agent entity if possible.
if ($modelId && $plugin->getPluginId() == $modelId) {
$plugin_agent = $this->entityTypeManager->getStorage('ai_agent')->load($modelId);
if ($plugin_agent) {
$config['provider'] = $plugin_agent->get('provider') ?? NULL;
$config['model'] = $plugin_agent->get('model') ?? NULL;
$config['provider_model_config'] = $plugin_agent->get('provider_model_config') ?? NULL;
}
}
That breaks the functionality and needs to be removed again. When I remove that again, then it works as expected.
When it comes to the 2-field approach, one for provider and one for model, it feels like overkill:
The provider name is a prefix of the model already, so I don't see the benefit of an extra provider field. Let's face it, how many providers would someone have available on a site at most? Maybe 3, probably 4. I'd say, this is still manageable in a single model field. I would remove the provider field, that simplifies your implementation so much.
And last but not least, I wonder why you haven't created a merge request yet? It would make review and collaboration a lot easier.
@voleger I just had another thought: you may want to give 💬 Missing third_party settings schema Active a try as rebuilding the container after enabling the module may be resolving your issue as well.
Tested by @glottus and reported success at https://www.drupal.org/project/bpmn_io/issues/3348716#comment-16302636 📌 Print a warning if the site is not using either Claro or Gin Active
Great that we found failing tests which was only because of changes of other components. I've fixed those and also configured a weekly test run of ECA 2.1 so that we get notified if something similar happens in the future.
The MR is now green and the code is the same as in the related issue ✨ Add migrate process plugin Active , so I'm merging this.
jurgenhaas → made their first commit to this issue’s fork.
The exception occurs in \Drupal\eca\EventSubscriber::onBeforeInitialExecution(), when ECA attempts to call $form_object->buildEntity($event->getForm(), $form_state).
Well, as you already stated, $form_object->buildEntity() is an official method for an entity form. It's nothing that ECA does here other than calling such an official API method. If that is broken due to a form from somewhere else, this needs to be addressed somewhere else, not in ECA. If we start to build workarounds for issues elsewhere, we will turn ECA into something that's unmaintainable. This may be inconvenient, but imagine what's the result of workarounds in 10 years from now. That ain't gonna work.
It's unlikely that we're going to merge this. There has been a lot of testing in different scenarios and the proposed - non-standard - way of operation for such an exceptional update with lots of new components is what worked everywhere, including Drupal CMS.
But if you want to use a modified version, then you can apply your MR as a patch for the updates you have to perform.
When you assign an issue to yourself, everyone else assumes that no help or response is required as you're working on it yourself. Is that what you intend?
If not, please have a look into the update instructions from v2 to v3 at
https://www.drupal.org/project/eca/releases/3.0.0 →
which explains that you need to run drush cr before running drush updatedb.
This certainly doesn't sound like a bug in ECA. What exactly is the exception being thrown? Please provide the exception message and a full stack trace.
Thanks for confirming the fix.
What do you mean by current version? This will go into the next patch release of 3.0, but it won't be back ported to version 2.
This is looking great, only one final (I guess) question left in the MR. And I also rebased the MR to fix the one failing test.
Back-porting as suggested in #9 will never happen, that's only going to be part of Gin in core.
But that raises a valid question: is this reproducible with 6.x and the given patch? Note, that also requires Drupal core 11.x-dev
Thank you @maxilein for writing up the instructions. Would you be interested in contributing that to the ECA Guide? If so, there is an issue for that at https://gitlab.lakedrops.com/drupal/documentation/eca/-/issues/69 and of you ping me on Drupal Slack, I could create an account for you to contribute there as well.
Thanks for testing the MR, I'm going to merge that.
As for the token name, I've created #3551750: Allow hash character as part of token names → as a follow-up issue. So we can close this one.
I don't understand where the idea of an end event chines from. And why should that be any special thing for that enqueue action, and how is that different from all the other hundreds of actions?
Also, there is no ui that allowed adding of end events, so why should we document to NOT use something that's not even there?
And another significant milestone: The PHP code of Gin 6.x has been cleaned up such that there is no procedural code and no includes any longer. Thanks to @nicxvan and his work over at 📌 Convert theme hooks to OOP Active to provide OO Hook support for themes. This is not yet merged into Drupal core 11.x, which is why Gin 6.x currently contains the current changes from that issue as a patch which should be applied to core before testing this.
This is great, thanks for reviewing. And also thanks to @nicxvan for working so hard on the Drupal core side to make all this even possible.
No end tasks. As I mentioned above, they are not a thing ion this context.
Well, we can't add a list of end events, as no such thing exists. And the UI doesn't even allow to add one, so it's even a mystery how that got even added to the model in the first place.
@sunlix confirmed on Slack that this is only an issue during a new site installation but not afterwards when all hooks have been executed.
So, let's try to fix that by setting the container_rebuild_required in the module's info file.
@sunlix can you please test this in your context?
The MR solves the problem described in #5 that the render:markup action doesn't work with a token, if that token just contains a scalar value. Maybe you can give that a try and confirm?
This is still a minor issue though. Perhaps the field validation could allow additional characters as long as they aren't in the "root" token name.
Can you provide an example for that please?
ECA doesn't support end events. Events are only supported for starting a flow.
What are you trying to achive by using an end event?
We've had this before several times, and opting out of token replacement is not a solution that would always work, as it's likely that you have strings that contain brackets and still need tokens to be replaced.
But the good news is that we found a solution for this in 💬 An option to enable tokens support Fixed .
Well, it's just about testing if the provided fix is actually solving your problem. To do that, you can either temporarily use the dev release, or you take the commit as a patch, apply that to the existing installation and find out if it works or not.