[D7]Og User Identity

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

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.

Automated Review: Looks clean. Great work!!

Manual Review:

1. In og_user_identity_group_block_content_form_submit() :

$redirect_url = variable_get('og_user_identity_page_redirect_url', '');

    $data = array(
      'form' => $form,
      'form_state' => $form_state,
      'redirect_url' => $redirect_url,
    );

    // Allow other modules to alter the redirect URL.
    drupal_alter('og_user_identity_redirect', $data);

    if ($data['redirect_url']) {
      drupal_goto($data['redirect_url']);
    }

You have not filtered the variable you got using check_url(). This might be a security vulnerability.

2. Also filter all other values you obtain using variable_get appropriately . Please read https://www.drupal.org/node/28984.

3. Consider adding @param and @return for your custom functions other than hooks.

Since the permission to set the "og_user_identity_page_redirect_url" variable is "administer site configuration" the user is trusted according to our policy: https://www.drupal.org/security-advisory-policy

Also, check_url() would be wrong before drupal_goto(). You would use url_is_external() to check untrusted user provided variables against Open Redirect attacks.

Can you be more specific where other variables should be filtered? I think they are all set with the "administer site configuration" permission, so I think the security tag is not justified here.

Any other vulnerabilities that you found?

Hello

Please provide the git command of your project in the project page.
I think your project git command will be

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/kzsolt/2500921.git og_user_identity

Thanks

@klausi: Sorry for the wrong review I made. I'll try to look deeper before marking security vulnerability from now on.

Hi,
I see below two files included in global scope if not required please use this only in the callback function where they are required,
else these files will load on every page.

module_load_include('inc', 'og_user_identity', 'og_user_identity.api');
module_load_include('inc', 'og_user_identity', 'og_user_identity.tokens');

Manual Review

1. og_user_identity.js: Consider putting this within a behavior
2. Consider adding better documentation and description for the og_user_identity_groups module

Manual review

It's hard to work out what specifically this module is intended to do from the description "provide some UI and backend feature". The list of features doesn't help much, perhaps some example use cases would make things clearer.

I'm not certain, but I think the aim is to add the concept of a user working in a specific group and provide an interface for switching between them then to pre-populate forms with the current group details. Building a list of groups that the user is a member of is possible with views (included by default even?), and using the entity_reference_prepopulate module it's possible to craft links to create content in a specific group by specifying the group node as a parameter in the url. You can likewise specify a redirect with the destination parameter (something like /node/add/contenttype?og_user_node=1&destination=node/1).

I have a fairly large og install and matching dev site, so am happy to test further once the intended use is more clear.

Please also fix #5 and #7, and give consideration to #8 above.

Finally, whilst I'll be happy to make a more detailed review of your code once you have fixed the above, it does show a reasonably good understanding of the Drupal API. It would however be helpful to spellcheck your 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.