? Success

User tests: Successful: Unsuccessful:

avatar faynt0
faynt0
11 Aug 2016

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.

avatar faynt0 faynt0 - open - 11 Aug 2016
avatar faynt0
faynt0 - comment - 11 Aug 2016

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

avatar izharaazmi
izharaazmi - comment - 11 Aug 2016

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.

avatar roland-d
roland-d - comment - 11 Aug 2016

@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.

avatar izharaazmi
izharaazmi - comment - 11 Aug 2016

@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.

avatar roland-d
roland-d - comment - 11 Aug 2016

@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.

avatar izharaazmi
izharaazmi - comment - 11 Aug 2016

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

avatar mbabker
mbabker - comment - 11 Aug 2016

@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.

avatar roland-d
roland-d - comment - 11 Aug 2016

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 :/

avatar mbabker
mbabker - comment - 11 Aug 2016

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.

avatar wilsonge
wilsonge - comment - 11 Aug 2016

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

avatar Bakual
Bakual - comment - 11 Aug 2016

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).

avatar izharaazmi
izharaazmi - comment - 11 Aug 2016

@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.

avatar brianteeman
brianteeman - comment - 11 Aug 2016

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

avatar mbabker
mbabker - comment - 11 Aug 2016

@brianteeman Everything in libraries/cms, libraries/joomla, and libraries/legacy should be all lowercased filenames since it uses Joomla's autoloader and it doesn't really account for or call for uppercased paths.

All of the namespaced code autoloaders pretty much call for title and camel cased paths. That's how PSR-0 and PSR-4 are defined and for Joomla's namespaced loader we follow the PSR-0 convention and most of the Composer dependencies are PSR-4.

So before this can be merged the path casing will have to be corrected.

avatar brianteeman
brianteeman - comment - 11 Aug 2016

@mbabker so my concern was correct?

On 11 August 2016 at 16:51, Michael Babker notifications@github.com wrote:

@brianteeman https://github.com/brianteeman Everything in libraries/cms,
libraries/joomla, and libraries/legacy should be all lowercased filenames
since it uses Joomla's autoloader and it doesn't really account for or call
for uppercased paths.

All of the namespaced code autoloaders pretty much call for title and
camel cased paths. That's how PSR-0 and PSR-4 are defined and for Joomla's
namespaced loader we follow the PSR-0 convention and most of the Composer
dependencies are PSR-4.

So before this can be merged the path casing will have to be corrected.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11553 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8eLnUwS2bDRJNgmvNNxJifTSyF7xks5qe0UbgaJpZM4Jh5_z
.

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

avatar mbabker
mbabker - comment - 11 Aug 2016

Yes.

avatar brianteeman
brianteeman - comment - 11 Aug 2016

I can see files being moved to new locations and folders being moved but doesnt this have to be handled in the com_admin/script.php for sites updating

avatar mbabker
mbabker - comment - 11 Aug 2016

Eventually, yes. However, as pointed out the layout stuff will need to be adjusted for B/C so that will presumably move the files back to their original location and that should cancel out any need for updating the script.php file.

avatar faynt0
faynt0 - comment - 12 Aug 2016

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

avatar brianteeman brianteeman - test_item - 12 Aug 2016 - Tested successfully
avatar brianteeman
brianteeman - comment - 12 Aug 2016

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.

avatar faynt0
faynt0 - comment - 12 Aug 2016

Added small test instruction for custom templates.

avatar brianteeman brianteeman - change - 18 Aug 2016
Category Libraries Plugins
avatar brianteeman brianteeman - change - 18 Aug 2016
Status New Pending
avatar andrepereiradasilva
andrepereiradasilva - comment - 4 Dec 2016

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

avatar Bakual
Bakual - comment - 4 Dec 2016

Why? this could already be done in J3 fine.

avatar mbabker
mbabker - comment - 21 May 2017

Is this still being worked on?

avatar franz-wohlkoenig franz-wohlkoenig - change - 22 May 2017
The description was changed
Status Pending Information Required
avatar joomla-cms-bot joomla-cms-bot - edited - 22 May 2017
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2017
Category Libraries Plugins Libraries Front End Plugins
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 May 2017

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

avatar roland-d
roland-d - comment - 30 May 2017

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

avatar faynt0
faynt0 - comment - 30 May 2017

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

avatar roland-d
roland-d - comment - 3 Jun 2017

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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 21 Jun 2017

@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.

avatar roland-d
roland-d - comment - 22 Jun 2017

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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jun 2017

thanks @roland-d

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jun 2017

@brianteeman can you please test again?

avatar roland-d
roland-d - comment - 8 Aug 2017

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

avatar wilsonge
wilsonge - comment - 8 Aug 2017

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!

avatar franz-wohlkoenig franz-wohlkoenig - change - 18 Aug 2017
Status Information Required Discussion
avatar wilsonge
wilsonge - comment - 23 Aug 2017

OK i've ported this in #17687

avatar wilsonge wilsonge - change - 23 Aug 2017
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2017-08-23 14:11:29
Closed_By wilsonge
avatar wilsonge wilsonge - close - 23 Aug 2017

Add a Comment

Login with GitHub to post a comment