Provide access control for unpublished nodes

Oh, you must be with the guys in Austria. :)

I'm not sure what might have changed since Drupal 6 in this regard. However, my first thought is to do away with view_unpublished, and instead replace core's published/unpublished functionality with a term reference field:

1. You'd have a vocabulary, something like Workflow state.
2. That vocabulary would include only the term Unpublished. (Though you could add more if desired!)
3. You would add a term reference field to the appropriate content types using the Workflow state vocabulary and the checkboxes field widget. (Edit: If needed, you could customize the node form to make it look the same to the end user with a hook_form_alter() or the like.)
4. Then, you could integrate the functionality of granting access to unpublished nodes into TAC's permissions schemes:
   • You could set the anonymous and authenticated users' grants for Unpublished to Deny.
   • For roles that should have access to view all unpublished nodes, and update only some, you would set the grants for Unpublished to View = Allow, Update = Ignore, Delete = Ignore (or Deny if they should never delete them).
   • Then, you would add additional grants for the appropriate terms in the other vocabularies for those roles.

So, in your example, Role B would have Allow/Ignore/Ignore for Unpublished, and Allow/Allow/Allow (if deletion is allowed) for Term A.

Oh, forgot to add, your administrative roles (whichever ones are setting nodes to unpublished in the first place) would need the Create grant for the Worfklow state vocabulary default, but normal authenticated users would not. No roles should have the List for this term.

Ahh, that makes sense. Yes, a simple workflow vocabulary will not work in this case.

Be sure to take a look at http://drupal.org/node/765832 (which is based on LTE Drupal 6 but is mostly still applicable). We might want to make a little helper module with a hook_node_grants_alter().

(Removing tag as this is not a release blocker.)

TAC doesn't check $node->status at all, and should not alter or override the core behavior. Keep in mind that TAC is actually currently supporting the core feature (since the core philosophy is that only administrators can access unpublished content), and it is view_unpublished that deviates from the core behavior.

The correct solution to integrate the two modules will be a hook_node_grants_alter() so that TAC can override view_unpublished's 0 for grant_update. We might also want to abstract this case to integration with other node access modules.

For rid 1 and 2 (anonymous and authenticated users) tac writes permissions into node_access no matter if a node is published or not but for roles depending on tags it only writes permissions if the node is published. So, to extend my previous example if I have tagged node#11 and configured tac in a way that role#6 has update=allowed for that term I get the following additional line if I publish my node:

11 6 term_access 0 1 0

This line is getting removed from node_access if I unpublish node#11.

This is strange. There is no code in TAC that would make this distinction. This behavior could even be coming from node.module. I will need to look into it further.

TAC doesn't check $node->status at all, and should not alter or override the core behavior.
But because tac doesn't check $node->status it grants view permissions like for any other published note which does override the default core behaviour in my case as it doesn't matter whether a node is published or not, it's treated the same way. So with tac enabled, there is basically no use for a node's status anymore as all nodes are being published because of the View=allow permission-

I was about to reply that this is not correct... then I tested it and confirmed it is correct. In Drupal 6 and prior, the node published state overrode any and all node access grants, from any access control module, so TAC could not grant access to unpublished nodes, no matter what. That's why view_unpublished came into existence. The fact that this appears to be different in Drupal 7 concerns me, because if I was not aware of it as a developer, I am certain many users will also not be aware of it. I am not sure if it is an intended core behavior change; I need to look into it more.

Edit: This is probably the reason for the difference between the Drupal 6 and Drupal 7 behavior with view_unpublished. Now view_unpublished is just another node access control module, and so collides with TAC in the grants table like any other node access control module. So the correct answer is still a hook_node_access_alter().

Found #920614: Unpublished content visible when a Node Access module is enabled, which is marked fixed, but... I will try to get some answers on this from core developers today.

Edit: http://drupal.org/node/920614#comment-3499932 is enlightening.

I would have marked it "Won't Fix" if I wasn't going to work on a solution. :) I just need to fix all these form errors first (#1214472: Undefined index: und in taxonomy_access_form_alter() -- incorrect assignment of language key etc.), and (now) get feedback from some core folks about what the design behavior of the unpublished state is.

DamZ explained this to me. A separate issue has been opened at #1216212: Do not indiscriminately override unpublished state. How we handle that will dictate the solution to this as well.

After thinking about this a bit, I think that maybe the best solution would be for TAC to provide a permission, "Use TAC grants for unpublished nodes" or something. That'd eliminate the need for view_unpublished entirely.

Edit: code snippet was incorrect. However, it would still be simple to implement.

One would simply add this permission to the given role at admin/people/permissions. What do you think? Would this work for your use case?

Try testing the patch in #1216212: Do not indiscriminately override unpublished state and seeing if it can provide the behavior.

1. Disable view_unpublished.
2. Apply the patch.
3. For any role that would previously have had view unpublished permission, instead check the box for "Use Taxonomy Access Control grants for unpublished nodes" at admin/people/permissions#module-taxonomy_access
4. Rebuild node access at admin/reports/status/rebuild. (This step is necessary due to a deficiency in the patch; for testing, rebuild each time you change the permissions at admin/people/permissions#module-taxonomy_access).

So, based on #962664: Taxonomy Index for unpublished entities, here is what I think an interim solution will have to look like.

1. Create a separate submodule, taxonomy_access_unpublished.
2. The submodule implements the permission we tried to add in #1216212: Do not indiscriminately override unpublished state.
3. For the time being, the submodule requires taxonomy_entity_index (henceforth TEI). Note: TEI adds overhead. For most sites it is probably not a huge concern, but for a high-traffic site, or a site with a very large number of nodes or users, it would be better to instead maintain a patch against core (from #962664: Taxonomy Index for unpublished entities).
4. The submodule does a hook_query_TAG_alter() on TAC's node access queries to make it use TEI the table instead of {taxonomy_index}. (Aside: This function would be named taxonomy_access_unpublished_query_taxonomy_access_node_access_alter(). Yowza. I wish for the hundredth time the module were named tac instead of taxonomy_access.)
5. The submodule implements hook_node_access_records_alter() to update TAC's grants to the logic we originally provided in #1216212: Do not indiscriminately override unpublished state.
6. The submodule rebuilds the appropriate node access when the permission configuration for its permission is updated, using TAC's API.

This is a not-insignificant undertaking, so I don't know how soon I could work on it. The roadmap is there, though, if you want to try your hand at a patch.

Why do you say the query is not built? That function calls TAC's query builder. The change needs to be module-wide because we are entirely changing the database table used.

Edit: I think I understand the confusion now. My suggestion was to add the query alter and make your own call to _taxonomy_access_node_access_records() in the hook_node_access_records_alter(). You need to do this so the submodule is robust. Otherwise it will break any time the query is used outside this specific case. Edit: the affected nodes queries would also need to be rewritten as dynamic queries and tagged, I think. Otherwise your nodes will not be updated properly when there are taxonomy or configuration changes. Edit 2: looks like they are already dynamic, so they just need a tag added.

Ping me on IRC if you like and I can explain further.

I just want to reiterate if you create a submodule without including the query alters, it will make your node access broken whenever any change is made other than via node_save(). Say, if the user changes a role's TAC configuration, or deletes a taxonomy term. All the other queries will not select from the unpublished nodes' relationships.

I posted #1220610: Tag all queries on {taxonomy_index} so that all queries against {taxonomy_index} can be tagged with taxonomy_access_node and thus manipulated from another module.

Yep -- though the new tag is more generic, so we thankfully got the hook name down to taxonomy_access_unpublished_query_taxonomy_access_node_alter().

I would keep the foreach loop, as it makes the hook more robust going forward as new functionality is added. Actually, for best practice, we probably want to loop over $tables looking for taxonomy_index and collect those aliases (since it is possible to join on the same table twice), then set them to the new table.

Also, I did not realize that TEI names the id field differently (though... of course, it has to). This means we need to check not only conditions, but also fields and joins, and replace the nid with $alias.entity_id as you have above.

The code above looks like the correct solution as far as I can tell at first read. Regarding the joins, when you complete the modifications in the hook and do a $query->__toString(), does it correctly use the new table in in that query even when the table is not the main FROM?

I also implemented hook_node_access_records_alter() and wrapped everything into a module with a dependency to tac and tei. But since I'm about 10 days into drupal community I don't really know what to do next. I can post my code here or give it to you to integrate it with tac or to open a new module, or I can open a new module on my own.
Or in case you're thinking it's not useful/practicable at all, I can just forget about it and only use it internally.

It's definitely worth using as a TAC extension module until #962664: Taxonomy Index for unpublished entities is fixed. I can maintain it, since ideally I'd like to integrate the functionality into TAC once TEI is no longer required. Go ahead and upload an archive of it as an attachment here once you're ready so that I can review it.

Excellent! I'll do a code review and some testing when I get the chance. If everything looks good, we can open a project for it.

We'll want to implement a hook_menu() and move the config to admin/config/people (per http://drupal.org/node/549094#config-people), and probably as a sub-tab under TAC's config. I've been meaning to add additional tabs to TAC's config for awhile anyway, though, so I'll work that issue out then.

Ah, I misunderstood. I see now. Up to davereid, then.

Ta-da!
http://drupal.org/project/taxonomy_access_unpublished
http://drupal.org/user/1058828/track/code

I made a few changes:

1. Cleanup for various Drupal standards (code and otherwise).
2. Changed the human-readable name to TAC Unpublished" for fewer keystrokes.
3. Rewrote the hook_help() a little.
4. Removed some extraneous stuff (unneeded .install file, nonexistent configuration path, etc.).
5. Discovered grant alter hook was bugged, and consequently...
6. Replaced the grant alter hook and its helper with a much simpler version based on my original patch attempt from #1216212: Do not indiscriminately override unpublished state.

I also made a TAC commit to support caching role lists per permission.

So far, the query alter seems to be working nicely. I tested it for anonymous and for a custom role. There's one outstanding issue (no node access update on permission change) but we can open that issue against the new project!

I've been using the module grants module http://drupal.org/project/module_grants to deal with node grants on unpublished content. I've tested it with TAC on unpublished content on D6.

#39: This issue was specific to D7, which completely changed the interactions between taxonomy, unpublished content, and node access.

Instead of using taxonomy_access_unpublished, why not just remove the conditional in the code snippet in #21? Needing this functionality on a very large multi-site, and worried about the non-trivial overhead from taxonomy_access_unpublished.

Thanks!