Hi,
I don't know how it happened, but we end up with 2 sites that didn't have a client associated to them. I created a user with the client's role and ask this user to go to the sites' page (hosting/sites). The user could see the 2 sites associated to a client whom his user is associated to (normal), and 2 more sites that don't have anything to do with him (not normal). I noticed these sites don't have any client associated to. I didn't go further with the person (the user). Maybe we could reproduce.

This behaviour is not good because the person saw other person's sites.

CommentFileSizeAuthor
#14 ghosts-only-delete.jpg116.21 KBomega8cc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

omega8cc’s picture

Project: Provision » Hostmaster (Aegir)
Version: 6.x-1.1 » 6.x-0.4-alpha3
Category: support » bug
Priority: Normal » Major

Moving to Hostmaster queue and changing to major bug.

Steps to reproduce the problem:

1. Create a client with space or dot in the name.
2. Create a site with this client as an owner.
3. Clone the site with different domain.
4. The clone task doesn't break anything.
5. The import task removes associated client - the bug is now visible.

This never happens when the client name has only /[0-9a-zA-Z]/.

Now the site is visible for all other clients in their Aegir accounts.

The same happens for deleted sites - they are also visible for clients
who were not an owners.

Probably related here bug: the autocomplete field on the site edit form
displays *both* human and machine name of the client to choose from.
It shows error when you will choose machine name, but it should never
offer this as an option here.

memtkmcc’s picture

Robin Millette’s picture

subbing

ShaneOnABike’s picture

We were having the same issue on our Aegir install. I can confirm this is fixed when assigning a client to the site.

Perhaps we can set this is a required field so that it can never be left empty?

anarcat’s picture

It *is* a required field, afaik. :)

ShaneOnABike’s picture

Whoops

anarcat’s picture

anarcat’s picture

I wonder what would be the proper behavior here. Who should have access to a site without a owner? Maybe this should just never happen...

omega8cc’s picture

Version: 6.x-0.4-alpha3 » 6.x-2.x-dev
omega8cc’s picture

Status: Active » Needs review

Here is the diff between 1.x and 2.x:

-function hosting_import_client($name, $organization = '') {
- $client = hosting_get_client_by_uname($name);
- if (!$client) {
- $client = hosting_get_client($name);
- }
- if (!$client) {
- $client = hosting_get_client_by_email($name);
- }
+function hosting_import_client($name) {
+ $client = hosting_get_client($name);

The problem is that it should use:

$client = hosting_get_client_by_uname($name);

and not:

$client = hosting_get_client($name);

This could go unnoticed when you use Client name which is the same as its machine name, but it will never work when you are trying hosting_get_client($name); and your client machine name differs from human readable name, stored in the node title.

And the patch: http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/00a44f3

omega8cc’s picture

And another patch to remove duplicate client nodes and make all sites with client relationship lost, owned by the default client:

http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/2e780fc97e5075...

omega8cc’s picture

anarcat’s picture

Status: Needs review » Needs work

The first patch looks fine. I have pushed it to 2.x.

However, I am somewhat puzzled about the second one..

+  $result = db_query("SELECT nid, uid, type FROM {node} WHERE uid = 1 AND nid != 1 AND type = '%s'", $type);
+  if (db_affected_rows($result)) {
+    while ($record = db_fetch_object($result)) {
+      if ($record->uid == 1 && $record->type == 'client' && $record->nid != 1) {
+        $exists = db_query("SELECT nid FROM {hosting_client} WHERE nid = %d", $record->nid);
+        if (!db_affected_rows($exists)) {
+          node_delete($record->nid);
+        }
+      }
+    }
+  }

I feel there are a few things wrong here. First, why check only nodes owned by uid 1 in this conditionnal?

if ($record->uid == 1 && $record->type == 'client' && $record->nid != 1) {

And even if so, why do we need to check this here if it's already in the request?

Second, It seems the first if (db_affected_rows($result)) { is redundant here. If there are no affected rows, the while() will just never be entered and that's fine, no need for the if.

Finally, we are really removing clients here! From the look of it, this is removing any client that doesn't have sites assigned to it, not only duplicate clients! Maybe I am misreading this...

The second update also has issues:

+  $found = $client_id ? $client_id : "1";

This will always assign 1 to $found, I don't see the reason for the conditionnal there. The we iterate over all clients, where we could just run this query, without a select:

UPDATE hosting_site SET client = 1 WHERE client = 0

The hook works, but it's really inefficient... The first iteration of the loop does everything and further executions are useless...

So in summary:

* make sure only duplicated clients are removed, not all clients without sites
* simplify SQL queries
* explain or remove the if uid =1 conditionnal
* remove redundant code

Thanks for the patch!

omega8cc’s picture

FileSize
116.21 KB

We don't delete clients here. We delete only ghost nodes of client type, which don't exist in the hosting_client table, so they are not really clients, they are ghost nodes only. We could probably improve this a bit to avoid double checks/queries. Note that every ghost node has always uid=1, so they are clear duplicates, because there is always a default client which is owned by uid=1, while all other real clients have different, unique uids. We could even assume that every node with type client which has uid=1 and nid other than 1 is a ghost to delete, but I think that it is more safe to cross-check client existence in the hosting_client table, so we do just that. Please see the screenshot for reference.

Ghosts

omega8cc’s picture

So the WHERE uid = 1 in the query narrows it to check for possible ghosts only, while the duplicate if() check doesn't hurt, but could be removed for clarity, sure. It is there mainly to make it more obvious what are we doing and satisfy the paranoid mode :)

omega8cc’s picture

Hmm, while thinking more about it, I think that we should test the same also without Automatically create user accounts for new clients option enabled, since it will make a difference probably.

omega8cc’s picture

So, as expected, when Automatically create user accounts for new clients is not enabled, all clients/nodes have uid=1, so we must check their existence/match in the hosting_client table. Still, the WHERE uid = 1 may help to narrow the query when Automatically create user accounts for new clients option is enabled. But we could just check if their matching nid exists in the hosting_client table.

anarcat’s picture

Status: Needs work » Fixed

Alright, I see the duplicate clients now. However, I think there's a much simpler way to clean those up. Here's an SQL request to clearly show the problem:

mysql> SELECT n.nid, n.title, hc.nid FROM node n LEFT JOIN hosting_client hc ON n.nid = hc.nid WHERE n.type = 'client';
+-----+-----------------+------+
| nid | title           | nid  |
+-----+-----------------+------+
|   1 | anarcat         |    1 |
| 127 | test            |  127 |
| 305 | testclient      |  305 |
| 313 | my test client. |  313 |
| 322 | mytestclient    | NULL |
+-----+-----------------+------+
5 rows in set (0.00 sec)

.. and here's the code to clean them up: http://drupalcode.org/project/hostmaster.git/commit/0bacae9

I think this can be now considered fixed: the dupe clients won't be created since the import is fixed, and the leftover crap is now properly removed too.

Status: Fixed » Closed (fixed)

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

  • Commit 76f1bed on 6.x-2.x, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x authored by omega8cc, committed by anarcat:
    Issue #1146014 by j0nathan - fix client import
    
    we we importing from the...
  • Commit 0bacae9 on 6.x-2.x, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by anarcat:
    #1146014 - cleanup duplicate and owner-less client nodes
    

  • Commit 76f1bed on 6.x-2.x, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x authored by omega8cc, committed by anarcat:
    Issue #1146014 by j0nathan - fix client import
    
    we we importing from the...
  • Commit 0bacae9 on 6.x-2.x, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by anarcat:
    #1146014 - cleanup duplicate and owner-less client nodes