[D7] Hellosign

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.

1. You should add more useful information to your hook_help. Hook_help should contain information about how to use the module and/or external links to different place in your website or external information.
2. On your README.txt you mention something about licensing. "License - GPL (see LICENSE)" you should not mention anything about licenses on your README.txt.
3. A personal recommendation divide your code into multiple documents it would be easier to read a long page, but this is a personal preference.
4. Here are some complains (warnings) from coder:
   

     123: @file description should be on the following line (Drupal Docs)
   
      * @file string
   
     178: in most cases, replace the string function with the drupal_ equivalent string functions
   
         if (strlen($esign_conf['hellosign_final_signer_email']) > 0) {
   
     300: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
       $response = $_POST['json'];
     

5. No complains from Pareview.

I think the only blocker in here is the line that talks about the license.

@darol100: It is fine if people mention the license in README.txt, the only requirement is that it says GPLv2+.

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

I'm a robot and this is an automated message from Project Applications Scraper.

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.

Hello @camprandall!

I see next things:

• README.txt - there's a great article how to write them. https://www.drupal.org/node/2181737
• Module does not check installed HelloSign library "sites/all/libraries/hellosign-php-sdk/vendor/hellosign". It's usually gives warning on module configuration page and status report pages.
• Please, remove .gitignore from you folder. It's usually done on a project level.
• You can add hellosign.api.php file with all hooks and description to them
• This module is clean API wrapper for Drupal. I think you better rename it to HelloSign API.
• Maybe you can provide some entity type to list signed documents?
• process-hellosign-callback from anonymous any user returns blank page and multiple warnings.

Example of warnings generated when i try to access "process-hellosign-callback":

Notice: Trying to get property of non-object in _hellosign_handle_esign_callback_request() (line 344 of /test/sites/all/modules/hellosign/hellosign.module).

From code site - it works!

Thanks @profak that sounds like useful improvements but surely not application blockers. Anything else that you found or should this be RTBC instead?

@klausi,
i didn't really like those 'white-screens' and multiple warnings with direct-access, and i can inject post data into "process-hellosign-callback" callback like that:

POST /process-hellosign-callback HTTP/1.1
Host: test.dev
Cache-Control: no-cache

----WebKitFormBoundaryE19zNvXGzXaLvS5C
Content-Disposition: form-data; name="json"

{ "event": { "event_time": "1440711025", "event_type": "test", "event_hash": "123" }, "signature_request": { "signature_request_id": "123123123" } }
----WebKitFormBoundaryE19zNvXGzXaLvS5C

And it just passed in. Not a security issue, but i never leave such things. Just in case.

Sorry, maybe it's not a place for such minor issues?

I think part of @profak's last comment above might be related to the issue I filed a few days ago (#2563529: If there is no API key, the hash should never be considered valid), which is sort of security-related. So I think that should be fixed.

I think the other suggestions above are all very good suggestions (including the non-security-related part of the last one, which is basically that the module should return "403 Access Denied" when an attempt to visit the callback URL fails validation, rather than an empty result like it returns now). But I don't think any of them are application blockers.

I have been working with this module on a site I'm developing and have looked over the code for that. I filed a few other issues in the queue that have since been fixed, and overall the module looks quite good to me. So besides #2563529: If there is no API key, the hash should never be considered valid I'd vote for this application to be RTBC.

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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Add support for the composer_manager module
2. *Library is not visabily checked so site admin does not know he forgot to install it.
3. +Module is untestable without more information how to setup an acocunt and what data needs to be entered into the module config

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.

A new function has been added to allow the cancelation of pending signature requests. This should be pretty useful for anyone that has a request with that need. HelloSign queues up cancelation requests and sends a callback event after deletion.

1)The variables "hellosign_api_key", "hellosign_client_id", "hellosign_cc_emails" and "hellosign_test_mode" are configuration variables and should be deleted on module un-installation. Check hook_unistall().

2) Its good if you can include all the code necessary to run the administrative interface in .admin.inc file.
By convention .module files should contains only those functions which are implementation of hooks.

Assigning to myself for a review, I hope to use this module on a production site either way :)

Non-blocking issues:

• https://pareview.sh found a few minor code style issues
• Variables should be removed in a hook_uninstall()

Consider documenting which portions of the HelloSign API are available through this module, and which features are not. Some additional API hooks could be useful as well. Maybe hook_hellosign_request_alter() to allow modifications to a signature request before it is sent out.

Otherwise very solid module!

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

• 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. _hellosign_handle_esign_callback_request(): this will throw PHP fatal errors if an attackers sends broken $_POST data to this page callback. You should check isset($data->event->event_time) before using it.
2. hellosign_generate_esignature_request(): doc block: @return shuld list all possible array keys that might be returned.

Otherwise looks good to me.

Thanks for your contribution, Clint!

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:

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