Adjust the logging for the local endpoint implementation

Created on 18 April 2024, 8 months ago
Updated 3 May 2024, 8 months ago

Problem/Motivation

There are a few pain points with the current logging for the local endpoint, which should be resolved:

  • 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, so we can log only once at the end of the chain.

Our returnResponse method would look like this then: private static function returnResponse(bool $success, string $errorId = NULL, string $error = NULL). Our original "$error" in returnResponse is already an errorId anyway, meaning the new logic would also help to make the final logging message easier to understand.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Fixed

Version

1.0

Component

Code

Created by

🇩🇪Germany Grevil

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Grevil
  • First commit to issue fork.
  • Assigned to Grevil
  • Status changed to Needs review 8 months ago
  • 🇩🇪Germany Anybody Porta Westfalica

    Here we go. But it might make sense to change the texts a bit so that the reader (admin) better understands the context and meanings?

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

    What do you think? (Could also be done in a follow-up to get this done.)

  • Status changed to Needs work 8 months ago
  • 🇩🇪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.

  • Issue was unassigned.
  • Status changed to Needs review 8 months ago
  • 🇩🇪Germany Grevil

    This is what I had envisioned, please 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!

  • Status changed to Needs work 8 months ago
  • 🇩🇪Germany Grevil

    Something is wrong, wait a bit with the review.

  • Assigned to jurgenhaas
  • 🇩🇪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?

  • Assigned to Grevil
  • 🇩🇪Germany Anybody Porta Westfalica

    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!

    Correct, if these are errors / notices the admin can do something about. If not, it should be off by default. And that was what I expected here.

  • Assigned to jurgenhaas
  • Status changed to Needs review 8 months ago
  • 🇩🇪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 jurgenhaas Gottmadingen

    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...

    That verify code comes from the friendly captcha folks, it's a clone of their own server. So, this is how they implemented that. I guess, that 4th argument is just relevant for that verify process.

    Here is the link to the original code: https://github.com/FriendlyCaptcha/friendly-lite-server/blob/e93be67590a...

    And indeed, they don't use that 4th one anywhere.

    I'd suggest that we check for at least 3 parts in the array, not for exactly 3 parts.

  • Assigned to Anybody
  • 🇩🇪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 Anybody Porta Westfalica

    I think we should check for the three required parameters ("at least three"), but add a comment about the 4th parameter:
    "$diagnostics" which is unused. Perhaps linked to this discussion?

  • 🇩🇪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:

  • Issue was unassigned.
  • 🇩🇪Germany Anybody Porta Westfalica

    @Grevil: Nice!

    I left some comments. These are just meant to be placed as prefix in front of the message so it's easier for the admin to see what it's about!

    Also I'm sure my comment form #10 applies. The admin can't do anything about it, so the debug mode should be opt-in!

  • 🇩🇪Germany Grevil

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

    LGTM now.

  • 🇩🇪Germany Grevil

    Here is the follow-up issue concerning #10: 📌 Create an option to turn off any logging from this module Active .

  • 🇩🇪Germany Anybody Porta Westfalica

    Nice! RTBC!

    If @jurgenhaas has comments on the commit, let's solve them in a follow-up.

  • Status changed to Fixed 8 months ago
  • 🇩🇪Germany Anybody Porta Westfalica
  • Pipeline finished with Skipped
    8 months ago
    #150911
    • Anybody committed 8cb00f99 on 1.x
      Issue #3441795 by Grevil, Anybody, jurgenhaas: Adjust the logging for...
  • 🇩🇪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.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024