@jo-sf is this PR ready for testing?
Does the new option "Encrypt Login / Registration Forms" disappear or be grayed-out when the Global Configuration is configured on "Entire Site"?

What about backwards compatibility?
What happens when people update there Joomla installation and have a mod_login form configured to use SSL/TLS?

When I have your answer, then I will discuss it with @roland-d (we just had a discussing about fixing this part of Joomla) and start testing everything.

Hi Wilco,
 
I'm currently away from home, so I can't answer all your questions immediately. Please give me some time before I'll answer you.
 
If I remember correctly the SSL/TLS settings for mod_login aren't grayed out if the SSL/TLS global configuration is configured to "entire site". Due to that I think I implemented the secure registration and login in the same way - because if ou set your entire site to "SSL/TLS encrypted" you'll nevel fiddle around with SSL/TLS settings for wither mod_login or for secure registration and login.
 
Your question about backward compatibility needs some checking from my side...
 
Regards,
 
jo-sf
 
-----Ursprüngliche Nachricht-----
Von: "Wilco Alsemgeest" [notifications@github.com]
Gesendet: Mo. 27.06.2016 02:04
An: "joomla/joomla-cms" [joomla-cms@noreply.github.com]
Kopie: "jo-sf" [jsf360@freenet.de], "Mention" [mention@noreply.github.com]
Betreff: Re: [joomla/joomla-cms] Improvements for secure registration and login (#8619)

@jo-sf is this PR ready for testing?
Does the new option "Encrypt Login / Registration Forms" disappear or be grayed-out when the Global Configuration is configured on "Entire Site"?
What about backwards compatibility?
What happens when people update there Joomla installation and have a mod_login form configured to use SSL/TLS?
When I have your answer, then I will discuss it with @roland-d (we just had a discussing about fixing this part of Joomla) and start testing everything.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8619. 
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
-----Ursprüngliche Nachricht Ende-----

Alle Postfächer an einem Ort. Jetzt wechseln und E-Mail-Adresse mitnehmen! Rundum glücklich mit freenetMail

Hi jo-sf,

Take your time.

One of the things we wanted to change in a PR was to gray-out the SSL/TLS option in mod_login when Global Configuration is on "Entire site".
If you can implement this into your PR, then I think we have a nice addition.

We wanted to change the language strings too.
SSL is something very which is not used anymore on a technical level, nowadays TLS protocol is used.
But to keep it understandable for the users we wanted it to change to SSL/TLS
E.g: Global Configuration - Force SSL, needs to be change to Force SSL/TLS

If you can update your new language strings, then I will check the existing ones and add these to your PR.

Regards,
Wilco

Hi Wilco,
 
unfortunately I'm still not yet at home but I will nevertheless try to answer as many points from your comments as possible.
 
Usage of "SSL/TLS" instead of "SSL" alone: this will be no problem, I'll do that as soon as possible - and doing so I can also check for other language strings using "SSL" too.
 
Gray out the SSL/TLS option in my PR if the SSL/TLS setting in global configuration is set to "entire site": yes that would indeed be a nice addition but I have to check whether there are already any provisions in the Joomla form code that allow this. I know that it is possible to dynamically show or hide fields in a form depending upon the setting of other fields in the same form, but I don't know of any interaction with settings in any other form or the global configuration. But I'll check...
 
Backwards compatibility: there are some points to consider. Any settings made before with respect to the mod_login's "SSL/TLS activation" are ignored, this setting is initialized with the com_users's setting for reasons of backwards compatibility with login form overrides coming from any third-party template (see change in modules/mod_login/mod_login.php). I decided to ignore the mod_login's setting because you need to find and save all mod_login option forms in order to remove old settings if the com_users's setting differs from the previous setting (mod_login: SSL/TLS on, com_users: SSL/TLS off). Moreover there is at least one mod_login usage where you cannot change options - this is the login form presented if you entered wrong login credentials.
 
Actuality of my PR: as you see from the initial date of my PR it was built upon an older Joomla version (I think it was the staging version after 3.4.8). I haven't checked yet whether any of the 37 files changed in this PR have been changed in an incompatible way such that this PR cannot be applied. On the other side I've applied my patches to a running Joomla 3.5.1 version without problems - so I expect no greater problems with this PR being applied to the current staging version, but I'll check this and report my results.
 
There is one point where I now think my patch can be simplified. This refers to the files modules/mod_login/tmpl/default.php and modules/mod_login/tmpl/default_logout.php: in both files the routing was done by means of JRoute::() with the third parameter set according to the setting of the "usesecure" setting, and the first parameter is in both cases the current URL. But since JRoute::() only works well with "index.php" based URLs and in case of SEF the current URL doesn't start with "index.php" I reinvented the wheel and replaced the call to JRoute::() with an URL calculated in place. In an older version of the modules/mod_login/tmpl/default.php file the first parameter passed to JRoute::() was simply "index.php" since the real target for logging in (or out) was at that time passed via the hidden field "return", and this field still exists, it still contains the target page and this value is still used (I'm sure about this because I tried with a template that overrides the login form and that still uses the old logic with calling JRoute::("index.php", ...)).
 
toString())" to JRoute::(). But I don't know what Kubik-Rubik had in mind when he changed these two files on 8 Dec 2014.
 
Regards,
 
jo-sf

Mail & Cloud Made in Germany mit 3 GB Speicher! Jetzt kostenlos anmelden

I haven't checked yet whether any of the 37 files changed in this PR have been changed in an incompatible way such that this PR cannot be applied.

@jo-sf Github reports conficts, after local test rebase i see only 1 conflict in administrator/modules/mod_status/tmpl/default.php

In my last comment written July 3 the last paragraph has been garbled, probably due to the fact that I sent this comment via mail. It should read as follows:

index.php was replaced by the expression htmlspecialchars(JUri::getInstance()->toString()) when calling JRoute::_(). But I don't know what Kubik-Rubik had in mind when he changed these two files on Dec 8, 2014.

Moreover JRoute::_() was garbled in every place I mentioned it and the text surrounding it is partially shown in italic - sorry for that.

@conconnl I finally managed to look a bit deeper into my PR today...

Regarding the graying out (or not showing) a field in a form if any flag in the global configuration is set to any of a list of values might work similar to the "showon" attribute that already exists, with the difference that "showon" triggers upon any other field in the same form and shows or hides the field dynamically whereas in the context of the global configuration the dependent field might be grayed out or not shown at all as long as the form is shown. Due to that the best place to implement this new feature would be in libraries/joomla/form/field.php.

When checking my PR I saw that the same sequence of code statements is repeated in many files. This sequence is about evaluating the URL for the site homepage when switching from the administration area to the site e.g. for previewing reasons. The code typically looks like that:

    $config = JFactory::getConfig();
    $siteUri = JUri::root();
    if ($config->get('force_ssl') == 1)
    {
        // force_ssl == 1 means: administration via HTTPS, site via HTTP
        $siteUri = JUri::getInstance($siteUri);
        $siteUri->setScheme('http');
        $siteUri->setPort($config->get('http_port'));
        $siteUri = $siteUri->toString();
    }

It's maybe a good idea to place this code in one PHP file and call it from everywhere else when needed. I however didn't check yet for a suitable place.

Since @wojsmol already reported conflicts between my PR and the current staging version (thanks @wojsmol !) I think it would be best if I create a new patch set with the additional features discussed so far (SSL/TLS, graying out fields depending upon global configuration settings, use of index.php instead of the current URL in modules/mod_login/tmpl/default.php and modules/mod_login/tmpl/default_logout.php, evaluation of the site URL at a single location) where this patch set is based upon the current staging code. I would then open an new PR with this new patch set and cross reference this PR and the new PR.

Is this OK?

Another item I'm not entirely happy with is the definition of the JRoute::_() method, especially of the third parameter $ssl which is by default null. From the logic of this parameter I would prefer the following:

• if the value is null the protocol of the URL returned will be the protocol currently used
• if the value is false the protocol of the URL returned always will be http:
• if the value is true the protocol of the URL returned always will be https:

The current code handles the default value null like the integer 0 and honors the integer values 1 and 2 for switching between the protocols. IMO this is not very self-explaining, especially with calls to JRoute::_() where a third parameter is passed.

Since the passing of the parameter $ssl to JRoute::_() is (as far as I've seen) only used in the context of logging in and out and in the context of user registration (including lost passwords) this PR might be the right place to change the definition and the handling of the parameter $ssl.

Hi @jo-sf

First of all, thanks for you extended information.
I think we indeed need to create multiple PRs, to keep a good overview and to be able to Commit small parts to the Joomla core.
Some changes don't have much impact or none at all and others need to be fully tested.

I hope I don't forget any.

1. Change the current language strings to match to correct usage for TLS/SSL words and descriptions. (I will scan Joomla and do a PR for all the language strings, then you can focus on the feature changes and maybe some new strings)

2. Your current changes, ideas and things for moving the configuration to a central place and off course without B/C break and changing the workings of current sites.

3. Your idea about moving the same code statement for TLS usage when switching between back- and front-end to a library something like /libraries/joomla/security/tls/admincheck.php

Changing the definition and the handling of the parameter is not something I can give an satisfying answer.
I always say "If it works don't fix it", but you can always create another PR after implementing the current changes and discuss it with the more advanced developers and PLT. I think you don't have to wait to create the PR, only wait with changing it in your current proposed changes, because it can just be the one thing why a certain PR is not approved.

At least, I will help you with all the PRs on the level of testing, giving ideas, opinions and everything within my capabilities.
I'm not good enough to program stuff, but I understand the things you write and the code fragments you show.

Update:
For as far I can see someone already updated all Language Strings.
So the first PR is not needed anymore.

Yes that was done quite some time ago

On 12 July 2016 at 23:53, Wilco Alsemgeest notifications@github.com wrote:

Update:
For as far I can see someone already updated all Language Strings.

So the first PR is not needed anymore.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/8619
https://issues.joomla.org/tracker/joomla-cms/8619.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#8619 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABPH8cKLP7-tbjoBc6dhD9IfDOAZkuxjks5qVBregaJpZM4GxPQy
.

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

Hi @conconnl

I also think it would be best to split this PR and the ideas we discussed so far into several separate PRs - as far as this is possible. There will be some dependencies between the main PR dealing with secure registration and login and other PRs implementing the additional ideas since these other PRs will probably change the code coming with the first PR (which means that those PRs are not independent from each other). Maybe I'll have to wait with some of these other PRs until the main PR has been accepted and merged into the staging code.

Currently I see the following PRs:

1. Changes for secure registration and login, this will mainly contain the changes from this PR
2. Usage of "index.php" as target for JRoute::_() in modules/mod_login/tmpl/default.php and modules/mod_login/tmpl/default_logout.php instead of the current URL (this will undo the changes made by @Kubik-Rubik in December 2014)
3. Implementation of the "gray out/hide" feature for form fields depending upon global configuration settings
4. Changes in the com_users configuration such that the "switch to SSL/TLS mode" field is shown only when SSL/TLS setting in the global configuration is set to "entire site"
5. Changes regarding the parameter $ssl of the method JRoute::_()

Since the code needed for evaluating the proper URL when switching from the administration area to the frontend view (e.g. for preview porposes) is new code and is part of the first PR mentioned above I think it is not necessary to introduce this code as a method by means of a separate PR (moreover this again will produce a PR dependant upon another PR). Due to that I plan to enhance my current PR such that this piece of code will be implemented as a method (in a place/file/class still to be determined).

Another dependency resulting from the list of PR suggested above is that with the new first PR there will be no graying out / hiding of the "switch to SSL/TLS mode" field in the com_users configuration when the global configuration is set such that both the backend and the frontend pages are delivered via SSL/TLS. This can only be implemented (by means of the fourth PR) if both the first PR and the third PR have been accepted and merged into the staging code.

I also hope I didn't forget any idea we discussed so far.

Hi @conconnl

despite having planned it other way round I today started with PR no. 3 ("gray out/hide" feature for form fields depending upon global configuration settings) after having solved (I hope) some problems related to "Force HTTPS" setting in the global configuration. You find this change as PR #11207.

Next (as soon as possible) I'll start with PR no. 1, and as far as I've already seen this will also contain PR no. 2 since there are changes in PR no. 1 that touch and change the JRoute::_() calls in those two files. In doing so I'll use the new field attribute "globalshowon" even if it will not work without PR #11207 being applied - but it will not harm either.

Hi @conconnl
I finally managed today to create a new PR based upon the current Joomla staging version that replaces this PR - see PR #11304.

@jo-sf so this can be closed than?

@zero-24 yes I think so unless @conconnl still needs this PR together with my new PRs #11207 and #11304.

@conconnl the two PRs #11207 and #11304 cover the topics 1 to 4 listed in my comment about two weeks ago (topic 3 is covered by #11207, topics 1, 2 and 4 are all covered by #11304). Only topic 5 is not covered but I think I'll leave that alone (never touch a working code).

@zero-24 @jo-sf
I think this one can be closed, as the other two PRs implements 1 to 4.

Closed as requested