This module allows a user to register and then login to a Drupal 7 site using a smart card. There is out-of-the-box support for DOD Common Access Card (CAC) as well as a hook to allow for validation of other types of smart card such as Soft Certs, PIV or any other PKI certificate.

http://drupal.org/sandbox/rickwelch/1663258

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/rickwelch/1663258.git pki_authentication

Thank you in advance for your review and feedback.

-Rick

Comments

ankitchauhan’s picture

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxrickwelch1663258git

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

sanchi.girotra’s picture

Status: Needs review » Needs work

Hi,
Manual Review of your module:
1. In .module file

 $form['pki_auth'] = array(
      '#markup' => "<br /><br />Access to this site requires use of a Common Access Card (PKI). To extract the information from your PKI you must click the link: <br /><br /> &nbsp; <b><a href='$url'>Extract PKI Data</a></b> <br /><br /> Once the username and email address fields are populated, the create account button will be enabled.<br /><br /> ",
      '#weight' => -20,
    );

$form['account']['mail']['#description'] = 'Email address is pulled from PKI or Smart Card.';

$form['account']['name']['#description'] = 'Username is pulled from PKI or Smart Card. Please click on the link above to populate this field';

$form['links']['#markup'] = '<div class="item-list"><ul><li class="first"><a href="' . $url . '" title="Request a new user account.">Request New Account <br />PKI Required</a></li></ul></div>';

All user facing text must be with in t() for translation;

2. In .module file drupal_goto(''); : See no use.

3. Use system_settings_form() in your form constructor pki_authentication_settings_form($form_state) then you don't need your submit callback at all as it automatically saves the data in the variable table.

rickwelch’s picture

Status: Needs work » Needs review

Hi,

Thank you so much for reviewing this module and sorry for the delay getting back to you. Please review this again as I was in the process of renaming the two sub-modules pki_authentication_template and pki_authentication_cac as suggested by the first automated reviewer. Am not sure what the state of those were when you reviewed this.

For #1, I am embarrassed that I missed those, thank you for catching them. For #2 the drupal_goto's need to be there to redirect to the front page after login and/or registration. I have changed them to drupal_goto('/') to make that more clear, hopefully. For #3 I had no idea about this, thank you for showing it to me. Will be sure to use that on all future modules.

Thank you again for your time and effort.

Cheers!

-Rick

dSero’s picture

Hi Rick,

This is a very good idea for a Drupal module. I sure many enterprises will be interested in such kind of a module.

1. You still have issues with the automated report. See: http://ventral.org/pareview/httpgitdrupalorgsandboxrickwelch1663258git. Please make sure that check it at every commit.
2. README.txt: You may better remove spaces at: PKI ( Public Key Infrastructure )
3. README.txt: There are lines at the file that are longer than 80 chars
4. pki_authentication.module: pki_authentication_menu(), all strings should use $t = get_t(); and $t('...') for all strings to keep localization support
5. pki_authentication.module: pki_authentication_user_block() {. There is no need to for the "else" in the if ($user->uid > 0) {, since if the case is true it results with a return
6. pki_authentication.module: pki_authentication_user_block() {. You used the LOGIN and LOGOUT texts without t('')
7. pki_authentication.module: pki_authentication_login($token = ''). From a security point of view, it's better to check invalid case, print the message and return, rather than having a canonical if.
8. pki_authentication.module: #211, missing t('')
9. pki_authentication.module: pki_authentication_user_register_submit. same remark as in #7

Keep on with this great work,

Best Regards,
Moshe Kaplan
dSero. The Anti AdBlock Creators

dSero’s picture

Status: Needs review » Needs work

And of course, it needs some work

rickwelch’s picture

Status: Needs work » Needs review

Hi Moshe,

Thank you so much for the review and the encouragement. Especially the encouragement, it means a lot! :-)

I believe that all of your comments have been addressed with the following exceptions in the ventral.org review.

First it seems to analyse INSTALL.TXT as if it were code so there are a lot of errors there. Not sure what is required to correct that. Any suggestions would be appreciated.

Also, the ventral script would like me to use proper Drupal functions in the request.php and login.php files. These functions are not available there. The only thing these files need to do is to extract information from the PKI and pass that into Drupal using a one-time-use nonce. Subsequently, they must live in a PKI protected directory outside of the regular Drupal environment so to keep things a clean as possible I only bring up enough of the Drupal stack to access the database. I hope this makes sense.

Again, thank you for your time and review.

-Rick

h3rj4n’s picture

Status: Needs review » Needs work

I didn't found much. There are one or two points.

You're doing a lot of validation in the submit function (line 266). Isn't it possible to do the validation in the validate function so that the error is displayed with the registration form? The form_set_error function doesn't really work at the places you use it right now. You should replace it with the drupal_set_message function at the places you're using it right now.

In the pki_authentication_cac module at line 33 and 45 you use a die to display an error message. Isn't it possible to rewrite this using the drupal_set_message function? The site still continues but displayes an error. A die in a live site / module isn't really nice I think.

rickwelch’s picture

Status: Needs work » Needs review

Hello,

Thank you for the review and feedback. I believe I have addressed the two issues you raise.

The initially submitted module was a work in progress on a development site. It has recently been deployed in an actual production site which required some hardening and a few new features. The module now allows for regular logins as well as PKI logins which is configurable. Other configuration options allow for deployment in a non-standard Drupal environment. Our client required several independent sub-sites, all running on different servers but to be under the same URL to appear to be a single site. There are new configuration options to support this if necessary.

Additionally, we have received a patch from a third party to allow regular logins to only users with specific roles, again all configurable. This extremely useful addition has also been fully integrated.

The new documentation should reflect all of the changes as well.

Thank you once again for your review and feedback.

Cheers!

-Rick

srutheesh’s picture

hi ,

There code style issues found, that are easy to fix:

http://ventral.org/pareview/httpgitdrupalorgsandboxrickwelch1663258git

FILE: /var/www/drupal-7-pareview/pareview_temp/INSTALL.txt
--------------------------------------------------------------------------------
FOUND 505 ERROR(S) AND 6 WARNING(S) AFFECTING 49 LINE(S)
--------------------------------------------------------------------------------
95 | ERROR | Constants must be uppercase; expected THIS but found This
rickwelch’s picture

Hi,

This has been happening since the beginning. The INSTALL.txt file was being checked against actual PHP code rather than documentation and I couldn't figure out why. The problem was a <?php tag within the document which caused ventral.org to think it was code and not documentation.

It now runs clean after removing that string.

Thanks for the poke!

Cheers!

-Rick

jhalak’s picture

Hi Welch,

I have checked out the coding standards through pareview.sh. Some coding related issues are found in the 7x-1x branch. You can check your code style online:
http://ventral.org/pareview/httpgitdrupalorgsandboxrickwelch1663258git-7...

Or install pareview.sh and check it on your server to use it as a drush command see:
http://drupal.org/node/1419988

I haven't checked your module manually but you better fix those issues generated from pareview.sh to get better response from the reviewers.

Thanks
Jhalak

jhalak’s picture

Status: Needs review » Needs work

Ohh, After you fix the code please put the status "needs review" to "needs work".

Thanks

PA robot’s picture

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

rickwelch’s picture

Status: Closed (won't fix) » Needs review
rickwelch’s picture

Status: Needs review » Needs work
bhosmer’s picture

@rickwelch I've been watching this for a while and would like to help you get it promoted.

Let me know when you have something.

PA robot’s picture

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

rickwelch’s picture

Assigned: Unassigned » rickwelch
Issue summary: View changes
Status: Closed (won't fix) » Needs work

There is renewed external interest in this project.

userballot’s picture

Though I am not yet an expert in the (specifically Drupal) review process it looks to me like this is probably pretty close.

1) The handful of issues mentioned by pareview automated tool are just preferred syntax and a 10 minute fix in total.
http://pareview.sh/pareview/httpgitdrupalorgsandboxrickwelch1663258git-7...

2) One thing I do notice is that t() is not used for the two drupal_set_message() calls in:
pki_authentication_cac_pki_authentication_validate()

3) I would generally say more inline comments in the main module file for others to help with potential bugs/issues that come up, and just to be able to track your own code, but that is my own code review standards and I'm not totally certain what the threshold is for that currently in the Drupal community.

Other than that things look pretty good: queries done appropriately to escape per D7 standards, etc...

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.

urdebest’s picture

Hi Rick,

I need implement PKI authentication on Drupal for my organization. I have not had much luck (even after appropriately configuring the module), with getting it to work. When I add some debug statements, it seems that my PKI string is being passed from the "extract_pki" script fine, but when the account is created, the PKI string field (greyed out) is blank. Any ideas?

If you're no longer maintaining this code, please just let me know and I'll use it as a starter for what we need to do...

Thanks!

colan’s picture

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

Waiting for #19.

colan’s picture

Status: Needs review » Needs work

Sorry.

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.