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.

Files: 
CommentFileSizeAuthor
#12 2081003.D6.do-not-test.patch843 bytescoltrane
#11 2081003.D6.do-not-test.patch1000 bytesgcassie
#9 2081003.patch1.57 KBgcassie
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]
#7 2081003.patch1.57 KBgcassie
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]
#4 2081045.patch4.3 KBgcassie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2081045_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 2081003.patch1.35 KBgcassie
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.35 KB
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]

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

+++ 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

Status:Needs work» Needs review
StatusFileSize
new4.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2081045_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks. Updated patch.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]

Let's try that again.

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?

StatusFileSize
new1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 356 pass(es).
[ View ]

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?

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.

StatusFileSize
new1000 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.

StatusFileSize
new843 bytes

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

Status:Fixed» Closed (fixed)

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