?
Referenced as Pull Request for: # 3123
avatar betweenbrain
betweenbrain
29 Jan 2014

From https://groups.google.com/d/msg/joomla-dev-general/08MfD2V11Bc/5FpOQuZYbW4J

  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.
avatar betweenbrain betweenbrain - open - 29 Jan 2014
avatar smanzi
smanzi - comment - 29 Jan 2014

Thanks, Matt!

avatar betweenbrain
betweenbrain - comment - 29 Jan 2014

Thanks, Matt!

Thank you!

avatar brianteeman brianteeman - change - 3 Sep 2014
Category Authentication
avatar vdespa vdespa - change - 13 Sep 2014
Category Authentication Authentication Code style
avatar brianteeman
brianteeman - comment - 2 Jan 2015

Is there going to be a PR for this - it has been over a year since it was first posted.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/2946.
avatar brianteeman brianteeman - change - 2 Jan 2015
Status New Information Required
avatar betweenbrain betweenbrain - close - 2 Jan 2015
avatar zero-24 zero-24 - close - 2 Jan 2015
avatar betweenbrain betweenbrain - change - 2 Jan 2015
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2015-01-02 03:33:12
avatar zero-24 zero-24 - change - 3 May 2015
Labels Added: ?

Add a Comment

Login with GitHub to post a comment