A user can see sites that don't have a client associated.

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.

See also side effect of this bug: #1153152: Duplicate client nodes created on site import

subbing

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?

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

Whoops

I believe this is related to #1530610: provision-import doesn't import client-name, ssl and language settings from alias file.

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...

Note that this issue has been fixed in 1.x and now 2.x is affected only. Not sure about #1530610: provision-import doesn't import client-name, ssl and language settings from alias file, however.

Previous related issues:

#1200066: hosting_get_client uses client title instead of the (unique) internal name
#1223506: cloning a site looses client site ownership

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

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...

Closing the other duplicate #1153152: Duplicate client nodes created on site import

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!

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.

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 :)

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.

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.

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.