[#39134] - Required changes after updating Composer dependencies
- Fixed in Code Base
- 22 Nov 2022
- Medium
- Build: 4.2-dev
- # 39134
- nikosdion:feature/update-composer-deps
Pull Request in continuation of gh-39123
Summary of Changes
- Updated the method signature of
\Joomla\CMS\Log\DelegatingPsrLogger::logto matchpsr/logversion 3 - Introduce
\Joomla\CMS\WebAuthn\Serverto abstract the common WebAuthn code and update implementation (system and multifactorauthentication plugins) with it. - Made the
\Joomla\CMS\Log\DelegatingPsrLoggerclass final and internal.
Testing Instructions
- Check out the
5.0-devbranch - Run
npm ci - Run
composer install - Try to install and use a Joomla 5.0-dev site on PHP 8.1
Actual result BEFORE applying this Pull Request
- You cannot access the site due to a PHP error about the declaration of
\Joomla\CMS\Log\DelegatingPsrLogger::log - If you patch the previous problem, Web Authentication does not work either for logging in or for Multi-factor Authentication
Expected result AFTER applying this Pull Request
- The PHP error is gone
- You can use Web Authentication for logging in and for Multi-factor Authentication
Link to documentations
Please select:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org: PR 62
Technical details
BACKWARDS INCOMPATIBLE CHANGE (PRE-EXISTING TO THIS PR). psr/log version 3 has changed some signatures, most importantly that of the log method. Any third party extension implementing a PSR logger will break following the update of PSR-3 in Joomla. I can tell you upfront that this already affects my own software and I have to fork psr/log v1 for use in my extensions. Noted in manual PR 62.
Fixing that would require forking the WebAuthn library, as I had said in #38209 (comment). This is the exact problem described in https://medium.com/@davert/why-i-dont-enjoy-writing-php-anymore-aee8a85ca8aa So, no matter what we do we will have a major problem: either we break b/c in software which is (correctly) using PSR-3 for logging, or we have to maintain a fork of a third party library. This is a fundamental problem of PHP which I had foreseen circa 2015, when PHP 7 introduced major b/c breaks.
Another workaround is to change the namespace of specific dependencies and their dependencies as I have described in https://www.dionysopoulos.me/book/advice-composer.html under “Dealing with namespace clashes and older dependency versions included with Joomla”. However, this is essentially the same as maintaining a fork with the asterisk that the fork is an automated process. The big caveat is we'd be relying on Yet Another Third Party Software (PHP-Scoper) and introduce one more convoluted step in the build process with everything that entails. The phrase "damned if you, damned if you don't" comes to mind.
Why did we introduce a WebAuthn Server class? The WebAuthn library decided to remove the Server class for reasons unknown. This would have led us to duplication of very convoluted code with a very high chance of introducing bugs now and in the future. Instead, I chose to reimplement the Server class (most of its code was already overridden in Joomla 4.2 with our custom code anyway) as a CMS library class and mark it BOTH final AND @internal to indicate that it is not part of the public Joomla API which is bound by the Joomla b/c promise. The only thing we can promise about this class is that it will change when we upgrade the WebAuthn library to version 5.
| Labels |
Added:
PR-5.0-dev
|
||
joomla-cms-bot
- | Category | ⇒ | Libraries Front End Plugins |
Done. Relevant PRs:
- Joomla 4.3, mark internal and add deprecation notice #39224
- Manual PR for Joomla 4.3 deprecation joomla/Manual#61
- Manual PR for Joomla 5.0 b/c break joomla/Manual#62
@nikosdion Could you fix the code style errors reported by drone? Currently they fail, and so other CI tests are not run. The errors are all about tabs being used in the Server.php file, but spaces should be used.
FILE: /********/src/libraries/src/WebAuthn/Server.php
----------------------------------------------------------------------
FOUND 19 ERRORS AFFECTING 19 LINES
----------------------------------------------------------------------
235 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
236 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
245 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
246 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
247 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
248 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
306 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
307 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
308 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
309 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
384 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
385 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
386 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
387 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
391 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
392 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
393 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
394 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
395 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 19 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
@richard67 Tabs came from a previous PR. No worries, I pushed a commit with spaces used for indentation.
Hmm I got this error message with vivaldi and windows hello? (pin)
I get the same error message with yubikey
Same error with windows hello, yubi key on chrome beta and firefox and edge on windows 10
Pull the changes I just made; it should work correctly now.
The new version of the WebAuthn library expects the authenticator attestation response data to be Base64-encoded without padding (no equals signs at the end), however JavaScript always adds padding. I had to add some server-side code to rectify this issue.
| Status | New | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-11-22 13:33:51 |
| Closed_By | ⇒ | HLeithner |
Thank I'm merging this for now since we need a working 5.0-dev branch. Hopefully we get some feedback about the direct usage of 3rd party libraries requiring PSR 3 v1 and v3 logger...
Thanks @nikosdion we need a documentation update for the deprecation/update/bc on manual.joomla.org and a deprecation warning for Joomla 4.3...
as discussed please change the DelegatingPsrLogger to final, no reason to extend this logger
Can you create the PR for 4.3 (add the deprecation and mark it internal) and I would add the deprecation / bc entry at https://manual.joomla.org/migrations/42-43/new-deprecations and https://manual.joomla.org/migrations/44-50/removed-backward-incompatibility