-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4.1] webauthn table accessibility #37464
Conversation
This pr adds the required table caption and column/row scope to the table. There are no visual changes and brings this table into line with all other admin tables. As webauthn requires https this PR cannot be tested on a site without https.
I have tested this item ✅ successfully on 724a7be This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37464. |
Would a test by review be sufficient, too? By review this PR looks right to me, but I don't have any Webauthn authenticator set up right now. |
@richard67 it would be ok for me as this is well established code |
Well established and the change is clear and easy to understand. |
P.S.: ... and I had tested similar PRs successfully in past. |
I have tested this item ✅ successfully on 724a7be This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37464. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37464. |
Thanks |
This has actually broken the Edit button in the WebAuthn management interface. Had any of you made even a cursory test you would have known. The problem is in line 114 of the The @brianteeman You have been complaining for a very long time that a. people contribute untested code and b. core committers push the merge button without thinking. In this case you did not test your change, you asked core committers to hit the merge button without having any tests at all and you did not even bother to ping me about this PR even though I am the designated code owner for WebAuthn as per This time y'all got lucky because I just so happened to stumble across it before it made it to production while I am refactoring the WebAuthn plugin for Joomla 4.2. The whole reason of the PR SOPs and code ownership is to avoid this trifecta of failures and to ensure that catching these issues happens long before merging and not because the code owner oh-just-so-happened to be refactoring the code after the breakage was merged into the repo! Please take the following corrective actions:
|
I just checked my branch and I had actually fixed the js - but stupidly only in /media and not build/media-source so it was not picked up by git. Mea culpa I assume that it doesnt need correcting as you have fixed it in #37673 regarding the codeowners file - its not going to work how you expect it to work. gh designed it so that the person named in the file woulod automatically receive notification BUT that only works if that user is also a member of the repo. |
No, it still needs fixing I'm afraid :( My PR is for Joomla 4.2 and not merged yet, your PR is merged and is for Joomla 4.1. If it's not fixed the next release of Joomla (4.1.3) will have a broken WebAuthn. If you could just copy over the same solution I have in my PR — which I made in a separate commit so you can do just that, by the way — it'd be very much appreciated. Regarding the CODEOWNERS file, ugh, well, at least having it complete would be a good start. If the owners of the |
That's this one here: 84b996a @brianteeman Will you make a PR? That would be great. |
yes i will make a pr later today when i finish testng the new pr for 4.2 yes you are correct about the purpose of codeowners - its a limitation of github that I brought up in their internals but got no traction. Your proposed solution might work @HLeithner is it possible you take a look |
I'm looking into the read only group and coordinate this with the other maintainers |
Thank you @HLeithner |
PR for my error #37676 |
joomla/joomla-cms#37115 + joomla/joomla-cms#37286 + (отдельно в 857dcac) joomla/joomla-cms#37464 + joomla/joomla-cms#36250 + joomla/joomla-cms#37527 + joomla/joomla-cms#37535 - (только для en-GB) joomla/joomla-cms#37559 + joomla/joomla-cms#37594 - (только для en-GB) joomla/joomla-cms#37588 + joomla/joomla-cms#37424 - (только для en-GB, у нас все в одном формате с другими расширениями) joomla/joomla-cms#37475 - (только для en-GB, у нас давно исправлено) joomla/joomla-cms#37564 + joomla/joomla-cms#37641 - (только для en-GB) joomla/joomla-cms#37657 + joomla/joomla-cms#37683 + joomla/joomla-cms#37666 + joomla/joomla-cms#37704 + joomla/joomla-cms#37689 + joomla/joomla-cms#37519 +
* Refactored WebAuthn plugin * Fix the WebAuthn management page which was broken in #37464 * Fix wrong `@since` doc tag * Fix docblock typo * Fix docblock typo * Fix docblock typo * Fix docblock typo * Fix docblock typo * Fix broken management interface * Make unnecessarily static method back into non-static * Replace static helper with injected object * Come on, commit the ENTIRE file! * Use the user factory * Fix error when going through the user factory * Fix: cannot add WebAuthn authenticator right back after deleting it * Remove useless switch branch * Remove useless exception * Display make and model of the authenticator, if possible * Add missing JWT signature algorithms * Fix copyright date * Fix for PHP 8 using FIDO keys and Android phones * Reactivate the tooltips after adding an authenticator * Option to disable the attestation support * The Windows Hello icon was invisible on white background * Attempt to fix Appveyor not having Sodium in the Windows build * Work around third party library bug... * Create events in a forwards-compatible manner * Concrete events * Fix event woes * Update plugins/system/webauthn/webauthn.xml Co-authored-by: Brian Teeman <[email protected]> * Update administrator/language/en-GB/plg_system_webauthn.ini Co-authored-by: Brian Teeman <[email protected]> * Improve the layout for editing an authenticator It now follows the Bootstrap 5 form aesthetic. Moreover, there are gaps between the text input and the Save and Cancel buttons. * Confirm deletion of authenticators * Make the bots happy again * Code polishing * Marking classes final * Use setApplication / getApplication in the plugin class * Remove unused `$db` from the plugin class * Blind fix Currently #38060 has broken everything it seems? * Bring application injection in sync with core * Remove whitespace * Add use statement * Fix wrong event creation in AjaxHandlerLogin * License change Co-authored-by: Richard Fath <[email protected]> Co-authored-by: Brian Teeman <[email protected]> Co-authored-by: Roland Dalmulder <[email protected]> Co-authored-by: Allon Moritz <[email protected]> Co-authored-by: Harald Leithner <[email protected]> Co-authored-by: George Wilson <[email protected]>
This pr adds the required table caption and column/row scope to the table.
There are no visual changes and brings this table into line with all other admin tables.
As webauthn requires https this PR cannot be tested on a site without https.