I created two basic pages and used protected_node to protect them. I set protected_node to use a global password and according to the statistics my pages are protected by the global passwords. However, the password is never accepted. I always see the Incorrect Password message.

If I switch to using a per-node password, even if it's the same password as the global password, protected_node will accept the passwords. However, I have to enter the password for both pages. I really need the persistence feature.

Hacking the code to figure out what is going on, it seems to completely bypass this block of code in protected_node.redirect.inc that begins:

$nid = db_query("SELECT nid FROM {protected_nodes} WHERE protected_node_passwd = :protected_node_passwd AND nid = :nid", array(':protected_node_passwd' => $passwd, ':nid' => $protected_node_nid))->fetchField();
  $node = node_load($protected_node_nid);
  if (empty($nid)) {

and goes straight to the error message.

I looked in the database and when a global password is used, no passwords are stored with my protected nodes in the protected_nodes table. select nid, protected_node_passwd from protected_nodes shows my two nodes with empty strings. I'm not sure if that is the expected behavior.

CommentFileSizeAuthor
#10 global_password_does-2266507-10.patch7.05 KBGrimreaper
#4 global_password_does-2266507-4.patch4.35 KBGrimreaper
#2 protected_node_remove_sql-2266507.patch5.31 KBGrimreaper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented

Hello,

Sorry to respond lately. I will see the problem this weekend.

Thanks to highlight this db_query, I forgot it when I rewrote the queries to use Drupal database query API. There was not the "todo" tag on it.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Assigned: Unassigned » Grimreaper
Status: Active » Needs review
FileSize
protected_node_remove_sql-2266507.patch5.31 KB

Hello,

The validation function is very weird. I fear to introduce regression by modifiying it.

Meanwhile, I found a solution, define a password for the content type and then the super global password will be taken into account. Very very weird.

So if a maintainer wants me to clean this function ok, I will do it. Otherwise I prefer rewrite this module completely (maybe a 7.x-2.x branch ?). If nobody will touch this function, I will document that trick.

I attach a patch that remove sql query (rewritten with the Drupal API).

I found a variable $sql not used, so I removed it.

I found a lack of test on a variable about email. I got a notice.

Oh, I forget, I witness that the bug exists :)

Thanks for the review.

izus’s picture

izus CreditAttribution: izus commented

Hi dsoini,
can you please test and confirm #2 in meeting the needs or not.
Thanks

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
FileSize
global_password_does-2266507-4.patch4.35 KB

Rebasing patch from comment #2.

izus’s picture

izus CreditAttribution: izus commented
Status: Needs review » Needs work 
  1. +++ b/protected_node.module
@@ -460,24 +460,27 @@
+          ->fields('protected_nodes', array('protected_node_passwd'))

    do we need selected fields here ? 

  2. +++ b/protected_node.redirect.inc
@@ -145,21 +145,26 @@
+    ->fields('protected_nodes', array('nid'))

    do we really need to select nid field ? we already have a condition on the nid we want to select 

  3. +++ b/protected_node.redirect.inc
@@ -174,21 +179,26 @@
+                ->fields('node', array('created'))

    d we really need the created field ?

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented

I didn't focus on the logic and questions about what we need.

I just rewrote the queries to use the Drupal database API.

I prefer focusing on "the great cleaning" later when all the tests will be here. :)

  • Commit 3d75bb5 on 7.x-1.x authored by Grimreaper, committed by izus: 
    Issue #2266507 by Grimreaper | dsoini, izus: Fixed Global password does...
izus’s picture

izus CreditAttribution: izus commented
Status: Needs work » Active

merged #4 as it seems ok for me and it's the cleanup we are lloking for.
but i'm not closing the issue as i didn't test if this solves it or not.
if you guys dsoini or Grimreaper tested and are ok. we can mark the issue ad fixed.
thanks

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented

No, the little cleanup has no relation with the issue.

I post the patch in this issue in comment #2, as I discover the queries not using the Drupal database API trying to solve the issue.

It's because there is this issue, that I wanted to first write tests, because I think solving this issue can have a lot of side effects.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Active » Needs review
FileSize
global_password_does-2266507-10.patch7.05 KB

Hello,

Here is a patch that add tests for global password features and fix the bug, also witnessed by the tests.

I made less modification as possible in protected_node.redirect.inc but I still found this validation function hard to read and understand.

I think it will be necessary to rewrite it when there will be no more bugs.

  • izus committed 6e4035c on 7.x-1.x authored by Grimreaper 
    Issue #2266507 by Grimreaper | dsoini: Fixed Global password does not...
izus’s picture

izus CreditAttribution: izus commented
Assigned: Grimreaper » Unassigned
Status: Needs review » Fixed

merged #11
Thanks

Status: Fixed » Closed (fixed)

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