[D7] UFP Identity module

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

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.

I really think you will struggle to get the module approved without moving the library out of the module and making it a shared library using the libraries api. I can't find on the UP website the licensing of the library but, if it isn't GPLv2, there's no chance it can be incorporated within the module.

Adding in Libraries isn't a chore and, these days, many modules use that method. Most significant sites are likely to already require the libraries module.

Even if the code is licensed under a compatible license to GPLv2+ 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.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

The point is that the library is originally hosted elsewhere on https://github.com/ufpidentity/identity4php . That means it is an existing third party asset with regard to drupal.org, and it is forbidden to include that in your drupal.org repository.

As the example explains that would only apply if you wrote identity4php originally in the drupal.org repository. Then the canonical home of the library would be drupal.org. Instead, the library already exists standalone on Github.

What is the problem with just telling users to download the library in the installation instructions, same as all other Drupal modules do?

Just because other modules violate that policy it does not mean the policy does not apply here.

So again: What is the problem with just telling users to download the library in the installation instructions, same as all other good Drupal modules do?

@klausi did you actually read #1710094: GeoPHP should not ship with the library directly?
I was making the same mistake as you are here.

https://www.drupal.org/node/2191979/revisions/view/6902021/8009007 is good enough for me.

I'm commenting on this purely as myself and not as a member of TWG but:

I agree with @tim.plunkett here. I think the policy is pretty clear as written that as the original author this restraint does not apply. I agree that the best practice here is to leave the library in an external repository imported using libraries API but as a SAAS product provider I can see why he's motivated to make it as easy as possible to use the GUI to download the module - currently that doesn't directly support downloading third party libraries at all.

I think what we need to look at is what the policy actually states - and here it states:

This policy does not apply to original code written by a project maintainer.

It does go on to say:

For example, if you write an integration library to connect a Drupal module to another API, you may include it in a repository (licensed under the GPL), since this will be the original version of the library.

In the example case the library seems to have *started* in the module repo but it's not clear to me at all that this is a requirement, just a particular example where ownership is clear. As long as @richardl@ufp.com is the author of the code it seems like it passes muster.

I requested a response from the Licensing Group about this, https://www.drupal.org/node/2427427

The current version at GitHub is NOT GPL, though; there is no LICENSE.txt or any explicit mention to the license in the Github repo.

The author themselves can do whatever with the library, since they own the copyright. They can license it as GPLv2+ on drupal.org, so I don't think we need a response from the licensing group here.

The TWG is about to make a comment here that we can continue this application with the library committed to the drupal.org repo.

The TWG discussed the issue and discussed with @klausi. The group's reading of this issue is in line with the one I personally had and laid out in comment #16. We need to evaluate this application based on the written policy which seems to allow adding the library to the project repository. Our discussion spawned a further discussion of clarifying this policy and whether this policy is overly restrictive and I have created a new issue (#2428079) to discuss that.

In any case the TWG feels that per the posted policy this application is not blocked on extricating the library or using the libraries system and that the inclusion of this third party library should not prevent review of this application from moving forward.

@mpdonadio: As @klausi points out, the fact that the author is the owner of the library gives him the ability to relicense. I am not a lawyer and this is just my personal opinion but I do not think we need him to explicitly post the GPL compatible license on github as the act of publishing the code on D.O. relicenses it as GPL2+ per the D.O. EULA and checkbox the author checked before getting the permission to push code to D.O.

Thanks @tizzo, and to the rest of the TWG for looking into this. I am assigning this to myself for my next review, which will likely be tonight or tomorrow morning.

Automated Review

Review of the 7.x-1.x branch (commit f24adfd): See attached. Note, some of these may be false negatives. Examine each one and decide.

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

No: Does not follow the guidelines for master branch.

Licensing

No: Doesn't follow the licensing requirements. Remove the LICENSE.txt file; this will be automagically added by the packaging script; it is also not the GPLv2 license. The library itself has a LICENSE.txt that is not GPL. Per https://www.drupal.org/licensing/faq#q4, by publishing on D.O. it becomes GPLv2 licensed, and this is not the GPLv2 license.

3rd party code

Yes: Follows the guidelines for 3rd party code. See #19

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

(*) If $context['name'] in identity.module came from user input, then this is XSS prone. See https://www.drupal.org/node/28984

(*) The $form_state['input'] errors look legit. Change these to $form_state['values']

(*) In _identity_form_user_login_alter_handler(), it looks like you are building #markup using string concatenation from what could be user input, so this is XSS prone.

I think this module needs further scrutiny for security.

Coding style & Drupal API usage

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

This is not a blocking issue, but I think that you should be using Libraries instead of including the library yourself. I have read your statement about this, but as a review admin still think Libraries is the better way to go.

Your .info can take advantage of autoloading, see https://www.drupal.org/node/542202#files

(+) Why the error_log in identity_admin_menu_block_page()? Also elsewhere.

(+) Page callback functions should return render arrays and not themed output, eg identity_admin_menu_block_page().

I think a better namespace for your project would be ufp_identity.

(+) Instead of your logic in identity_enable(), look into the hook_module_implements_alter() method.

You may need to function_exist() for openssl_x509_parse in hook_requirements to see if that extension is enabled.

(+) Why are you explicitly setting the timezone in the the .module? Comment needed.

(+) identity_get_service_provider() needs a proper docblock. Also elsewhere.

(+) Why are you disabling cache in identity_init()? Comment needed.

Not sure if I have ever seen something like $items['admin/config/security'] before. Normally you just the system block.

identity_admin() doesn't implment hook_admin. It's a form builder. See https://www.drupal.org/node/1354

Don't use string concatenation to build up strings from variables. Use t() or format_string().

Instead of identity_get_key(), I think there is a built-in drupal function you can use, but the name escapes me right now.

Why the Ubercart hooks? Comments needed.

Don't contatenate strings that get passed to t(), eg identity_handle_user_operation. Use placeholders.

(+) You need a hook_uninstall to variable_del() all of the variables you are creating.

Get rid of the no-op functions and commented out code.

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.

See the comment in #1 about getting a review bonus.

I did not review the library, but there is a lot of logging in it.

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.

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.