I've limited IP login pages at /admin/settings/ip_login by selecting "Login by IP only on the listed pages." and listing their paths.

Cleared all cookies, then went to a non-login page, but there were IP Login cookies as follows:

Array
(
    [ip_login_as_different_user] => 
    [ip_login_by_ip_complete] => 
    [ip_login_checked] => 1
    [ip_login_uid] => 0
)

Could you please make sure that if there are no cookies set, they should not be set unless the user goes to permitted login pages.

This is absolutely crucial for many sites that employ external caching, which is broken if there are session cookies. E.g. Varnish in Pressflow - see https://wiki.fourkitchens.com/display/PF/Modules+that+break+caching,+and...

CommentFileSizeAuthor
#4 ip_login.zip6.65 KBvacilando

Comments

vacilando’s picture

vacilando commented
Status: Active » Needs review

Here's a solution (sorry, no patch due to lack of time):

Make a new function ip_login_path() from the following code from ip_login_login():

    // Only do login on certain pages - if such pages are entered in admin page
    // adapted from core Block module -> block_list($region)
    if (!isset($_GET['ip_login_override_pages'])) {
      $pages = variable_get('ip_login_active_pages', '');
      if ($pages) {
        $page_match = FALSE;
        // first char, if variable set, is 'check_mode' - remainder is paths to match or PHP
        $check_mode = substr($pages, 0, 1);
        $pages = substr($pages, 1);
        if ($check_mode < 2) {
          $path = drupal_get_path_alias($_GET['q']);
          // Compare with the internal and path alias (if any).
          $page_match = drupal_match_path($path, $pages);
          if ($path != $_GET['q']) {
            $page_match = $page_match || drupal_match_path($_GET['q'], $pages);
          }
          // When $check_mode has a value of 0, the IP checkl happens on
          // all pages except those listed in $pages. When set to 1, IPs
          // are checked only on those pages listed in $pages.
          $page_match = !($check_mode xor $page_match);
        }
        else {
          // evaluate PHP
          $page_match = drupal_eval($pages);
        }
        // if we don't have a path/PHP match, don't log in
        if (!$page_match) return;
      }
    }

but make it return
if (!$page_match) return FALSE;

Then in ip_login_login() use
if (ip_login_path() === FALSE) return;

And then -- here's the actual solution -- add
if (ip_login_path() === FALSE) return;
as the first thing in ip_login_boot().

Works fine for me. Please test, apply to the release, and let me know.

jim kirkpatrick’s picture

jim kirkpatrick commented

OK, thanks Vacilando, I'll take a look later this week and post a new 6.x-2.x-beta2 release if your solution works as described.

jim kirkpatrick’s picture

jim kirkpatrick commented

Except the code in #1 doesn't and cannot work because drupal_get_path_alias() (or anything in path.inc) is not available during hook_boot, so the check cannot happen before more of the bootstrap process has happened. Presently I'm getting a 'Function not found' error when putting the ip_login_check_path() (as I named it) in hook_boot.

Your code makes sense in as far as that section of code should certainly be a separate function, but unfortunately it (on it's own) won't work.

It boils down to this: A path check is needed early to avoid cookie setting on every page, which messes up external caches like Varnish. BUT path checks can only safely/'properly' happen from hook_init() onwards, and most of IP Login's logic happens in hook_boot() to avoid the fact that hook_init() doesn't run on cached pages! Catch 22... See #509028: doesn't log in until after second page load and #791410: Improved: now supports logging in as alternate user plus comma separated values, IP ranges and wildcards for more background.

What might work is doing the preliminary checks in hook_boot, then storing the values in the global $user until hook_init is ready to use them and has path.inc available. BUT this means that we need another way to make sure we don't keep testing a user IP (with associated database query) every request AND a way to avoid caching skipping the hook_init() stuff altogether...

This may, because of the way Drupal's bootstrap process works, be impossible or at least very convoluted... I'll put my thinking cap on, but in the mean time I've committed the newer version to DEV without the call in hook_boot. It might be that path checking for IP login is just disabled when external caching is enabled, and your Varnish config will have to only allow cookies/pass through on the paths you would have put in the IP login path anyway... OR we need another - non-hacky! - way of checking paths that isn't reliant on path.inc...

Hmmm.... Ideas, anyone?

vacilando’s picture

vacilando commented
StatusFileSize
new ip_login.zip6.65 KB

Interesting. Function not found would return a fatal error, no? When I tested, it definitely worked -- I was logged in only on the pages I allowed for IP login and all other pages were nicely Varnished. Not having the time to re-test/debug right now, I quickly attach my version of ip_login.module as I had modified it.

A quick thought -- can't we just use $_GET['q'] in the path checking function and check it against path values saved at submitting the settings form? Might be a worthless idea -- really did not think it over.

jim kirkpatrick’s picture

jim kirkpatrick commented

$_GET['q'] is the staring point, yes. But these functions in path.inc do some other nice things, which now I think about them, I'm not sure we particularly need.

Well we could drop the need for drupal_get_path_alias() which calls drupal_match_path() by just plain not caring about non-aliased pages... I.e. an admin setting IP Login to occur for 'about/cheese' would also need to enter it's non-aliased path too, e.g. 'node/123' into the IP login admin page... Then we'd just need a note in the help section of that field.

That just leaves drupal_match_path() which is quite a simple function we could just copy into IP Login and change the name, like so:

function ip_login_match_path($path, $patterns) {
  static $regexps;

  if (!isset($regexps[$patterns])) {
    $regexps[$patterns] = '/^(' . preg_replace(array('/(\r\n?|\n)/', '/\\\\\*/', '/(^|\|)\\\\<front\\\\>($|\|)/'), array('|', '.*', '\1' . preg_quote(variable_get('site_frontpage', 'node'), '/') . '\2'), preg_quote($patterns, '/')) . ')$/';
  }
  return preg_match($regexps[$patterns], $path);
}

It's all slightly hacky, but since external caching is a big deal for me as one of my Drupal sites uses it too, and it's kinda 'the future' of fast Drupal installations, I'm inclined to go with the above.

So I have! It's committed to DEV, have a play and let me know.

jim kirkpatrick’s picture

jim kirkpatrick commented

I've now also updated the admin page help text to explain all this...

vacilando’s picture

vacilando commented

Nice! And it seems to work, too -- no session set on pages other than the permitted one!

As for the permitted path -- I have the login form embedded in a node which is aliased. When I specify only the alias on the admin page, I do not get logged in. When I specify the system path (node/x) then it works. So perhaps the help text is wrong... instead of "you must also enter system paths" it should say "you must enter system paths"?

jim kirkpatrick’s picture

jim kirkpatrick commented
Priority: Major » Minor
Status: Needs review » Needs work

Hmm... so close! OK, I'll take a look. Didn't realise $_GET['q'] would have the system path when requesting an alias... Will test and tweak accordingly later.

jim kirkpatrick’s picture

jim kirkpatrick commented
Status: Needs work » Closed (fixed)

I've checked this and it works as expected. Arriving at www.example.com/this-page means $_GET['q'] == "this-page", while arriving at www.example.com/node/123 means $_GET['q'] == "node/123". That's what the admin screen says:

You can choose specific paths that IP Login will or will not try to log in a user. Please note:

  • No alias or system path lookup is performed, so you will need to have both paths listed if you want IP logins to happen on both paths - e.g. alias "about/this-site" and system path "node/123" would both need to be added.
  • This will not affect users with can log in as another user permission who have already logged out and have a cookie.

All done, closing.