Problem with the update in protected_node_update_7102

The problem is that in update_7102 the 'bypass password protection' is tried to be assigned to the users.

However, in the same commit, the actual code mentioning that permission (the hook implementation protected_node_permission in the protected_node.module file) is removed. The user_role_grant_permissions checks all available permissions, an array that does no longer include 'bypass password protection'.
The line user_role_grant_permissions($rid, array('bypass password protection')); causes an 'Integrity violation : 'module' can't be NULL'

(The reason being the permission is not found, and thus the system does not know to which module it belongs, leaving a non-NULL field empty)

This in turn does not allow the update_7102 to finish correctly, and in turn does not allow the update_7103 to be run.

Hi izus,

I haven't been able to test it yet, but i'm pretty sure it's not correct.

It would go like this:

Before update_7103 was introduced, everything was fine. The update_7102 would run ok.
The call user_role_revoke_permissions just deletes a permission from the database.
It doesn't check if the permission is linked to a module, so this worked with update_7102 (although the code that was introduced in the same commit removed the view protected content permission)
The removed permission was just 'moved' to the bypass password protection.
That permission was available in code (i.e. listed in protected_node_permission())

But with the addition of update_7103 the above situation is no longer the case.
bypass password protection is no longer available in code, which gave us the original error in update_7102.
But update_7103 still needs that permission. Because it is removed from the code, it needs to be reassigned to some other permission, because otherwise permissions will be lost due to the update.

Your proposed patch however will now make update_7102 just skip the assignment of the bypass password protection (because it does no longer exist in the code) , and update_7103 will then falsely assume that no role has the bypass password protection assigned, and will not assign the permissions in it's foreach loop.

Note: This is of course a specific situation, for people who are updating from a version prior to the commit that has the update_7102 code.

Am i making sense ?

Hi izus,

To reitterate my 'problem' with some real life example, which might turn out to be rather exceptional (since i can't find any more tags on the repository):

PREREQUISITES:
- a site that has a version of the Protected Node module BEFORE commit 412cb2aceb139ebf9551 ( the one introducing update_7102 )
- users that have been assigned the 'view protected content' role

ACTION:
upgrade the protected_node module to a version AFTER commit a328ef52d0c52931f728e ( the one introducing update_7103 )

CODE CHANGES:
The commits with update_7102 & update_7103 make code changes to the protected_node_permission function, by removing several permissions.
The removed permissions include:
view protected content (update_7102)
bypass password protection (update_7103)

RESULT WHEN RUNNING THE UPDATE:
update_7102: the user_role_grant_permissions call (as mentioned in a previous comment) tries to grant the bypass password protection permission to users having the 'view protected content' permission (because it has removed it from the code)

That call however does not work (Integrity violation : 'module' can't be NULL ), because the function looks up the 'bypass password protection' permission, but does not find it anymore (because it was removed by the code with update_7103). The call to user_role_revoke_permissions however DOES work (it doesn't look up any permission, it just removes the permission by its name)

We now have lost information, because the users, that used to have the 'view protected content' permission, no longer have it. But they did not get granted the 'bypass password protection' , because it no longer existed.

update_7103 now tries to find users with the 'bypass password protection' permission, to give them different permissons.
However, because of the changes that update_7102 made, some of the users, that used to have the 'view protected content' permission, will not get those permissions, and will thus lose several permissions.

SOLUTION ?
Looks a bit tricky, because it has to work for people who are in between the two commits as well.

I'm hoping the specific situation/problem is now clear ?

hi izus,

i was going from the code without your first patch.
If that one was applied, the solution would not work.
It would however work if we extend it with a test for the other permission :)
That would work for me :)

Thanks for replying !

Hi izus,

A few small remarks:

1. +++ b/protected_node.install
   @@ -118,18 +118,23 @@ function protected_node_update_7101() {
   +      ->condition('permission', 'view protected content')
   
   @@ -155,14 +160,41 @@ function protected_node_update_7103() {
   +  // because 'bypass password protection' does no more exist in code.
   

   Small English correction: use "no longer exists" or "doesn't exist anymore" instead of "does no more exist"

2. +++ b/protected_node.install
   @@ -155,14 +160,41 @@ function protected_node_update_7103() {
   +  $permissions = user_permission_get_modules();
   ...
   +  if (!isset($permissions['bypass password protection'])) {
   ...
   +    }
   

   These lines are unnecessary: the code that update_7103 comes with, has removed the 'bypass password protection' permission, which means it will NEVER be set in the permissions array.

3. +++ b/protected_node.install
   @@ -155,14 +160,41 @@ function protected_node_update_7103() {
   +      // Remove "bypass password protection" from database.
   +      user_role_revoke_permissions($rid, array('bypass password protection'));
   

   There's no need to remove the 'bypass password protection' permission from these users, since they are the ones that never got this permission assigned to them in the first place.