Use "any" core node permission so that non-members can modify or delete content

> At the moment, og is restricting node access permissions to only those who are members.

That's not correct, see:

// We don't have a context, so we need to get all the permissions
// of all the groups. We don't intersect with the user's group, as
// groups might allow anonymous members access.
foreach ($gids as $gid) {

hook_node_access() deals only with OG's permissions. The "core" permissions are already handled via node module.

Ok, now I understand :)

I think this isn't a bug, but a feature, as one might want OG to take full control (actually up until now, there was no issue like yours, so I assume most people want it this way).

So I think our appraoch should be in og_node_access:

Instead of $return = NODE_ACCESS_DENY; we should have a variable:

$return = variable_get('og_node_access_strict', TRUE) ? NODE_ACCESS_DENY : NODE_ACCESS_IGNORE;

And in OG-UI add this setting. Of course a simpleTest will make sure it will be committed ;)

Thanks!

+++ b/og.moduleundefined
@@ -425,12 +425,11 @@ function og_og_default_roles() {
+  $return = variable_get('og_node_access_strict', TRUE) ? NODE_ACCESS_DENY : NODE_ACCESS_IGNORE;

Lets add comment why we have this variable -- i.e. OG full control VS allowing others to also decide.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+  ¶

Minor: Remove white space.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    // Create some users

Missing dot in the end (other comments as-well).

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->group_manager = $this->drupalCreateUser(array('access content'));

Isn't $this->drupalCreateUser() enough?

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $settings = array('type' => 'page', OG_GROUP_FIELD . '[und][0][value]' => 1, 'uid' => $this->group_manager->uid);

Please break this long line.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+  function testStrictAccess() {

Add PHPdocs.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    // Rebuild content access perms., and set group perms.
+    $this->drupalLogin($this->admin_user);
+    $this->drupalPost('admin/reports/status/rebuild', array(), t('Rebuild permissions'));
+    $this->drupalGet('node/' . $this->group_node->nid);
+    $this->drupalLogout();

We don't need this, as we don't deal with node access records.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->assertRaw('Access denied', t('A non-member with core node access permissions was denied access to edit group node.'));

Use $this->assertResponse()

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+
+    $this->drupalLogout();
+
+    // Login as a group manager and try to change group node..
+    $this->drupalLogin($this->group_manager);

I don't think you need to logout, before login -- doesn't it happen by itself? not sure.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->drupalLogout();

No need to logout, in the end of the test :)

+++ b/og_ui/og_ui.admin.incundefined
@@ -19,6 +19,13 @@ function og_ui_user_admin_settings($form_state) {
+    '#title' => t('Strict group node access permissions'),

Maybe better: Strict node access permissions

+++ b/og_ui/og_ui.admin.incundefined
@@ -19,6 +19,13 @@ function og_ui_user_admin_settings($form_state) {
+    '#description' => t('When enabled group node access permissions will supercede core node access permissions such as update and delete.'),

Maybe: When enabled OG takes full control over node access and if no access found based on OG access is denied. If disabled access will not be explicitly denied.

Thanks!

+++ b/og.moduleundefined
@@ -425,12 +425,11 @@ function og_og_default_roles() {
+  $return = variable_get('og_node_access_strict', TRUE) ? NODE_ACCESS_DENY : NODE_ACCESS_IGNORE;

Lets add comment why we have this variable -- i.e. OG full control VS allowing others to also decide.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+  ¶

Minor: Remove white space.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    // Create some users

Missing dot in the end (other comments as-well).

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->group_manager = $this->drupalCreateUser(array('access content'));

Isn't $this->drupalCreateUser() enough?

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $settings = array('type' => 'page', OG_GROUP_FIELD . '[und][0][value]' => 1, 'uid' => $this->group_manager->uid);

Please break this long line.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+  function testStrictAccess() {

Add PHPdocs.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    // Rebuild content access perms., and set group perms.
+    $this->drupalLogin($this->admin_user);
+    $this->drupalPost('admin/reports/status/rebuild', array(), t('Rebuild permissions'));
+    $this->drupalGet('node/' . $this->group_node->nid);
+    $this->drupalLogout();

We don't need this, as we don't deal with node access records.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->assertRaw('Access denied', t('A non-member with core node access permissions was denied access to edit group node.'));

Use $this->assertResponse()

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+
+    $this->drupalLogout();
+
+    // Login as a group manager and try to change group node..
+    $this->drupalLogin($this->group_manager);

I don't think you need to logout, before login -- doesn't it happen by itself? not sure.

+++ b/og.testundefined
@@ -326,6 +326,106 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->drupalLogout();

No need to logout, in the end of the test :)

+++ b/og_ui/og_ui.admin.incundefined
@@ -19,6 +19,13 @@ function og_ui_user_admin_settings($form_state) {
+    '#title' => t('Strict group node access permissions'),

Maybe better: Strict node access permissions

+++ b/og_ui/og_ui.admin.incundefined
@@ -19,6 +19,13 @@ function og_ui_user_admin_settings($form_state) {
+    '#description' => t('When enabled group node access permissions will supercede core node access permissions such as update and delete.'),

Maybe: When enabled OG takes full control over node access and if no access found based on OG access is denied. If disabled access will not be explicitly denied.

> Should I create a new branch and just grab the overall code and commit that, then format a new patch

Yes please:

git diff 7.x-1.x my-branch > patch-fie.patch

+++ b/og.field.incundefined
@@ -402,19 +402,21 @@ function og_field_attach_form($entity_type, $entity, &$form, $form_state, $langc
+    // Rebuild the form state values if OG_AUDIENCE_FIELD exists.

Why is this needed? This validation callback is called from OG-audience widget, so for sure the field is there. + it's not related to the issue ;)

+++ b/og.moduleundefined
@@ -431,18 +431,21 @@ function og_node_access($node, $op, $account) {
+        // og_node_access_strict will take full control of node access when true
+        // while it will pass through access to other modules when false.

Maybe better: Determine if OG should explicitly deny access.

+++ b/og.testundefined
@@ -326,6 +326,116 @@ class OgGroupApi extends DrupalWebTestCase {
+    $this->assertTrue(variable_get('og_node_access_strict'), t('Organic groups node access strict variable set to true.'));

No need for this assert.

+++ b/og.testundefined
@@ -326,6 +326,116 @@ class OgGroupApi extends DrupalWebTestCase {
+    // Login as a group manager and try to change group node.
+    $this->drupalLogin($this->group_manager);

Why do we need this part?

I have just committed this , which will allow to make og_node_access() shorter, something along this lines (untested):

/**
 * Implement hook_node_access()
 */
function og_node_access($node, $op, $account) {
  // If not a group type or the operation is node creation which still has no
  // groups so we can't check it yet, we ignore the access.
  $return = NODE_ACCESS_IGNORE;

  $type = is_string($node) ? $node : (is_array($node) ? $node['type'] : $node->type);
  if (in_array($op, array('update', 'delete'))) {
    $access = og_user_access_entity('administer group', 'node', $node, $account);

    if (is_null($access)) {
      // The node isn't in an OG context, so no need to keep testing.
      return NODE_ACCESS_IGNORE;
    }
    else {
      $access = $access ||
        // Any content.
        og_user_access_entity("$op any $type content", 'node', $node, $account) ||
        // Own content.
        og_user_access_entity("$op own $type content", 'node', $node, $account);
    }

    if ($access) {
      return NODE_ACCESS_ALLOW;
    }

    // No access, check if OG should explicitly deny.
    return variable_get('og_node_access_strict', TRUE);
  }

  return NODE_ACCESS_IGNORE;
}

> Should a user who can update a field be able to update the node as well?

No. But it seems that the failing test is "catching" a bug (as it passes without the test). Note that it fails in giving access to node-edit.

Oh, I see where it fails. We have the "update group" permissions defined in OG, but we don't explicitly check for it in the hook_node_access().

^^ It's good that we have tests ;)

Attach the patch the solves #17. Still reviewing...

Cleaned a bit more, and committed. Thank you mradcliffe!