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

Test for mcrypt support in the installer #9508

Merged
merged 1 commit into from
Apr 12, 2016
Merged

Test for mcrypt support in the installer #9508

merged 1 commit into from
Apr 12, 2016

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Mar 21, 2016

Partial Pull Request for Issue #9507.

Summary of Changes

This adds a test to Joomla's install app to check for mcrypt support. On a server without it, though Joomla is installable, a degraded experience will be present (namely two-factor authentication cannot be used and several libraries (JCrypt, FOFEncryptAes, the random_compat polyfill, and the password_compat polyfill) either use different encryption libraries or fail completely).

Testing Instructions

On the overview page of the install app, in the Pre-Installation Check section, you will see a new line testing for mcrypt support. This should report accurately depending on your server's configuration.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Mar 21, 2016
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on a236e7f


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

@brianteeman
Copy link
Contributor

Would it perhaps be better to put the check in the code that enables the 2fa


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

@mbabker
Copy link
Contributor Author

mbabker commented Mar 21, 2016

There should be a check there too, yes. As I said on the other issue, this is just a very small piece of things and at least adds a generalized check that didn't exist before.

@brianteeman
Copy link
Contributor

Either way - this is not a critical or a new issue and can wait until 3.5.1

On 21 March 2016 at 20:30, Michael Babker notifications@github.com wrote:

There should be a check there too, yes. As I said on the other issue, this
is just a very small piece of things and at least adds a generalized check
that didn't exist before.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9508 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman
Copy link
Contributor

Thinking about it this must go in the 2fa plugin code because a large % of
sites never use our installer and/or are upgrades from earlier releases and
the install check will be of no use to them at all

On 21 March 2016 at 20:31, Brian Teeman brian@teeman.net wrote:

Either way - this is not a critical or a new issue and can wait until 3.5.1

On 21 March 2016 at 20:30, Michael Babker notifications@github.com
wrote:

There should be a check there too, yes. As I said on the other issue,
this is just a very small piece of things and at least adds a generalized
check that didn't exist before.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9508 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman brianteeman added this to the Joomla 3.5.1 milestone Mar 21, 2016
@mbabker
Copy link
Contributor Author

mbabker commented Mar 21, 2016

To close out #9507 completely at a minimum you should have this PR merged, some kind of check in the com_users edit screens which disables the 2FA fields if the server doesn't support the requirements (and a big UI warning explaining why), and checks in the code which skip calling the functions if unsupported since you shouldn't rely on just the UI to block something. Ideally also the environmental checks that exist in the installer should be integrated into the system information view as a new tab so that information is available to the admin regardless of how they installed (plus just because you install on one server doesn't mean it will test the same on another server).

@brianteeman
Copy link
Contributor

Yes we need a complete solution not just a quick fix ;)

On 21 March 2016 at 20:37, Michael Babker notifications@github.com wrote:

To close out #9507 #9507
completely at a minimum you should have this PR merged, some kind of check
in the com_users edit screens which disables the 2FA fields if the server
doesn't support the requirements (and a big UI warning explaining why), and
checks in the code which skip calling the functions if unsupported since
you shouldn't rely on just the UI to block something. Ideally also the
environmental checks that exist in the installer should be integrated into
the system information view as a new tab so that information is available
to the admin regardless of how they installed (plus just because you
install on one server doesn't mean it will test the same on another server).


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9508 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@richard67
Copy link
Member

I have tested this item ✅ successfully on a236e7f

Successfully tested for the case of mcrypt support = true.

Pre-Installation Check section of the installation app shows this.

Unfortunately I have no other server available to test the other case where it is false.


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

@brianteeman brianteeman modified the milestones: Joomla 3.5.1, Joomla 3.5.2 Mar 22, 2016
@andrepereiradasilva
Copy link
Contributor

See for adding mcrypt info to sysinfo #9532

@brianteeman
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 24, 2016
@rdeutz rdeutz merged commit bd0a6eb into joomla:staging Apr 12, 2016
@mbabker mbabker deleted the Test-Mcrypt branch April 12, 2016 19:46
@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016
@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label May 11, 2016
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.

None yet

6 participants