It seems to me that this module, after installation, creates a major security hole in the site that it is enabled on.

Until the user has changed the passphrase or updated the settings.php file or at least checked the box to use the db password, pretty much anyone could log in to most of the accounts if they know what they are doing.

This patch creates a warning message in the Drupal status report section if the passphrase hasn't been added, and it also blocks logins that use any of the default passphrases.

A warning message is now generated if the Minimum permitted number is greater than the Current Validation number, but this can be done as there are times when this will be needed to do.

Comments

darrellduane’s picture

This patch also incorporates fixes for this issue: https://www.drupal.org/node/1775676

I'd recommend using this patch to include in the next release of this module.

I'm interested in being a maintainer if you'd like help.

daluxz’s picture

Status: Active » Needs work

Your patch needs some extra work:

  • $form['encryption']['urllogin_doc']
    The text within this element is now split up over several t()-functions. These should probably all be combined. Split up sentences are problematic for translators. Also the last t() has a comma that is not supposed to be there.
  • if($entry_codekey < $entry_codemin) {
    Coding standards. An "if" is always followed by a space.
  • drupal_set_message("You probably don't want to have the Minimum permitted number allowed for valid login to be greater than the Validation number, unless you don't want these codes to be useable at this time. You can enable these codes by setting the Minimum permitted number to be less than or equal to " . $entry_codekey,'error');
  • Don't put variables in translatable strings!

  • $resultmsg .= '<br>' . l(t('Add the "login via url" permission'),'admin/people/permissions#module-urllogin')
          . t(' for the role this user has.');</li>

    This is very hard to read, and translators won't really be happy with this either, since it results in a translation file with a bunch of cut off sentences.
    This would be better: t('<a href="@permissions">Add the "login via url" permission</a> for the role this user has.', array('@permissions' => url('admin/people/permissions', array('fragment' => 'module-urllogin'))))

  • return "<li>Passphrase has not been configured to a unique value -- "
    Not translatable
  • watchdog('urllogin', $resultmsg . ' - urllogin_get_login_link()', array(), WATCHDOG_WARNING);
    Don't put variables in translatable strings!
  • The function urllogin_get_login_link shouldn't be in this patch. Create a seperate patch for it.
  • t('Please ') . l(t('set a distinct passphrase for urllogin'), 'admin/config/people/urllogin') . t(' or set it in settings.php. See urllogin/README.txt for more details.'),
    This is hard to read/translate.
    This would be better t('Please <a href="@configuration">set a distinct passphrase for urllogin</a> or set it in settings.php. See urllogin/README.txt for more details.', array('@configuration' => url('admin/config/people/urllogin')))
  • t('Configured:' ) . urllogin_passphrase(),
    To give translators a better context, the following would be better:
    t('Configured: @passphrase', array('@passphrase' => urllogin_passphrase())
    That being said, I don't think you should print the passphrase so it gets visible to some users.

These are just some examples of things that should be improved after a quick review. A more thorough review could yield more improvements...

darrellduane’s picture

Thanks!

daluxz’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: -#2294425: How to set validation number, -#1775676: API for generate login url for user
StatusFileSize
new4.7 KB

I fixed a bunch of issues and rerolled your patch.

I removed everything that was not related to this issue. Smaller patches that only contain changes that are described in the issue title are easier to review, and hopefully will get committed faster.
I'm with you that this module creates somewhat of a security hole if not configured, so I hope this patch makes it.

Please create seperate patches for the other issues and create issues for them if you haven't already. (I also saw the API function in this patch, which I know you already made a seperate patch of)

The following has been left out:

There are also a number of other usability features added. For one, the download filename now incorporates the Validation number for generating new URLs in the filename, and in the top of the header.

Also, documentation and links to quickly edit the permissions for this module have been added, as well as instructions about how to set the UID for testing the URL login tokens.

Further, I've added documentation to fix the related issue #2294425

I hope to see your patches for these issues, especially for the documentation, since I think we should discuss that.

Updated the issue summary so it reflects the current patch.

darrellduane’s picture

Wow, thanks so much for your help. I'll start new or update existing issues with these other fixes, and clean up the translatability for them. Of course, if you want to help further and work on those, that's cool too.

daluxz’s picture

No problem. I'm using this module for a project at work, and I'd like it to be as decent as possible.
Since you also seem to be concerned about security, you might be interested in the issues I created: