Twilio Conferencing

Welcome!

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Repository seems empty? Git clone fails:
fatal: http://git.drupal.org/sandbox/caiovlp/1509002.git/info/refs not found

Repository viewer looks empty too: http://drupalcode.org/sandbox/caiovlp/1509002.git

Coding Standard looks good.
You should implement hook hook_help() http://api.drupal.org/api/drupal/modules!help!help.api.php/function/hook... because developer would like to read installation process in site as you have configuration on SID, TOKEN.
So it will help developer to read installation process from site rather than readme.txt
else Nice Work

/**
  * Implement hook_help()
  */
function yourmodulename_help($path, $arg) {
switch ($path) {
case 'admin/help#yourmodulename':
   $output = '';
   $output .= '<h3>' . t('About') . '</h3>';
   $output .= '<p>' . t("A helpful entry about your module") . '<p>';
return $output;
   }
}

Need Review http://drupal.org/node/1682682

@drupawash: how did you manage to examine the code? Did you clone it with git?

Let's leave it at "needs work" until the issue with the repository is resolved.

I have taken a fresh clone and installed in my locally and after manual review Instead of forcing, I'm just suggesting to implement hook_help().

This si just confusing. Which is the correct address to clone the repo?

This is not RTBC at all, there are a lot of stuff you still have to do get this as RTBC.

First of all, give us the address to the sandbox that anyone could use - the address you pasted is not availble for us other users, and git clone http://git.drupal.org/sandbox/caiovlp/1509002.git twilio_conferencing that is on the sandbox page does not work ("fatal: http://git.drupal.org/sandbox/caiovlp/1509002.git/info/refs not found: did you run git update-server-info on the server?")

Washim, how did you clone it?

The clone command only works for your user account. And before this can get a full project we need a working sandbox with working repository instructions and a working repository viewer. So I suggest to simply create a new sandbox, push the code there and copy the project page.

If you want to keep the history just change the git remote of the old checkout, then you can git push all the commits into the new sandbox.

Assigning myself for manual review.

Manual review:

Master Branch

There are still files in the Master branch, you should only keep a README that points to the right branch/branches

Inlcuding fiel that are not in the module folder

You are inlcuding 'inc/Twilio/Capability.php in twilio_conferencing_block_content. And inc/Twilio.php in twilio_conferencing_mute_participant. There are no inc folder in the repo.

Use module prefix for variable names

You should prefix your variables with the module name, so it should be twilio_conferencing_account_sid instead of twilio_account_sid for example.

Also there are hard coded stuff like: $client = new Services_Twilio($account_sid, $auth_token, '2010-04-01');, why?

Removing review bonus.

The project page is short, and does not explain so much about how it works, what Twilio conferencing is etc. You should also say that it is needed to download the libraries (some of us forget to check the README files ;-))

You really should not place stuff in the modules inc folder, that is bad Drupal-practice, you should use the libraries folder for that - look into the libraries module, and also you should add the check for the Twilio libraries in the install file.

Please tell us what you have done regarding #18. We need communication to get this going.

There are several places where you check_plain on receiving variables before doing something else with them, like inserting into a database. check_plain has a specific purpose: removing xss attacks by encoding user-supplied data before printing that data in an html context (i.e. the browser). If you check_plain a variable when you receive it then your later use of the variable in a non-browser context might be incorrect. I suggest removing any check_plain of incoming variables and instead doing appropriate filtering when (if) those variables are printed back to the browser. See http://drupal.org/node/263002 and http://drupal.org/writing-secure-code for more details.

I also noticed that most of the callbacks take action (update and delete queries) based on user input but do not validate user intent - this is a CSRF vulnerability. The impact appears to be reduced by the fact that an attacker would have to know the callSid which seems likely to be private. However, it's best to protect these actions against CSRF. The typical way to do that in Drupal is by using tokens - either via the Form API or manual calls to drupal_get_token/drupal_valid_token. See http://drupalscout.com/tags/csrf for more details.

First: I haven't installed the module b/c I don't want to setup a whole twilio account, so I apologize for not having full details of these ideas.

Security related issues

If you do need to use check_plain to format things before printing them in XML I suggest doing the check_plain just before printing as well.

For /twilio/mute-participant/%/% - I think you could trick a user with the permission "twilio_conferencing_conference_mute_others" into visiting the url and unmuting the line for someone else, right? To trick a user into visiting the url for my UID I could use an img with a src tag like src="http://example.com/twilio/mute-participant/36762/false".

/twiml POST
This comes from Twilio's service, so the user is not able to spoof it.

How do you guarantee that it's coming from the service? What would prevent me as a malicious user from making POSTS to that url? It seems like the only damage is bad data in the database (and they'd have to guess the user_token, which is somewhat random) so it's not a huge deal to get more validation that this is from the Twilio server, but if there is some way you can do that, please consider it.

Other suggestions

I noticed you're creating the user_token with the md5 function. While md5 might be appropriate for this purpose, it's ideal if you can avoid it altogether (there are some environments where anything with an md5 in it is immediately rejected, so it's just easier to avoid). It also seems like there's not much real randomness in the hash. I suggest using drupal_get_token with a string like twilio_conference or similar.

One other note: the database table name of 'conference' is likely to conflict with other modules. I suggest twilio_conference or something similarly prefixed.

Hi @Misc,
I have test this module cloning from http://git.drupal.org/sandbox/caiovlp/1687816.git
I'm confused because repository which you have specified(http://git.drupal.org/sandbox/caiovlp/1509002.git) and which I tested(http://git.drupal.org/sandbox/caiovlp/1687816.git) completely different.

I'm not doing manual test again but the maintainer should think about the valid repository link in project page.

Regarding md5: it is no longer used in Drupal 7.

Please add your reviews of other projects to the issue summary as outlined in #1410826: [META] Review bonus.

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

manual review:

1. twilio_conferencing_schema(): please add a description to each field in the table and the table itself.
2. twilio_conferencing_mute_participant(): access checks should be done in an access callback that you define in hook_menu(). Same for your other callbacks, user_is_logged_in() is not an appropriate access callback.
3. twilio_conferencing_block_view(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
4. twilio_conferencing_twiml(): do not use exit(), print the output and use drupal_exit() instead. Also elsewhere.
5. twilio_conferencing_twiml(): "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from http://drupal.org/node/28984 . Do not write check_plain()ed values to the database. You are not printing them here, so you don't need check_plain().
6. do not use jQuery(document).ready(), use Drupal.behaviors. See http://drupal.org/node/756722

Although you should definitively fix those issues they are no application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

No objections in more than a week, so ...

Thanks for your contribution, caiovlp!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.