Attached patch does essential Drupal 6 changes, incorporates http://drupal.org/node/168339, and squishes some notices. It's marked code needs work because the filter doesn't work right on my setup (i.e. setting to only block user/1 still requires a HTTP-AUTH login at ), but maybe that's just my system.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deekayen’s picture

oops, <front> was filtered out as a bogus tag in HTTP-AUTH login at <front>), but maybe

Junyor’s picture

Status: Needs work » Needs review
FileSize
8.44 KB

Note: I recommend against using this ptach on a live site until it is thoroughly tested.

Attached is my attempt at porting the 5.x-1.2 release to DRUPAL-6. It seems to work fairly well, but there are a couple things that need to be tested further:

* Does the auth dialog consistently get shown as it should for cached pages? This can be an issue because bootstrapping changed in Drupal 6.x so there's both hook_boot and hook_init (which Secure Site uses). The latter supposedly only runs for uncached pages, but Secure Site seemed to work fine just the same
* Does the updated password reset URL handling make sense? The parameters to user_pass_reset() changed in Drupal 6.x so they require a form state, which Secure Site doesn't have. It occurred to me that the menu system could just handle password reset URLs if Secure Site is bypassed, so that's what's done in the patch. I'm not sure if that opens up any holes, though

Detailed changelog for the patch:
* Updated documentation to reflect new paths
* Added required core version compatibility string to .info file
* Updated hook_menu() for the new menu system
* Removed call to user_pass_reset() and let the menu system handle the request based on the URL
* Updated to new user_authenticate(). Since distributed authentication is gone from core, user_authenticate() (and user_authenticate_finalize(), which it calls) now do some more of the stuff Secure Site used to do
* Updated all watchdog() calls (removed t())
* Fixed a PHP notice seen when logging out because there was no global $user object (may need to be done on the DRUPAL-5 branch, too)
* Updated securesite.inc inclusion
* Updated drupal_mail() handling. The mail should now be translatable, the new way of checking the mail status is used, and securesite_mail() was added (as needed)

I think that's it. I'd appreciate feedback on this. I'm unlikely to commit this and create a DRUPAL-6--1 branch until someone else has tested it.

NaX’s picture

@Junyor
I will try to review your patch as soon as I can, but it might be a while.
Thanks for all the hark work.

Junyor’s picture

@NaX: Great, thank you!

FWIW, I asked chx and Morbus to review Secure Site on Drupal Tough Love (http://www.drupaltoughlove.com/).

NaX’s picture

Status: Needs review » Needs work

I have reviewed the patch. I tested the patch against securesite 5.x-1.2 (Maybe it would have been better to use 5.x-1.x-dev) on D6.2

The patch did not apply cleanly and I had to apply it manually. After applying it manually I ran into one small problem related to argument changes to the url() function. The only place I found this problem was with the suffix in $form['authentication']['securesite_enabled'] of the settings form.

After fixing that small problem I found all the basic out the box features of securesite to work as expected. I am unable to test the more advanced setups and features like LDAP.

To answer your questions:

Caching:
When caching is enabled (NORMAL) it does not work. At first I thought it did but after browsing a couple pages with securesite off and caching on to get some pages cached. I then found securesite not to work any more. So I played around a bit with hook_boot() and found that it worked but required you to reload the admin/build/modules so that the system table could be rebuilt and the column bootstrap could be updated for securesite.

EG.

/**
 * Implementation of hook_boot()
 *
 * hook_init() cant be used as of drupal 6, it is not called on cached pages.
 * Implements the securesite authentication
 */
function securesite_boot() {
  global $user, $base_path;

Every thing seemed to work after this, but then I ran into a new problem. When hook_boot is called the only modules that are loaded are other bootstrap modules. So the user module and ldap module are not loaded.
So I added this above step6 and incremented step6 and 7.

  // Step #6: Load required modules
  $cache_mode = variable_get('cache', CACHE_DISABLED);
  if ($cache_mode != CACHE_DISABLED) {
    drupal_load('module', 'user');
    drupal_load('module', 'ldapauth');
  }

I don’t know if this is the best way to do things but it seemed to work.

Password reset URL handling:
I have not been able to test this fully yet, ran into some small sendmail issues. I will test this again when I have resolved my issues. But what I can tell you is that it looks ok, but I did find a problem.

Form what I can tell we need these '!username','!site','!uri_brief','!login_url','!edit_uri' variables for the email. But that’s not the problem. The problem is with caching again. Step #2 uses the t() function to set the message and that fails when caching is enabled. Maybe we should re visit this issue http://drupal.org/node/217466 or we could just remove the t() function call.

I think that’s all I have found. This was not the cleanest test as I had to apply the patch by hand and that alone could have introduced some problems. Maybe re-role your patch so I can test it cleanly and give you more reliable feedback.

Besides the caching issues that have always plagued securesite and change with every Drupal release, I think it wall works fine. Great work Junyor.

Junyor’s picture

So sorry! Here's a patch that applies cleanly to the Secure Site 5.x-1.2 release. I didn't retest, just tried to fix the errors in the patch.

Using hook_boot is going to be tough, but if that's what we have to do, we'll make it work. Same with t() during caching. We'll figure it out!

It's probably going to be a couple weeks before I can work on this further. I'm slowly working on adding correct Doxygen documentation right now. Then, I'll probably release 5.x-1.3 with a bug fix or two and try to base the DRUPAL-6 version on 5.x-1.3.

tsfftf’s picture

any news on a proper version 6 being released? doesn't seem like anyone is working on this module.

Junyor’s picture

I'm working on it. My plan is to fix #103268: Log-in dialog is displayed on public page after log-out, release 5.x-1.3, then finish the Drupal 6 port. I'd appreciate help, of course.

Junyor’s picture

I'll continue adding tests and release 5.x-1.3 when I'm comfortable that everything is working correctly, hopefully within the next two weeks. Test contributions are welcome. After 5.x-1.3 is released, I'll finish the 6.x port.

tsfftf’s picture

i'm willing to test this thing sure, not too good in the way of programming for drupal though but i'll help test it. you got an svn for the 5.x-1.3 atm?

Junyor’s picture

If you want to test what will become the 5.x-1.3 release, you can checkout the DRUPAL-5 branch of the Secure Site module from Drupal contrib CVS.

Junyor’s picture

Here's an updated patch for the latest DRUPAL-5 code. It still doesn't work correctly, but it's closer to what 5.x-1.3 will be.

Junyor’s picture

Updated again to the latest DRUPAL-5 code. Test suite pass rate is 100%. We're now using hook_boot() and immediately doing a full bootstrap.

tsfftf’s picture

cool, nice progress - eagerly awaiting the 6x release so i can upgrade my site - nice work junyor!

NaX’s picture

@Junyor
It’s looking really good.

Some suggestions when it comes to performance.
I think it would be a good idea to only do the bootstrap if securesite is enabled. To do this we should maybe consider moving securesite_filter_check() into its own step. This way we avoid having to do a full bootstrap when securesite in not enabled and even if secure site is enabled we avoid calling securesite_filter_check() when a user or guest has already logged in.

What you think?

EG.


// Step #1:
...

// Step #2:
// Do a full bootstrap since Secure Site uses many functions throughout Core,
// such as path.inc functions, user.module functions, t(), and theme()
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
// User not logged in, but accessing a page in the bypass list
if (!$user->uid && securesite_filter_check()) {
  return;
}
Junyor’s picture

@NaX: Yes, that's a good idea. I think we need to do at least a partial bootstrap so we can use variable_get(), user_access(), and request_uri(), but I'll test to see which step we need. The same could probably be done in DRUPAL-5.

Junyor’s picture

Status: Needs work » Fixed

Slightly modified version of last patch committed to the DRUPAL-6--1 branch.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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