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.0] Webauthn gmp warning #29731

Merged
merged 3 commits into from
Jul 20, 2020
Merged

[4.0] Webauthn gmp warning #29731

merged 3 commits into from
Jul 20, 2020

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Jun 21, 2020

Pull Request for Issue #29696 .

Summary of Changes

A very light touch change to reuse existing code to display a warning and remove the ability to add authenticators if the PHP extension GMP is not loaded.

Testing Instructions

Install Joomla 4 on a server (or docker container in my case) without GMP PHP Extension enabled (On a cPanel server you can toggle this, and its off by default in Easy Apache 4)

Ensure you access Joomla over a secure & valid SSL connection (learn ngrok if you need this, quick and easy)

Before PR

When adding an authenticator you get an internal server error

After PR

You get a error and no button allowing you to add a new authenticator (but you can still see the list of existing authenticators - if any - rename them, and delete them.)

Screenshot 2020-06-21 at 12 24 50

Who else to ping for visibility ;-)

// @nikosdion

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jun 21, 2020
@richard67
Copy link
Member

@PhilETaylor Maybe some general explanation about or PR descriptions: "Expected result" always shall describe what one expects, i.e. how it is with the PR applied, "Actual result" shall describe what one currently observes, i.e. how it is without the PR applied. Your description above currently mixes that up.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

ah well... "Expected" and "Actual" are very unhelpful terms anyway. "Before" and "After" are more suited to PRs IMHO.

Agree .. it's often subject of confusion.

@nikosdion
Copy link
Contributor

I'd actually change the detection code to use function_exists() with some GMP function names for two reasons.

First, I've seen many hosts where get_loaded_extensions itself is disabled, making that kind of code fail immediately with a fatal error. I know, these hosts are crap but they are used by people using Joomla...

Second, it's possible that the extension is enabled but the host disabled the functions. I have not seen that with GMP but I can't rule out outright stupidity when it comes to cheap hosting.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

@PhilETaylor Normally we do it the other way round, i.e. if (function_exists('gmp_intval') === false) and not if (false === function_exists('gmp_intval')). But I'm ok with it if that passes PHPCS.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

@PhilETaylor PHPCS is ok with it, so I am, too. Unfortunately I can't help with testing, because my local testing environment uses a self signed SSL certificate, and I haven't set up any webauthentication for me anywhere. And testing space on my domain which has a valid SSL cert is protected with password, so I am not sure if it would work there.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

richard67 commented Jun 21, 2020

@PhilETaylor You can't teach an old dog new tricks ;-)

(have to go back to my vi editor)

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

All checks have passed 🎉🎉

There must be something wrong ... ;-)

@Quy
Copy link
Contributor

Quy commented Jun 30, 2020

I have tested this item ✅ successfully on dba7e75

Thank you for the ngrok info. So simple!!!


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

@nikosdion
Copy link
Contributor

Self hosted alternative to Ngrok written in PHP: https://pociot.dev/28-introducing-expose-an-easy-to-use-tunneling-service-implemented-in-pure-php#introducing-expose-an-easy-to-use-tunneling-service-implemented-in-pure-php

Apparently it took the PHP world by storm over the last couple of weeks.

@PhilETaylor

This comment was marked as abuse.

@bonzani
Copy link

bonzani commented Jul 19, 2020

I have tested this item ✅ successfully on dba7e75


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 19, 2020
@richard67 richard67 merged commit 8d60c00 into joomla:4.0-dev Jul 20, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 20, 2020
@richard67
Copy link
Member

Thanks!

@richard67 richard67 added this to the Joomla 4.0 milestone Jul 20, 2020
dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Jul 21, 2020
…outs

* '4.0-dev' of github.com:joomla/joomla-cms: (612 commits)
  [4.0] Smart Search: Fixing ordering, order direction and disabled button (joomla#29474)
  [4.0] Generate routed Modal links for iframes when not on the root (joomla#30007)
  [4.0] Get menu directly in com_tags menu route helper (joomla#30039)
  Remove collapse when resizing from mobile to desktop (joomla#30132)
  [4.0] Wrap component output in `main` element to make Cassiopeia more accessible (joomla#29870)
  [4.0] Webauthn gmp warning (joomla#29731)
  [4.0] Refactor to return early, remove if depths and throw NotAllowed (joomla#29694)
  [4.0] CLI help text (joomla#29811)
  Feature/draggable typo fixes (joomla#29987)
  [4.0] Removing unnecessary workaround in finder indexer (joomla#30037)
  [4.0] Optimizing Smart Search for larger content (joomla#30008)
  [4.0] Fix js ajax for pre update checker (joomla#29980)
  [4.0] Cassiopea: Fixing modals custom-select fields display (joomla#30097)
  [4.0][com_fields] Fix draggable sorting (joomla#30094)
  [4.0] Correct incorrect @return documentation (joomla#30092)
  [4.0] Menu items modal: adding missing filters (joomla#30087)
  short to long php open tags with echo (joomla#30089)
  Use new Toolbar (joomla#30085)
  [4.0] Center status/date created headers (joomla#29249)
  [4.0] Fix Cassiopea searchtools alignment in modals (joomla#30077)
  ...

# Conflicts:
#	administrator/components/com_templates/src/View/Template/HtmlView.php
#	installation/sql/postgresql/base.sql
#	libraries/src/Application/AdministratorApplication.php
#	libraries/src/Application/SiteApplication.php
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
Add warning if GMP not loaded for Webauthn
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