[D7] miiCard

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

FILE: /var/www/drupal-7-pareview/pareview_temp/miicard-card.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
42 | WARNING | Code after BREAK statement cannot be executed
43 | WARNING | Code after BREAK statement cannot be executed
54 | WARNING | Code after BREAK statement cannot be executed
55 | WARNING | Code after BREAK statement cannot be executed

Remove the 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

Hi Paul,
Go through the "http://ventral.org/pareview/httpgitdrupalorgsandboxpablissimo1968606git" link to see your project related issues.
Some of them are:

FILE: ...www/drupal-7-pareview/pareview_temp/miiCard.Consumers/miiCard.Model.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
964 | ERROR | Using the e flag in preg_match is a possible security risk. For
| | details see http://drupal.org/node/750148
972 | ERROR | Using the e flag in preg_match is a possible security risk. For
| | details see http://drupal.org/node/750148
1076 | ERROR | Using the e flag in preg_match is a possible security risk. For
| | details see http://drupal.org/node/750148

-Pranit

miicard.admin.inc

Line 36

add t() for #description.

Here's an eyeball review.

Theme template:

>

print "{$miicard->getFirstName()} {$miicard->getLastName()}";

- Don't put function calls in TPLs. Dose us your $variables in a preprocessor.
- Indent your code in the TPL for if statements

> * Implements hook_admin_settings().
> * Implements hook _admin_settings_form_submit().

- what are these hooks?? It's a hook if there's documentation on api.d.org for hook_foo(). Otherwise, it's not a hook!!

> miicard_admin_settings_form_submit

Be careful of interaction between this and the submit handler system_settings_form() adds. system_settings_form() is the right way to go though, BTW.

> miicard.entity.inc

The entity controller is usually put in includes/MODULE.controller.inc. See entity API module.

> 'description' => $t('Consumer key and secret must be provided. !link', array('!link' => l($t('Settings page'), 'admin/config/people/miicard'))),

Read docs on t() for how to put a link into text like that. You should be using url(), not l().

  variable_set('miicard_oauth', MIICARD_OAUTH);
  variable_set('miicard_api', MIICARD_API);
  variable_set('miicard_add_link', MIICARD_ADD_LINK);
  variable_set('miicard_sign_in_link', MIICARD_SIGN_IN_LINK);
  variable_set('miicard_allow_create_accounts', MIICARD_ALLOW_CREATE_ACCOUNTS);
  variable_set('miicard_highlight_approved_users', MIICARD_HIGHLIGHT_APPROVED_USERS);

Things like this are a pet hate of mine.
Some of these constants are only used once, in which case they serve no purpose at all.
Some, like MIICARD_SIGN_IN_LINK seem to be output when presumably the user's override of that should be output.
Finally, because of the way the variable system works, you really don't need to set them on install -- that's what the second param of variable_get() is for.

> // Create a user role for miiCard verified users if it doesn't exist.

Have you considered the UX of having this?

> $queue->createQueue();
> $queue->deleteQueue();

AFAIK you don't need this in hook_(un)install() -- there's a hook to define it, no?

> function miicard_update_7004() {

What happened to 7001-7003?

> _miicard_get_registration_mode_wrapper

Document return value as a @return.

> miicard_block_info

Purely cosmetic, but space these out so it's easier to see how many blocks you're defining. I'd probably use arrray() for each block, but that is just my preference.

> return l($text, 'user/login/miicard', array('attributes' => array('class' => array('miicardLoginButton'))));

Drupal CSS classes tend to follow the pattern 'foo-bar', ie with hyphens not camel case.

> _miicard_identity_save_profile

- this needs documentation (in fact, anything that is not a hook needs @param and @return and possibly @throws)
- I can't see anywhere where you're actually making use of its return value. Save some effort and remove that -- it means you can lose the faffy $error variable you're carrying around the function -- just return inside the catch block.

> _miicard_identity_save

Unused $error.

/**
 * Get text for sign in notice.
 */
function miicard_sign_in_text() {
  return variable_get('miicard_sign_in_link', MIICARD_SIGN_IN_LINK);
}
/**
 * Get text for add link.
 */
function miicard_add_link_text() {
  return variable_get('miicard_add_link', MIICARD_ADD_LINK);
}
/**
 * Get text for remove link.
 */
function miicard_remove_link_text() {
  return variable_get('miicard_remove_link', MIICARD_REMOVE_LINK);
}

These are just silly! (It's pre-breakfast, I'm low on coffee! ;)

> * Make miiCard profile available to theme engine.
> * Add data for use by author_pane.

These should describe what you're actually theming. See core theme functions for examples.

> $comment->verified_author = FALSE;

I'm not 100% on how comment works, but isn't that a bit draconian?

> function _miicard_load_user(&$profile) {

Given you also work with Profile module (and I bet you'll soon get feature requests to work with Profile2), I'd be really wary of calling your own variable $profile!

> class Identity extends Claim {

You should prefix your classes with your module name to avoid clashes. If this is vendor code that's common to other integration modules, maybe investigate using a PHP namespace for it?

> require_once 'miiCard.Model.php';

Not needed if your file is declared in the .info file -- that registers classes.

That should be enough to go on for the time being :)
Document all your functions with @param and @return!

What I probably meant is this: system_settings_form() is a magic helper for settings form, and the way it works is that it takes EVERY item in the form values, and saves a system variable for it, named the same as the value key. This means that:

* you should be careful what you name your form items, as you will otherwise end up with system variables that are not prefixed with your module name. Or worse, you could clobber another module's variables.
* if you have form elements that you are saving yourself, in your own submit handler, because they require processing, then you need to take care that SSF doesn't also save them.

You should remove the .gitignore file from the repo. I'm not sure why you need to create a role in miicard_install()? Generally in drupal we create a permission and then check to see if any of your roles has been assigned that permission.

You have a a few errors from ventral, which unfortunately look like a security problem so should be fixed: http://ventral.org/pareview/httpgitdrupalorgsandboxpablissimo1968606git.

Otherwise it looks great. I'm not qualified enough to evaluate the security of your approach but you'll get an RTBC from me if you fix the ventral errors.

----
Top Shelf Modules - Enterprise modules from the community for the community.

> I'm not sure why you need to create a role in miicard_install()? Generally in drupal we create a permission and then check to see if any of your roles has been assigned that permission.

Well spotted!

Also, this is going to totally break:

define('MIICARD_USER_ROLE', 'miiCard Verified');
....
    $miicard_role = user_role_load_by_name(MIICARD_USER_ROLE);

Roles can be customized by the site admin. That means the site admin is free to change the name of that role. Which then means all of your code that depends on the MIICARD_USER_ROLE constant will break.

Also, it looks like you are using that role to mark users as having their miicard identity verified. This is also a mistake -- roles can be assigned and removed at will by admin users! Who will thus very easily break your system.

If you want to flag users in some way, you need to come up with something else!

You have a couple unused variables reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxpablissimo1968606git. But otherwise your new role approach looks good, thanks!

----
Top Shelf Modules - Crafted, Curated, Contributed.

I'll look at this now in the Project applications sprint

manual review:

1. miicard.info: if you require PHP 5.3 you should specify it here, see https://drupal.org/node/542202
2. miicard_init(): why do you need hook_init()? This will be executed on every single page request and you have hook_requirements() anyway for the library check? And the CSS should only be added if it is needed.
3. miicard.module: there are still page callbacks like miicard_remove() in that file, but should be in *.pages.inc?
4. miicard_remove(): this is vulnerable to CSRF exploits. user/miicard/delete is defined in hook menu without confirmation form and I cannot find any CSRF token check in the callback either. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . This is a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. miicard_preprocess_miicard_template(): this looks vulnerable to XSS exploits, where do you make sure that all the user provided data details are sanitized? See also https://drupal.org/node/28984

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.