Remove 'view' from hook_access() [#890600]

Thank you for creating this module. I am currently using it on my site. However, I am altering the module (through my own module) adding on the grants system so that I can limit access to Invoices to users I specify when creating the invoice. The problem I am having is in this code:

function invoice_access($op, $node, $account) {
  if ($op == 'view') {
    return user_access('access invoices', $account);
  }

The way Drupal's permission system works is that it goes through the following steps:
1) Check permission on a content-type basis in hook_access()
2) If no value is returned from hook_access(), use hook_node_access() to check for permissions on a node-specific basis

The problem for me is that because you are returning a value for 'view', the Grants system is never called for 'view'. This means that no access modules (mine being one) have the ability to add node-specific permissions to your module. As such, setting a value for $op == 'view' in hook_access is generally considered bad practice.

If possible, I would like to ask you to remove 'view' from hook_access(). I have done so in my own site, but if/when you update the module, it will fail.

If you would like, I can help you add my grants permission system to your module. I've already done this for my own site, enabling me to allow access to invoices to specific users specified when creating/editing the node.

Thank you.

Comment	File	Size	Author
#3	invoice_node_grants.1.patch	177.28 KB	Jaypan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pietervogelaar CreditAttribution: pietervogelaar commented 23 August 2010 at 07:49

Version:

6.x-1.0-rc3

» 6.x-1.0

Hi,

Can you provide me a patch with your solution? And do you have a page for me that says it's bad practice? The time I build it I used the example on the Drupal site..

Pieter

Comment #2

Jaypan CreditAttribution: Jaypan commented 23 August 2010 at 09:07

I'll look for the presentation I found that talked about why not to add 'view' to hook_access(), but I don't remember exactly where I saw it. However this was the exact case they gave as to why not to use it!

I'll also try to create a patch for you, but I seem to have troubles with my patches. They often don't seem to be usable by anyone else. So if I can't get a working patch for you, I'll just attach the code in a file for you to look at. Give me a day or so.

Thank you for your quick reply.

Comment #3

Jaypan CreditAttribution: Jaypan commented 24 August 2010 at 02:52

File	Size
invoice_node_grants.1.patch	177.28 KB

Ok, I am attaching the patch. I tested this with tortoise SVN and it seems to work, so hopefully you will also be able to apply it. After applying the patch, you will need to run update.php, as a new table needs to be added to the database, and both the menus and permissions tables have to be rebuilt. This will all be taken care of by update.php.

This patch does the following:
1) Creates a new path that is used for an autocomplete field, allowing for a list of comma separated users
2) Adds an autocomplete field to the node creation form, allowing for users to be given permission to view the invoice
3) Hooks into the grants system, providing or denying viewing access based on the list of users entered in #2. Users with 'administer nodes' privileges are outside of the grants system and will be able to view the node regardless of whether they are in the list or not.
4) Reinstates drupal_uninstall_schema() in hook_uninstall() (this shouldn't be commented out. If someone's admin gets hacked, they have bigger problems, and should be taking backups anyways. Modules should always uninstall their tables when a module is uninstalled).

I'll be honest, I haven't tested this extensively yet. I've only just put it on my own site (and as an add-on module, not as an edit to the original module like this patch performs), so it should probably be tested a little. But for the most part, I think it's probably OK.

Comment #4

pietervogelaar CreditAttribution: pietervogelaar commented 1 October 2010 at 11:29

I tried to apply the patch in netbeans and it did nothing. I also tried on the command line cvs diff -up > yourfile.patch, but that also didn't make any changes.

Would you please provide me the actual code? This may be the complete modified invoice module zipped or something.

Pieter