The module records a visitors session and lets the administrator (or who has access) to playback that session.

You can choose to filter recording on user roles, url, php, country, language, screen size of the visitor.

I have searched for module that does something similiar but could not find anything, the recorder module (https://www.drupal.org/project/recorder) records usage, but only page load etc. which is more similiar to Google Analytics but for backend. User records is more similiar to mouseflow.com

A video on how to setup and use the module can be found here:
https://www.youtube.com/watch?v=kwgJ328UYmo

The project page can be found here:
https://www.drupal.org/sandbox/marcus_johansson/2370893

Git Clone command is:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Marcus_Johansson/2370893.git user_recording

Reviews of other projects:
https://www.drupal.org/node/2360537#comment-9345617
https://www.drupal.org/node/2373267#comment-9333357
https://www.drupal.org/node/2367301#comment-9340343
https://www.drupal.org/node/2275959#comment-9334547
https://www.drupal.org/node/2365671#comment-9338489
https://www.drupal.org/node/2411721#comment-9554759
https://www.drupal.org/node/2348143#comment-9555115
https://www.drupal.org/node/2374429#comment-9555217

Files: 
CommentFileSizeAuthor
#12 coder-results.txt1.52 KBklausi

Comments

PA robot’s picture

Status: Needs review » Needs work

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

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.

Marcus_Johansson’s picture

Status: Needs work » Needs review

I fixed all the errors reported by the automated review tools except one - user_recording.min.js is supposed to be an obfuscated version of user_recording.js. So this issue exists by design.

FILE: /var/www/drupal-7-pareview/pareview_temp/user_recording.min.js
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
1 | WARNING | File appears to be minified and cannot be processed

Marcus_Johansson’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
k0teg’s picture

Automated Review

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

  • Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
    ./user_recording.min.js:        ASCII text, with very long lines, with no line terminators
    
    
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /var/www/drupal-7-pareview/pareview_temp/user_recording.min.js
    --------------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    --------------------------------------------------------------------------------
     1 | WARNING | File appears to be minified and cannot be processed
    --------------------------------------------------------------------------------
    
    Time: 398ms; Memory: 9.75Mb
    
  • 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.

Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. Use Drupal.t() function in Javascript to support i18n.
  2. There are code duplicates in user_recording_get() function.
  3. Use Drupal.behaviors.user_recording (or similar) instead of using $(document).ready().
  4. * Drupal 7 has built-in jQuery.cookie. It's better to use it instead of custom urGetCookie().
  5. You have a lot of hardcoded integers in Javascript. Imho, it's better to move all of this to drupal_settings_form().
  6. There is a function drupal_get_query_arguments(). It's better to use this instead of directly fetching data from $_REQUEST.
  7. * It's better to use drupal_json_output() command to return JSON or delivery_callback => 'ajax_delivery' in hook_menu.
  8. * There is a function https://api.drupal.org/api/drupal/includes!locale.inc/function/country_g... to return a list of countries.
  9. * It's more correct to add JS via #attached in hook_page_built() instead of preprocess_html().
  10. You can use db_merge() instead of db_update and db_insert if entry is not exist.
  11. There is a function ip_address() to properly determine IP address of a visitor.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

k0teg’s picture

Status: Needs review » Needs work
Marcus_Johansson’s picture

Status: Needs work » Needs review

Thank you so much for your review k0teg. It was really good things you pointed out that I had missed/did not know about. I have corrected all of them to fit Drupal standards now and will open up the application again for review.

Some things noted though that I did different from your suggestions:

6. There is a function drupal_get_query_arguments(). It's better to use this instead of directly fetching data from $_REQUEST.

I'm guessing this function was drupal_get_query_parameters() you were referring to? It's fixed now with that function.

8. * There is a function https://api.drupal.org/api/drupal/includes!locale.inc/function/country_g... to return a list of countries.

The problem with the locale function is that it's missing some name standards that's in the GeoIP database. But I didn't go so far as to look into the GeoIP module that I'm requiring - it had the function I was looking for, so now I'm using _geoip_country_values() from that module to fill the select input instead of a hardcoded file.

Thank you once again!

Marcus_Johansson’s picture

Issue tags: -PAReview: review bonus
Eugene Fidelin’s picture

user_recording.track.inc:13

For better readability, i would suggest replace

$var = array();
  $var[':user_id'] = isset($params['ur_track_id']) ? $params['ur_track_id'] : '';
  $var[':navigator'] = isset($params['ur_navigator']) ? $params['ur_navigator'] : '';
  ...

with

$var = array(
    ':user_id' => isset($params['ur_track_id']) ? $params['ur_track_id'] : '',
    ':navigator' => isset($params['ur_navigator']) ? $params['ur_navigator'] : '',
    ...
);
user_recording.track.inc:35

drupal_exit() should be used instead of exit, more information is here https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

user_recording.languages.inc

Duplicate array key (at line 32),
Duplicate array key (at line 139)
Duplicate array key (at line 157)

user_recording.js

Unterminated statement at line 103

user_recording_player.js

Constructor call without new (at line 70)

I would suggest rewrite this line in this way:

var re = new RegExp('[?&]' + name + '=([^&]*)');
var match = re.exec(window.location.search);
...
Swarnendu-Dutta’s picture

Status: Needs review » Needs work

Thanks for your contribution.
The module looks interesting and it does an awesome job.

Please address the issues mentioned by Eugene Fidelin.

user_recording.track.inc:35

echo drupal_json_output(array('status' => 'ok'));
Why are you using echo before the drupal_json_output which returns the data in JSON format.

Marcus_Johansson’s picture

Thank you for reviewing the code. I fixed all bugs that you both mentioned and also cleaned up the code as suggested by Eugene Fidelin.

Marcus_Johansson’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work
Issue tags: +PAReview: security
FileSize
1.52 KB

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

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalPractice has found some issues with your code, but could be false positives.
    
    FILE: /home/klausi/pareview_temp/user_recording.module
    ---------------------------------------------------------------------------
    FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
    ---------------------------------------------------------------------------
     17 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
        |         | #attached on the $page render array instead
     18 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
        |         | #attached on the $page render array instead
     19 | WARNING | Do not use drupal_add_css() in hook_page_build(), use
        |         | #attached on the $page render array instead
     56 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
        |         | #attached on the $page render array instead
     75 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
        |         | #attached on the $page render array instead
    ---------------------------------------------------------------------------
    
  • 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. user_recording_uninstall(): do not call drupal_uninstall_schema() here, this will be done automatically by Drupal core for you.
  2. user_recording_admin(): to access this form only the 'setup user recording" permission is required. But on that page you can configure arbitrary PHP code, meaning anyone with that permission can take over your site. So user_recording_page_build() should only call php_eval() if module_exists('php'), same as block_block_list_alter() for example does. And user_recording_admin() should check for the "use PHP for settings permission" same as for example block_admin_configure() does. This is a security blocker right now. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

The review bonus tag is already removed, you can add it again if you did another 3 reviews of other projects.

klausi’s picture

manual review continued:

  1. user_recording_list(): all user facing text such as "Play page" needs to run through t() for translation.
  2. Settings: Cookie time says "s" at the end, should this be "days" instead?
  3. user_recording_list(): this is vulnerable to XSS exploits. If an attacker send <script><lert('XSS');</script> as use agent then you will get a nasty javascript popup on this page. You need to sanitize all user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.
  4. "l(user_load($row->uid)->name, 'user/' . $row->uid)": use theme('username', ...) instead of l() here.
  5. user_recording_searchform_submit(): do not use drupal_goto() in form submit handler since it stops execution and exits. Use $form_state['redirect'] instead.
  6. Why do have geoip as hard dependency? I think you should just use it if it is there, but the module could work fine without it?
Marcus_Johansson’s picture

Status: Needs work » Needs review

Hi Klausi,

Thank you so much for your review, you pointed out very important things, especially the PHP security flaw and the XSS exploits.

So for each issue:

  • The Drupal Practice automatic issues - These are true, but I'm actually not sure what to believe here - I'm following the same standard that seems to be what other modules like Google Analytics, Modernizr and Caption Filter are using - adding the JS and CSS via drupal_add_js and drupal_add_css - this is changable of course, but I'm uncertain what the real standard is? I could actually not find any module that does this via "#attached" in hook_page_build.
  • user_recording_uninstall() - drupal_uninstall_schema() is gone. Did not know about this, thanks for pointing it out.
  • PHP filter - this is fixed in the admin area to check that the module exists and that you have right to use it. It also checks if the module is enabled when loading the javascript, and if not it will disable the process instead of giving a PHP error. Thanks for pointing this out.
  • t() - t() has been added to all the places I could find it missing, including field_suffix() in the settings area and the places you pointed out.
  • s to days - blatant use of copy&paste gone wrong, fixed :)
  • theme_username - is used now instead of l(...). I did not know about this at all, thanks for pointing it out.
  • drupal_goto on submit - brain freeze, I'm not sure what I was thinking about. Fixed to redirect instead.
  • XSS exploit - added check_plain() to all visible user generated output, also to json output.
  • geoip requirement - you are correct, there exists no reason to have this required. I removed the hard dependency and added info into the README.txt as well as to the admin area that you get extra functionality with it enabled. I also changed the project description.

Thanks once again for you review, I will do some more reviews to win back the bonus tag.

Marcus_Johansson’s picture

Issue summary: View changes
Marcus_Johansson’s picture

Issue summary: View changes
Marcus_Johansson’s picture

Issue summary: View changes
Marcus_Johansson’s picture

Issue tags: +PAReview: review bonus

Added 3 more review to the 5 I had, adding PAReview: review bonus again. Hope that's ok.

naveenvalecha’s picture

Congrats! You are near RTBC :) . Just few nitpicks
Manual Review :

  1. user_recording.install : Add the description to all fields defined in user_recording_schema that will make the other developers to easily understand the usage of this field and they will easily contribute to the module.
  2. user_recording.js : Define all the variables in single line that will helps in reducing the compressed assets(js) size, when js compression will be enabled.
          var urMouseMove = {};
          var urScroll = {};
          var urClicks = {};
          var urKeypress = {};
    
  3. (+)user_recording.js : Move these functions(ur_send_data , ur_make_id) under Drupal object because it might create some conflict with some other custom module as well.So I think this should be something like
    Drupal.user_recording = {};
    Drupal.user_recording.ur_send_data = function......
    Drupal.user_recording.ur_make_id = function......
    

    you can also add more add on functions under it.and please also change where are u calling these.After these you should call them like Drupal.user_recording.ur_make_id(); instead of ur_make_id();

  4. user_recording.js : function ur_send_data : Are we also doing check_plain on the parameter "ur_track_id" because cookies are also user supplied inputs as per the OWASP.
  5. user_recording.js : function ur_send_data : Are we also doing check_plain on the parameter "ur_track_id" because cookies are also user supplied inputs as per the OWASP.Would you please specify its usage.
  6. user_recording.admin.inc : user_recording_admin :
    $form['savability']['countries']['user_recording_countries'] = array(
          '#markup' => t('You must install the !geoip module for this to work', array('!geoip' => l(t('GeoIP API'), 'https://www.drupal.org/project/geoip'))),
        );

    As the GeoIP module has soft dependency.So I would suggest you to update this message a little.

  7. user_recording.get.inc : user_recording_get : Better to use drupal_substr instead of substr
  8. user_recording.module : user_recording_page_build : Using #attached is pefer over drupal_add_js , drupal_add_css, and its friends.
  9. user_recording.module : user_recording_menu :
     $items['admin/reports/user_recording'] = array(
        'page callback' => 'user_recording_list',
        'title' => 'User Recording List',
        'description' => 'See all sessions that has been recorded.',
        'access arguments' => array('view user recording'),
        'file' => 'user_recording.list.inc',
        'type' => MENU_NORMAL_ITEM,
      );
    
      $items['admin/config/system/user_recording'] = array(
        'page arguments' => array('user_recording_admin'),
        'page callback' => 'drupal_get_form',
        'title' => 'User Recording Settings',
        'access arguments' => array('setup user recording'),
        'file' => 'user_recording.admin.inc',
        'description' => 'Manage when the user should be recorded.',
        'type' => MENU_NORMAL_ITEM,
      );

    The hook_menu type "MENU_NORMAL_ITEM" is default so we can easily remove it.

  10. $items['user_recording_track'] = array(
        'page callback' => 'user_recording_track',
        'access callback' => TRUE,
        'file' => 'user_recording.track.inc',
        'type' => MENU_CALLBACK,
      );

    This menu path has direct access callback to true.The operation in function user_recording_track is not too much expensive that will leads to DOS.So I think it needs more experianced reviewers eyes.What they reckon on this ?

  11. user_recording.list.inc : user_recording_list : line 108 : check_plain($row->id), This is not a user provided input.check_plain is not needed here.
  12. user_recording.list.inc : user_recording_searchform_submit :
    function user_recording_searchform_submit($form, &$form_state) {
      $values = $form_state['values'];
      unset($values['submit']);
      unset($values['form_build_id']);
      unset($values['form_token']);
      unset($values['form_id']);
      unset($values['op']);
      $form_state['redirect'] = array('admin/reports/user_recording', array('query' => $values));
    }

    Only pass those parameters that are needed in query.

klausi’s picture

@naveenvalecha:
2) Writing verbose Javascript with multiple assignment lines is fine - JS minifiers will do the hard work for you anyway and more verbose code is easier to maintain.
9) including the type explicitly does not hurt and makes it more clear what the code is doing, so this is fine as well.
10) Well that page callback needs to accessible by any user, so that the recorded data can be tracked, right?

Marcus_Johansson’s picture

Status: Needs review » Needs work

Thank you both too naveenvalecha & klausi for your feedback. I'll work on the issues.

Regarding issue 10 naveenvalecha is correct - since you can choose which roles should be tracked in the settings, these roles should also be honored here with a function connected to access callback or with a check when the page is constructed. Right now anyone in theory could spoof post data even if they do not belong to a role that should be tracked. In theory someone could set that they only wants to track admin users for instance.

Marcus_Johansson’s picture

Status: Needs work » Needs review

@naveenvalecha: Thanks again, I fixed most of the notes you had including 10 since it was a real problem. 2 and 9 I skipped since they where ok according to klausi.

The two notes I did not fix was regarding cookies (notes 4 and 5) - I do understand that cookies can be edited and thus should be treated as malicious data (all the data should actually be treated this way since anyone could make a false post). So this data is only outputted on row 56 of user_recording_list.inc:
l($row->user_id, 'admin/reports/user_recording/' . $row->user_id),
Since it's using l without the #html option set to true, it is already going through check_plain() before being outputted.

klausi’s picture

Assigned: Unassigned » heddn
Status: Needs review » Reviewed & tested by the community

manual review:

  1. user_recording_page_build(): Doc block should state that this is a hook implementation, see https://www.drupal.org/coding-standards/docs#hooks
  2. user_recording_list(): do not call drupal_render() and theme() here, just return a full nested render array, Drupal core will render it later for you. See https://www.drupal.org/node/930760

But otherwise looks RTBC to me.

Assigning to heddn as he might have time to take a final look at this.

klausi’s picture

Assigned: heddn » Unassigned
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, Marcus_Johansson!

I updated your account so you can 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.