KeeShare Consolidated Issues

[droidmonkey]

Listing of issues found with KeeShare that need to be fixed prior to final release of 2.4.0:

• Nested KeeShare settings should not apply to imported sub-folders (ie, do not recursively apply sharing; do not allow shared folders to define share settings)

• There is no way to manually refresh a share

• When viewing a share, place a persistent banner above the entry list

• There is a floating scrollbar when viewing group settings
  

• Database settings for KeeShare are impossible to read
  

• Add button in group KeeShare settings to easily clear the form data

[droidmonkey]

QuaZip error messages are confusingly worded:

keepassxc/src/keeshare/ShareObserver.cpp

Lines 658 to 693 in 8bfc539

	{
	QuaZipFile file(&zip);
	const auto signatureOpened = file.open(QIODevice::WriteOnly, QuaZipNewInfo(KeeShare_Signature));
	if (!signatureOpened) {
	::qWarning("Embedding signature failed: %d", zip.getZipError());
	return {reference.path, Result::Error, tr("Could not embed signature (%1)").arg(file.getZipError())};
	}
	QTextStream stream(&file);
	KeeShareSettings::Sign sign;
	auto sshKey = own.key.sshKey();
	sshKey.openKey(QString());
	const Signature signer;
	sign.signature = signer.create(bytes, sshKey);
	sign.certificate = own.certificate;
	stream << KeeShareSettings::Sign::serialize(sign);
	stream.flush();
	if (file.getZipError() != ZIP_OK) {
	::qWarning("Embedding signature failed: %d", zip.getZipError());
	return {reference.path, Result::Error, tr("Could not embed signature (%1)").arg(file.getZipError())};
	}
	file.close();
	}
	{
	QuaZipFile file(&zip);
	const auto dbOpened = file.open(QIODevice::WriteOnly, QuaZipNewInfo(KeeShare_Container));
	if (!dbOpened) {
	::qWarning("Embedding database failed: %d", zip.getZipError());
	return {reference.path, Result::Error, tr("Could not embed database (%1)").arg(file.getZipError())};
	}
	if (file.getZipError() != ZIP_OK) {
	::qWarning("Embedding database failed: %d", zip.getZipError());
	return {reference.path, Result::Error, tr("Could not embed database (%1)").arg(file.getZipError())};
	}
	file.write(bytes);
	file.close();
	}

[mstarke]

The recycle-bin duplication is a problem of the merge algorithm and is nothing directly related to KeeShare. It is however a side effect. My suggestion would be to enhance the merge algorithm to handle recycle bin duplications in a better way

[ckieschnick]

@droidmonkey did you already make some progress on this ticket? I've got some time to spare this week, so I may be able to help with some issues.
Update: I'm going to start to implement some fixes.

[droidmonkey]

Besides the floating scrollbar, I have not worked on this one as I have been handling other matters leading up to the release. I also fixed an infinite loop caused by a non-single-shot timer. Please see https://github.com/keepassxreboot/keepassxc/pull/2731/files#diff-aa39f325618c1eea52459fe4757344a9R126

[ckieschnick]

I've got the first small fixes ready (clear button and better error messages added as two separate pull requests for easier review). For the next issues we need to discuss some points.

• What do you mean with "imported subfolders"? Why do you want to prevent to share the keys with others (most simple way to circumvent the restriction is using copy or references to a unrelated shared folder).
• Do you mean "force import" or "force export" by manually refreshing a share or possibly both?
• Viewing a share means, that we open a share database (without signature)? To allow this functionality, we need to inject some sort of flag into the database on write (but I would not force the read to require this flag).
• I could not reproduce the issue neither on Win10 nor on Debian (Virtual Box) with Qt 5.11. Both elide the long labels in the middle like configured.

[droidmonkey]

• What do you mean with "imported subfolders"?

This is not about sharing the keys, it is about nested sharing settings. There is a slightly larger problem with sharing in general in that we are using absolute directories which does not scale when moving across platforms or even users. You can also setup a recursive share where two shares are pointing at each other.

• Do you mean "force import" or "force export" by manually refreshing a share or possibly both?

I mean both. However it appears that the import/export now occurs after pressing OK from the "Edit Group" dialog. This may be fixed already (I think the issue was the broken SIGNAL/SLOT that was fixed in the beta1).

• Viewing a share means, that we open a share database (without signature)?

What I meant by "viewing a share" was when you are displaying a shared database's entries in the entry view. Basically we only have the little globe icon in the group list to indicate the status of the share. We should also place a persistent banner when viewing the contents of that share to indicate it's status. This would be in addition to / similar to the search results banner.

• I could not reproduce the issue neither on Win10 nor on Debian

It appears that you can resize the columns now, that is good. However the columns are not initially sized such that they contain the text that is in them.

[ckieschnick]

It appears that you can resize the columns now, that is good. However the columns are not initially sized such that they contain the text that is in them.

I'm not sure if it provide any improvement to resize the columns initially since the size is depending on the translation and paths. The current setting with resize as QHeaderView::Interactive allows the user to expand the columns as needed.

This may be fixed already (I think the issue was the broken SIGNAL/SLOT that was fixed in the beta1).

Sounds like the force button is not needed - when autosave is disabled, I think its better to not allow the user to force a save since this would allow to export changed entries which will be lost when the changes to the database are discarded.

This is not about sharing the keys, it is about nested sharing settings.

Currently I don't see any problem with nested groups with sharing enabled (even if we reference the same container). It certainly is a unwanted scenario when a user references the same container within a database multiple times, but due to the UUIDs there should be no problem with updates. A problem arises at the point when one wants to share structure (in this case we need to remove all reference settings from the shared groups since we do not want to give another any indication that we share something with somebody else).

[droidmonkey]

I am splitting hairs a little bit, I do not think the nested share settings needs to be addressed at this time.

[ckieschnick]

My previous post is not completely correct - referencing the same container multiple times from within the database does make a difference in an export scenario. In this case, the last group to export will overwrite all changes of others before which may be not that good considering that other instances may listen to the file changes (i.e. synchronizing using dropbox or similar). We can add a small check if the reference is already used within the database to notify the user about the conflict. I'm not sure, if this solution will prevent all cases - especially considering mounts and network files systems which may lead to referencing the same resource with multiple paths. But I don't think that this corner case is really something the application can solve.
I suggest adding the message when we detect, that a share container is used multiple times. Do we want to prevent exports in this case?

[droidmonkey]

Yes we want to avoid overwriting data, especially if it would be "unintentional"

[ckieschnick]

I implemented a naive check to prevent the overwrite issues. The ticket contains additional information and points we may need to discuss to determine if the implementation is sufficient.