Account created on 2 February 2021, about 3 years ago
#

Merge Requests

More

Recent comments

πŸ‡©πŸ‡ͺGermany Grevil

Ok, I fixed the first few pain points, this is ready to be partially reviewed. Now only the main module logic is left to be adjusted.

πŸ‡©πŸ‡ͺGermany Grevil

Ok, I still don't get the issue mentioned in #3 πŸ› Fix schema and other test issues Active . I disabled schema validation and now we get the error message from πŸ› Enabling the "billwerk_subscriptions_entities" throws an error and undocumentet d Active , so I'll postpone this issue on that one for now.

πŸ‡©πŸ‡ͺGermany Grevil

Kinda stuck here with the whole schema definition....

Drupal\Tests\billwerk_subscriptions\Functional\BillwerkSubscriptionsGenericTest::testModuleGenericIssues
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.action.billwerk_user_contract_ids_clear_action with the following errors: system.action.billwerk_user_contract_ids_clear_action:configuration missing schema

I doubt, that I need to define a system schema. Especially for an action which is basically untouched and doesn't have a "configuration" property.

UPDATE: Found it! Inside "config/optional/system.action.billwerk_subscriptions_fetch_and_assing_billwerk_contract_ids.yml", we had the following entry:

configuration:
  overwrite_existing: false

This doesn't make sense, as the action doesn't have any configuration.

πŸ‡©πŸ‡ͺGermany Grevil

Done, please check out the comment and test the changes live @Anybody!

πŸ‡©πŸ‡ͺGermany Grevil

Ignore last commit, it wasn't committed to 1.x anyway.

πŸ‡©πŸ‡ͺGermany Grevil

Alright, I just took a look at all the changes and made a few tiny adjustments. Also added a comment. Please resolve the comment @Anybody and give your final review!

πŸ‡©πŸ‡ͺGermany Grevil

Ok, this requires too many changes that need to be properly reviewed.

Let's resolve the schema errors in a follow-up issue. Please review!

πŸ‡©πŸ‡ͺGermany Grevil

We talked about this internally. Apparently it is wiser to invert this feature.

πŸ‡©πŸ‡ͺGermany Grevil

Thanks! Make sure, to always set the MR label manually. It doesn't get generated automatically according to the issue name / id anymore.

πŸ‡©πŸ‡ͺGermany Grevil

Captcha validation failed! ID: "solution_malformed" Message: "Malformed solution: Expected at least 3 parts, got 4.".

LGTM now.

πŸ‡©πŸ‡ͺGermany Grevil

Alright! Adjusted the code accordingly. :)
Also used @ instead of % for the placeholder replacements, as the tags are being escaped on the recent log message:

πŸ‡©πŸ‡ͺGermany Grevil

@jurgenhaas thanks for clearing this up! πŸ‘

Originally, I thought we should still check, that the array consists of exactly 4 parts, because in cases where it consists of only 3 parts the order of the array entries might be incorrect, leading to incorrect variable declarations, which might lead to other errors.
But one could think, that the friendlyCaptcha folks thought of that.
But since the original error with the 2 parameters instead of 4 puts me on the fence... Final thoughts @Anybody? Check for exactly 4 parts or at least 3?

πŸ‡©πŸ‡ͺGermany Grevil

Alright, all fixed and working as expected, please review!

πŸ‡©πŸ‡ͺGermany Grevil

@Anybody ok, I will create another follow-up issue for that. Let's keep this issue assigned to @jurgenhaas for the problem mentioned in #9 πŸ“Œ Adjust the logging for the local endpoint implementation Active

πŸ‡©πŸ‡ͺGermany Grevil

Hm, interesting. Our solutions array always consists of 4 parts, instead of the expected 3 parts. I already reinstalled the module and it still consists of 4 parts:

I mean our original code: [$signature, $puzzle, $solutions] = $solutionArray would simply ignore the 4th array entry, but I am still wondering what it is used for...

@jurgenhaas, any idea?

πŸ‡©πŸ‡ͺGermany Grevil

Something is wrong, wait a bit with the review.

πŸ‡©πŸ‡ͺGermany Grevil

And should we add an option to turn the noise off perhaps? ;)
Or the other way around: "Debug mode" with a matching description?

We could think of such a feature, I'd vote for having an option to turn off the noise. Not really a fan of the other way around, since I see the current error logging as necessary and something that you should deliberately have to turn off. But let's do that in another follow-up issue!

πŸ‡©πŸ‡ͺGermany Grevil

This is what I had envisioned, please review!

πŸ‡©πŸ‡ͺGermany Grevil

Not quite what I envisioned! Since we now still have the issue of duplicate logging. I'll show you what I envisioned here.

πŸ‡©πŸ‡ͺGermany Grevil

I would agree with @jurgenhaas, @Anybody should do the final decision here, merging...

Thanks everyone! :)

πŸ‡©πŸ‡ͺGermany Grevil

@jurgenhaas I agree! But I would rather clean that up in the follow-up issue! Hence, there are multiple critical log calls that should be info calls in my opinion:

Line 179:

$this->logger->critical('Signature mismatch. Calculated "%calculated", given "%given"', [</li>
        '%calculated' => $calculated,
        '%given' => $signature,
      ]);

Line 187:

$this->logger->critical('Puzzle "%puzzle" was already successfully used before', ['%puzzle' => $puzzleHex]);

Line 198:

      $this->logger->critical('Puzzle is too old (%age seconds, allowed: %expiry', [
        '%age' => $age,
        '%expiry' => $expiry,
      ]);

Line 212:

$this->logger->critical('Solution seen in this request before');

Line 221:

$this->logger->critical($currentSolution . ' (index: ' . $solutionIndex . ') is invalid (' . $first4Int . ' >= ' . $t . ')');
πŸ‡©πŸ‡ͺGermany Grevil

Alright, the issue should be fixed now!

I also just realized another few pain points concerning the logging:

  • We always log any failures as critical, "info" should be enough, as we don't want a bunch of critical log messages inside the log, just because our site is being attacked by bots.
  • We are logging inside verify() and later on again in friendlycaptcha_captcha_validation(). Instead, let's pass a logging message inside returnSolutionInvalid, returnSolutionTimeoutOrDuplicate, returnErrorEmptySolution and returnMalformedSolution to returnResponse, so our returnResponse would look like this: private static function returnResponse(bool $success, string $errorId = NULL, string $error = NULL). Our original "$error" in returnResponse is already an errorId anyway, this way everything is way cleaner.

I'll create a follow up issue for that.

πŸ‡©πŸ‡ͺGermany Grevil

Yea, I can not reproduce this issue either. If we have JS disabled, The captcha will automatically always fail and a text is displayed to the user, to disable Javascript:

You need Javascript for CAPTCHA verification to submit this form.
This question is for testing whether or not you are a human visitor and to prevent automated spam submissions.

I guess this is caused by some kind of bot, or someone trying to break through the captcha and brute force a login.

But we should fix it nonetheless. On it.

πŸ‡©πŸ‡ͺGermany Grevil

Through 3.x a lot of the UI has to change anyway, since the introduction of the notification "objects" will make a lot of the old configs irrelevant.

Let's also use a dedicated watchdog_mailer page as well, because we as a user you always need to know, that this module hooks into the "Logging and errors" form. For me personally, I always use the Admin Toolbar quick search feature to quickly look at the routes a module provides, but since this module has its own dedicated form, the form won't show up when searching for "watchdog_mailer". And it is also not listed as a sub-menu / sub-tab of "Logging and errors".

Let's fix everything in 🌱 [Meta] 3.x Feature / Concept Improvement issue Active .

πŸ‡©πŸ‡ͺGermany Grevil

Should we close this then in favor of a 3.0.0 release?

πŸ‡©πŸ‡ͺGermany Grevil

Just had another look with @Anybody. To be precise, we need to check "hasUid()" in the blockedIp() method in "web/core/modules/user/src/EventSubscriber/UserFloodSubscriber.php" and add the user id, if it exists. Afterward, we need to move the account and identifier logic up above the "user.failed_login_ip" isAllowed event call in "validateAuthentication()" of "web/core/modules/user/src/Form/UserLoginForm.php".

And adjust tests accordingly.

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ changed the visibility of the branch 3441424-include-username-tried to hidden.

πŸ‡©πŸ‡ͺGermany Grevil

This is already a feature of the core flood control but is triggered by the "user_limit" config instead of the "ip_limit" config

user_limit:
The allowed number of failed login attempts with one username within the allowed time window.

If this limit is reached, the "UserEvents::FLOOD_BLOCKED_USER" event is triggered and the UserFloodSubscriber will log the User as well:

$this->logger->notice('Flood control blocked login attempt for uid %uid from %ip', ['%uid' => $uid, '%ip' => $ip]);

The problem is, that if the "ip_limit" is reached, the code returns early and the handling for the "user_limit" is skipped entirely. Furthermore, from what I understand, the "ip_limit" is more easily reached, as the login attempts will not take the username into account.

But yea, in general the logging should already provide the user ID, I'll do some internal testing.

πŸ‡©πŸ‡ͺGermany Grevil

Fixed the issue! Simple logic error. Please review!

πŸ‡©πŸ‡ͺGermany Grevil

Yes, this is easily recreated on a brand new install.

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ made their first commit to this issue’s fork.

πŸ‡©πŸ‡ͺGermany Grevil

Alright, this issue can be reviewed. There are a few dangerous, but necessary changes here, we need to manually test them first, before merging.

The eslint issue needs to be fixed in a separate issue, they are related to the empty config file (which shouldn't be empty of course), but not related to this issue. I will quickly fix the remaining cspell issues tomorrow and do some quick manually tests for the form_state "getValues()" changes and then we can commit this. Codewise, this can already get reviewed!

I'd say this will be the last commit for 1.x any further work for refactoring, etc. should be done in 2.x. (Note that we also need to cherry-pick this into 2.x once finished).

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ made their first commit to this issue’s fork.

πŸ‡©πŸ‡ͺGermany Grevil

Everything green now!

As discussed internally with @Anybody, we will also implement the tests inside this issue here. :)

πŸ‡©πŸ‡ͺGermany Grevil

As discussed internally with @Anybody, we will implement this through πŸ“Œ Final code style cleanup Needs review .

πŸ‡©πŸ‡ͺGermany Grevil

Couldn't find a proper solution to this, so I quickly added a composer.json myself.

Please review!

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ made their first commit to this issue’s fork.

πŸ‡©πŸ‡ͺGermany Grevil

I think the UI is good! The only point against "checkboxes", would be that we disable the module functionality through unchecking both of these. But I guess, there is a use case hidden in that feature as well! :P

So yea, LGTM! And code looks good as well! I'll quickly add schema and install file entries, create an update hook, and add / refactor (existing) tests!

πŸ‡©πŸ‡ͺGermany Grevil

Works great! Made some minor adjustments to the code, I decided against an entry in the config install file (for now) as not a single config value is set there, and I don't want to make unrelated fixes here.

As commented in code, this module needs quite a bit of work anyway. Otherwise, this LGTM! And works as expected! Please do a final review of my changes @Anybody and then, we can set this RTBC.

PS: Already tested the update hook. Also works as expected.

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ made their first commit to this issue’s fork.

πŸ‡©πŸ‡ͺGermany Grevil

It might also make sense to log the user, if the error appears, but since captcha is often used for login, we might get a lot of anonymous user logged, but it is still a possibility, while we are at it!

πŸ‡©πŸ‡ͺGermany Grevil

Fixed! And tested with both the local and global endpoint, on both successful and unsuccessful result, as well as invalid API key.

Works great! :)
We are currently constructing the error message, even if the result is successful, this could be seen as a point of improvement, but this way, we don't need to create another if else case after the successful result check.

Please review!

πŸ‡©πŸ‡ͺGermany Grevil

Alright, all done.

Eslint, stylelint and cspell are still open, but I fixed phpstan and phpcs issues.

πŸ‡©πŸ‡ͺGermany Grevil

Jesus Christ, 700 phpcs issues? What happened here?

πŸ‡©πŸ‡ͺGermany Grevil

Grevil β†’ made their first commit to this issue’s fork.

πŸ‡©πŸ‡ͺGermany Grevil

Very simple, straight forward changes. Merging them, so I can begin with the code style fixes.

Letting this issue on "Needs review", as if @Anybody has anything he doesn't agree on, we can push a follow up commit.

πŸ‡©πŸ‡ͺGermany Grevil

I am just seeing, that we are using global defaults as a fallback... Meaning that this issue isn't fixable... The current implementation is as best as it gets. We can not implement this like in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.0.x/ui_suite_d..., since this way we only have either TRUE or FALSE using !empty(). But we want TRUE / FALSE / NULL (for the global fallback). I don't see any way to fix this. The only thing is, that we create a core issue to let drupal core's "Drupal\Core\Discovery\YamlDiscovery->decode()" method allow non-string keys, as the yml format originally supports that. But, I doubt, that they will do anything about that.

Let's use this issue to create a schema in the future. Setting this to minor, as it doesn't have any big impact on the code rn.

πŸ‡©πŸ‡ͺGermany Grevil

I am just seeing, that we are using global defaults as a fallback... Meaning that this issue isn't fixable... The current implementation is as best as it gets. We can not implement this like in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/1.0.x/ui_suite_d..., since this way we only have either TRUE or FALSE using !empty(). But we want TRUE / FALSE / NULL (for the global fallback). I don't see any way to fix this. The only thing is, that we create a core issue to let drupal core's "Drupal\Core\Discovery\YamlDiscovery->decode()" method allow non-string keys, as the yml format originally supports that. But, I doubt, that they will do anything about that.

Let's use this issue to create a schema in the future. Setting this to minor, as it doesn't have any big impact on the code rn.

Production build https://api.contrib.social 0.62.1