A 2-factor admin authentication method via jabber
https://drupal.org/sandbox/leandro713/2176783

CommentFileSizeAuthor
#3 XMPP.libraries.info107 bytesleandro713
#2 patch.patch3.71 KBdavid_garcia

Comments

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxleandro7132176783git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

david_garcia’s picture

StatusFileSize
new3.71 KB

[0] You are working on master branch, consider moving to 7.x-1-x

[1] Remove version from info file, this is automatically done by Drupal. Also I am not sure what this line is doing at the start of the info file (; $Id$) ??

[2] Do you really need to include the XMPPHP library on ALL request? Not good idea for performance. Consider just including the library in the contexts where you are going to use it. I'm not sure, but moving that into the _send_msg function makes sense as the libraries classes are only used there.

function pin_init() {
if ($path = libraries_get_path("XMPPHP")) {
require $path . '/XMPP.php';
}
}

[3] Revise the libraries module documentation: https://drupal.org/node/1342238 to properly use the API:

- You need to declare what libraries you are using in hook_libraries_info, ore provide an INFO file for the library.
- Library should be loaded using the methods shown in the attached link.

[4] There are many minor coding standard issues, consider using the automated review tool for that.

[5] Although this is a small module, consider moving any "helper code" (such as function _send_msg) into a separate file and including it only when it is used. Also consider renaming the method to avoid conflict with other modules.

Good design pattern is to use PSR-4, this will reduce D8 impedance mismatch and ease future migration. You can get PSR-4 functions in D7 by means of the XAutoload module. I attach a sample patch to give you a hint on how this would work like.

leandro713’s picture

StatusFileSize
new107 bytes

first of all, i am very pleased of your answer david_garcia_garcia :)

i was a bit confused about hook_libraries_info() with xautoload, so i did a .info file for the library (attached),
then applied your patch and i get this error
Fatal error: Class 'Drupal\pin\XMPPHP_XMPP' not found in /var/www/html/prupin/sites/all/modules/pin/lib/PinModuleHelperFunctions.php on line 27
i guess i don't know how to invoke the library class :(

it complains at

<?php
  $conn = new XMPPHP_XMPP(variable_get('pin_server'), ... );
?>
david_garcia’s picture

Just try with a Backslash:

  $conn = new \XMPPHP_XMPP(variable_get('pin_server'), ... );

Using namespaces and PSR-4 at first is quite confusing, what is happening is that it asummes you are calling XMPPHP_XMPP in the current namespace, that is "Drupal\pin\", but the class is actually defined in the global namespace.

Do not take my patch "seriously", It was an untested proposal so you could see how to deal with that if you wanted to adventure into the PSR-4 world.

Greetings,

leandro713’s picture

Yay, i did know what were happening, but didn't know how to solve it :)
I did some changes to de code, moved to a 7.x-1-x branch and created a README file
Hope these were useful

greggles’s picture

This seems like a great solution for added security. I wonder if it could be improved by being a plugin for the tfa module. There is a totp example module. Any thoughts on that?

PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.