[D7] PKI Authentication

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

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.

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

And of course, it needs some 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.

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

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

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

Thanks

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 I've been watching this for a while and would like to help you get it promoted.

Let me know when you have something.

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.

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...

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.

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!

Waiting for #19.

Sorry.

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.