Google Identity Toolkit

This review was too large, to paste it in here ;-)
I'll attach it as file, on next comment.

Here it goes:

the automated review does not show many faults anymore, but your still working in the master branch!
See here: http://ventral.org/pareview/httpgitdrupalorgsandboxeasyrp1357838git

Also remember to change back the status to 'needs review' after you think you fixed all issues

Dear Moderator,

Marking this project as duplicate of http://drupal.org/project/gconnect . D7 port is on the way for gconnect module & committed to git repository

http://drupal.org/node/1355236

Cheers,
Anil Sagar

Dear Moderator,

Gconnect module does use same API (Google Identity Toolkit API.). It's just matter of time we are going to have additional features like

• Options to choose different service providers.
• Options to choose language.
• Options to choose popup skins.
• Options to choose different GoogleIdentityToolKit Widgets like accountchooser, account tabs etc.

Benefits of using Gconnect module over sandbox project above.

• Compatibility with D6.
• Register Using Gconnect Plugin.
• Sponsored by SourceN Inc and Actively maintained.
• Ability to restrict certain domain names within Gmail service. Let's say if administrator want to allow only @standford.edu who uses Gmail as e-mail.

We are using this module in one of our own project and will be maintained actively in future.

Cheers,
Anil Sagar,
Sr. Drupal Developer,
SourceN,
www.sourcen.com

Hi,

I couldn't get gconnect to work on D7 (haven't tried out the D6 version), apparently some hardcore coding tweaks needs to be done in order to get it going.
Google Identity Toolkit on the other hand just works smoothly and nicely.

I think @anil614sagar and @easyrp should team up and work together on a single project. My suggestion would be for Google Identity Toolkit to make its way to the Full Project main stream as it is functionally ahead of gconnect at the moment.

Francisco Luz / Moderator,

We verified on D7 & tested before pushing to repository. I just verified module working with Fresh Drupal 7.10 instance after your comment. Without any changes to it, it works! Please post your specific issues to the gconnect issue queue here. http://drupal.org/project/issues/gconnect?status=All&categories=All

We are committed to release all the features mentioned in the project page http://drupal.org/project/gconnect end of this week for both Drupal 6 and Drupal 7 versions.

This module offers simplicity to the end user with a simple widget as it is now to Login Or Register using google and an additional option to use account chooser widget as shown in http://accountchooser.com/ by the end of this week.

Gconnect module has also been reviewed and approved by the Acquia dev team...

@sourcen #14

Hi mate,

Thank you for the clarifications.
I noticed that instructions for installing gconnect on D7 has been added in the project's main page overnight. So my issue was probably because I was following the D6 instructions. I will give it a go again later on today.

Thanks again,

Description modified in the wrong place

@FranciscoLuz , dont you think you were a bit too eager to RTBC this? It hasnt even been thoroughly reviewed yet by anyone and has only had automated reviews performed on it.

The current implementation of the online PAReview check is showing a number of errors/warnings, see the current report at http://ventral.org/pareview/httpgitdrupalorgsandboxeasyrp1357838git-7x-1x

Make sure to set this back to 'needs review' once you have addressed the issues highlighted here.

It would also be advantageous to yourself and future reviewers is you used the online review tool at http://ventral.org/pareview/ and posted the results back here after your next commit.

@raynimmo

dont you think you were a bit too eager to RTBC this?

No, actually I don't. I just wasn't too familiar with ventral.

Thank you for the heads up. ;)

Hi,

I have been using this module for a couple of weeks now on the website of the company I work for ( http://www.hmcservicos.com.br/pt-br ) .

I have posted a couple of patches for small feature requests (see the issue tracker for this module at http://drupal.org/project/issues/1357838 ), which are all already committed, and everything seems to be working just great.

The maintainer has promptly fixed all the code inconsistencies pointed out by the fellow reviewers above and I should add that this module is a must have set of functionality for the Drupal community.

Considering all that, I am setting the status to RTBC but if anyone has a different felling about this please let us know.

Thank you,

A few comments:

1. Description says this is the "plugin for Drupal 7.x", but the repository also includes a 6.x branch ... Please consider updating the module description on the project page.

2. hook_enable(), hook_disable(): I *think* that the standard is to put the database changes and variable_del calls in hook_install() and hook_uninstall() instead of enable/disable; so that users can disable the module without losing any existing module configuration ... and need to explicitly 'uninstall' the module if they want to blow away the module-specific settings.

3. Links in t() function calls: Please use placeholders for your links, as illustrated at http://drupal.org/node/322774 ... this will allow the links to be updated if they change in the future, without requiring the same change be made to every single translation file which may have since been created.

4. t() calls in module.install files: the t() function is not available during certain parts of the drupal install cycle, so you may need to use the st() function instead ... details as to when are at http://drupal.org/node/322731

5. variable name prefixing: It appears that you've used 'git' as the prefix for your module's variables ... due to the possibility of namespace conflicts with the 'git' versioning system used by Drupal, and to avoid confusion, I'd likely recommend the variable prefixes be changed to the full 'google_identity_toolkit' module name prefix instead of just 'git'.

6. "Git Language" admin setting: Just a suggestion, but could this setting be taken from the Drupal installation default?

7. drupal_add_js in forms: Consider using the $form['#attached'] FAPI element, instead of drupal_add_js(). (Details at http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....) ... I'm also not quite clear on what the .admin.inc javascript is trying to accomplish, and it seems there should be a more efficient way of doing it (though admittedly, I didn't dig very deep).

8. Bootstrap calls in included files: Are you sure you need to manually bootstrap Drupal? In what case would any of this code run outside of Drupal (generally, Drupal bootstraps itself before executing any module code).

9. Direct use of $_GET and $_POST values without sanitization: Your module references $_GET and $_POST values directly, and does not run them through Drupal's sanitization functions (such as check_plain, filter_xss, or filter_xss_admin). Looking at the handler class, it does not appear to have any input sanitization functionality of it's own, either. This can open up the *potential* for some pretty major security vulnerabilities. (I didn't dig deep enough to identify any specific vulnerability, but the lack of sanitization makes me pretty nervous!)

10. GitCallbackHandler contains a require_once 'autoload.php' line, but the autoload.php file is empty ... is there a reason you include an empty file?

Admittedly, this module goes a bit deeper into Drupal Core than I am familiar with, but there is enough here that makes me uncomfortable that I'm going to recommend that this be reviewed by a member of the security team (or at least someone with more in-depth knowledge of the core registration/login processes) before marking it RTBC.

manual reviewof the 7.x-1.x branch:

• git folder 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.
• google_identity_toolkit_enable(): doc block is wrong.
• google_identity_toolkit_enable(): you should use db_add_field() and db_field_exists(). And this should only be done in hook_install().
• "if (!function_exists('curl_version')) {"; if you require cURL you should implement hook_requirements() like here http://api.drupal.org/api/drupal/modules--simpletest--simpletest.install...
• google_identity_toolkit_disable(): empty function, so remove it.
• google_identity_toolkit.module: do not globally require your library, only in the functions where you actually need it. Otherwise the code is loaded on literally every page request which is bad for performance.

Get a review bonus and we will come back to your application sooner.

@easyrp

I've submitted some patches at http://drupal.org/node/1424812 in order to comply with the issue raised by klausi regarded to 3rd party code being hosted here at drupal.org.

Please check that out and please let me know if you got any question.

Cheers,

Hi All,

The GConnect module has been updated with the latest and most of what has been discussed here is now part of it.
http://drupal.org/project/gconnect

More exiting features coming soon...

Thanks

Hi All,

This project is Duplicate of http://drupal.org/project/gconnect. Gconnect project is posted much earlier than sandbox project.

Gconnect http://drupal.org/project/gconnect module has following benefits compared to the sandbox project.

• Support for 6.x version
• 92 Active Users who are already using module..
• Gives Ability to restrict or allow specific email addresses by domain names.
• Ability to enable / disable support for other providers like AOL, Yahoo, Hotmail.
• Maintained by SourceN actively.

Features that will be added very soon

• Ability to choose different widgets like account chooser in future along with current importer widget...
• Options to choose language.
• Options to choose popup skins using custom css.

Please mark this sandbox project as Duplicate. We welcome easyrp to submit patches to current gconnect module if he is willing to contribute any features.

Cheers,
Anil Sagar

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 34 | WARNING | Line exceeds 80 characters; contains 130 characters
 36 | WARNING | Line exceeds 80 characters; contains 87 characters
--------------------------------------------------------------------------------


FILE: .../modules/pareview_temp/test_candidate/google_identity_toolkit.admin.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 77 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------


FILE: ...ll/modules/pareview_temp/test_candidate/google_identity_toolkit.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 12 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------


FILE: ...all/modules/pareview_temp/test_candidate/google_identity_toolkit.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  94 | ERROR | There must be a single space before an operator statement
 338 | ERROR | Additional blank line found at the end of doc comment
--------------------------------------------------------------------------------

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

don't block deeper reviews because of these few minor issues found by automated tools

Hello easyrp

http://drupalcode.org/sandbox/easyrp/1357838.git/blob/refs/heads/7.x-1.x...
Looks like get_admin_js just returns a whole bunch of javascript. I think this should be in a separate .js file called by drupal_add_js

Empty description ran through t() function - http://drupalcode.org/sandbox/easyrp/1357838.git/blob/refs/heads/7.x-1.x.... Please add a description to this element.

Looks like hook_requirements doesn't deal with the library missing -

// Include the external library hosted at github.
https://github.com/drupalista-br/Drupal-Google-Identity-ToolKit.
include_once drupal_realpath(libraries_get_path('drupal_google_identity_toolkit')) . '/include_lib.inc';

Please review the drupal commenting standards - http://drupal.org/node/1354#general

The comments should all start with a capital letter and end with ? ! or .

Tagging as needing work

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

We are using gitkit since more than 10 months now in our site - oysterconnect.com

Everything was working fine till last evening. Today morning, all of a sudden, it stopped allowing our users to login.
When we click on Gmail / Yahoo / Hotmail, it shows us a popup - "Connecting to Server...", but it never goes off and nothing happens further.

Analyzing deeply into it, when I click on Gmail icon, a network request goes to GET proxy.html, which comes back with Status = 200
But there is a javascript error thrown in proxy.html file called "Uncaught Error : Bad hint"

Attached is the screenshot. Please confirm and help us resolve this asap.

*this* issue is a *closed* *project application*

this is not the place to report problems.

please read how to report bugs and use the projects issue queue to do so

re closing

Added Google Identity toolkit for D6 version