Refactor 2FA from FOF to Joomla Core #11553

faynt0 · 2016-08-11T08:40:39Z

Summary of Changes

Refactored 2FA from FOF to Joomla Core
Removed dependecies to FOF
~~Refactored 2FA templates to JLayouts~~ Reverted back for B/C, removed the FOF part that resolves the template paths and used JPluginHelper instead.

Testing Instructions

Step 1 Activate the Twofactor Auth plugins ( Google Auth, Yubikey) in the backend
Step 2 Frontend: Go to the "Edit user profile" page and activate 2FA (Google Auth) and set up the authenticator
Step 2.1 Select Yubikey from the dropdown menu and it's template should be loaded ( Yubikey FOF dependecy was only related to the template)
Step 3 Logout and log in using your Google authenticator ( Frontend & Backend)
Step 4 After applying the patch Step 1-3 should work as before.

To test that custom templates still work create a "form.php" in "templates/protostar//html/plg_twofactorauth_totp" (Google Auth) with an echo() or similar and
go to Frontend -> Edit User profile , select Google Autheticator and your form's content should be shown.

…tor_2FA_to_Joomla_core

…youts' into Refactor_2FA_to_Joomla_core # Conflicts: # plugins/twofactorauth/totp/totp.php

cleaned phpcs errors

izharaazmi · 2016-08-11T09:03:25Z

plugins/twofactorauth/yubikey/yubikey.php

-		// Stop output buffering and get the form contents
-		$html = @ob_get_clean();
+		$layout = new JLayoutFile('plugins.twofactorauth.yubikey.form');
+		$data = ['new_totp' => $new_totp];


This is >= PHP 5.4 only?

moved files up a level because there are no namespaces yet cleaned up xml files simplified array extraction

faynt0 · 2016-08-11T11:13:06Z

@izharaazmi thanks for pointing out, I changed the syntax.

izharaazmi · 2016-08-11T11:21:50Z

IMO, the plugin default layouts should be placed inside the plugin itself. Same for the language files though. But since this plugin is part of core, I am not sure if this applies here.

I also believe uninstall of the plugin should remove all its elements. Certainly, except the library classes it depends on.

roland-d · 2016-08-11T12:12:49Z

@izharaazmi The language files can stay where they are I believe because all other core plugins have their language files in the admin language folder.

As for removing the library classes, that is a more difficult question because you cannot delete them when you uninstall 1 plugin because the other needs it nor can you uninstall it when you delete the 2nd plugin because theoretically you could install another authentication plugin that requires it. So I would say we can leave it as-is.

izharaazmi · 2016-08-11T12:28:03Z

@roland-d I said except...

Certainly, except the library classes it depends on.

I meant to say – when uninstalling the plugin remove the default layouts too alongwith any plugin only files . And keep the library classes and any layout overrides which is obvious anyway. You too are saying the same.

IMO, the plugin default layouts should be placed inside the plugin itself.

IMO, /layouts is not the best place for plugin's default layout files and uninstalling the plugin will not (and should not) remove it from there.

roland-d · 2016-08-11T12:49:25Z

@izharaazmi Using the /layouts folder, we already have a layout there for the user profile plugin, so I believe it is the correct place.

As for the uninstallation, if I uninstall the profile plugin the layout is going to stay as well. The question should rather be should all Joomla core be rewritten to be fully uninstallable, perhaps but that is out-of-scope for this pull request.

izharaazmi · 2016-08-11T12:56:01Z

Yeah, agreed on that. We'll talk over it some other day 👍

mbabker · 2016-08-11T13:08:41Z

libraries/joomla/encrypt/Base32.php

+ *
+ * @since  1.0
+ */
+class Base32


This class and file name are incorrect.

mbabker · 2016-08-11T13:14:22Z

@izharaazmi Using the /layouts folder, we already have a layout there for the user profile plugin, so I believe it is the correct place.

Just because that has been done already does not make it correct or even set precedent. The voting plugin and page navigation plugins both use tmpl/ directories for layouts. The stats plugin uses JLayout with a bunch of code inlined into it. So the existing practice for the majority is to keep the code contained in the extension, NOT use the global directory.

roland-d · 2016-08-11T13:15:38Z

I am sure if we put it in the tmpl/ folder the comment would have been why we don't put it in the layouts folder :/

mbabker · 2016-08-11T13:24:40Z

Well right now the plugin makes use of a tmpl/ folder so for B/C reasons whatever the logic for layout overrides is has to stay in place otherwise this cannot be accepted before 4.0 because it will cause HTML changes on sites implementing an override.

wilsonge · 2016-08-11T13:39:00Z

This is a large b/c break - we have a JPlugin helper method (https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/plugin/helper.php#L38) that will keep b/c with the FOF layouts and whichever way we go with that extension specific layouts should 100% go in the extension directory and NOT the global layout directory. As Michael says see how roberto did the stats plugin

Bakual · 2016-08-11T13:57:56Z

Imho, as long as 3rd parties can't install stuff into the layouts folder without custom installation scripts, our core extensions shouldn't do it as well.
The layouts folder should only contain JLayouts which are reusable across various extensions. Obviously that isn't the case today but it should be.
The proper place for an extension specific layout is the tmpl folder.

Or we make a move and let 3rd parties install their JLayouts into eg /layouts/com_foo/bar.php (similar to the media folder).

izharaazmi · 2016-08-11T14:11:39Z

@Bakual 👍
However, I'd like to do it the other way around. Centralise extension specific files / layouts / media / language (already) within the extension directory as much as possible.

brianteeman · 2016-08-11T15:46:18Z

Cant say I am a fan of having a file called Totp.php and another called totp.php

refactored from FOF override path to use core functions

faynt0 · 2016-08-12T09:10:46Z

First of all thanks everyone for your input, in the recent changes I got the autoloading to work like some of you mentioned.

Due to your B/C concerns I reverted the changes to the templating but removed and replaced the FOF part that handles the custom template path

brianteeman · 2016-08-12T09:54:29Z

I have tested this item ✅ successfully on 4bed2e5

applied PR and confirmed that both yubikey and google authenticator worked for site and admin login

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11553.}

faynt0 · 2016-08-12T10:14:04Z

Added small test instruction for custom templates.

mbabker · 2016-08-12T10:50:34Z

plugins/twofactorauth/totp/totp.php

-
-		// Include the form.php from a template override. If none is found use the default.
-		$path = FOFPlatform::getInstance()->getTemplateOverridePath('plg_twofactorauth_totp', true);
+		$path = JPATH_THEMES . "/" . JFactory::getApplication()->getTemplate() . "/html/plg_twofactorauth_totp";


Use JPluginHelper::getLayoutPath(). It'll also eliminate the need for the if/else include, you just include the return from that method. Same for the other plugin.

Fixed xml files

izharaazmi · 2016-08-12T12:16:05Z

plugins/twofactorauth/yubikey/yubikey.php

-		{
-			include_once __DIR__ . '/tmpl/form.php';
-		}
+		include_once JPluginHelper::getLayoutPath('twofactorauth', 'yubikey', "form");


I think layouts should not be include_once'd. Please verify this and amend if needed.

@izharaazmi Why would we need include here? The form is not going to change during runtime is it?

In this specific case, it may not apply but it is a general practice to 'require' a layout file. An 'include' may also be preferred sometimes.

If we call this plugin twice in a request cycle, 2nd will output nothing. Again for this case this may not be an issue, but *_once are costly anyway.

It has always been a require_once as you can see in the old code and hasn't given a problem so far.

... hasn't given a problem so far.

is not a good reason to keep doing something. To me *_once should be used only when any attempt to include the file for a second time may cause an issue such as for class definitions or function declarations etc.

Any other code should be able to load as many time as needed. See the various Joomla modules for example:

mod_footer would normally never be called twice but modules/mod_footer/mod_footer.php:37 still calls it using require. For me it is the correct approach. No need to pose an unnecessary job to check against included_files to the interpreter.

andrepereiradasilva · 2016-12-04T01:48:45Z

if the intention is to remove fof in joomla 4.0 this should be rebased to 4.0 branch

Bakual · 2016-12-04T07:15:39Z

Why? this could already be done in J3 fine.

mbabker · 2017-05-21T23:30:28Z

Is this still being worked on?

ghost · 2017-05-27T13:36:36Z

If this PR get no Response, it will be closed at 22th June 2017.

roland-d · 2017-05-30T13:42:39Z

I guess the students have given up on this. @icampus any idea?

faynt0 · 2017-05-30T14:03:27Z

I kind of fail to see what is left to be done here, is it about the include vs. include_once debate?

roland-d · 2017-06-03T09:01:38Z

@faynt0 At least the PR needs to be updated so it is in-sync with the staging branch.

ghost · 2017-06-21T11:43:46Z

@roland-d can you please have a Look at Status of this PR?

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11553.}

roland-d · 2017-06-22T15:07:22Z

@franz-wohlkoenig As far as I can see this needs testing as @faynt0 already updated it with the staging branch.

ghost · 2017-06-22T15:09:40Z

thanks @roland-d

ghost · 2017-06-23T13:30:21Z

@brianteeman can you please test again?

roland-d · 2017-08-08T13:54:03Z

@faynt0 could you please bring this up-to-date with the J4 codebase? Pretty please :) Ping @wilsonge as well if you have questions.

wilsonge · 2017-08-08T22:20:18Z

We need to update the Encrypt code with the latest version. change the branch to be against J4 and I promise I will get this merged!

wilsonge · 2017-08-23T14:11:40Z

OK i've ported this in #17687

faynt0 and others added 7 commits August 8, 2016 11:16

updated totp & yubikey templating to use JLayout

240f12c

removed FOF dependeny from Yubikey

e1a0876

refactored Totp class from fof package to libraries/encrypt package

a60e5a3

Merge remote-tracking branch 'origin/refactor_fof_to_core' into Refac…

2bc2b76

…tor_2FA_to_Joomla_core

Merge remote-tracking branch 'origin/Feature_Refactor2FATemplateToJLa…

ddb7795

…youts' into Refactor_2FA_to_Joomla_core # Conflicts: # plugins/twofactorauth/totp/totp.php

removed obsolete constructor

2b0a95f

cleaned phpcs errors

cleaned phpcs errors

7391252

izharaazmi reviewed Aug 11, 2016
View reviewed changes

changed array for < PHP 5.4 compatibility

9af2659

moved files up a level because there are no namespaces yet cleaned up xml files simplified array extraction

mbabker reviewed Aug 11, 2016
View reviewed changes

libraries/joomla/encrypt/Base32.php Outdated

*

* @since 1.0

*/

class Base32

Copy link

Contributor

mbabker Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class and file name are incorrect.

brianteeman added the PR-staging label Aug 11, 2016

faynt0 added 3 commits August 12, 2016 10:15

changed file and classname to match the autoload name convention

0671a46

reverted layout changes due to B/C

0fdb34a

refactored from FOF override path to use core functions

Added proper attribution

4bed2e5

mbabker reviewed Aug 12, 2016
View reviewed changes

Use JPluginHelper to resolve the template path

dc2655d

Fixed xml files

izharaazmi reviewed Aug 12, 2016
View reviewed changes

zero-24 mentioned this pull request Aug 19, 2016

Bundled FOF 2.4.3 is out of date and incompatible with PHP 7 #11673

Merged

Merge branch 'staging' into Refactor_2FA_to_Joomla_core

f68b544

wilsonge closed this Aug 23, 2017

Uh oh!

Refactor 2FA from FOF to Joomla Core #11553

Refactor 2FA from FOF to Joomla Core #11553

Uh oh!

Conversation

faynt0 commented Aug 11, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Uh oh!

izharaazmi Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faynt0 commented Aug 11, 2016

Uh oh!

izharaazmi commented Aug 11, 2016

Uh oh!

roland-d commented Aug 11, 2016

Uh oh!

izharaazmi commented Aug 11, 2016

Uh oh!

roland-d commented Aug 11, 2016

Uh oh!

izharaazmi commented Aug 11, 2016

Uh oh!

mbabker Aug 11, 2016

Choose a reason for hiding this comment

Uh oh!

mbabker commented Aug 11, 2016

Uh oh!

roland-d commented Aug 11, 2016

Uh oh!

mbabker commented Aug 11, 2016

Uh oh!

wilsonge commented Aug 11, 2016

Uh oh!

Bakual commented Aug 11, 2016

Uh oh!

izharaazmi commented Aug 11, 2016

Uh oh!

brianteeman commented Aug 11, 2016

Uh oh!

faynt0 commented Aug 12, 2016

Uh oh!

brianteeman commented Aug 12, 2016

Uh oh!

faynt0 commented Aug 12, 2016

Uh oh!

mbabker Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

izharaazmi Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

roland-d Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

izharaazmi Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

roland-d Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

izharaazmi Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bakual commented Dec 4, 2016

Uh oh!

mbabker commented May 21, 2017

Uh oh!

ghost commented May 27, 2017

Uh oh!

roland-d commented May 30, 2017

Uh oh!

faynt0 commented May 30, 2017

Uh oh!

faynt0 commented Aug 11, 2016 •

edited by joomla-cms-bot

Loading

izharaazmi Aug 11, 2016 •

edited

Loading

andrepereiradasilva commented Dec 4, 2016 •

edited

Loading