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.

CommentFileSizeAuthor
#12 2081003.D6.do-not-test.patch843 bytescoltrane
#11 2081003.D6.do-not-test.patch1000 bytesgcassie
#9 2081003.patch1.57 KBgcassie
#7 2081003.patch1.57 KBgcassie
#4 2081045.patch4.3 KBgcassie
#1 2081003.patch1.35 KBgcassie

Comments

gcassie’s picture

gcassie commented
Status: Active » Needs review
StatusFileSize
new 2081003.patch1.35 KB
coltrane’s picture

coltrane
he/him
commented
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

coltrane
he/him
commented 
+++ 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

gcassie commented
Status: Needs work » Needs review
StatusFileSize
new 2081045.patch4.3 KB

Thanks. Updated patch.

Status: Needs review » Needs work

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

coltrane’s picture

coltrane
he/him
commented

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

gcassie’s picture

gcassie commented
Status: Needs work » Needs review
StatusFileSize
new 2081003.patch1.57 KB

Let's try that again.

coltrane’s picture

coltrane
he/him
commented

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

gcassie commented
StatusFileSize
new 2081003.patch1.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

coltrane
he/him
commented

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

gcassie commented
StatusFileSize
new 2081003.D6.do-not-test.patch1000 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

coltrane
he/him
commented
StatusFileSize
new 2081003.D6.do-not-test.patch843 bytes

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

coltrane’s picture

coltrane
he/him
commented
Status: Needs review » Fixed

Committed. http://drupalcode.org/project/acquia_connector.git/commit/bbe2e25 and http://drupalcode.org/project/acquia_connector.git/commit/f8edb06

Status: Fixed » Closed (fixed)

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