miiCard is an identity assurance service that proves your identity to the same level as a passport or other photo ID but purely online. This D7 module integrates with the miiCard service to allow you to sign into your Drupal site using a miiCard account, attach a miiCard to an existing account and advertise your identity verification status.

Comments

PA robot’s picture

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.

pranit84’s picture

Category: task » bug
Status: Needs review » Needs work

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

pranit84’s picture

Category: bug » task

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

pablissimo’s picture

Status: Needs work » Needs review

Hi Pranit,

Thanks for looking, very much appreciated. The warnings you highlighted have been resolved by removing the offending lines, as they weren't giving quite the right behaviour in some cases anyway.

Good spot on the master branch - had thought I'd already done this, but had managed to get the remote repo in an unusual state so had to act as per http://stackoverflow.com/a/4949821/677173 to tidy it up. Now deleted.

Re-marking as Needs Review.

Paul

pranit84’s picture

Status: Needs review » Needs work

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

pablissimo’s picture

Status: Needs work » Needs review

The errors you mentioned I had already suggested were false-positives in my initial post. I think they're coming from issue #1779020 in Coder: 'Invalid match for "Using the e flag in preg_match"'. The offending lines are all variants of the following, which doesn't use the /e flag:

preg_match('/\/Date\((\d+)\)/', Util::TryGet($hash, 'LastVerified'), $matches);

If you believe this not to be a false-positive then can you please indicate what you think the cause is?

The other warnings have been resolved.

Thanks,

Paul

zterry95’s picture

miicard.admin.inc

Line 36

add t() for #description.

pablissimo’s picture

Thanks, that slipped through the net and has been addressed in latest code.

joachim’s picture

Status: Needs review » Needs work

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!

pablissimo’s picture

Brilliant stuff Joachim, thanks - will push back to our devs and get things fixed up

pablissimo’s picture

Status: Needs work » Needs review

So I've made some substantial changes (with a little help) and think I've addressed most of the issues raised by Joachim. Changed to use Libraries API for vendor library dependency, changed vendor library to use namespaces, removed support for Profile module (with a view to introducing Profile2 support when requested), hopefully improved themeability, addressed the line-items raised above.

Wasn't sure about this one though:

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

What sort of things should I be looking out for here?

joachim’s picture

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.

pablissimo’s picture

Thanks Joachim, in a previous commit I removed the fields that needed explicit submit-handling (they related to a feature I cut), and all of the form fields are prefixed with 'miicard_' so I think I'm doing right by system_settings_form just now but will keep an eye out in future.

kscheirer’s picture

Status: Needs review » Needs work

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.

joachim’s picture

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

pablissimo’s picture

Thanks for your comments both - I'd like to have a go at addressing the role issue below:

The role approach is only a utility to let the administrator section off parts of the site to people who've validated their identity with the service, but conceptually it's little different than any other role (so the idea that the administrator could put anyone into the role irrespective of verification status doesn't sit too badly with me - it's their site).

Importantly the core verification status of a user - in terms of whether they get the little 'tick' icon and have the associated property set against them in the database accessible by code - is unrelated to the role.

However, clearly if the role names are up for grabs then that's a different problem. Would a more appropriate approach be to allow the administrator to select which role to add verified users to (if, indeed, any), and store the role ID rather than the name as a configuration setting?

Overall it's not a feature I'm wed to, especially if it's so fragile in its implementation, but I'd rather try to steer it the right way than remove it if possible.

kscheirer - I'll address those issues, thanks for spotting them - it looks like my local Coder setup's missing a few options as I'd have expected it to flag that.

pablissimo’s picture

Status: Needs work » Needs review

I've made updates to address the ventral issues mentioned and it now (again) passes without errors or warnings.

On the roles - I've changed things around so that:

  • No roles are created on install or anywhere else
  • You can configure to add miiCard-verified users to a role of your choice, defaulted to off, and reimplemented to use the RID instead of a role name
  • You can configure that users should be removed from that role if they remove their miiCard link or the cron job detects that their identity is no longer assured

I've also updated both the readme and sandbox page to highlight that there's nothing to prevent an administrator from adding arbitrary, unverified users to the role.

pablissimo’s picture

Priority: Normal » Major

Bumped priority one notch for > two weeks inactivity as per Review Process guidelines and with grateful thanks to all reviewers so far whose input has been very constructive.

kscheirer’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

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.

klausi’s picture

Assigned: Unassigned » klausi

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

klausi’s picture

Assigned: klausi » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +PAReview: security

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