Some of your link shares have been removed

[sciurius]

Issue #15318 addresses this but was quickly closed without a satisfactory solution.

I, and several other users, frequently get messages "Some of your link shares have been removed" without an indication which shares. Personally I have a single share and it is still there so I think this message is erroneous and irritating.

[tessus]

/ref https://help.nextcloud.com/t/some-of-your-link-shares-have-been-removed-15-0-8/53522

Is there a log file that shows which links were removed? Nothing was in the nextcloud.log, but I don’t know, if the process just doesn’t log anything or if nothing has been removed (and the message was thus emitted in error).

In any case, you can’t just notify a user that something has been removed without telling anyone what it was.

[tessus]

To put it in a nutshell: Which links were removed?

[rockmn]

I have this issue on 16.0.1. We have link that we have put into manuals so it would be nice to know what the problems is. I just a upgrade from owncloud9>ownclous10>nextcloud>12.nextcloud13>nextcloud>14> nextcloud 15>nextcloud 16.

[stratege1401]

yes, put in an other prospective, 1789548254 files for 11.7 To and 85 users !!!

I am thinking taking a vacation to find out witch files, but my boss said no to my request ! :)

[garyhowell]

Hello. I was also concerned about this issue so I did some digging in the codebase.
In short, the notification is send to admins even if no shares were deleted.

To confirm, look at https://YOUR_SITE/index.php/settings/admin/logging with an admin account (see screen shot attached)

When the process starts, we get "\OC\Repair::startProgress: Starting ... Remove potentially over exposing share links (0)". Note the 0, which seems to indicate there are no dodgy shares.

then, we get
\OC\Repair::info: Repair info: Sending notifications to admins and affected users
\OC\Repair::info: Repair info: Removed potentially over exposing link shares

Looking at the code:

	private function repair(IOutput $output): void {
		$total = $this->getTotal();
		$output->startProgress($total);

		$shareCursor = $this->getShares();
		while($data = $shareCursor->fetch()) {
			$this->processShare($data);
			$output->advance();
		}
		$output->finishProgress();
		$shareCursor->closeCursor();

		// Notifiy all admins
		$adminGroup = $this->groupManager->get('admin');
		$adminUsers = $adminGroup->getUsers();
		foreach ($adminUsers as $user) {
			$this->addToNotify($user->getUID());
		}

		$output->info('Sending notifications to admins and affected users');
		$this->sendNotification();
	}

The getTotal() function seems to use the same SQL query to get the total number of dodgy shares but it is only used for reporting purposes. The repair function then continues to delete 'zero' shares and reports that it has deleted any 'potential' dodgy ones.

The following could probably resolve this poor reporting by only deleting/reporting dodgy shares if the total was more than 0, so

	private function repair(IOutput $output): void {
		$total = $this->getTotal();

		if ($total > 0) {   // WRAP THE DELETION/REPORTING PROCESS
			$output->startProgress($total);

			$shareCursor = $this->getShares();
			while($data = $shareCursor->fetch()) {
				$this->processShare($data);
				$output->advance();
			}
			$output->finishProgress();
			$shareCursor->closeCursor();

			// Notifiy all admins
			$adminGroup = $this->groupManager->get('admin');
			$adminUsers = $adminGroup->getUsers();
			foreach ($adminUsers as $user) {
				$this->addToNotify($user->getUID());
			}

			$output->info('Sending notifications to admins and affected users');
			$this->sendNotification();
		}
	}

I don't have the infrastructure to test this and make a pull request, but perhaps the original developer could do so. It might then prevent this false reporting, albeit not adding the details of the shares that are actually deleted.

[kesselb]

Thank you @garyhowell for digging 👍

[tessus]

@garyhowell

Note the 0, which seems to indicate there are no dodgy shares.

It could also mean that the return code was 0.
It also does not tell us what the names of the removed links were. Without a dev explaining this, nobody will ever know which links were removed, but we are all still waiting to get any info from any dev.

[kesselb]

It could also mean that the return code was 0.

No ;) 0 is the number of removed shares.

[tessus]

@kesselb the number of removed shares is known when the process starts? Usually you know that info only after the process had been run.
anyway, so now to the most important information of this entire issue:

Which links were removed?

[kesselb]

Which links were removed?

@rullzer 🏓

the number of removed shares is known when the process starts? Usually you know that info only after the process had been run.

server/lib/private/Repair/RemoveLinkShares.php

Lines 101 to 131 in 7f689af

	private function getTotal(): int {
	$subSubQuery = $this->connection->getQueryBuilder();
	$subSubQuery->select('*')
	->from('share')
	->where($subSubQuery->expr()->isNotNull('parent'))
	->andWhere($subSubQuery->expr()->eq('share_type', $subSubQuery->expr()->literal(3, IQueryBuilder::PARAM_INT)));
	$subQuery = $this->connection->getQueryBuilder();
	$subQuery->select('s1.id')
	->from($subQuery->createFunction('(' . $subSubQuery->getSQL() . ')'), 's1')
	->join(
	's1', 'share', 's2',
	$subQuery->expr()->eq('s1.parent', 's2.id')
	)
	->where($subQuery->expr()->orX(
	$subQuery->expr()->eq('s2.share_type', $subQuery->expr()->literal(1, IQueryBuilder::PARAM_INT)),
	$subQuery->expr()->eq('s2.share_type', $subQuery->expr()->literal(2, IQueryBuilder::PARAM_INT))
	))
	->andWhere($subQuery->expr()->eq('s1.item_source', 's2.item_source'));
	$query = $this->connection->getQueryBuilder();
	$query->select($query->func()->count('*', 'total'))
	->from('share')
	->where($query->expr()->in('id', $query->createFunction('(' . $subQuery->getSQL() . ')')));
	$result = $query->execute();
	$data = $result->fetch();
	$result->closeCursor();
	return (int) $data['total'];
	}

[garyhowell]

@tessus Looking at the SQL the links appear to be external 'shares' that have been shared by someone else on the 'master' share list (i.e. by someone the admin has given permission to allow a share to be shared).

On the face of it looks like the shares being deleted are essentially shares used by third parties and not the 'master' share holder. Operationally this seems reasonable to prevent third parties accessing stuff they should not.

My concern (as yours I'm sure) was that files or file links (as in symbolic links) were being deleted and this does not appear to be the case.

If someone can point me to the database documentation for the 'shares' table I might be able to work through it to determine if it's possible to come up with a simply 'list' of shares affected. However, even it is possible to do this, the performance overhead of doing so might be restrictive on a large NC installation causing the maintenance jobs to be running for a longer period.

[tessus]

@garyhowell

My concern (as yours I'm sure) was that files or file links (as in symbolic links) were being deleted and this does not appear to be the case.

Right, in either case removed links should be logged and presented to the end user. The fact that the message was emitted in error is an entirely different issue.

However, even it is possible to do this, the performance overhead of doing so might be restrictive on a large NC installation causing the maintenance jobs to be running for a longer period.

No need to create an additional statement. The statements run by this script already give you all the info you need. The info is just not logged.

Btw, the performance is already at risk by running this statement in the first place. Actually this repair step runs the statement twice. Once for counting and a second time to process the list.
Something is a bit wonky, because if 0 is returned the output should have been No need to remove link shares..
Also, I didn't see any logging in case a share is removed. I guess there's room for improvement. ;-)