D7 port of Module Grants

Hi Ben,
I went to that session.

Your are mostly right, as confirmed here http://drupal.org/update/modules/6/7#hook_node_access_records :

In Drupal 6, any node grants provided by hook_node_access_records() would only apply to published nodes. In Drupal 7, hook_node_access_records() now applies to both published and unpublished nodes, giving the developer more flexibility to manage node access. Also, the 'view own unpublished content' was added to still grant users view access to their own unpublished content.

However, I believe Module Grants will still be necessary in some form, especially when multiple content access modules are in play.
But the code will be simpler and more elegant, as more "hooks" have been made available in D7.

Rik

Subscribe. We're considering specifying Drupal 7 for a site that has a fairly long deadline, I'm assessing the D7 status of modules we might need.

I can't make a DX7 pledge yet. Although with D7 things are easier, it also means that I will have to rewrite large sections pretty much from the from the ground up, as the current D6 code no longer applies. So it's more than just tweaking for D7.
Right now I don't have the spare, unpaid time to do this. This may change though.

OK. I could possibly help out further down the track.

What's the status on this now?

Have you decided whether there will be a D7 version of this module?

subscribing

Is it ok to turn this one into a task, so we can keep related discussions in one place instead of opening more issues?

subscribe

Subscribing cause this seems to be the solution to work with Domain Access and private Organic groups

subscribe

subscribe

Subscribing

To all reading this thread. Help me get my brain around the D7 port and answer #1170324: Mini-poll: how are you using Module Grants?.

Subscribing. I voted on the poll as well :)

Subscribing.

Subscribing

Subscribing

I agree that the multiple node access module functionality is really needed in Drupal 7. I'm in the same situation as some others here that I'd like to be able to use Organic Groups and Domain Access together, which currently isn't possible without a module like this. See #1355272: Organic Groups and Domain Access where a couple of us have discussed this issue with DA's maintainer @agentrickard. He seems to think it wouldn't require much code, but it's really above my skills. It seems logical for Module Grants to provide this functionality in Drupal 7 since people know this is the go-to module for this functionality in Drupal 6 (5000+ sites using Module Grants.)

I can definitely help test, if anyone has the time (and the right skill set) to work on a D7 version.

I'd also really love to see a D7 version of Module Grants. In particular, to get Organic Groups and Workflow playing nicely together.

subscribing

Any D7 version planned yet?
Cheers,

Followed.

Following

@Ozzie Please use the new "follow" button at the top of issues rather than posting a comment. See http://drupal.org/node/1306444 for more info.

That's fine.. Can you list that on the Project page so folks know that there are no plans for D7.

@mgiffod, #26:

Done.

It says: D6 is approaching end of life. We'll be doing maintenance fixes only. This module will not be ported to D7 by the current maintainer. The module wouldn't be as useful in D7 as in D6, as D7 core is an improvement over D6. If you wish to be a maintainer of this module, let the current maintainer know.

Thanks! And appreciate your work on this module over the years.

You're welcome, Mike.
Rik

I know this issue is closed, but I thought I'd post this here for those who are interested.

I have written a module for Drupal 7 that ANDs module grants instead of OR. The approach used is a generalization of the method described in: http://drupal.org/node/1355272 and used in: http://drupal.org/sandbox/babbage/1891566 . It has not been tested extensively, and what little testing done was only with the ACL + Content Access + OG access control combination. You might get lucky with other combinations, or you might need to add a little customization. If the latter, try looking at the modules_access_acl and modules_access_content_access modules included.

Implementation Specific Quirks:
- does not try to handle priority, other than to propagate them by max() function
- assumes that modules which have no grants for a node doesn't care about access for that node
- uses the gid entry of the first access module, and encodes the gid for others in the realm by appending to the realm with '-'
- creates custom realms by concatenating them with the '|' character

demos, I have tried your module with Domain Access ( http://drupal.org/project/domain ), ACL, & Content Access enabled. All seems well now where before installing your module I could enable Domain Access and Content Access, but Content Access was overriding privileges and allowing content to show on both domains.

Thank you!

I'm glad the module is working for you. Here is an updated version. It is mostly a code clean-up, and provides no additional functionality.

Update instructions:
1) Replace the old files with the new files
2) Rebuild permissions at admin/reports/status
3) Clear the cache at admin/config/development/performance

@demos you up for being the new D7 maintainer?

It's not that I'm averse to maintaining the code, but I'm not at a point where I'm relying on it to be stable for any critical project. If this changes, I might reconsider. As it stands, I don't think I can be as responsive and reliable as I should be in order to be a maintainer. I would have no problem, however, with anyone else taking the code and doing whatever they wanted with it, including making it a part of their own project. Just don't claim I didn't write it if anyone asks.

You don't have to be an official/dedicated maintainer. Just a person with commit rights to the branch so you can act when people submit patches. A part-time/best-effort maintainer is better than none at all.

Ok, here's my first attempt at porting this module to D7, please review. Only the ANDing of grants feature is ported, which I guess is all we need for D7.

Thanks jrao and demos for the ports.
jrao, is your port based on demos' port in #32? Or is it very different?

As stated on the project page, I won't be maintaining this module for D7. If one of you wants to do it, I'll give you maintainer rights.

Rik

@RdeBoer: It's using a different approach from demos' port, demos' version has no mangling of node.module's hooks, so works well within the confines of what is known. Mine has some unusual usage of hooks in order to override node.module's node access methods, so it's more risky, but it doesn't change the node_access table records and I think its grant logic is more inline with D6 version of module_grants. Hopefully some expert can review my version and confirm it doesn't break anything.

If my version is deemed to work, I'm willing to maintain it for D7 at least for the time being.

Ok, 7.x-1.x branch created from 6.x-3.x branch and set as default.
Note: the 6.x-4.x branch was a mistake from way back and should not be used. I tried to delete it, as well as the master branch, but couldn't. So just ignore it.

jrao given permissions to check in and change project page.

If someone else (@demos ?) wants permissions also, let me know.

Rik

Hi jrao and RdeBoer,

I've taken a quick look at jrao's code approach to the problem. I have to preface the following comments with the disclaimer that I have not bench-marked anything, nor have I even tested jrao's code in an actual install.

From my inspection, it seems that jrao's code is exactly what he claims it to be. It does not change any node_access database records, and is much more in-line with the approach used in the 6.x version of module grants than my code. The access calculation (AND-ing) is done on look-up, for both node viewing and node listing (for views). I believe the code shows the author to have a much better understanding of the inner plumbing of the Drupal system than me. I quite liked (as a curiosity) the way native hook processing was bypassed, particularly in the "module_grants_node_access" function.

Having said that, code quality aside, I believe the fundamental approach I took to the problem to be better for the following reasons:

1) It is much less invasive, code-wise.

jrao's code re-implements large parts of core logic. This makes it sensitive to core code changes (probably not a big issue in practice) and makes the module more complex in what it has to do. That sort of approach was necessary in Drupal 6, but Drupal 7 introduced some node access api changes that allows us to accomplish our goal in a less invasive and indirect manner.

2) It trades increased space usage for lower execution time.

I don't have the performance numbers to back this up, and even if I did there may be certain cases where you would want to use less DB space in place of increased execution time. However, I feel that my approach of pre-calculating the AND-ing results and storing the results rather than doing the calculation on every look-up is a better approach in most cases. I believe it is the difference between gathering several DB entries and checking each for allowance, as opposed to looking for a single record by itself. Perhaps there is some implementation nuance that makes this point moot, but I think it's worth mentioning if only to be able to debate the point.

3) It appears to be the preferred method.

It's true that my approach is more database-invasive, but based on forum posts like this: https://drupal.org/node/1266388#comment-4933830 as well as the comment and project I mentioned in post #30 ( http://drupal.org/node/1355272 , http://drupal.org/sandbox/babbage/1891566 ) , it seems that altering the access records themselves is the favored way to handle modification of access logic.

Now for a disadvantage of my implementation:

Access records and grants have to be manually sorted by realm to categorize them by module, since Drupal does not natively make a by-module separation available. This is something I wish would be changed in Drupal core itself, but would require some api hook additions and alterations.

Although I believe my approach to be better, and would prefer it over the alternative, the truth is that I don't think jrao's approach is bad at all. Also, I have no desire to create any sort of competing project that serves the same purpose. No matter which method is used in the end, I'll more than likely switch my project over to whatever becomes module_grants for Drupal 7. As a final note, I am willing to take on commit rights if some semblance of my code and it's approach is included. Otherwise, I'm not sure if I can really contribute in a way that justifies having those rights.

Hi demos,

Thank you so much for your original patch/rewrite and for your comments in #40.

I have not studied either of the patches/rewrites by @jrao and @demos and have been away from this project too long to comment on the pros and cons of these.

I have granted permissions to both @jrao and @demos to push the project in a 7.x-1.x direction.

May the force be with you both!

Rik

How about this: create a 7.x-1.x branch that is in par with the 6.x logic of the module and let @jrao be the maintainer of that branch and also create a 7.x-2.x branch that will explore using the "new" method proposed by @demos.

Even better: add both methods in a single branch with one of them being the default and make it possible to switch between them through the UI. This way, you'll get people testing and benchmarking these two methods. Given enough time and once enough stats/feedback has been gathered, you can at some point deprecate one of the two available methods in favor of the best one of them.

Anyways, my point is that no matter which way we choose to go, we should at the very least document the alternative way just in case (say in D8 or D9) we hit a dead end and are thus forced to seek other ways. My 0.02$

Re #42:
Not a bad idea klonos, or in fact two ideas!
I'll leave it up to the new maintainers to decide.
Rik

@RdeBoer: Thank you for the commit permission, I'll leave the module here a while longer in case someone else want to review it and commit it in the next few weeks if there're no objections.

@klonos: I'm not sure we should mix the code of the two modules, since the logic of either is already pretty complex, mixing them together may make them very hard to understand. We could put demos' module as a submodule, and disable my logic if the submodule is enabled, although I'll have to spend some time thinking about how best to disable my module's hook changes.

@demos: Thank you for the review. For 2), performance is actually one of the reason I wrote mine, the problem is not exactly with your module (although I'm guessing it does increase the # of grants to be calculated), but with node.module's way of using db_select condition to add grant criteria:

      foreach (node_access_grants($op, $account) as $realm => $gids) {
        foreach ($gids as $gid) {
          $grants->condition(db_and()
            ->condition('gid', $gid)
            ->condition('realm', $realm)
          );
        }
      }

When # of realms/grants are large (say thousands), all the conditions added by this code will need to be compiled which takes quite some time. So I found I have to override this logic using the hook mangling techniques. And since I'm overriding node.module anyway, it might be more straightforward to just do the grant checking there too.

@jrao : I found that the number of grants to be calculated depends on the number of access modules and number of grants for each. In some cases it was the same or less than without the AND-ing process, but in cases of larger scale, it was pretty much always more. I even added some sub-modules to try to alleviate this by grouping compatible access module grants together. If this is the limiting performance factor, and your override reduces the execution time for it, then it is almost certainly better for speed. It seems we both have issues with the way Drupal handles the node access logic natively.

I was unaware that you had changed the database look-up routine. Looking at it now, it seems that it replaces the one giant db query that is constructed with a separate query per module. It looks like it also separates the grant collection database look-ups and incorporates them with the grants checking queries. It seems pretty plausible that this can increase performance in many average case scenarios, and i guess it reduces the queries overall due to combining the grant collection and checking. Is this right?

I still think my other points are valid, but speed rules.

@everyone: I agree with jrao that it's not a good idea to mix the code of the two. They would just get in the way of each other. In the absence of solid benchmarks, through the magic of theory-craft, jrao's implementation is better for performance. I think we should just go with that, and leave my module in the forum as an historical curiosity.

Fair enough.

Ok, code checked into 7.x-1.x branch.

Thanks jrao! I've added 7.x-1.x as a release branch on the project page.

Thanx! Just FYI still doesn't show any 7.x version in the project's page though - had to click the "View all releases" link.

That's because the Release managment on drupal.org is stuffed up since they changed to D7....
Hopefully they'll fix it soon.

...thought so. Thanx for taking the time to reply.

@demos and those who are looking for a way for Content Access and Organic Groups 7.x-2.x to work together.

FYI I tested your module and the 7.x-dev version and found yours worked correctly for me when the 7.x-dev version did not.

My site has one content type has pages that should be visible to anonymous, with all other content types visible only to authenticated or some subset dictated by roles; I use Content Access module to achieve this. Within these restraints, Organic Groups is used to ensure that some content is only available to particular users.

With 7.x-dev with and without strict access control didn't give the results I needed. Without strict access some content was visible when I expected it to be hidden. Similarly, when strict control was in place, some content that I expected to be visible was locked down.

I have found one problem with your module - together with Devel Node Access, I get errors/warnings. E.g. Warning: array_search() expects parameter 2 to be array, null given in _devel_node_access_ng_alter() (line 478 of /sites/all/modules/devel/devel_node_access.module). I had a quick stab at debugging but didn't solve the problem, but it doesn't affect a live site where this module would not be enabled.

@Jeff Veit It has been a while since I looked at my modules_access code, and even longer since I tested it in combination with Devel's node access testing. At that time the two modules seemed to work fine in combination. I am currently still using my module, but expect to migrate over to jrao's version eventually.

I'm not sure when or if I'll be able to examine the issue, but I'll try to take a look when I have the time. However, the best way to "solve" this issue is probably to get it working for the 7.x-dev version, since that is the official code path of the future for this type of functionality. Perhaps a new bug report or feature request is in order?

Thanks to @jrao and everyone
testing module_grants-7.x-1.x-dev and so far it seems to be working
with og-7.x-2.x-dev and taxonomy_access-7.x-1.x-dev

i'm wondering what plans, if any, there are to keep the 7.x branch of this module alive?

I hope no one minds I have changed the version of this issue.

I know this issue is marked as closed and I am very tempted to change it to Active but know that is not generally appreciated.

@demos thank you for the module in #32 and comments in #40.

We have recently used the module_grants-7.x-1.x-dev with og and taxonomy_access and encountered the issue #2389415: Unpublished nodes visible to all roles including anonymous.

The module by @demos in #32 does not have that issue which to us is critical. And in other ways it appears to behave as expected in early testing.

There may be other issues related to performance and scalability but that is not a big concern to start.

I see the suggestion of having a 7.x-2.x branch to preserve both approaches and allow for further comparisons as possibly useful.

I have not changed the status to Active, although tempted, I will leave that for someone else. The other issue might then be closed as a duplicate of this one.

Thanks.

I have decided to change the status to active based on my previous comments in #56.

This thread includes discussion of two approaches to the D7 port of module grants. One has been committed as module_grants-7.x-1.x-dev and the other modules_access remains uncommitted and yet it has advantages. I would like to see the code of modules_access committed to some branch so that it remains more accessible and allows for further work as need.

Reading the comments here, it seems that some of the code/logic in the modules_access.zip attached to @demos' comment #32 should be considered added to the current -dev version.

Indeed I can confirm that #32 is an elegant approach and it works with other type of access control modules too. Here I tested with a totally custom access module and OG Access.
I applied Drupal Coding standard and some other best practices to the code in #32, I attached the new full source code.

@demos I'd encourage you to start a sandbox project (https://www.drupal.org/node/1011196) for this code and then, after it gets mature, it can be a fully featured module on drupal.org

Or indeed, we can do a search & replace, rename this goodness and make it module_grants D7 (stable?).

And this of course needs review.

+1 to see this move forward.