Right now acquia_spi_check_login() only accounts for the Secure Pages module, but Secure Login is also able to force user authentication over HTTPS.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gcassie’s picture

Status: Active » Needs review
FileSize
1.35 KB
coltrane’s picture

Status: Needs review » Needs work
  1. +++ b/acquia_spi/acquia_spi.module
    @@ -671,7 +671,27 @@ function acquia_spi_check_login() {
    +    // all the required forms should be enabled
    

    Minor: capitalize and end in period please

  2. +++ b/acquia_spi/acquia_spi.module
    @@ -671,7 +671,27 @@ function acquia_spi_check_login() {
    +      if (!variable_get($form_variable)) {
    

    Needs a default or else throws a notice

  3. +++ b/acquia_spi/acquia_spi.module
    @@ -671,7 +671,27 @@ function acquia_spi_check_login() {
    +    if ($forms_safe && !variable_get('https'))  {
    

    Needs default

coltrane’s picture

+++ b/acquia_spi/acquia_spi.module
@@ -671,7 +671,27 @@ function acquia_spi_check_login() {
+  else if (module_exists('securelogin')) {

Also, "elseif" please per https://drupal.org/coding-standards

gcassie’s picture

Status: Needs work » Needs review
FileSize
4.3 KB

Thanks. Updated patch.

Status: Needs review » Needs work

The last submitted patch, 2081045.patch, failed testing.

coltrane’s picture

@gcassie #4 patch looks to be to evergage module and not acquia_spi

gcassie’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

Let's try that again.

coltrane’s picture

Secure Pages still needs mixed mode support https://drupal.org/project/securepages so the variable 'https' would be TRUE. What do you think about leaving out that check and just relying on the forms?

gcassie’s picture

FileSize
1.57 KB

Sorry, that ! on the variable_get('https', FALSE) check is wrong (new patch). I think we should still check for it, though - relying on just the forms doesn't actually make for a secure login, does it?

coltrane’s picture

On further review I think your first patch (#7) is actually correct. For securelogin support $conf['https'] should be FALSE, so the !variable_get() will evaluate TRUE. I'll do little more testing and then commit.

gcassie’s picture

FileSize
1000 bytes

Yes, I got the two module names crossed as we talked - #7 is what we want I think. Here is a D6 version of it.

coltrane’s picture

FileSize
843 bytes

conf['https'] didn't exist in Drupal 6 so quick reroll

coltrane’s picture

Status: Fixed » Closed (fixed)

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