Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enabling "invite people" for password-protected folder/file #9931

Merged
merged 5 commits into from Nov 16, 2023

Conversation

jacob-nv
Copy link
Contributor

Description

Enabling users to switch link permission to invited only for password protected folders/files.
While switching to invited only, the password and expiration date is removed

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@jacob-nv jacob-nv self-assigned this Nov 10, 2023
Copy link

update-docs bot commented Nov 10, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@@ -639,7 +646,7 @@ export default defineComponent({
this.hasPublicLinkEditing,
this.hasPublicLinkContribute,
this.hasPublicLinkAliasSupport,
!!link.password,
false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this always work? for example if you have password enforced for editable links only and switch permissions from existing link that only has view permissions to editable permissions, will the password required still pop up as it did before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janackermann Could you please navigate me more in detail on how to recreate this use case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, please remove all the other env settings and set

SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD
SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD

To true

https://owncloud.dev/services/sharing/configuration/

This will let you create a standard readonly link but as soon you want to change to edit permissions the popup opens and asks you to set a password

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got popup asking to setup password in case of "can upload", "can edit" and "secret file drop". Still able to change to "invited people" which will drop password. After dropping password, I'm only able to set to "can view" without popup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read the code correctly you can remove the hasPassword param from the LinkShareRoles.list and LinkShareRoles.filterByBitmask functions entirely, right? If that's correct then please do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kulmann
I was attempting to remove it from the start, but wasn't successful, quickest fix was setting false for it in specific instances.
If I would to test again removing it completely, how would this condition would be resolved?
...(hasAliasLinks && !hasPassword ? [linkRoleInternalFile, linkRoleInternalFolder] : []),
packages/web-client/src/helpers/share/role.ts LinkShareRoles.list

Don't have enough understanding of what and how alias link works and what it could affect

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] how would this condition would be resolved? ...(hasAliasLinks && !hasPassword ? [linkRoleInternalFile, linkRoleInternalFolder] : []), packages/web-client/src/helpers/share/role.ts LinkShareRoles.list

Alias links are what we now call only invited people or internal links in the ui. They don't hold permissions on their own, they just serve as a pointer to a file or folder (but in the same data structure like a public link). Meaning: the one who clicks on such a link needs something else which grants permission to the file/folder, i.e. a share or a space membership with certain privileges.

That being said, a password doesn't make sense at all for an alias link, because the receiver of the link needs to hold permissions to access the linked file/folder anyway. Don't know why we would ever hide the alias link role when a password has been set before. Removing the && !hasPassword in fact seems to be the underlying bugfix, that you addressed by always setting it to false. Just remove the && !hasPassword condition entirely please. And with that there are no other references to the hasPassword param anymore => you can get rid of the param.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me now. Please add a bugfix scoped changelog item :-)

Copy link

sonarcloud bot commented Nov 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

70.6% 70.6% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit 6314825 into owncloud:master Nov 16, 2023
4 checks passed
AlexAndBear pushed a commit that referenced this pull request Dec 13, 2023
* enabling invite people for password-protected folder/files

* removed hasPassword from LinkShareRoles

* added invite people for password-protected folder changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling the password requirement for the link breaks the "invite people via link" feature
4 participants