PlgAuthenticationJoomla::onUserAuthenticate inconsistent?

[fanno]

Hi everyone

What is correct? The onUserAuthenticate function is returning nothing .. then true ... then false and empty return;

phpstorm is complaining, i realize it may not be an actual error. but as far as i can tell/understand returns are never used. so why is it used and why is it not consistent if it is to be used.

(i am developer on a open source project where we have a auth plugin, where i am working on implementing the two factor login..)

-Thanks

[Matt Thomas]

Hi Fanno,

The onUserAuthticate method is used as an event that triggers any installed plugins that use that event. What I believe you are seeing is that the User plugins enabled in your Joomla installation differ in what they are returning, if anything. You should see different results by disabling some of them and then authenticating with the site.

I can't tell you why some are returning something, and I don't believe what is returned is ever used. But, if you do see that core plugins are doing this, it would be good to investigate if they can be normalized.

Hope that helps.

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-gene...@googlegroups.com.
To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

[fanno]

I know that is is an event and what it is used for. i just do not understand why it seems there is no "consistency" in what is returned, if it is never to be used should it not always return "void"? 

What i mean with some are returning and others are not. i mean within (PlgAuthenticationJoomla::onUserAuthenticate) the "core" 

see links below:

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L42 return false

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L102 return void

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L139 return void

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L143 return void

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L183 return true

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L192 return false

https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L223 function ends returning nothing (void)

I would expect it to return nothing or something usefull.. i understand what i need is in &$response, and i have no problem with that, but i want my plugin to follow the correct usage. but here i am confused as there is confusion.

My english is not perfect so i hope this post makes up for maybe misunderstandings from the first post.

-Thanks

Den søndag den 26. januar 2014 02.08.04 UTC+1 skrev betweenbrain:

Hi Fanno,

The onUserAuthticate method is used as an event that triggers any installed plugins that use that event. What I believe you are seeing is that the User plugins enabled in your Joomla installation differ in what they are returning, if anything. You should see different results by disabling some of them and then authenticating with the site.

I can't tell you why some are returning something, and I don't believe what is returned is ever used. But, if you do see that core plugins are doing this, it would be good to investigate if they can be normalized.

Hope that helps.

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.

On Jan 25, 2014 5:03 PM, "fanno" <fan...@gmail.com> wrote:

Hi everyone

What is correct? The onUserAuthenticate function is returning nothing .. then true ... then false and empty return;

phpstorm is complaining, i realize it may not be an actual error. but as far as i can tell/understand returns are never used. so why is it used and why is it not consistent if it is to be used.

(i am developer on a open source project where we have a auth plugin, where i am working on implementing the two factor login..)

-Thanks

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

[Bakual]

This is where the call is made:

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/authentication.php#L286

As you see, the return value isn't processed. The only thing that matters is the "$response" object.

So you can return whatever you want, it doesn't matter at all :-)

[fanno]

right, that is my point, just seems to me then that it is just confusing that core onUserAuthenticate returns actual values a bit confusing to me. 

[Matt Thomas]

Thanks for the links, they helps allot. To answer your question as to why it isn't consistent, I would imagine that it has to do with the fact that different people have contributed code to that file over time and no one has corrected the inconsistencies that you have noticed.

In my opinion, I would prefer to return false, rather than void, as it is more clearly written code and easier for others to understand the intent.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-gene...@googlegroups.com.

[Bakual]

I agree with Matt here. Even if we don't use the return value, it may help to return true (success) or false (failed) to make the code better to understand.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsubscribe@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

[Sergio Manzi]

On 2014-01-26 16:17, Bakual wrote:
> This is where the call is made:
> https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/authentication.php#L286
>
> As you see, the return value isn't processed. The only thing that
> matters is the "$response" object.
> So you can return whatever you want, it doesn't matter at all :-)

... then, if I'm not mistaken, all the code from line 178 to line 193 is
useless and could be replaced with a simple "return;"...

[Bakual]

It may be that these are called from 3rd party extensions and there it may matter ;)

[morten hundevad]

regardless of how you look at it. i think that the phpDOC on onUserAuthenticate should match what is going on.

2014-01-27 Bakual <werbe...@bakual.ch>

--
You received this message because you are subscribed to a topic in the Google Groups "Joomla! General Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/joomla-dev-general/08MfD2V11Bc/unsubscribe.
To unsubscribe from this group and all of its topics, send an email to joomla-dev-gene...@googlegroups.com.

[Bakual]

Feel free to improve it if you think it's wrong.

Am Montag, 27. Januar 2014 21:20:43 UTC+1 schrieb fanno:

regardless of how you look at it. i think that the phpDOC on onUserAuthenticate should match what is going on.

2014-01-27 Bakual <werbe...@bakual.ch>

It may be that these are called from 3rd party extensions and there it may matter ;)

Am Montag, 27. Januar 2014 16:37:32 UTC+1 schrieb Sergio Manzi:

On 2014-01-26 16:17, Bakual wrote:
> This is where the call is made:
> https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/authentication.php#L286
>
> As you see, the return value isn't processed. The only thing that
> matters is the "$response" object.
> So you can return whatever you want, it doesn't matter at all :-)

... then, if I'm not mistaken, all the code from line 178 to line 193 is
useless and could be replaced with a simple "return;"...

--
You received this message because you are subscribed to a topic in the Google Groups "Joomla! General Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/joomla-dev-general/08MfD2V11Bc/unsubscribe.

To unsubscribe from this group and all of its topics, send an email to joomla-dev-general+unsub...@googlegroups.com.

[morten hundevad]

I guess i will pout it on my todo list my curent setup to run phpunit is no longer working i think. =P

-Thanks

2014-01-27 Bakual <werbe...@bakual.ch>

To unsubscribe from this group and all of its topics, send an email to joomla-dev-gene...@googlegroups.com.

[Sergio Manzi]

--

You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-gene...@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

If this is the case this should be documented or one day someone could try to be smart and just say "pew!", replace it with a "return;" and cause a regression in some 3d party extension...

I mean, if this is really the case, it should be up to the 3d party extension developer who relies on this code to put at least a small comment in it or (even better) amend the docs so that they reflect the state of things, don't you agree?

[Matt Thomas]

Hi Sergio,

That is a great idea. I think it would be very helpful if 3PD were to document their usage like this. Would adding a Docblock help here?

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.

[Sergio Manzi]

Hi Matt,

to be honest I'm not that familiar with Docblocs to express an opinion. My understanding was that docblocs  were conceived to document functional elements (functions, classes, etc.) and not just "a piece of code", but I may be easily wrong...

Out of curiosity, do we know which 3d party extension uses this non-standard return codes? Also (and I *really* hope nobody will take this as a personal attack) I must say that I'm a bit surprised that a piece of code needed solely by a 3d party extension has its home in the Joomla! core: from a pure logical point of view I
think that if this is of general interest it should be officially accepted and documented or otherwise... just not be there. Not to be mistaken, I'm saying this also in the interest of the (unknown to me at this point) 3d party developer, who is otherwise risking to see his needed code scrapped out.

[Bakual]

Out of curiosity, do we know which 3d party extension uses this non-standard return codes? Also (and I *really* hope nobody will take this as a personal attack) I must say that I'm a bit surprised that a piece of code needed solely by a 3d party extension has its home in the Joomla! core.

I didn't say there is such an extension. I said there may be one. We just don't know.

Btw: The docblocs for all core authentication plugins say they return a boolean, while the unit test fake plugin says it returns void

So there is an inconsistency, but I think it doesn't matter at all.

For core, we don't need a return value. But for the understanding of the code, it may help to have one. So no harm done if there is a return value, but also if there is none. Personally I don't bother to change that, there are so many other things which need testing and fixing :D

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

If this is the case this should be documented or one day someone could try to be smart and just say "pew!", replace it with a "return;" and cause a regression in some 3d party extension...

I mean, if this is really the case, it should be up to the 3d party extension developer who relies on this code to put at least a small comment in it or (even better) amend the docs so that they reflect the state of things, don't you agree?

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

[Sergio Manzi]

Hi Bakual,

I'm not sure this is the place to discuss such issues and if it is not, please forgive me and point me to the right place:

I may be really OT, but the problem with that piece of code (https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L178-193) is not that it returns invalid/inconsistent/undocumented values, but that, IMHO (with a really big "H"), it is just wrong code:

I traced that code to be something introduced by elinw on last October as a result of #31561 (Add support for BCrypt encryption of passwords and rewrite cookie based authentication to use it, Fix #2101).

I *think* that Elin's intention was to return a non-authenticated state ($response->status = JAuthentication::STATUS_FAILURE;) in the "else" case, but she probably forgot to put it before the "return;". I'm also wondering if a "$response->status = JAuthentication::STATUS_SUCCESS;" is needed in the "true" case of the test (probaly, not, it should already has been set...)

Sergio

On 2014-01-28 07:49, Bakual wrote:

[Bakual]

Yes, this looks indeed wrong. Just returning without setting the $response isn't doing anything good. However I'm not sure if that code is really used. It looks related to TFA and since TFA is actually working, I doubt there is an issue with it. Either it's correct or isn't used :)

[Sergio Manzi]

It seems to be triggered by an unusual condition, when "Two factor
authentication enabled and no OTEPs defined", so it *may* be that it
normally works but then...
Is there a way to bring this to Elin's attention?

[Bakual]

Maybe better ask Nicholas from Akeeba. I think he was the one who actually wrote the whole TFA code.

[Sergio Manzi]

OK, I have his e-mail: I'll ping him...
tnxs!
Sergio

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-gene...@googlegroups.com.

[Sergio Manzi]

Hi Bakual (and all the others following this thread),

I talked to Nicholas and he too thinks the code is broken, but he also added that all the OTEP stuff is already handled (or should be already handled, I didn't got that clearly)  in the com_users model and doesn't even belong to the plugin.

Unhappily Nicholas is really busy at this time and he said a PR is not coming real soon. I'm very busy too and next Monday I will leave for a two months stay abroad where I will not have any Internet connection (or at least on very rare occasions).

So I'm asking the favor, to you or anybody else who is willing, to open a bug so that this matter will not fall forgotten.

Thanks!

Sergio


On 2014-01-28 13:52, Bakual wrote:

[Matt Thomas]

Hi Sergio,

Thanks. I'd be happy to open an issue for you (and, yes, I too think this code is broken). Would you mind doing me the favor of writing a one or two line summary of it?

Best,

Matt Thomas
203.632.9322
http://betweenbrain.com/

Sent from mobile. Please pardon any typos or brevity.

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-gene...@googlegroups.com.

[Sergio Manzi]

OK, I will do that, but not right now, sorry. I will definitely do that before the week-end, anyway. Thanks!

[Matt Thomas]

Not a problem. I'll open an issue and post the link here.

Best,

Matt Thomas
203.632.9322
http://betweenbrain.com/

Sent from mobile. Please pardon any typos or brevity.

[Matt Thomas]

Hi Sergio,

I've created an issue for this at https://github.com/joomla/joomla-cms/issues/2946 so that it doesn't get lost. Thanks for your careful attention!

Best,

Matt Thomas

203.632.9322
http://betweenbrain.com/

[Sergio Manzi]

Matt, here is brief summary of the issue:

1. The issue regards the Joomla Authentication Plugin (/plugins/authentication/joomla/joomla.php)
2. The issue has been discovered by analyzing the code and, AFAIK, there is no known case where this has created real problems.
   
3. The issue regards OTP authentication and is found  from line 178 to line 193 (https://github.com/joomla/joomla-cms/blob/staging/plugins/authentication/joomla/joomla.php#L178-193), but probably also the surrounding code needs a review. (see below, point 5)
   
4. The main problem is that at line 192 there is a "return false;" statement implying a non-authorized result, but this has no effect as a non authorization is achieved not by a "return false;" but by setting "$response->status = JAuthentication::STATUS_FAILURE;"
5. According to Nicholas Dionysopoulos (who is, if I'm not mistaken, the main author of the TFA code) this OTP checks belongs not to the plugin but to the com_users model

As a personal note I would add that:

1. the onUserAuthenticate is declared in the Docbloc to return a boolean, but in fact its function is performed not through its return value but on the value of the $response (as a matter of fact unit test seems to expect a void return).
   
2. According to some returning a boolean can clarify the intent of the code
3. IMO it makes things less clear and it open the road to the false assumption (as in this case) that it is the return value that has "value" while instead it has none.

Cheers!

Sergio

P.S.: I'm also sending this in Bcc: to you and to Nicholas

[Sergio Manzi]

woops... I just sent you an e-mail with the detalis of the issue...  :-)

[Matt Thomas]

No problem, I'll update it. Thanks!

Best,

Matt Thomas

203.632.9322
http://betweenbrain.com/

[Sergio Manzi]

Thank you! :-)

[Matt Thomas]

Thank you!

Best,

Matt Thomas

203.632.9322
http://betweenbrain.com/

[fanno]

I guess this could be a dumb question. but the two factor check would it not be best to have this check AFTER onUserAuthenticate so that 3rd party plugins do not need to add code to support two factor...

pehaps make a new event

EG: onUserAfterAuthenticate

That are called if auth is sucess refardless and then have a two factor plugin that has onUserAfterAuthenticate, this would mean auth plugins do not need to add support for it them self, and every plugin don't need to change the code if two factor code has to be changed.

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/authentication.php#L289 could be something like this

// If authentication is successful break out of the loop
if ($response->status === self::STATUS_SUCCESS)
{
		$app = JFactory::getApplication();

		$app->triggerEvent('onUserAfterAuthenticate', array($credentials, $options, &$response));
.....
.....
.....

    if (empty($response->type))
    {
        $response->type = isset($plugin->_name) ? $plugin->_name : $plugin->name;
    }
    break;
}

Tho i guess it is poosible that this can cause other issue i am not thinking about at this point in time. issue could possible be that $credentials may not hold info that can be used inside onUserAfterAuthenticate to get the joomla use and that $response->email may properly be better way inside two factor onUserAfterAuthenticate to read the.

-Thanks

Thank you!

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.

To post to this group, send an email to joomla-de...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-general.
For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "Joomla! General Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-general+unsub...@googlegroups.com.