[D7] Stitchz Social Login

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

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.

HI stitchzdotnet,

I would put every (sub)module in a custom directory.

I would also separate the templates in an directory.

In your queries you select direct from the stitchz_social_login_identities table. This wouldn't work if Drupal was installed with a prefix. (See an example: https://api.drupal.org/comment/7019#comment-7019)

Hi,

Is the git clone command valid, because i am not able to clone the code

Prajwal

• stitchz_social_login.install
  Don't think you need to explicitly call drupal_install_schema in hook_install or drupal_uninstall_schema in hook_uninstall. They are automatically taken care of in Drupal7.
• stitchz_social_login.module
  

  /**
   * Implements hook_user_delete().
   *
   * Cleans up identity table when the user is deleted.
   */
  function stitchz_social_login_user_delete($account) {

  Convert db_query in D7 Database api.

• Automated review still have issues on docblock & char limit per line exceeding 80 chars.

Automated Review

Best practice issues identified by pareview.sh seems to be clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Does not follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

1. Your GIT Clone code is not working, it should be git clone --branch 7.x-3.x http://git.drupal.org/sandbox/stitchzdotnet/2321765.git stitchz_social_login
2. Please fix your README.txt file and there should be no INSTALL.txt file.

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.

This review uses the Project Application Review Template.

That are not application blockers, anything else that you found or should this be RTBC instead?

@klausi, you are correct. Thanks for the correction!

Except the README section this module is perfect. Hence moving it to RTBC.

Thanks for contributing to Drupal @stitchzdotnet

Assigning to myself for next review.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Review of the 7.x-3.x branch (commit b2592e7):

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) No. If "no", list security issues identified.

1. stitchz_social_login_admin_format_provider_list(): This is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> in the admin settings form, get a nasty javascript popup. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again.

   In given code apikey and redirect_uri variables coming form user provided input and you are directly rendering without sanitize that's why these popup coming. Also check the same for other places too in your code.

Coding style & Drupal API usage

1. (+) stitchz_social_login_config_settings_form() : You have defined max length for some fields in stitchz_social_login_schema() but in admin settings form no maxlength defined, if user input long data it produces fatal error on form submission that looks weird.
   
   
   
2. stitchz_social_login_schema(): Correct TYPO stitchz login primaary key. Some fields having length 2000, really required, comment needed?
3. stitchz_social_login_user_delete(): You are deleting some data from db through db_query() instead use db_delete().
4. (+) stitchz_social_login_user_delete(): You are writing Reserved Words in sql query like SELECT, INNER JOIN with ALL in SMALLCAPS that looks like a a bad taste. You are repeating the same at number of places. See https://www.drupal.org/node/2497 again.
5. stitchz_social_login_config_settings_form(): Using #attached with render arrays is preferred over drupal_add_css().
6. (*) stitchz_social_login_config_settings_form_submit(): This submission code is not well managed. At time of insertion you are inserting data directly in db without any check_plain() and on update time using check_plain(). Never reformat data when storing it like 'apikey' => check_plain($values['apikey']),. This should happen at page render time. Its part of the golden rule of data in Drupal. You should only escape variables if you are actually printing them.
7. stitchz_social_login_authorization_handler_check_dup_username() : do NOT use db_select() for simple static queries, see https://www.drupal.org/node/310075 again. You are same approach at number of places in your module.
8. (*) stitchz_social_login_addin_block_view(): If user input some special characters in admin form, at line 35 logic fail that causes following fatal error.
   
   
   
9. stitchz_social_login_get_access_token() : cache_clear_all(); why you need this? You are using this function 8 times in your module. Comment needed.
10. Instead of using date(), should use format_date().
11. stitchz_social_login_addin_format_provider_list(): Instead of using substr() better to use drupal_substr().
12. md5() should not be used for anything security related. You should use drupal_hmac_base64() instead. Can you just use drupal_get_token() instead?
13. In stitchz_social_login_menu(), The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. A separate permission would be good, too.
14. stitchz_social_login_get_access_token(): To be honest, I had a hard time tracing things out. Usage of session make it look like you are bypassing the Form API to accomplish your tasks (which would be a pretty big issue). Can you explain what this is doing and why you need to do it?

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.

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

All issues found were minor, so I am leaving at Needs Review for further reviewers to verify all is ok. Looks good to me.

Automated Review

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. Add comment as to why this code block is commented out.

         // $form['stitchz_social_login_config_settings_form']['remove'] = array(
     // '#type' => 'button',
     // '#value' => t('Reset Settings'),
     // '#ajax' => array(
     // 'callback' => 'stitchz_social_login_config_settings_form_remove_settings',
     // 'wrapper' => 'stitchz_social_login_config_settings_form',
     // ),
     // );
         

2. Line 295, the $default = ''; seems pointless as it is always overwritten in the if/else. The same is in line 204 of addin module.
3. I've never seen a .gitignore in a project review before. I can't find anything on this but I'm guessing it is best removed as it is not directly associated to the project, is it?
4. It doesn't look like $redirecturi is used in addin module after being set in line 113.

         // Make sure the redirect uri ends with the proper path.
       if (strpos($redirecturi, '/stitchz_social_login/auth') === FALSE) {
         $redirecturi .= '/stitchz_social_login/auth';
       }
         

5. If possible, provide a demo login account for Stitchz with details in the issue summary to allow reviewers to test easily with demo content.

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.

Assigning to myself for another review.

I started to review your code again but found lot of check_plain() taken place in your code that shows still you need to know where Sanitization functions are required to escape string. You can not use these functions at every place, in one line I would say Filter on output, not input.

See https://www.drupal.org/node/28984 and https://www.drupal.org/node/263002 again.

(*) stitchz_social_login_config_settings_form(): In this form, for every element you are using check_plain() to set default value that's wrong. 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.

I have input <script>alert("XSS APP Key")</script> in apikey field and after successful form submission it looks like

<script>alert("XSS APP Key")</script>

Input <script>alert("XSS APP")</script> in Social Login Notes field, it looks like

<p>alert("XSS APP")</p>

(*) stitchz_social_login_admin_check_is_valid_api_url() : Wrong way to validate rather use hook_validate(). Because if user have input some values, due to this function all user input values reset with default values if validation fails that cause inflammation to end user.

(*) stitchz_social_login_authorization_handler_save_oauth_credentials(): Why are you using db_query() for update instead use db_update().

(+) drupal_add_css() still exists at 5 places in your code rather use #attached.

One more thing either address or at least comment the issues pointed out in #20.

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.

I'll stop there as this project is huge...

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

Automated Review

Review of the 7.x-3.x branch (commit b0dfcd0):

• 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

My blocking issues from #25 have been addressed, looks great improvement. Good Job!

1. stitchz_social_login_config_settings_form_validate(): It produces following notice as $arr['host'] is not checked before use. See http://php.net/manual/en/function.isset.php.
   

   Notice: Undefined index: host in stitchz_social_login_config_settings_form_validate() (line 50 of C:\xampp\htdocs\drupal\sites\all\modules\stitchz_social_login\stitchz_social_login.module).
   

2. stitchz_social_login_addin_block_view(): I think, you are using (!$user->uid) to check whether user is anonymous, rather use user_is_anonymous().
3. (*) http://static01.stitchz.net/images/logo-55x40.png, can you point me usage of this image is OK, not violating third party content policy. See https://www.drupal.org/node/422996 again and confirm the same.
4. stitchz_social_login_addin_remove_identity(): There is no need to use return with drupal_goto. return drupal_goto('user/' . $uid . '/social_login_accounts');
5. stitchz_social_login_authorization_handler_prepare_and_send_v2_request() : $json_response = drupal_json_decode(drupal_json_decode($request->data)); Really drupal_json_decode() required here twice, comment needed?
6. stitchz_social_login_config_settings_form_submit() : Instead of using db_insert() and db_update() separately, you can manage the same using db_merge() also.
7. stitchz_social_login_authorization_handler_get_settings() : There is no compulsion to define empty array() with db_query(), you can remove it. Same applies with stitchz_social_login_addin_get_settings().

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.

I am little bit unsure about third point as 3rd party code and Licensing issues are major blocker, want clarification from your side. Apart from this, I am not seeing any blocker.

Thanks!

Automated Review

I had reviewed the plugin there is no issue in it. Link

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause.

Licensing

N/A

3rd party assets/code

N/A.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

No: Does not follow the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. List of security issues identified.

Assigning to myself for next review.

Automated Review

Review of the 7.x-3.x branch (commit 79c6be8):

• 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. stitchz_social_login_config_settings_form(): '#markup' => t('Connect your Drupal site with Stitchz Login by completing the fields below, sync your providers list, then click Save. The below information should be copied directly from your <a href="https://login.stitchz.net/">Stitczh Login Application Settings</a>.'),

   do not create link markup yourself, use url() or l() instead. Do correct this at other places too in your code.

2. stitchz_social_login_config_settings_form(): When using explicitly set HTML id attribute, better id should run through drupal_html_id().
3. Instead of using time(), should use REQUEST_TIME.
4. (+) stitchz_social_login_addin_format_provider_list(): Don't use theme() directly, use render arrays. The main reason why the render api exists is it allows for altering by other modules before any html is produced. Also it provides one consistent system to produce any output. Both blocks and pages can be altered just as forms have been alterable for some time. A page callback should return a render array, as should hook_block_view()'s $block['content']. This allows your module and other modules to treat the content as data for as long as possible in the page generation process.

   See:

   1. https://api.drupal.org/api/drupal/modules%21block%21block.api.php/functi...
   2. https://www.drupal.org/node/930760
5. Considering using theme_html_tag() instead of writing direct html tag.
6. (+) $provider_html variable contains <img> tag directly at number of places in your code instead use theme() or theme_image().

   something like this echo theme('image', array('path' => drupal_get_path('module', 'stitchz_social_login') . '/images/logo-55x40.png'));

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.

@stitchzdotnet, I am not seeing any blocking issues so further moving to RTBC but you should fix fourth point before stable release of this project.

I tested this module functionality and it works as intended. Also inline comments and To Dos docs looks good to me.

As this module is huge, I believe it needs another pair of eyes. Assigning to mpdonadio for a second look if he has time.

I just want to give a status update that this is in review. The code weighs in at nearly three thousand lines. This take a while to review. I have a first pass done, and have not found anything major, but I have several things I want to double check from a security perspective (mainly the actual authentication process and the API calls out).

Automated Review

Review of the 7.x-3.x branch (commit 79c6be8):

• 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

(skipping the stuff already covered in the review template and just doing a code walkthrough)

.info files don't need quotes around things.

In stitchz_social_login_addin_block_view(), since you are checking for the anon user, you may want to set explicit DRUPAL_CACHE_PER_ROLE on the block.

stitchz_social_login_addin_format_provider_list() should return a render array any not markup; #markup should really be avoided for anything complex. Also similar places elsewhere.

I think stitchz_social_login_addin_display_provider_list() missues #prefix a little bit...

stitchz_social_login_addin_menu(), you don't need to specify the file for the callback when it is in the same file as the hook.

stitchz_social_login_addin_profile_accounts() is missing the @return in the docblock.

stitchz_social_login_addin_get_user_identities() can likely use one of the ->fetchAll() variants instead of looping over ther results.

Your URL building should really use the Drupal functions for building it up rather than string concatenation.

(+) stitchz_social_login_addin_format_provider_list(), there is some untranslated text towards the bottom (Powered By). Double check the module for untranslated strings.

stitchz_social_login_addin_add_identity(), use user_is_logged_in().

In stitchz_social_login_addin_add_identity(), how can you be logged in and not be logged in at the same time? Something looks weird with this logic.

stitchz_social_login_addin_add_identity(), avoid splitting strings across lines (this is a valid exception for PAReview).

stitchz_social_login_addin_prepare_and_send_v1_request(), use drupal_http_build_query()

stitchz_social_login_addin_prepare_and_send_v1_request(), cURL is not a better option. Use the Drupal API whenever you can.

stitchz_social_login_addin_prepare_and_send_v1_request(), why the double JSON decode? Comment needed.

stitchz_social_login_authorization_handler_response_reader(), remove the commented out code

stitchz_social_login_config_settings_form(), $base_url is almost never needed directly. Use url() to make an absolute URL if needed.

(*) In your requests to HTTP requests out (eg, stitchz_social_login_addin_prepare_and_send_v1_request) your usage of check_plain is wrong. These functions are for escaping things going to user output. These aren't. Also, you should really build up the query string with drupal_http_build_query().

(*) I can't totally trace this out, but I think you have a security problem in stitchz_social_login_addin_format_provider_list() and similar places with the authentication_url. You should be using url() to build up the URL instead of doing so manually. By doing this dangerous protocols will be stripped via drupal_strip_dangerous_protocols(), and you will be sure your parameters get handled properly.

The starred items (*) are fairly big issues and warrant going back to Needs Work (these are the only things that I see that must be fixed; and should be fairly easy to get this approved). 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.

Review of the 7.x-3.x branch (commit f9d0db0):

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

  FILE: ...view_temp/authorization/stitchz_social_login_authorization_handler.info
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   4 | ERROR | [x] Expected 1 newline at end of file; 2 found
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS 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. stitchz_social_login_authorization_handler_response_reader(): why are you not using the drupal core function user_external_login_register()?
2. stitchz_social_login_config_settings_form_validate(): this function should be in the stitchz_social_login.admin.inc file, right? Same for the form building function itself.
3. stitchz_social_login_config_settings_form_validate(): doc block is wrong, this is not a hook. See https://www.drupal.org/coding-standards/docs#forms
4. stitchz_social_login_config_settings_form_submit(): no need to document $form and $form_state here, see https://www.drupal.org/coding-standards/docs#forms
5. stitchz_social_login_profile_identities_block.tpl.php: "remove this identity provider": all user facing text must run through t() for translation.
6. Project page: comparison to existing modules is missing. Why should I use the external service when I can do this just fine with hybridauth for example? https://www.drupal.org/project/hybridauth

But otherwise looks ready to me.

Since this was RTBC already and the comments seem to be addressed ...

Thanks for your contribution, stitchzdotnet!

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.