[D7]Kandy

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxkandy-io2445545git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

Hello, and welcome to the review process!

Take a look at the pareview results and correct them if feasible as they are warnings not errors.

FILE: /var/www/drupal-7-pareview/pareview_temp/includes/RestClient.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
10 | WARNING | Class name must be prefixed with the project name "Kandy"
16 | WARNING | Class name must be prefixed with the project name "Kandy"

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

Review :-

1. I think there is no need to have hook_install() and hook_uninstall() as drupal takes care of this stuff itself.
2. You have created multiple menu callbacks in module. You should use 'file' attribute in hook_menu() instead of defining callback in module file.
3. You have created a file name kandy.api.php which contains your useful methods. Rename this file as in drupal, *.api.php file is used for documentation/help purpose.
4. In few of your methods, you are getting data from query-string without sanitizing it. Use filter_xss() there for security or XSS.
5. In your info file, I think you have typo "Kanky" => Kandy
6. Instead of creating your own RestClass, you can use drupal_http_request() method.

What I meant in point 1 is that you dont need to write hook_install() to install a table. You just need to have hook_schema() and when you install the module, it will automatically create the table.

For hook_uninstall(), drupal itself will remove the table when you uninstall the module (not disable). So no need to have hook_uninstall() to remove the table. You can check this on your local environment.

As per point 3, you are still using the *.api.php file in wrong context.

Also you don't have to attach zip/tar of your code. You just update your code and push into the repositor.

Thanks

In kandy_api.php,

module_load_include('php', 'kandy', 'kandy_api');

this is irrelevant.

I am removing, PAReview tag now. Please do some more review to add this tag.

That minor problem alone is surely not application blocker, anything else that you found or should this be RTBC instead?

@klausi - I am agree with you as this is not a blocker.

The major problem that I have found (as per my thought) is in this submit handler.

     function kandy_file_edit_form_submit($form, $form_state) {}
   

A you see here, content is collected from $_POST instead of $form_state and I think it is major.

Apart of this, I can still see some minor issues (not blocker).

PAReview: review bonus tag is a way to encourage people to review other project applications and with this, they can learn new things.

Why did you remove my PAReview: review bonus tag?

As per https://www.drupal.org/node/1975228, check What Happens Next section.

Also, as per my comment #16, please check whether you should use $_POST or $form_state.
Also as per @klausi #15, I am not marking is as Needs Work, but please check for your code formatting for variable_get() and set as it is not consistent and also please use t() function wherever you print the output.

manual review:

1. kandy_file_edit_form_submit(): the $_POST usage seems to be a blocker here, why can't you use $form_state['values']? Please add a comment.
2. kandy_file_edit_form_submit(): this looks dangerous, can an attacker write to arbitrary file locations with "../../" parts here?
3. kandy_file_edit_form_submit(): writing to the module folder will fail on most Drupal installations because the webserver user should never be allowed to write to module folders. See https://www.drupal.org/node/244924 . Why can't you use the usual public:// or private:// files folder?
4. kandy_file_edit_form_submit(): filter_xss() is wrong here, since you are not printing the data to HTML. Make sure to read https://www.drupal.org/node/28984 again.

Variable_get() formatting - Some places you have used like this

'#default_value' => variable_get('kandy_api_key', ''),

and some places like this -

    '#default_value' => variable_get(
      'kandy_domain_secret_key',
      ''
    ),
  

So its not a blocker or major thing, but I prefer if you have time then format that.

Removing review bonus tag, you have not done all manual reviews, you just repeated the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Anyone can correct git clone commands in the issue summary of applications, feel free to do so.

Review of the 7.x-1.x branch (commit 9cc6f26):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...htdocs/d7.dev/sites/all/modules/sandbox/pareview_temp/kandy.module
  ---------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  ---------------------------------------------------------------------------
   289 | ERROR | Array indentation error, expected 8 spaces but found 0
   668 | ERROR | Array indentation error, expected 8 spaces but found 0
   683 | ERROR | Array indentation error, expected 8 spaces but found 0
  ---------------------------------------------------------------------------
  UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
  ---------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review :

1. kandy_help : you can also directly
2. kandy_file_edit_form : Instead of building anchor tag here from html use l() function instead.
3. (+) Module script,style and assignment customizations menu paths are opened without any permission which will leads to access by pass.
4. kandy_get_domain_access_token : use drupal_json_encode instead of json_encode.

Assigning to myself for next review and will continue tonight.

Hi :)

You can remove width to "kandyDrupal.css"

Line 31 .kandyButton .kandyButtonComponent input[type='button'] {
Line 32 cursor: pointer;
Line 33 width: 90px; <---------------- you repeat in line 24 :)
Line 34 }

Line 36 - line 58: All display none, add only 1 display:none.

Ej:

.kandyButton .kandyButtonComponent .someonesCalling,
....
.kandyButton .kandyVideoButtonCalling,
.kandyButton .kandyButtonComponent .someonesCalling{
display: none;
}

Line 180 use REM niiiiiceeeeeee :) IE8 not good, but... IE8 xD

Removed one none-manual review.

Review of the 7.x-1.x branch (commit d5413bc):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. project page is too long. Any further detailed information should be on a doc page, leaving the project page with the most important information. See also https://www.drupal.org/node/997024
2. includes/help.php should probably be includes/help.html since it does not contain PHP?
3. kandy_schema(): is the kandy user password stored in plain text here? Please add that to the column description.
4. kandy_assignment_page(): this looks vulnerable to XSS exploits. $rows contains the raw user name populated in kandy_get_user_data(). While user names are usually validated against malicious content when accounts are created it is still considered a security vulnerability if account names are printed unsanitized because user names could have been imported from a third party system or whatever. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. kandy_assignment_page(): why are you talking about date formats here in the #empty text?
6. kandy_menu(): why are there so many unprotected menu entries that any anonymous user can visit and potentially edit stuff? So we have lots of access bypass security issues here where anonymous users can view and edit on those pages. Please replace or document all your "'access callback' => TRUE," entries.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I know @Klausi is busy helping many people so I try to help you about point 1.

Here you can find great tips:
https://www.drupal.org/node/997024
http://growingventuresolutions.com/blog/module-owners-how-make-your-modu...

If I would be in your situation I would do the following:

• Replace this section "Kandy components and shortcode syntax:" with only a list of the features (just the name), and put the rest in the README.
• Move the "All support script callback:" to the README.
• It's enough to say that you have an API and let the rest be detailed in the README.

I am not in charge to say how you should format your project page, these are just some suggestions based on my experience what I have seen on other module pages.

You could also link the README to the project page like this http://cgit.drupalcode.org/sandbox-kandy-io-2445545/tree/README.md

Great work,
Keep up the good work

There is no need to write a hook_uninstall_schema for your module if you have hook_schema then drupal will uninstall your schema automatically when you uninstall your module.

change

/**
 * Implements hook_uninstall().
 *
 * Removes the database tables and persistent variables managed through Drupal.
 *  Anything set in the Kandy Admin Interface will be forgotten.
 *
 * Uninstall the DB schema and clean up as much as is feasible.
 */
function kandy_uninstall() {
  // Drop my tables.
  if (db_table_exists('kandy_users')) {
    drupal_uninstall_schema('kandy');
  }

  variable_del('kandy_domain_name');
  variable_del('kandy_api_key');
  variable_del('kandy_domain_secret_key');
  variable_del('kandy_js_url');
  variable_del('kandy_fcs_url');
  variable_del('kandy_jquery_reload');
}

into

/**
 * Implements hook_uninstall().
 *
 * Removes the database tables and persistent variables managed through Drupal.
 *  Anything set in the Kandy Admin Interface will be forgotten.
 *
 * Uninstall the DB schema and clean up as much as is feasible.
 */
function kandy_uninstall() {
  variable_del('kandy_domain_name');
  variable_del('kandy_api_key');
  variable_del('kandy_domain_secret_key');
  variable_del('kandy_js_url');
  variable_del('kandy_fcs_url');
  variable_del('kandy_jquery_reload');
}

Removing review bonus tag, you have not listed any additional review in the issue summary? There should be at least 6 review links before you add the review bonus tag again. Thanks!

Review of the 7.x-1.x branch (commit 1617d63):

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. kandy_assignment_page(): "@return mixed" should be "@return array", see https://www.drupal.org/coding-standards/docs#functions hook_menu() page router callback functions
2. shortcode dependency is missing in the info file, see https://www.drupal.org/node/542202#dependencies
3. kandy_uninstall(): The drupal_uninstall_schema() call here should really not be necessary, Drupal deletes the table on uninstallation for you.
4. kandy_list_users(): no need for the foreach loop, you can just use ->fetchAll() or similar on the query result. See https://www.drupal.org/node/1251174
5. kandy_logout(): instead of having inline Javascript here you should put that into a dedicated file and pass down settings with Drupal.settings, see https://www.drupal.org/node/756722
6. kandy_publish_assets(): doc block just repeats the function name. What assets get published where? Why is this function necessary?
7. There are still security vulnerabilities present in the code and I'm assigning this to Manjit.Singh as part of our git admin training. I will post the details about the vulnerabilities in one week if he does not find the time.

1. Use the drupal variant of http_build_query() see drupal_http_build_query()
2. The response got from your drupal_http_request() need to be sanitized before use. Example :
   

   <?php
   $response = json_decode($response->data);
     if (isset($response->message) && check_plain($response->message) == 'success') {
       return array(
         'success' => TRUE,
         'message' => '',
         'data' => check_plain($response->result->domain_access_token),
       );
     }
   ?>
   

3. The function kandy_assignment_sync_page() is vulnerable with open redirect because these variable $_GET['returnPath'] and $_GET['id'] are not safely exploited.
4. Sanitise main_user_id and user_id variable before calling kandy_unassign_user or kandy_assign_user in kandy_assignment_edit_form_submit
5. Why not use $form_state['redirect']instead ofdrupal_goto ?
6. Most of vulnerabilities are related to incoming data cleansing and checking. See here to write more secure code.

Check_plain() should only be used when actually printing something to html. If the data is just used for comparisons then no sanitization is necessary.

The point about the open redirects is a good one, I found that too.

Just bypassing...

You can reduce a lot of the code which would allow for a faster code review, manly related to your form menu callback definitions and implementations.

  $items['kandy/get_user_for_search'] = array(
    'title' => 'Get User Name by kandy user id',
    'page callback' => 'kandy_get_user_for_search',
    'access arguments' => array('access content'), # <<< Normally everybody can access this
    'type' => MENU_CALLBACK,
  );

Firstly, is this information in the public domain?

$kandy_user->user_id . "@" . $kandy_user->domain_name;

With this menu callback everybody can see this. Seems to be a few possible areas in question depending on this answer. Was this what custom permission 'access kandy content' was for?

You shouldn't be using exit(), drupal_json_output() does this for you.

-  echo drupal_json_encode($result);
-  exit(0);
+  drupal_json_output($result);

You may be using term as a search key, but the query doesn't use this, and it loads all users.

Recommend limiting this to a query that matches the term AND adding a limit. Running on drupal.org, you would hit millions of returns here!!

db_select()->range(0, 25)

Read up on menu wild card loaders and let Drupal take care of some basic loading... %user will accept a {user}.uid and pass the fully loaded user object via user_load(%).

-  $items['admin/config/content/kandy/assignment/edit/%user'] = array(
+  $items['admin/config/content/kandy/assignment/edit'] = array(
    'title' => 'Kandy User Assignment',
    'page callback' => 'drupal_get_form',
-    'page arguments' => array('kandy_assignment_edit_form'),
+    'page arguments' => array('kandy_assignment_edit_form', 6),
    'access arguments' => array('administer site configuration'),
    'type' => MENU_CALLBACK,
  );
.......

- function kandy_assignment_edit_form() {
+ function kandy_assignment_edit_form($form, $form_state, $user) {
  module_load_include('php', 'kandy', 'kandy_api');
-  $form = array();
-  if (isset($_GET['id'])) {
-    $id = $_GET['id'];
-    $user = user_load($id);
-    if ($user) {
.......
      return $form;
-    }
-    else {
-      drupal_goto(
-        url(
-          'admin/config/content/kandy',
-          array('absolute' => TRUE)
-        )
-      );
-      return array();
-    }
-
-  }
-  else {
-    drupal_goto(
-      url(
-        'admin/config/content/kandy',
-        array('absolute' => TRUE)
-      )
-    );
-    return array();
-  }
-
}

Rather than the all powerful, nearly god like permission, 'administer site configuration', you could define your own :)

  drupal_goto(
    url(
      'admin/config/content/kandy/assignment',
      array('absolute' => TRUE)
    )
  );

Any reason why you don't just use this?

  drupal_goto('admin/config/content/kandy/assignment');

Or when you have the form state:

  $form_state['redirect'] = 'admin/config/content/kandy/assignment';

But make sure you are passing this by reference (&so that it works:

-function kandy_assignment_edit_form_submit($form, $form_state) {
+function kandy_assignment_edit_form_submit($form, &$form_state) {

You should always have this set, so no need to check this nor to have the else { drupal_goto() }

function kandy_form_submit($form, &$form_state) {
  if (isset($form_state['values'])) {

Another possible place for menu placeholders?

-  $items['admin/config/content/kandy/file/edit'] = array(
+  $items['admin/config/content/kandy/file/edit/%/%'] = array(
    'title' => 'Kandy File Edit',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('kandy_file_edit_form', 6, 7),
    'access arguments' => array('administer site configuration'),
    'type' => MENU_CALLBACK,
  );
.....
-function kandy_file_edit_form() {
+function kandy_file_edit_form($form, $form_state, $file_style, $file_name) {
-  $form = array();
-  if (isset($_GET['fileStyle']) && $_GET['fileName']) {

    $public_path = "public://kandy";
-    $file_style = preg_replace('/[^A-Za-z]+/', '-', $_GET['fileStyle']);
-    $file_name = preg_replace('/[^A-Za-z]+/', '-', $_GET['fileName']);
+    $file_style = preg_replace('/[^A-Za-z]+/', '-', $file_style );
+    $file_name = preg_replace('/[^A-Za-z]+/', '-', $file_name);

.....

-  }
-  else {
-    drupal_goto(
-      url(
-        'admin/config/content/kandy',
-        array('absolute' => TRUE)
-      )
-    );
-    return array();
-  }
}

And a couple other minor points.

By using $user as a variable name, you can quickly hit some nasty bugs if you accidentally do this:

global $user;

Try to use $account to avoid this from happening.

I accidentally done this once, assigning everybody as the admin 1 account... luckily only on my local dev environment! It did take a while to track down though.

Hyphens are nicer than underscores: "kandy/get_user_for_search" c/f "kandy/get-user-for-search"

More of a style thing

Good luck with your project :)

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.

I fixed all issues above and committed some changes for new features.

Need reviews. Thanks!

The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722

jquery.rateit.min.js: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Can you also fix the coding standards according to http://pareview.sh/pareview/httpgitdrupalorgsandboxkandy-io2445545git before we start the next round of reviews?

And the automated review reports a security issue: DrupalPractice has found some issues with your code, but could be false positives.

FILE: /home/klausi/pareview_temp/kandy.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------
  163 | WARNING | Open page callback found, please add a comment before
      |         | the line why there is no access restriction
  173 | WARNING | Open page callback found, please add a comment before
      |         | the line why there is no access restriction
---------------------------------------------------------------------------

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.

Looks like you forgot to push your changes with git?

@kandy-io Please add some recent manual code reviews to boost up the project application.

They already did 6 reviews, so that is fine.

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.