@seeduardo

@cmlara: Many thanks for progressing the issue and apologies for late response - just no time recently to work on this properly, but agree with you on the general point about greater backwards compatibility and the change you've made. Anything further needed to move this forward and change the status?

Ok thanks. To clarify further to anyone else trying to follow this too, I'll try to explain further below, where for ease of reading OTP = christian-riesen/otp and CTE = paragonie/constant_time_encoding:

I think what @cmlara was trying to tell me is that even though TFA on the 8.x-1.x branch is currently pulling in OTP version 2.1, from version 2.4 OTP itself requires the same package my change was about, namely CTE. However, I was trying to get TFA to use version 3.0 of CTE, whereas OTP only specifies to use version 1.0 or 2.0, not any other. Even though version 3.0 of CTE solved the original problem for me and didn't cause any clashes right now, it would prevent the use of OTP from version 2.4 onwards in the future - so I simply downgraded the version of CTE required by TFA to 2.0 and updated the merge request, then retested - all was well with that on my end and I therefore hope that's right overall.

Ok many thanks for that @cmlara, makes sense - though what doesn't is that without the change I suggested (ie// adding in the ParagonIE dependency here directly), we get the abovementioned error in our system, where christian-riesen/otp ISN'T pulling in the ParagonIE dependency and we have to require it in our project-wide composer.json to proceed past login. Is it just because of the specific version number I stipulated? Is that what you mean by 'compatible'? Or something else? I'd be happy to give this more time if I can but any clues where/how to look most certainly welcome.

Many thanks @cmlara, I actually have already created issue #3498141 🐛 ParagonIE\ConstantTime\Encoding dependency not installed from TFA Needs review - I thought that by raising the merge request here, it would reopen the issue, but then realized only module maintainers can do that, and didn't have the time to create the new issue until now. Thanks anyway for your comment and I don't suppose you have any input on the matter, particularly whether the change is correct or why the issue appears to have gone undetected (if even worthy of note now) for 7+ years?

I guess now that the link to the new issue is in this comment and the link to this issue is in the description of the new one, I will stop posting here - apologies if I've created any unnecessary noise for anyone up till now.

seeduardo → created an issue.

We were experiencing the following error on our D10.3.10 site with TFA on 8.x-1.9 (full stack trace to follow in image at bottom):

Error: Class "ParagonIE\ConstantTime\Encoding" not found in Drupal\tfa\Plugin\TfaValidation\TfaTotpValidation->validate() (line 316 of modules/contrib/tfa/src/Plugin/TfaValidation/TfaTotpValidation.php).

We fixed it by requiring 'paragonie/constant_time_encoding' on the whole project, but really it seems to me from reading through this issue and the other related issues sprouting from/to it (principally TFA issue #2866841 → and Google Authenticator issue #2880601 → ) that said dependency should be brought into the codebase just by TFA itself. Hence, I added it to the composer.json and raised this merge request. I've tested it locally and it works, though I accept there may be some reason it was excluded or some other better solution - please do let me know if so.

Finally, I did and still do wonder why nobody else has reported the problem we had, that I could find anyway, especially seeing as this closed issue is so many years old now. My only guess so far is that all other projects in similar circumstances already have something else in their codebase pulling in the dependency. Any other ideas? Apologies if I've missed something glaringly obvious and many thanks in advance for any assistance or further info.

Full error:

seeduardo → made their first commit to this issue’s fork.

While I can confirm the code in #5 does work on D10.3, it is simply a repetition of what's in the merge request mentioned in #3. I agree with dqd in #4 that the change could be rewritten, but the code in that comment isn't quite right and would be a breaking change, so I've tweaked it in the patch attached here, which does work for me.

seeduardo → created an issue.

This has been tested by running the SG API module as it already was on a local site and then loading the changes onto the same site to compare - all looks good.

Tested and all good.

Ok appears the solution is robust enough, thanks so much for all testing and comments and others who helped with this - merging and closing now.

Ready for review, thanks anyone and everyone.

seeduardo → created an issue.

Looks all good, thanks @arcaic.

Many thanks for your comments @vishal.kadam. You will note as at comment #17 here, that PHPCS has been stringently run on the Site Guardian code, and according to stricter standards than normally required. None of the issues you mention have ever come to light that way, and running the sniffer again just now, they still don't. Nevertheless, I agree that the class constructor docblocks you mention are contemporarily extraneous, and not necessary in our case anyway, so I have removed them altogether.

However, I was more confused by your comment that 'Function and method declarations are written on a single line.' Are you still referring to the class constructors from which you pasted code? If so, I'd have to say that the coding standards are more ambiguous on this matter than you are.

For instance, I could stick all the arguments for the SiteGuardianController constructor (as per your second code block example) onto one line, but then PHPCS would indeed throw up an error of exceeding the 80 character limit. The coding standards on function declarations → actually don't mention the single line issue you're talking about at all, and whilst it is true that the standards section on line length → does nominally permit longer function definitions to exceed 80 characters, it doesn't say they should or have to - it is left to interpretation.

Above, in comment #16 here, @apaderno is far more stringent than this: while Drupal.org's wording states "In general, all lines of code should not be longer than 80 characters.", he instead, in this very project application, writes that "the Drupal coding standards say text lines must not exceed 80 characters, so longer lines is bad, not good" (my emphasis added), which would certainly contradict what I interpret as your somewhat tacit prescription of an exceedingly long class constructor declaration with all its arguments on a single line.

Indeed, you'll note that there are instances in Core itself where function declarations are not just spread across multiple lines but also decidedly more convoluted than mine, for example in the AttributeClassDiscovery plugin, whose constructor declaration (at time of writing) starts at line 45 but runs down to line 49, and it's arguable that perhaps it's best that it does.

For all these reasons, and above all else for what I see as plain old readability's sake, I'm electing to leave any multi-line constructor declarations mentioned in your comments just as they are.

Many thanks once again @vinaymahale and @apaderno - I have now made changes to the project in line with recommendations, as well as a couple of related/corollary changes.

Thanks @arcaic, I've tested this now and it all looks good. Merging forthwith.

Thanks @arcaic, I've tested this now and it all looks good. Merging forthwith.

Just reporting that the patch at comment #2 fixes the generic incompatibility complaint on a site's D10 Upgrade Status page:

Please merge when you can, many thanks in advance.

Just reporting that the patch at comment #2 fixes the generic incompatibility complaint on a site's D10 Upgrade Status page:

Please merge when you can, many thanks in advance.

Thank you all @vinaymahale, @dunx and @apaderno. The PHPCS issues reported in this thread have now been addressed, as well as others noted by using more stringent set of standards (interesting exercise to compare these).

All small things, but important and reasonable. Looks good to me, thanks @the_g_bomb, merging.

I can confirm that the patch at #101 is working correctly for me on Drupal 9.5.10, without the issues mentioned in #106 - thanks for that, and see below when Body field set to 5 rows, for an example:

All changes (including those being "considered" in the description above) have now been included in Site Guardian 1.0.1. Thanks @i-trokhanenko and to all who contributed.

@apaderno, @Rassoni, @daceej - thanks to you all for your comments. The Site Guardian team raised tickets for them all and we have now been through the process of making all the changes necessary, please do refer to the SG issue queue as required. I therefore submit this security advisory coverage application for further review here once again, now that the above issues have been addressed. Many thanks in advance!

Looks fine to me too, thanks both - merged.

The change in patch at #2 has been included with a raft of other D10 changes elsewhere, so not merging here but leaving the issue open so Drupal Rector can continue to do its thing.

I would like to politely suggest that this issue please be reopened.

The changes made in this issue, merged into version 3.4.0, meant that when we upgraded from version 3.3.2, while Admin users were totally unaffected, non-Admin users could no longer log into the site, which was in fact broken for them at that point ("The website encountered an unexpected error."). The logs show that this was due to a PHP error:

UnexpectedValueException: base:devel/taxonomy_vocabulary/<vocabulary-name> has no corresponding route. in Drupal\Core\Url->getRouteName() (line 567 of /core/lib/Drupal/Core/Url.php).

Indeed, this error didn't occur on versions of the site where the Devel module was already enabled, and cycling between enabled and disabled on any server in pre-production essentially served as a switch to turn the above error off and on at will. However, as we don't want Devel enabled everywhere for performance (and other) reasons, this was only a temporary solution.

I narrowed the problem down further to the access check being done in the lines surrounding the diff on this merged issue, and simply reversed the changes therein - the problem is now gone for us whether Devel is enabled or not, but of course, this is at the expense of the desired outcome of the entire issue raised here. This doesn't seem to affect us for now but is obviously of relevance to you and perhaps to other people using the module, suggesting the main issue here has not really been fully tested and/or resolved yet.

Thanks so much for this useful input @i-trokhanenko - I would tend to agree, but we can discuss further first if you like @the_g_bomb.

I tried this out myself and looks good. Hitting an incorrect endpoint 10 times means even if the 11th attempt is correct, you still get 'Access denied'. Conversely, alternating correct and incorrect endpoints doesn't activate Flood even after 10 incorrect attempts (ie// 20 attempts altogether), which is as it should be - it's only 10 incorrect ones in a row in a short space of time that would be suspicious/problematic. All of this is corroborated in the logs. Cheers @the_g_bomb.

How did you get on with your testing @i-trokhanenko? Please let us know and I will look at merging.

Thanks @the_g_bomb and @i-trokhanenko, also reviewed and tested and looks good to me as well. Committing forthwith.

Minor changes also checked, thanks @i-trokhanenko! Merging....

Code reviewed, spun up again on simplytest.me and then phpcs run on for again, with success, thanks @the_g_bomb!

Also checked with simplytest.me, code looks good too, thanks both.

seeduardo → made their first commit to this issue’s fork.

@apaderno - many thanks for your comments and assessments, and apologies that it's taken so long to get back to you, but we just wanted to let you know that we are working through the recommendations you made. Hopefully they will all be done soon!

@TheodorosPloumis - closing this issue now due to inactivity, please do get in touch should you want to revisit.

@TheodorosPloumis, many thanks for your useful comments and suggestions - indeed, I have opted for your "better" option and added endpoints to their own page, the latter of which is reachable from the Site Guardian settings page or from the main menu; please see screenshots of the new page and how to get to it. I hope this adequately addresses your needs/concerns, do let us know. Marking as "Needs Review" for your perusal (I also changed the title of the issue to reflect its contents more accurately).

Endpoints page

How to get there

Thanks to you both, do @the_g_bomb 's comments answer your concersn @tijsdeboeck? Moving to 'Needs Review' for you to check.