Comments

marcingy’s picture

Status: Needs review » Closed (won't fix)

node_access alreay does this so the additional is not required

amitaibu’s picture

I think This is not correct, for example:
1) Deny access content to anon users
2) Allow 'load any node data' to anon users
3) Try to get.Node - anon user will be able to view the node.

The current node_service_get_access() doesn't check node_access.

marcingy’s picture

Which is the same as standard Drupal. To overcome this issue you need to install a content access module which allows more ganular control of node grants.

amitaibu’s picture

Which is the same as standard Drupal.

I accept this statement. I'd just like to refine my use case. I'm using organic groups. Because of the nature of Drupal's access system you can only grant access - you can't really deny.

So installing OG and content access module for example, is problematic.
Having this service, might simplify how things are handled - and since service already has an access mechanism I think it's appropriate to use it to deny access, same as we are doing with (i.e. 'load own node data').

With that said, I of course respect any decision you take :)

marcingy’s picture

Druapl access system can be used to deny as well as grant access - if a grant isn't present in the node_access table then access is denied. Content_access combined with ACL allows per user and per role access. The way to think of this is how would you enforce it if the services module wasn't involved. Then once those controls are in place assuming it utilises node access services will then respect those access controls.

amitaibu’s picture

if a grant isn't present in the node_access table then access is denied.

1) That is correct, however if there are several rows in node_access - having a single row with grant is enough - another module can't 'deny', that's what make it problematic in using several node_access modules, the fact that node access is using OR and can't use AND.

2) Either way, if services doesn't wish to implement it's own access_check then 'Load own node' isn't following this concept.

btw, Are you in the IRC?

amitaibu’s picture

I was thinking about #3, and I think the problem is that Services simple bypasses node access...

The user was not allowed to access content, but she still can using services.

The whole statement should be something like

return (node_access('view', $node) && (($node->uid == $user->uid && user_access('load own node data'))  || user_access('load any node data')));
marcingy’s picture

Status: Closed (won't fix) » Needs work

Thanks for this input and now that it put this way it makes prefect sense, and I now understand what the issue is.

Can you roll a patch and I'll get this committed as quickly as possible.

marcingy’s picture

Category: feature » bug
amitaibu’s picture

Title: Add 'load accessibale node data' permission to node.load » Add check permissions to node service
Status: Needs work » Needs review
StatusFileSize
new909 bytes

attached patch.

amitaibu’s picture

After IRC with RobLoach, this patch makes it even more sane, and let's other content access module the chance to work - by keeping only a single permission 'load node data'.

snelson’s picture

Wierd. If you look at Services for D5, this is exactly how its doing it. Don't know how that got changed.

snelson’s picture

Status: Needs review » Fixed

Commited to D6.

Thanks Amitaibu

amitaibu’s picture

Thanks snelson :) 2 remakrs:
1) Maybe there should be a security announcement about this node access bypass.
2) I see you commited it with 'and' instead of '&&' - however core uses the later.

snelson’s picture

I agree about the security announcement. Let me finish up a few more things, get a release out, and I'll contact security team.

"and" and "or" is used in several places throughout the module. But, I agree, we should keep up with Drupal coding standards. I'll do a separate patch that takes care of all these.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.