Skip to content

Required changes after updating Composer dependencies#39134

Merged
HLeithner merged 9 commits intojoomla:5.0-devfrom
nikosdion:feature/update-composer-deps
Nov 22, 2022
Merged

Required changes after updating Composer dependencies#39134
HLeithner merged 9 commits intojoomla:5.0-devfrom
nikosdion:feature/update-composer-deps

Conversation

@nikosdion
Copy link
Copy Markdown
Contributor

@nikosdion nikosdion commented Nov 2, 2022

Pull Request in continuation of gh-39123

Summary of Changes

  • Updated the method signature of \Joomla\CMS\Log\DelegatingPsrLogger::log to match psr/log version 3
  • Introduce \Joomla\CMS\WebAuthn\Server to abstract the common WebAuthn code and update implementation (system and multifactorauthentication plugins) with it.
  • Made the \Joomla\CMS\Log\DelegatingPsrLogger class final and internal.

Testing Instructions

  • Check out the 5.0-dev branch
  • 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.

Nicholas K. Dionysopoulos added 4 commits November 2, 2022 13:02
Update DelegatingPsrLogger.php after psr/log update
in gh-39123.
Introduce common WebAuthn server helper
Replace WebAuthn Server with our own.

The original Server class from the WebAuthn library was
removed in version 4 of the library.
Mark our custom server as internal
@HLeithner
Copy link
Copy Markdown
Member

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

@nikosdion
Copy link
Copy Markdown
Contributor Author

Done. Relevant PRs:

@richard67
Copy link
Copy Markdown
Member

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

@nikosdion
Copy link
Copy Markdown
Contributor Author

@richard67 Tabs came from a previous PR. No worries, I pushed a commit with spaces used for indentation.

@HLeithner
Copy link
Copy Markdown
Member

Hmm I got this error message with vivaldi and windows hello? (pin)
image

@HLeithner
Copy link
Copy Markdown
Member

I get the same error message with yubikey

@HLeithner
Copy link
Copy Markdown
Member

Same error with windows hello, yubi key on chrome beta and firefox and edge on windows 10

@nikosdion
Copy link
Copy Markdown
Contributor Author

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.

@HLeithner HLeithner merged commit ed4be02 into joomla:5.0-dev Nov 22, 2022
@HLeithner
Copy link
Copy Markdown
Member

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

richard67 added a commit to richard67/joomla-cms that referenced this pull request Mar 11, 2023
HLeithner pushed a commit that referenced this pull request Mar 11, 2023
…remove 4.x update SQL scripts (#40083)

* Init deleted files and folders for 5.0

* Add deleted files and folders from #38406

* Add deleted file from #38405

* Add deleted files and folders from #39134

* Init renamed files for 5.0

* Add deleted and renamed files and folders from dependency updates

* Fix alpha odering of deleted files

* Remove j4 update SQL scripts and add initial one for j5

* Use real DDL in update SQL script
@richard67 richard67 added this to the Joomla! 5.0 milestone Apr 4, 2023
@heelc29 heelc29 mentioned this pull request Jan 21, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants