Skip to content
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

Merged
merged 3 commits into from
Apr 3, 2022
Merged

Conversation

brianteeman
Copy link
Contributor

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.

image

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.
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.1-dev labels Apr 3, 2022
@Quy
Copy link
Contributor

Quy commented Apr 3, 2022

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.

@richard67
Copy link
Member

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.

@brianteeman
Copy link
Contributor Author

@richard67 it would be ok for me as this is well established code

@richard67
Copy link
Member

@richard67 it would be ok for me as this is well established code

Well established and the change is clear and easy to understand.

@richard67
Copy link
Member

P.S.: ... and I had tested similar PRs successfully in past.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 724a7be

By review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37464.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37464.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 3, 2022
@Quy Quy added this to the Joomla 4.1.3 milestone Apr 3, 2022
@Quy Quy merged commit cd9c48a into joomla:4.1-dev Apr 3, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 3, 2022
@Quy
Copy link
Contributor

Quy commented Apr 3, 2022

Thanks

@nikosdion
Copy link
Contributor

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 manage.php file where you changed the TD to a TH.

The build/media_source/plg_system_webauthn/js/management.es6.js file is targeting the TD elements inside the TR, see lines 192 to 196. By changing the element to a TH you broke the Edit button.

@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 .github/CODEOWNERS.

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:

  • Fix the management.es6.js file.
  • Change the .github/CODEOWNERS to include me as the code owner of the layouts/plugins/system/webauthn/manage.php file since it was moved outside the plugin's folder.
  • Never contribute untested code especially if you think you are too good / your change is too small to need any testing at all.
  • Never set code to RTC without any tests whatsoever on the basis that the contributor pinky swears it's all good.
  • Never merge code that has an owner without asking the code owner for feedback.

@brianteeman
Copy link
Contributor Author

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.

@nikosdion
Copy link
Contributor

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 Joomla organisation on GitHub could add a read-only team and invite us code owners into it it'd solve the automation problem. Otherwise there's no point in having a code owner who's never notified preemptively but might still be called to fix what's broken after the fact. The whole point of code ownership is for us to share our domain-specific knowledge very early to avoid SNAFUs, am I right?

@richard67
Copy link
Member

which I made in a separate commit so you can do just that, by the way

That's this one here: 84b996a

@brianteeman Will you make a PR? That would be great.

@brianteeman
Copy link
Contributor Author

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

@HLeithner
Copy link
Member

I'm looking into the read only group and coordinate this with the other maintainers

@brianteeman
Copy link
Contributor Author

Thank you @HLeithner

@brianteeman
Copy link
Contributor Author

PR for my error #37676

Kostelano added a commit to JPathRu/localisation that referenced this pull request May 2, 2022
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 +
HLeithner added a commit that referenced this pull request Jun 27, 2022
* 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 <brian@teeman.net>

* Update administrator/language/en-GB/plg_system_webauthn.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* 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 <richard67@users.noreply.github.com>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Roland Dalmulder <contact@rolandd.com>
Co-authored-by: Allon Moritz <allon.moritz@digital-peak.com>
Co-authored-by: Harald Leithner <leithner@itronic.at>
Co-authored-by: George Wilson <georgejameswilson@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants