[login forms] performance improvements in two factor authentication by andrepereiradasilva · Pull Request #8951 · joomla/joomla-cms

andrepereiradasilva · 2016-01-20T19:54:39Z

Description

This PR improves performance and memory usage of the admin login page, the offline page, all the pages that have a login module in the frontend and the component users view login form.

Joomla is loading fof just to check if any two factor auth plugins are enabled to show the "Secret key" field. IMHO we can use Joomla (already loaded) libraries for that.
This way we can have performance improvements(around 4-6ms PHP 5.6.16) in every page that use this login modules.

Performance before PR (frontend mod_login as example)

Performance after PR (frontend mod_login as example)

How to test

Install Joomla with latest staging with sample data
Enable Google Authenticator (or Yubikey) two factor authentication
Apply patch
Create a dummy user and configure it to use with Google Authenticator (if you don't have use a chrome plugin for that, for instance https://chrome.google.com/webstore/detail/authenticator/bhghoamapcdpbohphigoooaddinpkbai)
Check Two auth is working:
- Go to frontend and try to login with the new user without the two auth secret key. You can't
- Go to frontend and try to login with the new user with the two auth secret key. You can

Note: for test the performance go to any (or all this files):

/modules/mod_login/mod_login.php
/administrator/modules/mod_login/mod_login.php
/templates/system/offline.php
/components/com_users/views/login/view.html.php

And put a mark in the debug profiler before and after the call to get the two factor methods.
Example: /modules/mod_login/mod_login.php

JDEBUG ? JProfiler::getInstance('Application')->mark('- start getting twofactorauth methods') : null;
$twofactormethods = [....];
JDEBUG ? JProfiler::getInstance('Application')->mark('- <strong>Time to get</strong> twofactorauth methods') : null;

Observations

Suggestions, improvements and code reviews are welcome.

… offline template

mbabker · 2016-01-20T20:04:14Z

libraries/cms/authentication/helper.php

You're using PHP 5.4 syntax here.

@mbabker

thanks again @mbabker

andrepereiradasilva · 2016-01-20T20:26:07Z

travis seems to be confused...

wilsonge · 2016-01-21T01:04:57Z

Restarted Travis. FWIW it's not FOF that's the problem. You can remove the FOF code by replacing those two lines with the Joomla specific versions:

JPluginHelper::importPlugin('twofactorauth');
$identities = JEventDispatcher::getInstance()->trigger('onUserTwofactorIdentify', array());

And I went from the method taking 84ms to 82ms :P (for the admin login page). Therefore I assume the time is actually gained by not running the plugin event (and it's a long know but impossible to fix without screwing b/c fact) that Joomla's plugin system is slow as hell. Therefore the question to me is that is it b/c doing what you are doing. Other 3rd party two factor plugins may have more logic in their onUserTwofactorIdentify methods than we do and now they won't be run in the future :/

Just as a sidenote we actually load FOF on every page :P https://github.com/joomla/joomla-cms/blob/staging/libraries/cms.php#L42-L46

andrepereiradasilva · 2016-01-21T12:48:07Z

@wilsonge didn't notice fof was loaded every time on every page. thanks for enlightening me. Don't know yet Joomla code so well. Is there a reason for fof being loaded in everypage and not only when needed?

And I went from the method taking 84ms to 82ms :P (for the admin login page). Therefore I assume the time is actually gained by not running the plugin event (and it's a long know but impossible to fix without screwing b/c fact) that Joomla's plugin system is slow as hell.

Yeah, you're right again.
I think it's triggered in https://github.com/joomla/joomla-cms/blob/staging/libraries/fof/integration/joomla/platform.php#L537-L558
I forgot about that...

Therefore the question to me is that is it b/c doing what you are doing. Other 3rd party two factor plugins may have more logic in their onUserTwofactorIdentify methods than we do and now they won't be run in the future :/

Yes, it seems it's only used in joomla plugins to get the section in the two auth plugins in joomla.
See https://github.com/joomla/joomla-cms/blob/staging/plugins/twofactorauth/totp/totp.php#L64-L97 and https://github.com/joomla/joomla-cms/blob/staging/plugins/twofactorauth/yubikey/yubikey.php#L64-L96

It that i mind i will see it after the changes needed this PR still makes sense later.

mbabker · 2016-01-21T13:19:52Z

That file is just loading FOF's autoloader, not including the whole library into memory.

andrepereiradasilva · 2016-01-21T22:23:38Z

@mbabker, ok. thanks for explaining.

@wilsonge, after several testing i revised the PR to use the onUserTwofactorIdentify trigger, but with joomla.

We still get a performance/memory usage improvement in making the same thing so i think this PR still makes sense and this way, i think, there are no B/C problems.

andrepereiradasilva · 2016-01-24T14:56:13Z

Did some tests with PHP 5.6.16, without Joomla caching, with PHP OpCache disabled and without open_basedir.
Also test where made using the profiler before and after the xxxxxxxxxxx::getTwoFactorMethods(); call.

JDEBUG ? JProfiler::getInstance('Application')->mark('- START') : null;
$twofactormethods = xxxxxxxxxxxxxx::getTwoFactorMethods();
JDEBUG ? JProfiler::getInstance('Application')->mark('- <strong>DONE</strong>') : null;

Before PR

Page	Two Auth plugins enabled	Time Taken	Memory used
Frontend Mod login	0	~5.5 ms	0.605 MB
Frontend Mod login	1	~6.6 ms	0.659 MB
Frontend Mod login	2	~7.4 ms	0.714 MB

After PR

Page	Two Auth plugins enabled	Time Taken	Memory used
Frontend Mod login	0	~1.4 ms	0.125 MB
Frontend Mod login	1	~2.4 ms	0.179 MB
Frontend Mod login	2	~3.4 ms	0.235 MB

So at least 4 ms and 0.5MB difference. The same could apply to all the other pages that have the login form (admin login, offline, com_users login).

roland-d · 2016-08-01T19:09:31Z

@andrepereiradasilva Could you resolve the conflicts here please?

andrepereiradasilva · 2016-08-01T21:17:36Z

conflicts fixed. let's hope this additional work will make someone test this ...

roland-d · 2016-08-01T21:26:58Z

@andrepereiradasilva I am going to get it tested tomorrow ;) Thanks.

brianteeman · 2016-08-01T21:28:49Z

Me too

andrepereiradasilva · 2016-08-01T21:32:12Z

thanks guys

brianteeman · 2016-08-02T09:39:23Z

Tested with patch and could login ok in all 4 places using the yubikey

Tested performance of the front-end login module with the supplied instructions

Before

21.05 ms / 573.97 ms Memory: 0.573 MB / 11.59 MB Application: - Time to get twofactorauth methods

After

Time: 9.10 ms / 551.29 ms Memory: 0.161 MB / 11.17 MB Application: - Time to get twofactorauth methods

brianteeman · 2016-08-02T09:39:45Z

I have tested this item ✅ successfully on f279041

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

alimpam · 2016-08-02T11:31:32Z

I have tested this item ✅ successfully on f279041

tested @icampus:

Two Factor Authentication - Google Authenticator (On Google Chrome,local server)

Before PR
Time to get twofactorauth methods : 0.18 ms
After PR:
Time to get twofactor methods: 0.15 ms

Although there is a small difference (probably because of my machine) there is a difference
I tested it several times

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

roland-d · 2016-08-02T12:28:01Z

RTC as we have 2 successful tests

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

@mbabker

…oomla#8951) * improve get two factor authentication in mod_login (site & admin) and offline template * uniformize spaces * use php 5.3 compatible syntax thanks again @mbabker * the same for frontend component user view login too * continue using onUserTwofactorIdentify trigger * cs - empty line at the end of file * fix conflicts 1 * fix conflicts 2 * fix conflicts final * __DEPLOY_VERSION__ * improve code blocks

@mbabker

…oomla#8951) * improve get two factor authentication in mod_login (site & admin) and offline template * uniformize spaces * use php 5.3 compatible syntax thanks again @mbabker * the same for frontend component user view login too * continue using onUserTwofactorIdentify trigger * cs - empty line at the end of file * fix conflicts 1 * fix conflicts 2 * fix conflicts final * __DEPLOY_VERSION__ * improve code blocks

andrepereiradasilva added 2 commits January 20, 2016 19:10

improve get two factor authentication in mod_login (site & admin) and…

4a123a0

… offline template

uniformize spaces

a01415e

joomla-cms-bot added the PR-staging label Jan 20, 2016

mbabker reviewed Jan 20, 2016
View reviewed changes

libraries/cms/authentication/helper.php Outdated

Copy link
Copy Markdown

Contributor

mbabker Jan 20, 2016

Choose a reason for hiding this comment

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

You're using PHP 5.4 syntax here.

andrepereiradasilva added 2 commits January 20, 2016 20:10

use php 5.3 compatible syntax

d70d32d

thanks again @mbabker

the same for frontend component user view login too

8f3f213

andrepereiradasilva changed the title ~~[login forms] bug correction and performance improvements in two factor authentication~~ [login forms] performance improvements in two factor authentication Jan 20, 2016

andrepereiradasilva added 2 commits January 21, 2016 22:09

continue using onUserTwofactorIdentify trigger

b3e1a4e

cs - empty line at the end of file

3fa6f10

andrepereiradasilva added 5 commits August 1, 2016 22:13

fix conflicts 1

0db43a5

fix conflicts 2

df3f400

Merge branch 'staging' into twofactor-performance

99ca187

fix conflicts final

3d85df6

__DEPLOY_VERSION__

14b35fe

improve code blocks

f279041

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 2, 2016

roland-d added this to the Joomla 3.6.2 milestone Aug 2, 2016

rdeutz merged commit fd97974 into joomla:staging Aug 13, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 13, 2016

Uh oh!

Conversation

andrepereiradasilva commented Jan 20, 2016 • edited by roland-d Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Performance before PR (frontend mod_login as example)

Performance after PR (frontend mod_login as example)

How to test

Observations

Uh oh!

mbabker Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jan 20, 2016

Uh oh!

wilsonge commented Jan 21, 2016

Uh oh!

andrepereiradasilva commented Jan 21, 2016

Uh oh!

mbabker commented Jan 21, 2016

Uh oh!

andrepereiradasilva commented Jan 21, 2016

Uh oh!

andrepereiradasilva commented Jan 24, 2016

Before PR

After PR

Uh oh!

roland-d commented Aug 1, 2016

Uh oh!

andrepereiradasilva commented Aug 1, 2016

Uh oh!

roland-d commented Aug 1, 2016

Uh oh!

brianteeman commented Aug 1, 2016 via email

Uh oh!

andrepereiradasilva commented Aug 1, 2016

Uh oh!

brianteeman commented Aug 2, 2016

Before

After

Uh oh!

brianteeman commented Aug 2, 2016

Uh oh!

alimpam commented Aug 2, 2016

Uh oh!

roland-d commented Aug 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andrepereiradasilva commented Jan 20, 2016 •

edited by roland-d

Loading