currently, the access control for platforms is really backward. we only check for permissions but not for client access, even though we have client ACLs for platforms.

we should implement proper node_access level permissions so that people that don't have access to a client don't see their platforms at all. right now, the check is only done in the site form, but if this is done right, the user will see only the platforms he has access to.

this means dropping the _hosting_get_allowed_platforms() hack.

CommentFileSizeAuthor
#1 hm_platform_node_access.diff5.76 KBadrian
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adrian’s picture

This is my implementation of it , but it is fraught with peril.

We need to have more granular permissions.

Basically tasks inherit the permissions of their parent. So if you can view the platform, you can view all it's tasks.
And then there is only a "create X task" permission, which makes no difference between being on platform or site nodes.

SO it will happily allow you to delete a platform.

I've made some fixes to that, but even then you have a situation where you can view all the task logs as the client, if you have access task log.

adrian’s picture

Status: Active » Needs work

i reshuffled the current implementation a bit , but i opted not to go for the node level access at this point.

too many weird things happening down the road.

Anonymous’s picture

Sorry if this is the wrong ticket to discuss this..
So since _hosting_get_allowed_platforms() has been dropped (side issue: the function is still in there but its implementation was removed), if one does not check any clients in the platform form for access, 'all access' is assumed. That's fine.

If you select a client for access on one platform, and not on another, the one you *did* select as having access, disappears from the site form
If you select the client for access on both platforms, *both* platforms disappear from the site form.

The problem seems to be around this array_intersect on $platform->clients in the hosting_site_form().

    $platforms = _hosting_get_platforms();
    $user = user_load($GLOBALS['user']->uid);
    if (sizeof($platforms)) {
      foreach ($platforms as $nid => $title) {
        $platform = node_load($nid);
        if ($platform->platform_status != HOSTING_PLATFORM_LOCKED) {
          if (!isset($platform->clients) || sizeof(array_intersect(array_keys($user->client_id), $platform->clients))) {
            $options[$nid] = $title;
          }
        }
      }

I did a print_r($platform->clients); within the foreach and I get this:

Array ( [1] => 1 ) Array ( [1] => 1 )

mysql> select * from hosting_platform_client_access;
+-----+-----+
| pid | cid |
+-----+-----+
|  71 |   1 | 
|   4 |   1 | 
+-----+-----+
2 rows in set (0.00 sec)

I don't understand why the actual $platforms (the $options[$nid]) are being dropped when a client is given access. It seems it is the reverse of what should be happening.

Anonymous’s picture

Weirdly, I think it might be because my second client is being loaded first instead of me, in the form, and so the sizeof is 0, and that client has no access to these platforms (since selecting my first client (me) for explicit access on each platform means the second client has none)

dumping $user->client_id after the $user = user_load($GLOBALS['user']->uid);

array(1) { [73]=>  array(1) { [0]=>  string(0) "" } }    

I can verify this by removing the explicit access to the platforms of the first client, and giving explicit access to the second client for each. Then the form works as expected.

So this technically works properly! The problem is that after a second client is created, the second client is being loaded by default in the site form instead of the first.

omega8cc’s picture

It is probably related: if you will disable "access task logs" for the user, he will not be able to run any task, even with properly enabled permissions for every kind of task.

omega8cc’s picture

This one looks funny, I hope it doesn't work! (didn't tried yet) http://skitch.com/omega8cc/natkq/fullscreen

Anonymous’s picture

Re this: http://skitch.com/omega8cc/natkq/fullscreen

Are you referring to the fact that there's a Platform task? That's the crux of the whole platform management work that made it into this release. If a platform has no sites on it, the Delete task will delete the whole platform off the server.

omega8cc’s picture

Yes, that makes sense, but I was curious why that "Run" button is active (it is possible to run delete task) also in the "root", Aegir hostmaster platform. I think it should be disabled for any platform with at least one active site, and for main hostmaster platform should never be available. Of course it fails to run, so it is only minor usability issue.

Anonymous’s picture

The way it fials to run is by design, we can't check from Provision whether the site is deleted in the frontend (that I know of?)

Also, afaik, we don't have a way of telling which platform is the one the Aefir frontend uses. I did think of this when I wrote the platform management work, but I couldn't find a way to determine which platform the frontend is using.

But probably we could dig a little deeper and load the site based on which site is using the hostmaster profile, and in turn, which platform that site is using, and in that case, disable the task from being run, but it is tricky.

And it is a separate issue from this ticket I think.

Anonymous’s picture

Weirdly, I think it might be because my second client is being loaded first instead of me, in the form, and so the sizeof is 0, and that client has no access to these platforms (since selecting my first client (me) for explicit access on each platform means the second client has none)

This is an incorrect theory as it continues to occur even if a) CLient module isn't enabled and b) there is no other client other than the 'main' one.

No idea how to fix this.

Anonymous’s picture

The problem seems to be that the value of array_keys($user->client_id) is 0 and so the array_intersect does not return the right values.

I guess this is getting into the '# find the right client' logic in hosting_site.module that has always sent me cross--eyed..

Anonymous’s picture

Actually it works as expected for users other than uid 1 that have access to a $client. It is only once you enable the Clients feature and start creating clients/users associated to those clients that the hosting_client_user table starts to be populated. For a vanilla Aegir installation, hosting_client_user table is null, thus hosting_get_client_from_user($user->uid) returns null and thus for the admin uid 1 user, $user->client_id is NULL.

So:

1) We fix it so that even with only uid 1 in a vanilla system, *something* goes into hosting_client_user. This infers the Client feature should simply always exist - it kind of 50% does anyway, there is already a ticket about this somewhere, in that the Client module cannot really be turned 'off' as the logic seems a grey area

2) We simply add a conditional to this:

        $platform = node_load($nid);
        if ($platform->platform_status != HOSTING_PLATFORM_LOCKED) {
          if (!isset($platform->clients) || sizeof(array_intersect(array_keys($user->client_id), $platform->clients))) {
            $options[$nid] = $title;
          }
        }

to also negate the check if $user->uid == 1 or something, thus assuming uid 1 *always* can see any unlocked platform irrespective of permissions. This is why it Just Worked prior to this when my 'hack' access function was in place, because the negation was based on (user_access('administer sites')

In short, *something* needs to happen, because it is a shock to check access for *any* user to a platform and then find that as uid 1, you can no longer use the platform to provision sites on.

Thoughts?

Anonymous’s picture

Fixed everything I spammed about from #3, #4, #10, #11,#12 here

Edit: however, we need to find a way to actually insert a record for hosting_client_user for uid1 and the first client, which get created in hostmaster.profile, rather than hardcoding $user->uid == 1 in the conditional here (another mighack special)

sfyn’s picture

OK, so, I re-implemented _hosting_get_allowed_platforms in my fix for #937490: Fix Platform access control to deal with profile > platform paradigm.

We do need some api function to return these values no? What's wrong with using _hosting_get_allowed_platforms, and changing it as the implementation changes?

Robin Millette’s picture

subbing

ergonlogic’s picture

Version: 6.x-0.4-alpha3 » 6.x-2.x-dev

Cross-referencing: #2031747: Properly control Views access, and see dev/views-access branch.

  • Commit dd7602e on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by adrian:
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • Commit 596c17b on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • Commit dd7602e on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by adrian:
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • Commit 596c17b on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • adrian committed dd7602e on dev-helmo-3.x
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • mig5 committed 596c17b on dev-helmo-3.x
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • adrian committed dd7602e on 7.x-3.x-1995506
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • mig5 committed 596c17b on 7.x-3.x-1995506
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • adrian committed dd7602e on dev-2223033
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • mig5 committed 596c17b on dev-2223033
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • adrian committed dd7602e on dev-2359571
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • mig5 committed 596c17b on dev-2359571
    fix for #725952 - always show all enabled platforms for uid 1. Only show...

  • adrian committed dd7602e on 7.x-4.x
    partial fixes on #725952 , but it turns out to be not as reliable as the...
  • mig5 committed 596c17b on 7.x-4.x
    fix for #725952 - always show all enabled platforms for uid 1. Only show...
ergonlogic’s picture

Issue summary: View changes
Status: Needs work » Closed (outdated)

The 6.x-2.x branch will go EOL along with Drupal this week. So I'm closing this issue. If it remains a confirmed issue in 7.x-3.x, feel free to re-open, or better yet, create a new issue referencing this one.

ergonlogic’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Component: User interface » API
Status: Closed (outdated) » Active

This remains an issue, and is referenced from within the code, as a TODO: http://api.aegirproject.org/api/Hosting/client%21hosting_client.module/f...

Essentially, the current implementation makes hosting_client more-or-less a dependency of core Aegir.