[D7] SSOXS (Single Sign On for eXternal Services)

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

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 are working on master, move code to branch such as 7.x-1.x

[2] Do not use include_once instead use drupal's module_load_include

[3] All your Javascript files are using $(document).ready, this should instead be done by implementing Drupal behaviours: Drupal has a scripting framework and it should be used.

[4] If your script and css files are form dependent, use form's #attach instead of drupal_add_js, for example in ssoxs_addservice_page in ssoxs_services.inc

[5] This is the hell of a big and heavy module, consider refactoring overal structure for PSR-4 code styling using XAutoload. This will easy future migration to D8 and regarding the size of the module, and will help a lot structuring it's code, improving maintainability and readability.

[6] There are many minor coding issues, It would be nice to see them fixed, just use the automated review tool for that.

[7] I can see this is sort of a D6 port, but is way lagging behind in using modern D7 tools, consider upgrading all you direct database manipulations to the Entity Framework. This will easy maintainability and make it future-proof.

[8] Some of the CSS selectors are very loosely defined, such as ".table span" there is a high probablity of undesired behaviours when using that sort of styling. Consider being more specific ore using custom classes.

Overally, this is a very big module and needs deeper review. Many of my statemtents are just recomendations and some of them (such as using PSR-4) would require revising the whole module head to feet as into how it is structured, but you have a good strating point because you are already using classes all around. It has good coding styles and comments.

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.

@mvdijk, there are a lot of projects to be reviewed right now. If you get a review bonus (see second paragraph of comment #1), you will get on the high priority list. Thanks for your patience with this.

Changing the GIT clone URL

Codespell has found some spelling errors in your code.

./includes/ssoxs_services.inc:239: seperated ==> separated
./includes/ssoxs_toolkit.inc:1092: seperated ==> separated

Also solve the pareview.sh errors
http://pareview.sh/pareview/httpgitdrupalorgsandboxmvdijk2123977git

Removing review bonus tag which is not applicable here.

Automated Review

Reported several problems by pareview.sh:
http://pareview.sh/pareview/httpgitdrupalorgsandboxmvdijk2123977git-7x-1x

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Not sure if cause duplication and/or fragmentation.
Is this module trying to solve the same issue? https://www.drupal.org/project/openid_sso_provider

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

No: Doesn't met the security requirements.
* ssoxs_toolkit.inc:1646 : Use construction of the query directly. Even with addslashes, we should use db_insert or db_query (properly) which implement PDO.

Coding style & Drupal API usage

* ssoxs.module:25 : unnecessary initialization, as each case contains their own returns.
* ssoxs.module:28 (and more): style inside the tags make difficult override them via css if necessary. They must be moved to a css.
* ssoxs.module:712 : variable initialization not necessary.
* ssoxs.module:745 : declaration of a class in the module file. Please use the file[] directive for clarity.
* ssoxs.module:745 : declaration of a class in the module file. Please use the file[] directive for clarity.
* ssoxs_user_logout duplicates code in ssoxs_user_delete... it needs refactoring.
* ssoxs_toolkit.inc:66 : I think to force https in an url you can use drupal_goto($path, array('https' => TRUE). Is there any reason to do it on this way? I think the module should force the availability of https in the settings if necessary.
* ssoxs_toolkit.inc:302 : Does user_save throw exceptions?
* ssoxs_toolkit.inc:302 : Can SsoxUser have its own file? Please use the file[] directive for clarity.
* ssoxs_toolkit.inc:432 : That's dangerous. If other modules are adding properties to the user object (for different reasons) here the module is completely overriding it. Can we do a merge instead of a full override, or use another variable name with the scope of this method?
* ssoxs_toolkit.inc:640 : Can SsoxService have its own file? Please use the file[] directive for clarity.
* ssoxs_toolkit.inc:1136 : Can SsoxBlowfish have its own file? Please use the file[] directive for clarity.

The code is really hard to review. The functions do a lot of things, all together. For example, the function ssoxs_user_editservices_form has 440 lines by itself, doing tons of things that are really hard to follow. Some of the code is duplicated, like db_select('ssoxs_services', 'n') with a condition by the machine_name... etc. I think it needs a bit of work to solve the automated tests, and making the code a bit more clear for any developer that want to install it and check what it's doing.

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.

Hi.

Congrats on the project.

Just some items:

A)
File: /CHANGELOG.txt
Line 3: Line exceeds 80 characters; contains 88 characters

B)
File: /includes/ssoxs_services.inc
Line 971: Unused variable $validated_urls.

C)
File: /includes/ssoxs_formtypes.inc
Line: 184: Missing class doc comment

<?php
class SsoxsAhahIntegerType {

  /**
   * Class to manage the variable data field of type 'integer'.
   */

  protected $type = "SsoxsAhahIntegerType";
  public $form;
  public $delete = 0;
?>

Regards

@renatog: those are some nice tips, but on their own surely not application blockers. Anything else that you found or should this be RTBC instead?

manual review
- master branch should be removed
- possibly split the readme so that the installation instructions can be in a separate file
- $form_state['input'] shouldn't be used

I am new to the review bonus program so I might be wrong, anyhow, I found the following:

Manual Review

Master Branch

[No: Does not follow] I can still see a master branch, as per https://www.drupal.org/node/1127732, master should be removed.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows / No: Does not follow] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.
Does not follow in the following points:

• The README-file should repeat the synopsis on the project page on Drupal.org.
• If the README-file becomes large or complex, installation and configuration instructions may be moved into an additional file named INSTALL.txt or INSTALL.md (this one doesn't seem to be mandatory anyhow)

Code long/complex enough for review

[Yes: Follows]

Secure code

[No: List of security issues identified.] Security standards discourage the usage of $form_state['input']
Files: /root/repos/pareviewsh/pareview_temp/includes/ssoxs_jobadmin.inc
/root/repos/pareviewsh/pareview_temp/includes/ssoxs_statistics.inc
are including that, even if it is taken from e date field, and I guess in this case code injection is less probable, yet it is a coding standard issue.

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. FILE: /root/repos/pareviewsh/pareview_temp/includes/ssoxs_overview.inc does have some Line 100:
   $service_edit_link['data'] = l('', $edit_path, array('attributes' => array('title' => 'Edit')));
   It does need translatable text

CONCLUSION

Possibly the only important things are:
- security, use $form_state['values'] instead of $form_state['input']
- git, remove the Master branch (possibly you can create a 7.x-1.x-dev for your development needs

Manual Review

Master Branch

[No: Does not follow] I can still see a master branch, as per https://www.drupal.org/node/1127732, master should be removed.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.
Does not follow in the following points:

• The README-file should repeat the synopsis on the project page on Drupal.org.
• If the README-file becomes large or complex, installation and configuration instructions may be moved into an additional file named INSTALL.txt or INSTALL.md (this one doesn't seem to be mandatory anyhow)

Code long/complex enough for review

[Yes: Follows]

Secure code

[No: List of security issues identified.] Security standards discourage the usage of $form_state['input']
Files: /root/repos/pareviewsh/pareview_temp/includes/ssoxs_jobadmin.inc
/root/repos/pareviewsh/pareview_temp/includes/ssoxs_statistics.inc
are including that, even if it is taken from e date field, and I guess in this case code injection is less probable, yet it is a coding standard issue.

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. FILE: /root/repos/pareviewsh/pareview_temp/includes/ssoxs_overview.inc does have some Line 100:
   $service_edit_link['data'] = l('', $edit_path, array('attributes' => array('title' => 'Edit')));
   It does need translatable text

CONCLUSION

Possibly the only important things are:
- security, use $form_state['values'] instead of $form_state['input']
- git, remove the Master branch (possibly you can create a 7.x-1.x-dev for your development needs

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.