Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BenK’s picture

Subscribing... very interested in D7 port as well.

beedaddy’s picture

subscribe

adeb’s picture

this is a must have

Dave Reid’s picture

Subscribing...maybe be able to help with this.

spacereactor’s picture

subscribe

BenK’s picture

Now that the Drupal 7 beta release is right around the corner (scheduled for May 21), I thought I'd check back in to see if anyone had made any progress on this D7 port....

--Ben

BenK’s picture

@Dave Reid: I'm familiar with your great work on a bunch of other modules. Are you still able to help with this?

Also, just a note that the ACL module seems to have a mostly working Drupal 7 port... so for those of us who use Content Access with ACL, we're at least part way there. :-)

Here's a link to the D7 release of ACL:

http://drupal.org/node/694210

--Ben

atchijov’s picture

FileSize
41.08 KB

If anyone interested, I have done quick and dirty port (module attached) It works well enough for me on alpha-5.

One caveat, I am not 100% sure that it is related to this module, but once I got this module running I start getting "1052 Column 'nid' in where clause is ambiguous" message (line 1285 of /Library/WebServer/Documents/touchNOC/includes/menu.inc). I was able to get rid of this message by changing line

$select->condition('nid', $nids, 'IN');

to

$select->condition('node.nid', $nids, 'IN');

As you probably can imagine, I do not provide any guaranties, but I do not mind to help to get this module running if you hit any snags.

atchijov’s picture

FileSize
41.15 KB

Much improved version. Pretty much everything works.

BenK’s picture

Hey atchijov,

Thanks for getting the ball rolling on this! I just tried installing the version of the module you posted in #9 and I got the following error message:

DatabaseSchemaObjectExistsException: Table content_access already exists. in DatabaseSchema->createTable() (line 605 of /xxx/xxx/xxx/public_html/includes/database/schema.inc).

Any ideas?

It does look like the module installed, however, and I'll continue testing. But I thought I would post this first.

--Ben

Francewhoa’s picture

Status: Active » Needs work
FileSize
21.97 KB
22.25 KB

Same here. I tested #9 and Drupal display the same error message as #10. Find attached screenshot to clarify. Hope this helps.

Using 7.0-alpha5 2010-May-23
Fresh install

Steps to reproduce issue
*Copy module to sites/all/modules folder.
*Go to #overlay=admin/modules Activate module.
*Click on Save configuration button.
*Drupal display error message

BenK’s picture

Status: Needs work » Active

Hi atchijov and everyone,

I've just completed some initial testing of atchijov's D7 port attached to #9. First, the basic functionality of the port works pretty well. I was able to control view, edit, and delete access by role for a given node. Nice work.

I did find, however, some bugs... some major and some minor ones. I've described them below:

1. CONTENT TYPE DEFAULT SETTINGS IN WRONG LOCATION: At first, I didn't see any way to set the access control defaults for a given content type. For the "article" content type, for instance, when I went to edit page for the article content type, I didn't see a tab named "Access" as I would have in the D6 version. Likewise, I tried to manually go to "admin/structure/types/manage/article/access", but that didn't work either.

Then I remembered that in the D6 version, the path would have been different. So I manually went to admin/content/node-type/article/access and I was able to find the content type access settings. So we need to get this settings page to appear on the right path for D7 with the appropriate tab on the content type edit page.

2. WARNING MESSAGE: On the content type access control defaults page, when trying to save changes to "view any content" or "view own content," I'm getting the following warning message:

"Warning: htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 1473 of /home/sandboxes/sandbox10/public_html/includes/bootstrap.inc)."

This doesn't happen every time, but if you fiddle with the settings a few times (saving each time), you'll eventually see it.

3. EDIT AND DELETE DEFAULT PERMISSIONS WON'T SAVE:

On the content type default access control page, when changing the "edit any content," "edit own content," "delete any content," and "delete own content" permissions, it won't save for default Drupal roles (anonymous, authenticated, and administrator). For roles created by the site admin (I created a role called "editor"), I'm getting the following error message:

"PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '4-4' for key 1: INSERT INTO {role_permission} (rid, permission) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 4 [:db_insert_placeholder_1] => 4 ) in content_access_save_permissions() (line 282 of /home/sandboxes/sandbox10/public_html/sites/all/modules/contrib/content_access/content_access.admin.inc)."

4. PER CONTENT NODE CHECKBOX NOT CHECKED BY DEFAULT BUT STILL IS ACTIVATED: On the content type access control defaults page, the "Enable per content node access control settings" checkbox is not checked by default even though this functionality is being enabled by default. The checkbox, however, does appear to function properly once you manually check it and uncheck it. The way its supposed to work (based on the D6 version) is that it is unchecked by default and the per content node access control is disabled by default.

5. OVERLAY NOT WORKING ON CONTENT TYPE ACCESS CONTROL DEFAULTS PAGE: Even though the "Seven" administration theme is being called, the page is not being displayed in the D7 admin overlay.

6. OVERLAY AND ADMIN THEME NOT WORKING ON PER NODE ACCESS CONTROL PAGE: On the "Access" page for a given node, not only is the overlay not working, but the page is being displayed in the Garland theme instead on in the Seven default admin theme. As a result, some of the usability improvements aren't present (making it easy, for instance, to click the "Reset to defaults" button when you meant to click the "Submit" button).

7. RESET TO DEFAULTS ERROR: When controlling the access for a given node, I clicked the "Reset to defaults" button and received the following error message:

"Recoverable fatal error: Argument 2 passed to db_query() must be an array, string given, called in /xxx/xxx/xxx/public_html/sites/all/modules/contrib/content_access/content_access.module on line 415 and defined in db_query() (line 2226 of /xxx/xxx/xxx/public_html/includes/database/database.inc)."

Checking the page after this error message, the defaults are not being re-set.

Well, that's it for now. Thoughts on the causes or potential solutions for any of these issues? atchijov got the ball rolling and let's keep up the momentum on this!

--Ben

BenK’s picture

Status: Active » Needs work
Francewhoa’s picture

Thanks for testing Ben :)

BenK’s picture

FileSize
75.54 KB

One more minor bug I just noticed: On both the default content type settings page and the access control page per node, "View any Content" and "View own Content" are not being lined up vertically (unlike the D6 version). The same holds true for "Edit any content" and "Edit own Content," as well as "Delete any Content" and "Delete own Content." This is a usability issue that makes it a bit too easy to select an unintended permission.

To show what I mean, I've attached a screenshot to this comment.

--Ben

atchijov’s picture

Hi Ben,
I have seen this error message. It looks like it does not prevent module from functioning. When I have few sec free, I will try to see what is going on

Andrei

atchijov’s picture

Ben,
Do you mean that it supposed to be 3 columns (I have not worked with D6)? I can see that it will make total sense.

Andrei

BenK’s picture

Hey Andrei,

Thanks for checking back in. Yes, it's supposed to be 3 columns as follows:

Left Column:
"View any content" (top)
"View own content" (bottom)

Middle Column:
"Edit any content" (top)
"Edit own content" (bottom)

Right Column:
"Delete any content" (top)
"Delete own content" (bottom)

Did the rest of my comments and bug reports in #12 make sense?

Thanks again,
Ben

atchijov’s picture

Ben,
Yes, all of your comments make total sense. I can not promise that I will address all of them soon, but we do use this module extensively and I will try my best to fix it.

Andrei

PS Am I missing something or there is no way to to have drupal.org e-mail you when particular issue was updated?

Francewhoa’s picture

@atchijov: I don't know if drupal.org can e-mail you. I wish ;) What I do to keep track of updated issues is going to the following page. Hope this helps. Thanks for your contributions :)

http://drupal.org/project/issues/user

Then under the column Summary search for the red Updated word(s). There are also handy shortcuts under the column Replies. Read more at http://drupal.org/node/317

atchijov’s picture

FileSize
41.2 KB

Some more bug fixed. Most notably: #3 and #7 from comment #12.

BenK’s picture

Hey Andrei,

Thanks for posting the new version in #21. I did some testing on it (focusing on #3 and #7 from comment #12 since you said those were fixed). Here's what I noticed:

#3: Thanks for the improvements on this, but it doesn't appear to be totally fixed. I'm no longer getting the error message (that's great!), but there is a bug when saving changes to the "edit any content," "edit own content," "delete any content," and "delete own content" permissions. Basically, when ADDING these permissions to any role, it saves fine. But when trying to REMOVE any of these permissions, saving doesn't work. Thus, once you grant those permissions, you're stuck with them. Additionally, when viewing a node's access control page, those permissions that have been granted as default for the content type are grayed out... thus you can't remove them at the node level either. Note that these problems appear to be confined to "edit" and "delete" permissions... "view any content" and "view own content" seem to be working properly.

#7: This appears to be fixed and is functioning properly. Thanks!

Comment #15: The permissions now seem to be lined up vertically. Thanks for fixing this.

Also, one quick question: Did you fix the error message mentioned in comment #10? I didn't have time to check this since it would involve re-installing the module on a fresh D7 site.

Thanks,
Ben

BenK’s picture

Andrei,

We also discovered another bug when using your version of Content Access with the ACL module.

Basically, in content_access.admin.inc, there needs to be a change on Line 63 (to prevent the error message reported at http://drupal.org/node/824642):

Change Line 63 to read:

acl_node_add_acl($node->nid, $acl_id, (int) ($op == 'view'), (int) ($op == 'update'), (int) ($op == 'delete'), content_access_get_settings('priority', $node->type));

Note the inclusion of "(int)" in several places. This is because the grants should be ints (i.e. 0 or 1). For more details on this, see comment #3 at http://drupal.org/node/824642.

Andrei, can you make this change in the next version of Content Access that you post? I really appreciate all of your hard work!

Thanks again,
Ben

BenK’s picture

Hey Andrei,

Just wanted to check in to see if you had made any more progress on the remaining issues... I'm happy to do extensive testing.

--Ben

BenK’s picture

Hey everyone,

I've tried to use this D7 version of Content Access in combination with the ACL module (see http://drupal.org/node/798106, Comment #12 for a fix to ACL needed to make this work). But when I've tried to grant a user view access, update access, or delete access to a node by clicking the "Add user" button on the "Access Control" tab, the page reloads, but nothing happens (the user is not added).

After trying this for a few times, I eventually get the following error message:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2' for key 1: INSERT INTO {content_access} (nid, settings) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => a:6:{s:4:"view";a:3:{i:0;i:1;i:1;i:2;i:2;i:3;}s:8:"view_own";a:3:{i:0;i:1;i:1;i:2;i:2;i:3;}s:6:"update";a:1:{i:0;i:3;}s:10:"update_own";a:1:{i:0;i:3;}s:6:"delete";a:1:{i:0;i:3;}s:10:"delete_own";a:1:{i:0;i:3;}} ) in content_access_save_per_node_settings() (line 398 of /home/sandboxes/sandbox10/public_html/sites/all/modules/contrib/content_access/content_access.module).

Any ideas? What I'm not clear about is whether this is a bug with the ACL module or with Content Access itself.

Thanks,
Ben

atchijov’s picture

FileSize
41.38 KB

I have fixed few problems and I have submitted patch for ACL module to make it work (new version attached).

I can not reproduce problem #3 (failure to un-grant permissions once you grant them). Could you please confirm that this is still a problem?

BenK’s picture

Hey Andrei,

Thanks so much for posting your updated module! Very exciting. I've been testing the version of the module you posted in #26 and I'll be posting a full bug review shortly.

But first, I did want to let you know that problem #3 (failure to un-grant permissions once you grant them) seems to have gone away. So whatever you did, it appears to have fixed it. So we can cross #3 off the list.

And problem #7 (reset to default error) has been fixed, too. So we can definitely cross that off the list as well.

I'll be posting more of my testing results in a subsequent comment....

--Ben

BenK’s picture

Andrei,

Here's a full roundup of the bugs I found in the most recent version of the module (comment #26). Thanks again for all of your hard work. The only thing I haven't yet tested is the ACL integration, but I'll post about that in a subsequent comment:

NEW BUGS:

A. ERROR WHEN ADDING NEW CONTENT: After installing the module, I created a new content type ("Test Type") and tried creating some new Test Type content. But when I tried to save the new content, I got a white screen of death and the following error appeared in my apache log:

"PHP Fatal error: Unsupported operand types in /home/sandboxes/sandbox10/public_html/sites/all/modules/contrib/content_access/content_access.module on line 97, referer: http://sandbox10.example.org/node/add/test-type"

The only way to get rid of this error (and WSOD) was by checking "Enable per content node access control settings" on the Test Type access control default page. When I did this, I then received the following error:

"Recoverable fatal error: Argument 2 passed to db_query() must be an array, string given, called in /home/sandboxes/sandbox10/public_html/sites/all/modules/contrib/content_access/content_access.admin.inc on line 468 and defined in db_query() (line 2226 of /home/sandboxes/sandbox10/public_html/includes/database/database.inc)."

But once I did this, the original error went away and I could create new Test Type content. Any ideas?

B. ARRAY DATA DISPLAYED WHEN SAVING ACCESS CONTROL DEFAULTS PAGE: When visiting the access control default page for a given content type and clicking "Submit," I'm getting a huge amount of array data displayed as a confirmation message. Here's a little excerpt of the voluminous code that's being printed to the page:

"array ( 'build_info' => array ( 'args' => array ( 0 => 'test_type', ), 'file' => 'sites/all/modules/contrib/content_access/content_access.admin.inc', ), 'rebuild' => false, 'redirect' => NULL, 'temporary' => array ( ), 'submitted' => ........"

Did you add some debug code to print messages and forget to remove it? ;-)

C. EXTRA "SUBMIT" WHEN SAVING: This is very minor. When saving access control settings for a node, in addition to the "Your changes have been saved" confirmation message, I'm also getting a 2nd confirmation message that says "submit".

D. ERROR WHEN INSTALLING MODULE: When installing the module, I'm getting the following error message:

"DatabaseSchemaObjectExistsException: Table content_access already exists. in DatabaseSchema->createTable() (line 605 of /xxx/xxx/xxx/public_html/includes/database/schema.inc)."

However, the module does seem to install properly.

PRIOR BUGS (AS FIRST REPORTED IN COMMENT #12):

1. Content Type Access Defaults in Wrong Location: Still a problem. Default settings should be located at "admin/structure/types/manage/test-type/access" instead of "admin/content/test-type/access" ('test-type' is the machine name of the content type).
2. Warning Message on 'View Any' or 'View Own': Still a problem.
3. Failure to Un-grant Permissions: FIXED!
4. Per Node Checkbox is Being Activated: Still a problem. May be related to NEW BUG #A (above).
5. Overlay Not Working on Defaults Page: Still a problem.
6. Overlay and Admin Theme Not Working on Node Access Page: Still a problem.
7. Reset to Defaults Error: FIXED!

As always, I'm interested to hear your thoughts....

Best,
Ben

BenK’s picture

Okay, I just finished testing the ACL module fixes and posted my comments on that thread:

http://drupal.org/node/836822#comment-3142272

Basically, most of the integration with ACL appears to be working now except for when viewing/editing the node itself. More specifically, everything works fine if the node's title and teaser is displayed in a list of nodes (such as on the default front page). The node title and teaser do appear to the user granted view access (and disappear if the user's view access is revoked). But if the user then tries to click through to view the node itself, he receives an "access denied" message.

Thanks again, Andrei, for your help with the ACL issue. It seems like we're almost there!

Cheers,
Ben

atchijov’s picture

FileSize
41.39 KB

Most of problems (except overlay related) should be fixed with this version.

BenK’s picture

Andrei,

Wow, terrific update! You've fixed the vast majority of bugs. So here's a roundup of all of the previously reported bugs (as described in Comment #28) and their current status:

A. ERROR WHEN ADDING NEW CONTENT Fixed!

B. ARRAY DATA DISPLAYED WHEN SAVING ACCESS CONTROL DEFAULTS PAGE Fixed!

C. EXTRA "SUBMIT" WHEN SAVING Fixed!

D. ERROR WHEN INSTALLING MODULE: This problem still exists. Looks like something needs to be updated in content_access.install

1. Content Type Access Defaults in Wrong Location Fixed!
2. Warning Message on 'View Any' or 'View Own' Fixed!
3. Failure to Un-grant Permissions Fixed!
4. Per Node Checkbox is Being Activated Fixed!
5. Overlay Not Working on Defaults Page Fixed! (Even though you said it was still a bug!)
6. Overlay and Admin Theme Not Working on Node Access Page: Still a problem. But I've got a potential fix for this (will post to a subsequent comment.)
7. Reset to Defaults ErrorFixed!

I did, however, notice three new (small) bugs:

i. Not Respecting Drupal Core Permissions By Default on Access Control Defaults Page: When a new content type is created, Content Access is supposed to respect Drupal core permissions by default. (This is how it works in the D6 version of the module.) So if authenticated and anonymous users have been granted the "View published content" permission in Drupal core (admin/people/permissions), then when a new content type is created, the access control defaults for that content type should give both anonymous and authenticated users the "View any content" and "View own content" permissions by default. However, in the current module, "View any content" and "View own content" permissions are unchecked by default (except for roles with the "administer content" permission) regardless of the current core permission settings.

Note that everything seems to work properly for Edit Any, Edit Own, Delete Any, and Delete Own permissions. It also works properly if you make subsequent changes to core "View published content" permissions (removing view permission for anonymous users, for instance, will remove it from the access control defaults page for each content type). It just doesn't work when you create a new content type (view permissions are always unchecked by default). This could cause confusion to a site administrator who doesn't understand why users can't view content of the newly created content type. (By the way, I think this worked properly in a prior version of your port, so a recent change may have caused this to regress.)

ii. Incorrect Link on Access Control Defaults Page: Very minor. At the access control defaults page for a content type (admin/structure/types/manage/[test-type]/access), under the "Per content node access control settings" header, there is an incorrect link to the "permissions" page. The link currently goes to "admin/user/permissions" but in D7 is should go to "admin/people/permissions".

iii. Consistent Menu Tab Language: The sub-menu tab of the access control defaults page (for each content type) should probably say "ACCESS CONTROL" instead of just "ACCESS". Obviously not a big deal, but probably should stay consistent with other mentions within the module.

Great work,
Ben

BenK’s picture

Andrei,

In regard to Bug 6 (Overlay and Admin Theme Not Working on Node Access Page) in comment #31, I think this could caused by a corresponding bug in the D6 version of the module that was just recently fixed (it's a short, simple fix). To read about this, go to this thread:

http://drupal.org/node/811338

Also, the D6 patch that was committed (which fixes the bug) can be found here:

http://drupal.org/files/issues/811338-use_admin_theme_6.patch

Basically, the admin theme wasn't being called properly for that page. I think all of the overlay stuff will work once the admin theme is called.

So can the code from the patch be copied (or adapted) to fix Bug 6 in our D7 port? Could you add that to the next version of the module that you post?

Thanks again for all your help,
Ben

atchijov’s picture

FileSize
41.56 KB

I think that patch did solve the problem with overlay. See new version attached...
Personally, I keep my overlay module off. Not sure what is the value of it (beside esthetics)

BenK’s picture

Thanks, atchijov. Will test the new module in #33 later today. Quick question: In addition to the overlay patch, does the module include fixes to D., i, ii, and iii above (in comment #31)?

Cheers,
Ben

atchijov’s picture

@BenK #33 only overlay fix.

BenK’s picture

Andrei,

I tried out your updated module in #33, but unfortunately, the overlay issue doesn't seem to be fixed. Perhaps a small tweak is necessary to make the patch code function in D7? Any ideas?

Thanks,
Ben

P.S. Once we get this working, I think it will also benefit those who turn off the overlay. Since the code allows the node access control page to use the admin theme (rather than the site default theme), it would benefit anyone who uses a different theme for administration tasks.

BenK’s picture

Hey Andrei,

I don't know if you had a chance to take a look at salvis' comments on the ACL module thread (http://drupal.org/node/836822, comments #17 and #20), but he pointed out that there may be an issue with the node access grants currently generated by Content Access.

Currently, if you use the Devel Node Access module to see the "Access Permissions by User" for a given node, it will report access denied as "No: by content_access". But salvis said it shouldn't do this (based on how D7 is supposed to work). Here's what he wrote:

This is new in D7. In prior versions only the module that created the content type was able to authoritatively say NO. In D7 any module can do it, but it should rarely do so, because this overrides the node access completely.

I don't remember what the third value (besides YES and NO) is, but Content Access should return that third value, so that the decision is passed on to the node access system, where ACL's (and CA's!) grants are evaluated. DNA should say something like "YES: node access".

So I'm not exactly sure about the implications of this, but until it's fixed, I'm wondering if this could cause Content Access to have difficulty playing well with other node access modules (like ACL or anything else). Does this mean that Content Access is currently overruling node access set by other modules?

Thoughts?

--Ben

BenK’s picture

Hey Andrei,

Okay, here's the latest update: salvis and I have confirmed that the reason we're having problems with integrating Content Access with ACL is that Content Access is overruling node access set by ACL on node pages. How this works changed in D7.

I did some more research on this (see http://api.drupal.org/api/group/node_access/7). Basically, it appears the root cause of the problem is that Content Access is returning NODE_ACCESS_DENY (which blocks access to the node no matter what ACL says) if someone unchecks a box (for view, edit, or delete permissions) on the access control page. Instead, we should probably be returning NODE_ACCESS_IGNORE to allow other modules or the node_access table to control access (so Content Access doesn't override all other modules).

Here's some more specific information taken from the D7 node access API page (http://api.drupal.org/api/group/node_access/7):

In determining access rights for a node, node_access() first checks whether the user has the "bypass node access" permission. Such users have unrestricted access to all nodes. user 1 will always pass this check.

Next, all implementations of hook_node_access() will be called. Each implementation may explicitly allow, explicitly deny, or ignore the access request. If at least one module says to deny the request, it will be rejected. If no modules deny the request and at least one says to allow it, the request will be permitted.

If all modules ignore the access request, then the node_access table is used to determine access. All node access modules are queried using hook_node_grants() to assemble a list of "grant IDs" for the user. This list is compared against the table. If any row contains the node ID in question (or 0, which stands for "all nodes"), one of the grant IDs returned, and a value of TRUE for the operation in question, then access is granted. Note that this table is a list of grants; any matching row is sufficient to grant access to the node.

In node listings, the process above is followed except that hook_node_access() is not called on each node for performance reasons and for proper functioning of the pager system. When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access" to ensure that only nodes to which the user has access are retrieved.

Note: Even a single module returning NODE_ACCESS_DENY from hook_node_access() will block access to the node. Therefore, implementers should take care to not deny access unless they really intend to. Unless a module wishes to actively deny access it should return NODE_ACCESS_IGNORE (or simply return nothing) to allow other modules or the node_access table to control access.

So, Andrei, can you make the needed changes and post a new version of the module as soon as you have a chance?

We'll probably need to fix both the content type access control defaults page as well as the per-node access control settings page. As always, I'll be an eager tester. It will be great to get all of this wrapped up!

Best,
Ben

BenK’s picture

Just a quick note: The D7 port of ACL is pretty much done and working well. So we only have the remaining bugs on this thread and then we're done! :-)

BenK’s picture

FileSize
29.24 KB

In doing a review of what's left to be done on this D7 port, I noticed that the content_access.rules.inc file may not have been ported yet. Since the Rules 7.x-2.x-dev branch is working well these days, this is something that we should do ASAP. A lot of people (including myself) use the Rules integration along with the ACL module to grant access to a node on specific event triggers.

Fortunately, there are not too many lines of code in that file. I did some research on upgrading the integration to Rules 7.x-2.x-dev and here's a link to a recent thread where Rules API changes (from 6.x-1.x-dev to 7.x-2.x-dev) are discussed:

http://drupal.org/node/832046

As you'll see from fago's comments on the thread, he jotted down some of the biggest changes in comment #2.

Also, the Rules 7.x-2.x-dev module itself appears to have some helpful documentation in the rules.api.php file. I've attached the file to this comment (renamed it with a .txt extension) so that it's handy for reference.

--Ben

BenK’s picture

Hey everyone,

Given all of the comment on this thread, I thought it might be helpful to post a summary of the bugs/issues remaining to be fixed on the D7 port:

a) ERROR WHEN INSTALLING MODULE: Looks like something needs to be updated in content_access.install. See comment #10 for more details on the error message.

b) Not Respecting Drupal Core Permissions By Default on Access Control Defaults Page: When a new content type is created, Content Access doesn't respect Drupal core permissions by default. See comment #31 for more details.

c) Change NODE_ACCESS_DENY to NODE_ACCESS_IGNORE: This change will prevent Content Access from inadvertently overriding other access control modules like ACL. See comments #37 and #38 for more details.

d) Port content_access.rules.inc to D7: We need this file updated so that it can work with Rules 7.x-2.x-dev. See comment #40 for more details.

e) Overlay and Admin Theme Not Working on Node Access Page: We tried a fix for this, but it's still a problem. The patch used may need to be updated to D7. See comment #32 for more details.

f) Incorrect Link on Access Control Defaults Page: Very minor. At the access control defaults page for a content type, there is an incorrect link to the "permissions" page. See comment #31 for more details.

g) Consistent Menu Tab Language: Very minor. The sub-menu tab of the access control defaults page (for each content type) should say "ACCESS CONTROL". See comment #31 for more details.

Well, that's it! Not too much left to do...

Hope this helps,
Ben

atchijov’s picture

I am having problems with this. Either I do not understand how it is supposed to work or there is an issue wit the whole scheme of things.

Let assume that we want to configure access in such a way that anonymous users can access most of published articles, but not some. In current form I can do it, by enabling "View published content" for role "anonymous" (via "standard" user interface), then going to "Content/Access Control" and disabling "view any content" for anonymous user on particular nodes. In order to be able disable access, I have to return "deny".

If we do not want to use "deny", then the only way to do it is to disable "View published content" at "role" level, then enable "view any content" at content type level and then disable "view any content" for particular nodes.

I have 2 problems with this:
1) disabling "View published content" at "role" level - looks too drastic
2) "Access Control" in current form, will disable particular checkbox if role lucking given permission (so if we deny "View published content" for "anonymous", then in Access Control, "View any content" checkbox for role anonymous will be disabled).

Bottom line: it seems to me that at some point someone should be able to say "deny". If you have multiple modules which participate in node_access - then I do not understand how it is supposed to work.

BenK’s picture

Hey Andrei,

I'm trying to do more research into the issues you brought up about NODE_ACCESS_DENY (what you are saying makes sense), but in the meantime, have you had a chance to work on the other issues/bugs?

Thanks!

--ben

atchijov’s picture

FileSize
41.6 KB

from #41, I think I fixed a, c, e, f, g. Maybe b as well. Will deal with Rules over weekend.

BenK’s picture

Hey Andrei,

I tested out your patch in #44 and things are looking good. I can confirm that a, c, e, f, and g are fixed. In particular, Content Access and ACL are now working together well!

So here's a summary of what's left to be fixed (b & d), plus two new bugs that recently surfaced (h & i):

b) Not Respecting Drupal Core Permissions By Default on Access Control Defaults Page:

I asked salvis about this on another thread, just to make sure my understanding of the proper defaults were correct. (It was.) Here's what he said:

"View any" should be checked by default for both anonymous and authenticated (to impersonate core).

"View own" shouldn't matter if "View any" is checked, just set it to whichever state is more convenient (needs nothing to be stored). I'm not intimately familiar with CA's inner workings, but that's the reasonable behavior.

If you have "View any" unchecked upon installation, then all content will vanish and many admins will be shocked and cry for help. So, to avoid unnecessary support issues, the initial state should be checked.

The only modification to these defaults is if the 'access content' core permission is unchecked for a role. If this is the case, here's what salvis says should be done:

all of the checkboxes for that role should be disabled (and unchecked) for all content types. 'access content' is a global permission, not content-type-specific.

d) Port content_access.rules.inc to D7: That's awesome you're working on this over the weekend. I'll be ready to test.

New Items:

h) Error Message - Duplicate records returned from hook_node_access_records(): There's one new error message that surfaced (reported by Devel Node Access). When visiting a node with per-node access control settings enabled--but no individual users granted view, update, or delete access--I'm getting the following error message reported by Devel Node Access:

Devel Node Access has detected duplicate records returned from hook_node_access_records():
4/acl/0/000 by acl
4/acl/0/000 by acl
4/acl/0/000 by acl

Interestingly, if I grant a user one permission (either view, update, or delete), one of the "4/acl/0/000 by acl" will go away. If I grant a user two or more permissions, then the entire error will go away completely.

i) Error When Saving Site Permissions: I'm not sure this is a Content Access error, but it surfaced right after I changed some of the Content Access permissions located at admin/people/permissions. (I gave a role the "Grant Own Content Access" permission.) The permission initially saved fine, but then when returning to the permissions page, making other changes, and trying to save the page, I started getting the following error message:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1-use text format 2' for key 'PRIMARY': INSERT INTO {role_permission} (rid, permission, module) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => use text format 2 [:db_insert_placeholder_2] => filter ) in user_role_grant_permissions() (line 2865 of /Users/benkaplan/git/drupal/modules/user/user.module).

Once I get this error message, changes to permissions are no longer being saved.

I'm not quite sure how to debug this. Any ideas as to what could be causing it?

Thanks for all of your awesome work! :-)

--Ben

atchijov’s picture

will look into these new things over weekend.

BenK’s picture

Hey Andrei,

It seems that h (Error Message - Duplicate records returned from hook_node_access_records) has been fixed in the latest -dev version of the ACL module. salvis found the issue with ACL and committed the fix late yesterday. So make sure you have the latest version of ACL and we don't have to worry about h) :-)

So all we have left is b, d, and i...

--Ben

BenK’s picture

Andrei,

I did some more testing of i (Error When Saving Site Permissions) to try to figure out what was causing the error. I was able to re-create the error on a clean install of the latest -dev version of D7 core (with Content Access and ACL installed)... and I didn't have this error before installing Content Access.

The error seemed to surface after I granted "Edit Any" and "Edit Own" permission to my "editor" role for the Article content type (on the content type access control defaults page). This seemed to cause a corresponding change on the D7 core permission page (as it should), which then caused the "Edit Any" and "Edit Own" permission to be disabled (and checked) on an individual Article node's access control page. But perhaps there is an issue with how Content Access is changing the core permission, thereby causing some type of duplicate entry.

Any thoughts on why this error might be occurring? It's obviously a pretty major issue if it's causing the core permission page to no longer work...

Thanks,
Ben

BenK’s picture

In regard to bug i) Error When Saving Site Permissions, I also asked salvis what he thought. Here's what he said:

Does CA try to re-set all permissions, including those that are already set? The permissions tables have changed and there may be subtle changes in the API, too.

Just thought this might help....

--Ben

fago’s picture

I think there some new API functions too: http://api.drupal.org/api/search/7/user_role_*

I'd be fine committing this once it is basically working, so we can roll patches for the remaining issues afterwards.

atchijov’s picture

RE: #45. Last issue.

I have run into it as well. I do think that there is a bug in user.module. I was able to fix this issue by apply following fix to user.module

replace this: (~line 2863)

db_merge('role_permission')
    ->key(array(
        'rid' => $rid,
        'permission' => $name,
        'module' => $modules[$name],
    ))
    ->execute();

with this

db_merge('role_permission')
    ->key(array(
        rid' => $rid,
        'permission' => $name,
    ))
    ->fields(array(
        'module' => $modules[$name],
    ))
    ->execute();

Basically in definition of role_permission only rid and permision are parts of primary key. module is not.

BenK’s picture

Hey atchijov,

Thanks for the suggestion in #51. It worked for me and the error went away. Yup, it's a core bug. There's a core patch to fix it submitted here: http://drupal.org/node/607238#comment-3218818. Perhaps you can give the patch a +1 over on that thread? This way, we have more people saying it's been tested and can get it committed ASAP.

So that leaves only b) and d) left to fix... How are things coming on your end?

Cheers,
Ben

BenK’s picture

@fago: Hey fago, great to see you chiming in on this thread. Yes, we're very close to a fully functional D7 port and once we fix the last two issues (atchijov is currently working on them and I am ready to test his fixes), it will be awesome to open up an official D7 branch. Also, we'd love to have your help with updating the Rules integration since you are the ultimate resource! :-)

Cheers,
Ben

BenK’s picture

In follow up to #52 (related to the 'Error When Saving Site Permissions' bug), the patch to the core bug was just committed here:

http://drupal.org/cvs?commit=404470

So that issue is now totally resolved!

--Ben

P.S. The only things we have left to fix in the D7 port are:

b) Not Respecting Drupal Core Permissions By Default on Access Control Defaults Page
d) Port content_access.rules.inc to D7

Can anyone assist with either of these? I'm happy to review/test any patches as soon as they are posted.

mertskeli’s picture

subscribing

fago’s picture

I've not tested or looked at the code yet, but I'm willing to commit it to CVS so we can start rolling separate patches for the remaining issues. To proceed, please roll a patch as usual against the latest 6.x-dev.

fago’s picture

FileSize
27.17 KB

I had a short look at it, however the code has some serious flaws. I decided to better postpone committing it until at least the serious stuff is fixed.

I rolled it into a proper patch.

Issues:

Serious:

+function expand_checkboxes($element) {
What's that? Looks like a weird copy/paste fix. Solve it properly and don't violate namespaces.

* Code style. Changes introduce code style violations.

- acl_node_add_acl($node->nid, $acl_id, $op == 'view', $op == 'update', $op == 'delete', content_access_get_settings('priority', $node->type));
+ acl_node_add_acl($node->nid, $acl_id, (int)($op == 'view'), (int)($op == 'update'), (int)($op == '
!?!?

+// $form['per_role']['#after_build'] = array('content_access_force_permissions');
Why that!?

- drupal_install_schema('content_access');
+ // happen automatically in D7
+ // drupal_install_schema('content_access');
}

/**
@@ -15,7 +16,8 @@
function content_access_uninstall() {
variable_del('content_access_settings');
// Remove tables.
- drupal_uninstall_schema('content_access');
+ // happen automatically in D7
+ // drupal_uninstall_schema('content_access');
}

What about removing lines? ;)

Non serious:

-// $Id: content_access.admin.inc,v 1.1.2.24 2010/07/19 11:40:11 fago Exp $
+// $Id: content_access.admin.inc,v 1.1.2.23 2009/07/31 10:03:22 fago Exp $

The port is outdated. We need to integrate recent changes.

* Code needs to update to d7 coding standars. Coder module?

-function content_access_admin_settings(&$form_state, $type) {
+function content_access_admin_settings( $form, &$form_state, $typeObject ) {
+ $type = $typeObject->type;
+

we do not use camelcasing outside of classes

BenK’s picture

Hey fago,

Thanks for rolling the patch file and your helpful comments. I'll try to research each of the points you brought up, but I wanted to first address one bit of code you mentioned:

- acl_node_add_acl($node->nid, $acl_id, $op == 'view', $op == 'update', $op == 'delete', content_access_get_settings('priority', $node->type));
+ acl_node_add_acl($node->nid, $acl_id, (int)($op == 'view'), (int)($op == 'update'), (int)($op == '
!?!?

This code change was actually suggested by salvis, the ACL module maintainer, at the following thread:

http://drupal.org/node/824642#comment-3080112

Basically, without this change we were getting an error message when viewing a node's "Access Control" tab.

Because my own coding skills aren't great, I asked Berdir about the purpose of this in IRC. He said that:

the (int)($op == 'view') is probably required because the arguments are directly passed to the db layer and that requires an 1 instead of TRUE. D6 does automatically convert that.

So is this code change OK with you? Or is there some other way you'd rather deal with it?

Thanks,
Ben

BenK’s picture

Hey fago,

I was able to reach atchijov and ask him your question about:

+function expand_checkboxes($element) {
What's that? Looks like a weird copy/paste fix. Solve it properly and don't violate namespaces.

atchijov said that he added this because expand_checkboxes is a function from D6 which is missing in D7. Because Content Access is using this function, he just copied it "AS-IS" from D6 (just to keep the port moving along).

I asked Berdir in IRC if he knew of a D7 equivalent to expand_checkboxes. He said that expand_checkboxes was renamed form_process_checkboxes in D7:

http://api.drupal.org/api/function/form_process_checkboxes/7

Because form_process_checkboxes looks virtually identical to expand_checkboxes (see http://api.drupal.org/api/function/expand_checkboxes/6), replacing expand_checkboxes with form_process_checkboxes should be enough to make everything work properly. atchijov concurs with this.

--Ben

mertskeli’s picture

@fago #57
I can confirm that #44 had been working.
What do you mean by "serious stuff"?

BenK’s picture

Status: Needs work » Needs review
FileSize
28.1 KB

Hey fago,

Thanks for the patch file and your code review in #57. I've gone through all of the issues you raised (all excellent points!) and have fixed them one by one. A new patch file is attached. If it meets your approval, can we create the new 7.x branch?

Here's is a summary of how I fixed each of the issues you raised:

+function expand_checkboxes($element) {
What's that? Looks like a weird copy/paste fix. Solve it properly and don't violate namespaces.

>> I got rid of this code because it was a temporary hack. expand_checkboxes was renamed form_process_checkboxes in D7, so once I implemented that change, this code was not necessary.

- acl_node_add_acl($node->nid, $acl_id, $op == 'view', $op == 'update', $op == 'delete', content_access_get_settings('priority', $node->type));
+ acl_node_add_acl($node->nid, $acl_id, (int)($op == 'view'), (int)($op == 'update'), (int)($op == '
!?!?

>> This code change was necessary because of a change in the D7 version of the ACL module. The change was actually suggested by salvis, the ACL module maintainer, at the following thread: http://drupal.org/node/824642#comment-3080112

+// $form['per_role']['#after_build'] = array('content_access_force_permissions');
Why that!?

>> This was a code fragment left over from the debugging process. I deleted it. I also found several other ones like this and I deleted those too.

- drupal_install_schema('content_access');
+ // happen automatically in D7
+ // drupal_install_schema('content_access');
}

/**
@@ -15,7 +16,8 @@
function content_access_uninstall() {
variable_del('content_access_settings');
// Remove tables.
- drupal_uninstall_schema('content_access');
+ // happen automatically in D7
+ // drupal_uninstall_schema('content_access');
}

What about removing lines? ;)

>> Yes, I've deleted these lines.

The port is outdated. We need to integrate recent changes.

>> I've updated the port to all the latest commits. It's now current. Thanks for pointing this out.

* Code needs to update to d7 coding standars. Coder module?

>> I ran the Coder module and made a bunch of coding standard improvements suggested by the module.

-function content_access_admin_settings(&$form_state, $type) {
+function content_access_admin_settings( $form, &$form_state, $typeObject ) {
+ $type = $typeObject->type;
+
we do not use camelcasing outside of classes

>> I fixed this (including both the example you mentioned and a couple other examples found elsewhere).

So I think we should be all set to open the new D7 branch. We also did another round of manual module testing on our end and things are working great.

Cheers,
Ben

Scott J’s picture

Status: Needs work » Needs review

Thanks to everyone for this effort.
#44 is working OK for me with ACL on Drupal 7 Beta 1.

fago’s picture

Status: Needs review » Needs work

Thanks for working on this - you made good improvements :)

-  drupal_set_title(t('Access control for %title', array('%title' => $node->title)));
+function content_access_page($form, &$form_state, $node) {
+  drupal_set_title(t('Access control for "' ). $node->title.'"');

That's a wrong usage of t(). This could be converted to be set via hook_menu() or just tell drupal_set_title() to not check plain (have a look at its docs).

+  $form = content_access_role_based_form( $form, $defaults);

There is leading space in front of $form.

+      acl_node_add_acl($node->nid, $acl_id, (int)($op == 'view'), (int)($op == 'update'), (int)($op == 'delete'), content_access_get_settings('priority', $node->type));

I see your argument, makes sense. But please add a space after (int) to be consistent with the coding standards.

-
+  

You are introducing whitespaces.

   }
+  
   // Save per-node settings.

The same here.

-function content_access_admin_settings(&$form_state, $type) {
+function content_access_admin_settings( $form, &$form_state, $typeobject ) {
+  $type = $typeobject->type;
+

Again the leading space and bad variable naming. Better just stay with $type and then use $type_name.

+  foreach( $result as $rolepermission ) {
+    if( !  isset( $permissions[ $rolepermission->rid ] )) {
+      $permissions[ $rolepermission->rid ]  = array();
+    }

Lots of too many spaces. It should be

+  foreach ($result as $rolepermission) {
+    if (!isset($permissions[$rolepermission->rid])) {
+      $permissions[$rolepermission->rid] = array();
+    }
-  return $permissions;
+
+    return $permissions;
 }

Again spaces.

+    foreach (content_access_get_settings($op, $type) as $rid) {
+    
+//     debug( content_access_get_permission_access(content_access_get_permission_by_op($op, $type)), $op . ' - ' . $type . ' = '. content_access_get_permission_by_op($op, $type));
+    
+//     foreach( content_access_get_permission_access(content_access_get_permission_by_op($op, $type)) as $rid ) {
+     

forgotten debug statement? :)

You also introduce several empty lines containing spaces ins the .module - watch out for those or configure your editor to kill them automatically.

content_access_node_type( 'delete', $info );

You like that extra spaces? :) It should always be
content_access_node_type('delete', $info); - please watch out for that too.

dawehner’s picture

Status: Needs review » Needs work

A short review of the patch:

+    db_query('DELETE FROM {role_permission} WHERE rid = :rid', array( 'rid' => $rid ));

Shouldn't db_delete always be used?

+foreach( db_query("SELECT settings FROM {content_access} WHERE nid = ?", array( $node->nid)) as $record ) {

I'm not sure whether ? is a good placeholder here. I think :nid is a better one

Some other things

hook_nodeapi -> hook_node_delete

+//  db_query("UPDATE {content_access} SET settings = '?' WHERE nid = ?", array(serialize($settings), $node->nid));

I guess also db_update should be used when possible.

 function content_access_get_permissions_by_role() {
-  $result = db_query('SELECT r.rid, p.perm FROM {role} r LEFT JOIN {permission} p ON r.rid = p.rid');
   $permissions = array();
-  while ($role = db_fetch_object($result)) {
-    $permissions[$role->rid] = array_filter(drupal_map_assoc(explode(', ', $role->perm)));
+  $result = db_query('SELECT r.rid, p.permission FROM {role} r LEFT JOIN {role_permission} p ON r.rid = p.rid');
+  foreach( $result as $rolepermission ) {
+    if( !  isset( $permissions[ $rolepermission->rid ] )) {
+      $permissions[ $rolepermission->rid ]  = array();
+    }
+
+    $permissions[ $rolepermission->rid ][] = $rolepermission->permission;
+  
   }
-  return $permissions;

There is a api for this user_role_permissions(user_roles()). I'm not sure whether this is the right usage of the function.

good_man’s picture

subscribing, thanks for all the efforts!

good_man’s picture

Let me know how I can help in making a stable version, I enabled it with the latest ACL dev version and gettings WSOD when adding individual users to nodes, got the following error:

Fatal error: Call to undefined function _acl_edit_form_after_build() in /var/www/drupal-7-HEAD/drupal/includes/form.inc

I'll try to dig into this error and see how I can fix it.

good_man’s picture

Let me know how I can help in making a stable version, I enabled it with the latest ACL dev version and gettings WSOD when adding individual users to nodes, got the following error:

Fatal error: Call to undefined function _acl_edit_form_after_build() in /var/www/drupal-7-HEAD/drupal/includes/form.inc

I'll try to dig into this error and see how I can fix it.

BenK’s picture

@good_man: Did you use the patch in #61? Last I checked it worked fine with ACL. What version of D7 core are you using?

--Ben

good_man’s picture

I'm sorry for duplicating the reply, it's varnish issue in d.o infrastructure, anyhow I filled an issue in ACL issue queue
#946636: "Call to undefined function _acl_edit_form_after_build()" — acl_edit_form() signature changed!

FiNeX’s picture

subscribing too

Anonymous’s picture

where I can find content_access module to test in a D7 fresh installation?

Scott J’s picture

giorez,
It's still being built! But there's a version to get started with up above at #44. Then follow the discussion after that to see where it is up to.

BenK’s picture

You should use the patch in #61. That is the latest version. (The zip file in #44 is an older version.)

--Ben

thekevinday’s picture

subscribe

gorillaz.f’s picture

subscribe

BenK’s picture

For anyone who can help with this, here's a quick summary of where things stand:

1. The patch in comment #61 is the latest version. That should be the basis of future work.

2. We need to incorporate the code cleanup specified in comment #63.

3. We need to review and possibly incorporate the code suggestions in comment #64.

4. ACL module has changed the signature of acl_edit_form(). We need to make corresponding changes for Content Access to work with ACL. This is a recent change as ACL was previously working well with CA. See http://drupal.org/node/946636#comment-3665690 for more info.

5. Rules integration still needs work to be functional.

--Ben

jhedstrom’s picture

Subscribing.

andypost’s picture

subscribe

smls’s picture

subscribe

amanaplan’s picture

subscribe

cnolle’s picture

Subcribe

trekoid’s picture

subscribe

harriska2’s picture

subscribe

emmeade’s picture

subscribe

thekevinday’s picture

I have encountered the following when renaming a content type machine name:

    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).
    * Notice: Undefined index: testing_1 in content_access_node_type() (line 554 of sites/all/modules/content_access/content_access.module).

The code in question:

<?php
    case 'update':
      if (!empty($info->old_type) && $info->old_type != $info->type) {
        $settings = content_access_get_settings();
        foreach (content_access_available_settings() as $setting) {
          $settings[$setting][$info->type] = $settings[$setting][$info->old_type];
          unset($settings[$setting][$info->old_type]);
        }
        content_access_set_settings($settings);
      }
?>

Would it be more appropriate to add an if (isset()), such as:

<?php
    case 'update':
      if (!empty($info->old_type) && $info->old_type != $info->type) {
        $settings = content_access_get_settings();
        foreach (content_access_available_settings() as $setting) {
          if (isset($settings[$setting][$info->old_type])){
            $settings[$setting][$info->type] = $settings[$setting][$info->old_type];
            unset($settings[$setting][$info->old_type]);
          }
        }
        content_access_set_settings($settings);
      }
      break;
?>
Finn Lewis’s picture

FileSize
53.94 KB

@BenK - thanks for this.
I too am keen that this module be ready for D7 production soon.
Following your list,

I took the patch in #61 and applied it to the 6.x-1.x-dev branch from CVS.
The limited testing I did gave me the results I was expecting, so it works for me.

I reviewed the comments in #63 as suggested, and cleaned up what I could.
Coder module suggested a who lot more cleanup - mostly related to spaces, so I went through these until coder module was almost happy.

Interestingly, it still had two complaints, which didn't seem to make sense to me:

sites/all/modules/custom/content_access-DRUPAL-6--1/content_access.admin.inc
content_access.admin.inc
    * severity: criticalLine 17: Potential problem: d() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using c(), f() or similar. (Drupal Docs)
        drupal_set_title(t("Access control for !node_title", array('!node_title' => $node->title)));
    * severity: criticalLine 427: Potential problem: "SELECT FROM {node}" statements should probably be wrapped in d and with the alias for {node} table defined (e.g. {node} n)
        $result = db_query("SELECT n.nid FROM {node} n WHERE type = ':type'", array( 'type' => $type));

Is that just coder being strange?

That's as far as I got tonight. I looked at the comments in #64, but wasn't sure which should be applied.
I attach a patch of the clean-ups made. Hope this helps and look forward to helping further with the other items you mention.

good_man’s picture

We did something very similar to Content Access for D7, the main reason was that we needed a similar simpler approach ASAP with a simple API methods to call, please checkout Flexi Access, and I'm keen to hear thoughts & how to-not duplicate any efforts.

cnolle’s picture

The patch in #61 seems to work quite nicely. A couple of issues.
- Nodes that are supposed to be hidden from certain roles still show up on taxonomy/term pages.
- If I enable individual access control on nodes and click on access control for a particular node I get a white screen (as if page not found, BUT not a drupal rendered 404).

alayham’s picture

sub

cewolcott’s picture

BenK,

Are overlays still an issue?

BenK’s picture

@cewolcott: As of the patch in #61, overlay stuff seemed to work fine.

cewolcott’s picture

BenK,

The UI looks great when I perform:
Admin -> Structure -> Content Type -> ContentType Edit -> Access Control

I can see the Content Access Form (Role Based Access Control Settings, Per Content ...., Advanced).
I then select "Enable per content node access control settings" and submit.

I then navigate to Admin -> Content and select an instance of the content type that I allowed node access control settings.

When I select an instance I now have the View, Edit and Access Control tabs. But when I select the Access Control tab the screen goes grey (IE) or white (Firefox, Chrome, Safari).

Any thoughts.

BenK’s picture

@cewolcott: That sounds like a fatal error rather than anything to do with the overlay. Do you have ACL module installed to grant access to individual users?

cewolcott’s picture

@BenK (#93) Yes I do. I dowloaded acl-7.x-1.0-beta1.tar.gz from Nov 6, 2010.

I have received this issue on both my home laptop running Windows 7 and IIS 7.5 and at work running Windows XP and IIS 6.0.
Both sites are Drupal 7 RC2 and then just the latest ACL module (above) and Content Access 6.x-1.x-dev with patch from #61. I also tried the patch from #86.

Any insight on were to look?

BenK’s picture

@cewolcott: Can you confirm that the problem goes away if you disable the ACL module?

The cause of the fatal error is probably this:

ACL module has changed the signature of acl_edit_form(). We need to make corresponding changes for Content Access to work with ACL. This is a recent change as ACL was previously working well with CA. See http://drupal.org/node/946636#comment-3665690 for more info.

--Ben

cewolcott’s picture

@BenK,

My apologies. It does go away when I diable the ACL module. I saw the comment you highlight when resding the thread, but when I finally had a chance to test everything out I forgot about that statement. I thought Content Access required ACL?

Thanks this allows be to continue with the testing.

BenK’s picture

Now we just need someone to help implement fixes related to http://drupal.org/node/946636#comment-3665690 so that ACL can work with Content Access again. Any takers?

--Ben

P.S. I can't work on this myself now because I'm swamped with stuff for the 7.x ports of Private Message and Userpoints modules.

good_man’s picture

FileSize
9.21 KB

Note: The attached port contains a small bug, better see comment #100.

This D7 port is based on comment #44, current D6 dev (2010-Sep-03), code reviews, patches in previous comments, and some hard work while I was eating sweet potatos!

A list of changes since initial port in comment #44:
content_access.module
- no need for hook_init() to set the admin theme, D7 handles this.
- removed old content types commented code from hook_menu().
- adding some explanation comments.
- changed D6 hook_nodeapi() to the new node api hooks.
- changing D6 DB queries to D7 new DB abstraction layer.
- removed many trailing tabs, extra spaces, and modified some code to match Drupal's coding standards & coder module reviews.

content_access.install
- no need for hook_install() and hook_uninstall() to install/uninstall the schema, since D7 automatically do that.

content_access.admin.inc
- using the new permissions & roles api in D7 mentioned in comment #64, removed unused function as a result and changed the permissions array a little to match D7 permissions array.
- removed many extra white spaces
- removed expand_checkboxes() and used the D7 form_process_checkboxes() instead.
- t() in D7 takes vars as @varname
- changing D6 DB queries to D7 new DB abstraction layer.
- removed many trailing tabs, extra spaces, and modified some code to match Drupal's coding standards & coder module reviews.

content_access.rules.inc
- NOT YET

tests
- NOT YET

Questions:
- why in content_access_save_per_node_settings() you are using a foreach approach instead of the old approach (update then insert if no records found)?
- why using old and new node type hooks? why not just using the new ones?

Also,

b) Not Respecting Drupal Core Permissions By Default on Access Control Defaults Page

I didn't understand it very well, or how to reproduce, any how maybe this issue is now gone since I used D7 permissions & roles API.

BenK’s picture

@good_man: Thanks for getting involved! :-) Can you post your changes as a patch? It's much easier to review (and make sure that all prior patches were included). Also, if we want fago to review and commit the final version, he'll need to see it as a patch, too.

And did you address the ACL integration issue mentioned in comment #95 and #97?

As for your questions:

a. Not sure.
b. Probably just an oversight. The developed who did the original D7 port on this thread hasn't been working on this issue for quite awhile.
c. As for "Not Respecting Drupal Core Permissions By Default," here is an explanation:

When a new content type is created, Content Access is supposed to respect Drupal core permissions by default. (This is how it works in the D6 version of the module.) So if authenticated and anonymous users have been granted the "View published content" permission in Drupal core (admin/people/permissions), then when a new content type is created, the access control defaults for that content type should give both anonymous and authenticated users the "View any content" and "View own content" permissions by default (they are checked by default). However, in the current module, "View any content" and "View own content" permissions are unchecked by default (except for roles with the "administer content" permission) regardless of the current core permission settings.

So I'm not sure if your changes fixed this... When you create a new content type, is "View any content" and "View own content" checked by default for the anonymous and authenticated user? If so, then it is fixed.

--Ben

good_man’s picture

FileSize
31.65 KB

Here is a patch against the current dev version (sep 3), and to see the changes go to comment #98.

re. point b) yes it's working as expected, only manual testing though, and #95, #97, yes they are compatible with the latest changes of ACL, as I opened that issue in ACL :)

cewolcott’s picture

@good_man - Thanks for the patch. It fixes my issue in #92. The ACL and Content_Access modules work well together. On the further testing.

AdrianC-1’s picture

Subscribing

good_man’s picture

@cewolcott: thanks for testing, anyhow want to work on tests today, and release a new patch with tests.

re. rules: is the new rules 7.x API stable?

Scott J’s picture

I know that Rules is being successfully relied on by Commerce module. Just be sure to use dev versions of both Rules & Entity as both modules have recently had their structure reorganised to prevent a problem with Update manager.

http://drupal.org/node/660108

BenK’s picture

@good_man: Yes, rules 7.x API is quite stable. Private Message module and Userpoints module have successfully integrated with it for a few months now. So any work you can do on porting content_access.rules.inc would be great! :-)

--Ben

cewolcott’s picture

@good_man & BenK,

I have applied the latest patch (690610-100.patch) and found the following issue.

I enabled "Per content node access control settings" for the Article content type. I then surfed to a article node and selected the Access Control tab. I then de-selected a checkbox and pressed the Submit button and received an http 500. After adding echo statements (because I do not know any other way to debug) I found the following issue.

File: content_access.module
Function: content_access_save_per_node_settings
Issue: I believe the db_result() needs to be removed from around the db_query and a -->fetchfield() added to the end. This will set the $result variable to the number of rows found (which will be 0 or 1).

The http error went away and a row was actually added to the content_access table.

good_man’s picture

@cewolcott, there is an error in the count query (based on old count query from D6), I changed it to be based on the new D7 DB layer, it should be fixed now.

@Scott J & @BenK, good news, working on it now.

expect a new patch soon!

Edit: @cewolcott will be in the next patch not the one in #100.

binford2k’s picture

subscribe

good_man’s picture

Updates:
content access was using Rules 1.x way to attach it's form to the action UI, this is changed now, and Rules 2.x should handle your data by providing them using metadata & entity API.

For allowing Rules to intelligently handle saves of your data type, you have to provide an entity, e.g. by using the Entity CRUD API. Entities are automatically supported, just provide metadata for them. Metadata for non-entity data structures may specified using hook_rules_data_info().

from 8. Updating module integration

The main reason behind this big change for content access 7.x is that there is no way in Rules 2.x to save your form's data on submit (i.e. in Rulex 1.x, content access uses action_name_form() and action_name_submit() with the form & form_state as a param, now you can't pass form or form_state to the new action_name_process() only rules' settings array is passed). Also, Rules 2.x docs are still lacking many examples & howto. Any thoughts?

fago’s picture

http://drupal.org/node/878940 might help you, in Rules 2 you usually don't have to write forms at all. Just specify your parameters. If really necessary, you can use the form_alter callback though.

good_man’s picture

Thanks you are so fast! was talking with Berdir on IRC and he suggested to open an issue in Rules queue, but it seems you were faster :)

Anyhow to summarize the problem:
in Rules 1.x there was a callback for configuration form submit:

rules_action_callback_submit(&$settings, $form, $form_state)

The nice thing in this method, is it takes 3 params. which is important if you want to save back the settings to $settings & take values from $form_state

In Rules 2.x there is no easy way to do so. Tried the form_alter with $form['#submit'], but it doesn't take $settings, only $form and $form_state, so no way to save the config. form.

Hope the problem is more clear now, thoughts?

good_man’s picture

FileSize
31.68 KB

Just a patch to solve #106 before the Rules integration patch.

fago’s picture

There is nothing like that in Rules 2.x, because you are not supposed to add your own settings via a form. Use parameters.

good_man’s picture

In the begining I thought it's an overhead to use entity for a simple case like this, but after looking at entity & metadata for a while, all I can say *it must be in D8* :)

wasare’s picture

+1

cestmoi’s picture

subscribing

BenK’s picture

@good_man: How goes the Rules integration? Anything you want me to test?

--Ben

good_man’s picture

harddisk crashed last week, still in the process of recovery to get my work on rules back, if anyone can help please jump in, otherwise I'll need at least 4-5 days.

danmcewan’s picture

subscribing

Hawkwala’s picture

Category: task » bug

I installed content_access from #98 and get following error when trying to set permissions:

* Notice: Undefined index: access comments in user_role_grant_permissions() (line 2966 of /../../../../modules/user/user.module).
* PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null: UPDATE {role_permission} SET module=:db_update_placeholder_0 WHERE ( (rid = :db_condition_placeholder_0) AND (permission = :db_condition_placeholder_1) ); Array ( [:db_update_placeholder_0] => [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => access comments ) in user_role_grant_permissions() (line 2968 of /../../../../modules/user/user.module).

Following are installed:
ACL 7.x-1.0-beta2
Content Access No information appears in modules - installed from #98

Any ideas or help will be appreciated.

jarush’s picture

I'm looking for this module for a long time......

Yannakis’s picture

subscribing

daviesap’s picture

Subscribing

spykerc8’s picture

Subscribing

Jiri Volf’s picture

Subscribing

skov’s picture

Subscribe

snupy’s picture

Subscribe

freddan’s picture

subscribe

gadams’s picture

subscribing

Csabbencs’s picture

I'm facing the same issue as #120.

BenK’s picture

Category: bug » task

@Csabbencs: Did you try the patch in #112? That is the latest version. Can you see if you get the same problem and report back?

The post in #120, unfortunately, tested an older version of the patch.

--Ben

Csabbencs’s picture

@BenK: I applied the patch #112, but got the same result. :(

cloudbull’s picture

Status: Needs work » Active

Subscribe

ianm’s picture

Subscribe

gcastagneti’s picture

@good_man: Could you create a new .tar.gz file with all of patches applied? It'll be really useful
Thank you very much!
With kind regards,
Gabriele Castagneti

ropaolle’s picture

Subscribe

emptyvoid’s picture

Very cool, I am the maintainer for the node_access module and I plan to not release a D7 version in favor of directing users to use your module instead. I'd like to review the code and contribute if I can, there are a few differences between node_access and content_access and when I have time, I'll list the business logic differences. I'd love to hear your input on what would be useful features to incorporate (or not).

http://drupal.org/project/node_access

good_man’s picture

Status: Active » Needs work

okay back to work again, will try to provide the patch ASAP. Thanks all for testing, I'll try to fix that too.

sludwig’s picture

Subscribe

good_man’s picture

@BenK, the case in #120 and #130 is true, it appears when you disable a module, it's permissions still (won't be removed unless you uninstall it), and granting this disabled module's permission will raise that exception, a check for activated modules' permissions should do the job. The next patch should fix it.

I want a D7 branch to commit changes so I won't lose my changes if any accedent happens like #118 which I lost long working hours. In the mean time I'm mainting the project using github.

good_man’s picture

@emptyvoid: can you open another issue? I want to discuss flexiaccess too.

good_man’s picture

Status: Needs work » Needs review
FileSize
16.72 KB
32.48 KB

Okay a new patch to fix #120 and #130. NOTE: make sure to use the dev version of ACL as it fixes #1024114: Fatal error in function acl_get_id_by_name.

A version of the module in tar format for easy testing.

Can anyone confirm if upgrade from Drupal 6 to 7 with Content Access enabled works? (please take a backup first)?

BenK’s picture

Status: Needs review » Needs work

@good_man: Great to have you back! :-) Yeah, working on github sounds great. Makes it easy for me to clone your repository and pull in changes as you have them. I did a quick search and I'm assuming this is the repository you're using:

https://github.com/khaledalhourani/Content-Access

Let me know when you want me to start another round of testing (or any other help you need) and we can get this done! It shouldn't be much more work (mostly just the rules stuff and notices) to get something good enough for fago to start the official D7 branch.

--Ben

BenK’s picture

@good_man: I think we just cross posted. Do you ever hang out in IRC? I can usually be found in #drupal-games and I'm there right now. So if you ever want to chat directly and coordinate, you should stop by.

--Ben

good_man’s picture

you are super fast! I was just updating the git repo..

I'm working on rules now, it won't take long, after that we should do the testing.

Next steps IMO should be:
- Make sure the upgrade path is working.
- Update the tests if necessary.

After that we should release the D7 version.

xlyz’s picture

subscribing

sludwig’s picture

i used the tar.gz from #142 with the lastest dev version of ACL and everything seems to be fine on a fresh installation of D7. Really good work

venkatd’s picture

I want to thank you all for the hard work on the upgrade! I also used the .tar.gz from #142 and so far it has worked flawlessly.

This came in the nick of time for the project I am working on.

justin.maclean’s picture

Category: task » bug

Thank you for this great module, and for the Port to Drupal 7! I'm looking to implement this on a site that I started creating in Drupal 6, and have upgraded to Drupal 7.

During my testing of this module, I, have encountered the following issue:
1) Steps begin Prior to Content Access / ACL Being Installed
2) Anonymous users are set up with no permissions (they do not have even the "View published content" permission).
3) Using Drupal 7's private files to manage attached documents, create a node and upload a file associated with it (content type = Page).
4) Attempt to access the uploaded file using an anonymous user, access is denied as expected.
5) Install the content_access module, and set the Content Type of Page to allow per node access control.
6) Attempt to access the node created in step 3 as an anonymous user. Access is denied as expected.
7) Attempt to download the file uploaded in association with the node, and the file is downloaded to an anonymous user. This should not be allowed.

Thinking this could be related to the conversion from D6 to D7, I ran this same test on a "fresh" D7 install, and was able to duplicate.

This same scenario works as I would expect in the D6 version of the module (i.e. access denied message is issued to an anonymous user when attempting to access an attachment.)

Thanks in advance for any guidance you can provide on this!

justin.maclean’s picture

I should have mentioned, I'm using the tar.gz file from #142 and the development version of ACL.

BenK’s picture

Category: bug » task
ropaolle’s picture

Hi justin.maclean. Can't help you but I can confirm that I reproduced the same issue.

yngens’s picture

subscribe

mindgame’s picture

subscribe

fago’s picture

Patch already looks good! Thanks!

I think we can care about the rules integration in a follow-up issue. Let's concentrate on the main functionality first, and make sure the tests run. Then we'll can commit this and move on with follow-up issues.

In regard to node-access the module update-docs mention:
- http://drupal.org/update/modules/6/7#hook_node_access_records

So I think, best we take care of unpublished nodes too?

Jej’s picture

subscribe

eddin’s picture

Subscribe!

emptyvoid’s picture

I wonder if integrating with the entity access module would provide the most coverage for security?

http://drupal.org/project/accessapi

Mainly because D7 is based on the entity system and this module provides the CRUD API for almost every object.

http://drupal.org/project/entity

I'm curious if just adding a GUI would allow for the ability to not only define role and user level access to actions (view, edit, delete) of content types but also users, files, whatever is an entity?

good_man’s picture

@fago: yeah it won't hurt since D7 is doing so with the unpublished nodes.

@emptyvoid: the plan now is to provide the required tests and release a "port" release, there are many new features to be added but not now, one if them is abstracting the module to provide access for different entities.

Cyberwolf’s picture

Subscribing.

fago’s picture

#158 sounds interesting, but is certainly out-of-scope for content access as its name say, it focusses on content == nodes.

Also, yep we really should concentrate on porting here, but not on adding new stuff. That can be done in follow-ups.

emptyvoid’s picture

Sounds like a plan to me, I could see adding small sub-modules to enable a GUI for each type of entity later on.

threewestwinds’s picture

Subscribe. I'm also very keen on Rules integration - I'd love to use content access for a new site I'm setting up, which basically runs on a combination of rules and userpoints.

good_man’s picture

FileSize
60.36 KB

The current patch includes the updated test cases with all tests passing.

Slightly changed from the previous patch to include better form elements labels.

@BenK: the case you told me about is true in D6 version, you can't override type's settings in node's settings, it's a module design issue, maybe we can discuss later if you found that it's better to be able to override it.

good_man’s picture

FileSize
77.23 KB

A patch with the current work of Rules integration.

Note: rules integration port is still in progress and should only be used for testing (not for production).

kudos to Benk for testing and feedback.

BenK’s picture

I've just finished testing the upgrade path from D6 to D7 and everything is working great. I started with a clean D6 install with both Content Access and ACL enabled. I created some nodes and configured node access at both the content type and node levels. I also granted specific users view, update, and delete access via ACL.

Then I upgraded the entire site to D7 and checked all of my previously created nodes. All of the node access settings were retained (including the user ACL settings) and access to the nodes functioned just like in the D6 version. In addition to logging in and out as different users, I also used Devel module's Devel Node Access and Devel Node Access by User blocks to double-check that all of the node access tables looked good. Everything functioned terrifically. So the upgrade path is good to go.

Great work by good_man on this! :-)

Additionally, I manually tested good_man's latest commits via github (including commits that aren't included in the patch in #165) and everything worked very well. He even fixed a few little form bugs. We've now got cleaned-up code, functioning tests, functioning basic rules integration (although more work is needed), and solid integration with ACL.

So good_man, can you upload a final patch with all of your latest commits?

Once we do that (I can test the new patch one final time), then I think we're ready to ask fago to open up the official 7.x branch! :-) And then we can finish up the remaining issues (like the missing rules integration features) on separate follow-up issue threads.

Cheers,
Ben

good_man’s picture

Status: Needs work » Reviewed & tested by the community

Hi BenK, thanks a lot for testing and for this summary, actually the last patch #165 is already containing my latest commits and the commits that came after it to git repo. are just a sync commits because I forgot to update the repo. after the patch :)

I think we are ready to ask for a D7 branch now @fago.

BenK’s picture

@good_man: Yes, I now see that the patch in #165 does contain all of your latest github commits.

So I agree that this is RTBC and the patch in #165 should be the one used to create the official D7 branch. Yay! :-)

@fago: Can you create the D7 branch as soon as you have a chance? We can then do follow-up issues in separate threads.

Cheers,
Ben

bryancasler’s picture

subscribe

nuttaphat’s picture

subscribe

Breakerandi’s picture

subscribe

anselm.marie’s picture

Subscribe. I wish I could help with the code but I'm not at that level yet.

sw3b’s picture

Subscribe !!!

giullina’s picture

subscribing to this... Content Access is a life saver!

tmm360’s picture

subscribe

fago’s picture

Status: Reviewed & tested by the community » Fixed

Wow, nearly 200 comments!

+
+  // @todo is this required?
   cache_clear_all();

Yes, we need to clear the page and block caches and maybe other caches where the node might appear now. So I removed that comment.

-    'title' => 'Access control',
+    'title' => 'Access Control',

As in core and in d7, we should just capitalize the first character, so I've changed that back.
Analogously I've fixed the permission labels.

+    // @todo why content_access.admin.inc is not loaded before?
+    module_load_include('inc', 'content_access', 'content_access.admin');

Indeed, that looks confusing, in particular as it looks like you have moved content_access_get_acl_id() to .admin.inc ? We should not put API-like functions like this in an include, but we can clean that up in follow-ups.

-    end($roles);
+    #end($roles);

No, that's needed. I've re-added it and added a comment to explain it:

    // Make sure to get a role from the end, thus not the authenticated users.
    end($roles);

I've also replaced all "Implementation of" strings with "Implements", as it is used in d7.

Maybe caused by the roles change, I still got a test-case fail. Anyway, I decided to commit it right now, so we can move on. Let's handle the rest in follow-up issues. Thanks for the great work!

threewestwinds’s picture

Awesome. Testing out the CVS version right now.

I don't see an option to open issues against a 7.x version yet - is there a setting you have to toggle somewhere on d.o for that?

nitrospectide’s picture

I was researching access solutions today, and this module seemed to be *the* one, but the apparent lack of a D7 version in the works seriously concerned me - that and the "Maintenance fixes only" on the status. Someone on IRC then pointed me to this thread. Was a checkbox missed somewhere that makes the D7 dev version not show on the project page?

Scott J’s picture

Struth mate! Are you joking? It's been "in the works" since #8 posted by atchijov on June 3, 2010 at 6:11am.

Here, it was only committed yesterday:
http://ftp.drupal.org/files/projects/content_access-7.x-1.x-dev.tar.gz

nitrospectide’s picture

Scott J: I saw that as soon as I was linked to this thread. My point was that there is no evidence of it on the actual project page - down where recommended/development releases show up. That, combined with no mentions in the description made it look like this module had stalled out.

AdrianC-1’s picture

Title: Port to Drupal 7 » Content Access - Port to Drupal 7

Agreed - let's get it on the project page, if the dev version's mature.

(Renamed for thread tracking)

good_man’s picture

@fago, the end thing in test, is because in D7 test it's default to give a user an auth. role + the new passed role when created, so the new passed role as param will go first in array, and then goes the auth. role, so when using end() you'll get the auth. role not the new one, anyhow it's hack y way, that's why I've commented it instead of removing, we should find better way to do so.

fago’s picture

hm, in my tests end gave me the right role, but anyway I agree that we should find a more reliable way instead.

The dev-snapshot appears on the project page now.

good_man’s picture

Status: Fixed » Closed (fixed)

ok will close this issue now and open #1058526: Rules 2.x integration, don't reopen this issue if you find a bug instead create a new issue.