Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#14 | ghosts-only-delete.jpg | 116.21 KB | omega8cc |
Comments
Comment #1
omega8cc CreditAttribution: omega8cc commentedMoving 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.
Comment #2
memtkmcc CreditAttribution: memtkmcc commentedSee also side effect of this bug: #1153152: Duplicate client nodes created on site import
Comment #3
Robin Millette CreditAttribution: Robin Millette commentedsubbing
Comment #4
ShaneOnABike CreditAttribution: ShaneOnABike commentedWe 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?
Comment #5
anarcat CreditAttribution: anarcat commentedIt *is* a required field, afaik. :)
Comment #6
ShaneOnABike CreditAttribution: ShaneOnABike commentedWhoops
Comment #7
anarcat CreditAttribution: anarcat commentedI believe this is related to #1530610: provision-import doesn't import client-name, ssl and language settings from alias file.
Comment #8
anarcat CreditAttribution: anarcat commentedI wonder what would be the proper behavior here. Who should have access to a site without a owner? Maybe this should just never happen...
Comment #9
omega8cc CreditAttribution: omega8cc commentedNote 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
Comment #10
omega8cc CreditAttribution: omega8cc commentedHere is the diff between 1.x and 2.x:
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
Comment #11
omega8cc CreditAttribution: omega8cc commentedAnd 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...
Comment #12
omega8cc CreditAttribution: omega8cc commentedClosing the other duplicate #1153152: Duplicate client nodes created on site import
Comment #13
anarcat CreditAttribution: anarcat commentedThe first patch looks fine. I have pushed it to 2.x.
However, I am somewhat puzzled about the second one..
I feel there are a few things wrong here. First, why check only nodes owned by uid 1 in this conditionnal?
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, thewhile()
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:
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:
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!
Comment #14
omega8cc CreditAttribution: omega8cc commentedWe don't delete clients here. We delete only ghost nodes of
client
type, which don't exist in thehosting_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 typeclient
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 thehosting_client
table, so we do just that. Please see the screenshot for reference.Comment #15
omega8cc CreditAttribution: omega8cc commentedSo the
WHERE uid = 1
in the query narrows it to check for possible ghosts only, while the duplicateif()
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 :)Comment #16
omega8cc CreditAttribution: omega8cc commentedHmm, 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.Comment #17
omega8cc CreditAttribution: omega8cc commentedSo, as expected, when
Automatically create user accounts for new clients
is not enabled, all clients/nodes haveuid=1
, so we must check their existence/match in thehosting_client
table. Still, theWHERE uid = 1
may help to narrow the query whenAutomatically create user accounts for new clients
option is enabled. But we could just check if their matching nid exists in thehosting_client
table.Comment #18
anarcat CreditAttribution: anarcat commentedAlright, 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:
.. 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.