Module Name: overlay_userlinks
Drupal Version: 7.x

Description:
Overlay userlinks is a small module that creates a menu and three links in the site it gets enabled that display the login, register and password reset forms using overlay.
Anonymous users need to have access to overlay (set from admin/people/permissions).
There is no further configuration apart from moving the menu entries in any way one prefers.

It is compatible with LoginToboggan and E-Mail Registration modules and with custom fields that appear in the registration page.
The following screenshot is with LoginToboggan and custom fields: http://srm.gr/sites/myfiles/overlay_userlinks_registration.jpg

Project Page: http://drupal.org/sandbox/bserem/1811262
Git Clone: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/bserem/1811262.git overlay_userlinks

Thank you for looking into this!

Applications I participated:
http://drupal.org/node/1431424 (never got a reply from the original developer)
http://drupal.org/node/1771086

Comments

bserem’s picture

bserem commented

There is a demo link here at the moment: http://paylos.devel.srm.gr/
but it won't stay there forever (it is a development site). Once it goes live I'll update the thread.

The overlay links are the three links on the top-right.
Sorry for the greek language. The module is in english of course and uses t() when needed as per drupal coding standards.

soraver’s picture

soraver commented
Status: Active » Reviewed & tested by the community

it is strange that such a module was not already implemented.
I would like to have the opportunity to add this overlay feature on any link i want but till then this is a nice addon.

Thank you bserem :)

bserem’s picture

bserem commented

@soraver you can try out http://drupal.org/project/overlay_paths as I say inside the readme. It is a more advanced module which might help you accomplish what you need.

However, I find my approach simpler than overlay paths, colorbox and ctools modal boxes that give you the power to accomplish amazing thins. That's why I believe there is place for a module as Overlay Userlinks, for simplicity in smaller projects.

Thank you for your review!
ps: if you have some specific task that is troubling you with overlay I believe I can be of assistance, look into overlay_paths however :)

bserem’s picture

bserem commented
Issue summary: View changes

added one application I participated

cubeinspire’s picture

cubeinspire commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +PAreview: security

Hi, nice module, I like the idea of creating the menu on hook_enable() then check if menu module 'menu' has been called instead of simply creating the menu on hook_install(). That takes care of the situation where menu is un-installed and installed again.

Manual review:

Small issues

1. I see that you are calling hook_modules_installed(), on the comments you typed:
@see overlay_userlinks_install() but that function is not defined on the .install file.

2. More an improvement idea than an issue... the overlay block should be hidden when the user has logged in, otherwise the links have no meaning.
I know that can be done by configuring the block but It could be nice if it was done automatically.

3. There are two things here: http://ventral.org/pareview/httpgitdrupalorgsandboxbserem1811262git

Important issues

4. Regarding the access to the administrative overlay, there are security concerns about it, I think it should be mentioned on the module project page and the README.txt file, so administrators can assume responsibilities.

Right now I cannot find a proper documentation about the security implications of allowing anonymous users to access the overlay module, I just found this link and by memory I remember a more experienced developer told me to never activate the administration overlay to anonymous users.
Have you any information about this potential security issue ?

See: http://www.drupalgardens.com/documentation/site-management/admin-theme

5. There are warnings on line 72 and 88.
To avoid that drupal_get_form has to be saved into a variable and not directly called inside drupal_render();
Strict warning: Only variables should be passed by reference in overlay_userlinks_login() (line 72 of /var/www/d7/sites/all/modules/overlay_userlinks/overlay_userlinks.module).

bserem’s picture

bserem commented

Thanks logicdesign for your review!

I fixed:
1, it was a typo, I meant the .install file
3
4, added information about it, I'll also update the project page and find information about exploits using Overlay and add them to the readme.
5, but I need to ask you where those errors appeared because I never saw them... I'll have my eyes more open next time.

I kinda lost you about the installation and un-installation. I'll read about it.

I'll also look into your idea (2) although I haven;t messed with blocks in D7... I was a D6 user untill recently (don't tell anyone!).
I'm not sure how/if this can be done, because the block is not provided by my module but from the drupal menu system. This will be a tricky one.

cubeinspire’s picture

cubeinspire commented

Regarding point 5 notices and warnings can be activated/hidden on
Home » Administration » Configuration » Development

bserem’s picture

bserem commented

Thank you logicdesign!

I updated the readme file with more info about security concers.
I'm looking into (2) before re-submitting this for review

bserem’s picture

bserem commented
Status: Needs work » Needs review

Logged in users now do not see the links, that is n.2 of reply # by logicdesign.
With this commit all issues/suggestions made by logicdesign are addressed.

I'm updating the project page with more information about security concers and resubmitting this for review :)

cubeinspire’s picture

cubeinspire commented

I created a discussion on the security group.
It can be interesting for you: http://groups.drupal.org/node/263223

bserem’s picture

bserem commented

I've seend it and replied to the thread.

Greggles reply is actually what I've found by my searches: nobody seems to be sure of any security issues.

cubeinspire’s picture

cubeinspire commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: security

Well I remove the security tag as we couldn't find any information regarding the security problem stated at that blog. I think it would be a good idea to keep a track on information about possible exploits when Overlay is activated for anonymous users.

The module is working fine and I don't see any blocker on this module, so I think this is RTBC.

But... there are some CSS problems. On a 1024px width screen the overlay is making the horizontal scroll bar appear and the overlay is sticked to the left with a big space at the right.
I also encourage you to keep an eye on possible security exploits of the Overlay module.

bserem’s picture

bserem commented

With which browser do you experience the CSS issue? Is it limited only to my module all to all overlay pages?
With Opera and Firefox no horizontal scroll bar appears, unless I resize the window to 840px width.

Thank you once again for your effort and your support :)

cubeinspire’s picture

cubeinspire commented

With this one:
Chromium 18.0.1025.168 (Developer Build 134367 Linux) Ubuntu 10.04

I guess with other Chrome (windows/mac) and with Safari (webkit based also) there will be the same problem under 1024 resolution screens.

lolandese’s picture

lolandese commented

@logicdesign:
I also used 18.0.1025.168 (Developer Build 134367 Linux) Ubuntu 12.04 and had a hard time with css overflow problems only on that browser. Finally I gave up and decided to:

It solved my issues. See if it works for you.

bserem’s picture

bserem commented

Chromium 22.0.1229 is affected by this. I do not have Chrome, I'm waiting for arch repositories to deploy Chromium 23.x and see if it behaves better.

However it seems like a browser issue instead of a module issue

cubeinspire’s picture

cubeinspire commented

Well this is not a blocker, more like an improvement to make happy chrome/chromium users :-)
This is why I changed the status to RTBC

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

manual review:

  1. " $output .= "You are already logged in!";": all user facing text must run through t() for translation.
  2. overlay_userlinks_register(): why do you need that function? You could directly specify drupal_get_form as page callback in hook_menu() and user_register_form as argument?
  3. Same for overlay_userlinks_password().
  4. This module seems a bit short to approve you as git vetted user, but since you alredy contributed jumpstart I think promoting you now is fine.

Not real blockers, so ...

Thanks for your contribution, bserem!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

bserem’s picture

bserem commented

Thank you very much klausi! I have a lot of reading to do now, thank you for all the links.
I already pushed a fix for #1. I'll look into changing the code for #2 and #3.

I do know that my module is short, and jumpstart was short too... I'm doing the best I can trying to keep up with the fast pace of Drupal :)
It certainly helps me write better code every time!

Thanks to all reviewers too!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added to applications I participated