Avoid duplication of public quick links on the same resource

[JammingBen]

Description

Enhancement: Extend public quick link feature

With this PR, public quick links created via the quick links icon in the file list will be created only once. We do this by adding isQuickLink to the share's attributes. See https://github.com/owncloud/enterprise/issues/4730#issuecomment-911610309 for the expected behavior.

Extends #39130

Related Issue

• Fixes https://github.com/owncloud/enterprise/issues/4730

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[phil-davis]

After checking out this branch I thought that I would have to do something to get the databse migration to run. But:

$ php occ upgrade
ownCloud is already latest version

And when I click the "quick link" button in the UI, I get "The public link could not be created. Please contact the administrator for help."

Is there something else that needs to happen so that the new migration in files_sharing gets run?

[phil-davis]

"This branch is 5 commits ahead, 9 commits behind master."
Next time you change something, it would be good to take the chance to rebase/squash.

[AlexAndBear]

@phil-davis we had running it via php occ migrations:migrate files_sharing I can imagine, that the migration only gets affected by php occ upgrade if the version of the app gets bumped?

[phil-davis]

I can imagine, that the migration only gets affected by php occ upgrade if the version of the app gets bumped?

That is probably true. Should the version be bumped now, in this PR?

[AlexAndBear]

@phil-davis I thought versions will be bumped if a new release of the core will be done ? I don't know the process in detail maybe @micbar knows?

[micbar]

Oh! Migrations of the share table?

This is unexpected for me.

[AlexAndBear]

@phil-davis I bumped the version string, as recognized, that it was bumped before as migrations came in 👍

[micbar]

Needs product discussion please if we want a migration of the share table.
@pmaier1 @hodyroff @cdamken

[phil-davis]

Tested and works:
$ php occ upgrade does the migration.

Then:

• create a public quick link, and check the link in the clipboard
• logout and login again
• click the public quick link icon again
• check the link in the clipboard - it is the same
• check the Sharing, Public Links panel - there is only one "Public quick link"

This looks good. It just needs a decision about the implementation of how to remember which link is the "Public quick link".

[jvillafanez]

I'm not sure if we need this... I mean, you can have multiple links with different permissions, so for me it's fine to prevent creating a link if it has the same permissions of another link (we might need to add some additional checks such as having the same owner).
In this context, a quick link doesn't need to behave differently. You create a link with a fixed set of permissions, and you need to check if there is a link already created with the same permissions.

[JammingBen]

We had the same initial idea. A Public quick link is created with the following props:

• Read-only permissions
• No password protection
• Expiry based on the setting, default none

Hence we would need to search for a link matching all these values. Which is hardly possible if the expiry gets set automatically. Also what happens if you've created a public link via quick action and then change it to read-write? In this case, a quick action would create another public link.

So we decided this might be the easiest way to achieve this, even though it includes a migration.

[phil-davis]

There might be multiple public links with read-only permission. They might have different expiry dates. The user might have "manually/purposely" created multiple read-only public links, even all with the same expiry date (or no expiry date) so that they can give individual public links to different people.

The rule could be that if there is any read-only public link(s) already existing, then clicking the "quick link" button will copy the "first" one found to the clipboard.

But the user probably would like the behavior to be:

1. if no read-only public links exist on first click, then create one
2. if a read-only public link(s) already exists on first click, then copy that to the clipboard
3. if the "quick link" icon has been clicked already in the past, then when I click it again it should return the same public link that it returned the first time.

(1) and (2) are easy.

(3) is easy of there is only 1 read-only public link.

(3) is harder if someone creates a read-only "quick link" and then goes into the full Public Links panel and creates other read-only public links, and changes all their names etc. It might be more difficult to know which was the "first" one created by the "quick link" icon. Maybe there is already some primary-key or... sorting in the table that can be used to reliably choose the "first" one, regardless of if the name, expiry date... have been changed later.

[jvillafanez]

Which is hardly possible if the expiry gets set automatically.

I expect the expiration for the quick link will be set, for example to the 23th Sept at 00:00 (or 23:59), so if you click again on the same date, the expiration should be the same.
If you generate a quick link today and another quick link 3 days after, the links will be different because the expiration date is different (of course, assuming the links are created with an expiration date). For me, this is fine because the fresh link will expire later, which is probably what the user wants.

Also what happens if you've created a public link via quick action and then change it to read-write? In this case, a quick action would create another public link.

The link is different due to different permissions, so for me it's ok.

The key point for me is that the quick link action just creates a new link with specific data. That's all it needs to do. I expect the DB and / or the share manager to take care of duplicates in order to return a specific error.
If the link isn't duplicated, it's created and shown using the normal flow, otherwise a "the link already exists" error message should be shown.

Maybe what it's lacking is a way to tell the user that the quick links will be generated in a particular way, such as read-only and expiring in 15 days (or whatever is configured)

[AlexAndBear]

For me, this is fine because the fresh link will expire later, which is probably what the user wants.

Unfortunately, this is not fine for the customer, as spoken with @pmaier1

If the link isn't duplicated, it's created and shown using the normal flow, otherwise a "the link already exists" error message should be shown.

In which case should an error message be shown? This is not user friendly in any way

We should really implement an as easy as possible solution, like this PR does

[phil-davis]

Yes, for the customer, and even for a user like me, I would like that if I:

• create a "quick link" today
• paste that link from the clipboard into an email, or chat session or whatever with the person that I want to know and use the link
• forget what was the link URL
• login tomorrow and click the "quick link" icon
• my clipboard should always have the same link URL that it had when I first created a "quick link" (as long as that public link still exists, and even if someone has gone into the Public Links panel and edited the name, or expiry date, or added more public links)
• and I can resend that same public link URL again to the person that "lost" it

This PR achieves the above. There might be other trickier ways to achieve the requirement without a database table migration - someone (tm) has to decide if it is worth trying to implement a "trickier way" that really covers edge cases...

[JammingBen]

We decided to use the attributes property that each share has to avoid any migration. PR updated.

[phil-davis]

Works for me. I checked cases like:

• logout and login again. The original quick-link is remembered
• create an ordinary read-only public link, then a "quick link". The one that is a "quick link" is correctly remembered.
• share to another user. The other user creates a quick link of the received share. It works

the db migration has been removed

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

85.2% Coverage
0.0% Duplication