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.

Sign up for GitHub

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

Jump to bottom

Fix public links actions #1951

Merged
merged 4 commits into from
Oct 15, 2019
Merged

Fix public links actions #1951

merged 4 commits into from
Oct 15, 2019

Conversation

LukasHirt
Copy link
Collaborator

Description

Disable quota for public links and pass header for upload in password-protected links.

Related Issue

How Has This Been Tested?

  • test environment: Manually
  1. Rename file in folder shared with public link
  2. Delete file in folder shared with public link
  3. Upload file into folder shared with public link

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:

@LukasHirt LukasHirt added Status:Needs-Review Needs review from a maintainer bug-solved labels Sep 11, 2019
@LukasHirt LukasHirt self-assigned this Sep 11, 2019
@PVince81
Copy link
Contributor

PVince81 commented Sep 16, 2019 •
edited
Loading

Tests

Link without password

Contributor (view, download, upload - no delete nor rename)

  • Create folder
  • Upload file
  • Download file
  • Cannot rename folder/file => ok as not permitted
  • Cannot Move into / outside of folder
  • Cannot delete folder/file

Editor (view, download, edit, delete, upload)

  • Create folder
  • Upload file
  • Download file
  • Rename folder/file
  • Move into / outside of folder 🚫 dragging not possible ?! not implemented yet
  • Delete folder/file

Link with password

Editor (view, download, edit, delete, upload)

  • Create folder
  • Upload file
  • Download file
  • Rename folder/file
  • Move into / outside of folder 🚫 dragging not possible ?! not implemented yet
  • Delete folder/file

@individual-it
Copy link
Member

deleting a file with password protected link does not work

error: /yLLig11l5lUQLZs/block-aligned3.txt not deleted: Error: No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No public access to this resource., No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured, No 'Authorization: Bearer' header found. Either the client didn't send one, or the server is mis-configured, No 'Authorization: Basic' header found. Either the client didn't send one, or the server is misconfigured

@individual-it
Copy link
Member

@PVince81 try as "Collaborator" I think there is a permission/description mismatch see #1979

@LukasHirt
Copy link
Collaborator Author

LukasHirt commented Sep 26, 2019 •
edited
Loading

deleting a file with password protected link does not work

Seems we are sending password as a wrong argument.

https://github.com/owncloud/owncloud-sdk/blob/36e131b085b1a2f4ecb6449a88dc5a471ebd4bcf/src/publicLinkAccess.js#L142

https://github.com/owncloud/phoenix/blob/b936e6378830b291afb704f9b05f91c4f30afd81/apps/files/src/store/actions.js#L448

@PVince81 PVince81 assigned PVince81 and unassigned LukasHirt Sep 27, 2019
@PVince81
Copy link
Contributor

@PVince81 try as "Collaborator" I think there is a permission/description mismatch see #1979

Yes, correct. I've adjusted my test results now.

@PVince81
Copy link
Contributor

I've rebased and it seems it's more broken than before, possibly because of those JS issues.
Test report here #1951 (comment)

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from e33690a to fea4c96 Compare September 30, 2019 08:23
@PVince81
Copy link
Contributor

seems currentFolder is null which is likely the reason the perms are not read correctly.

I'll investigate...

@PVince81
Copy link
Contributor

Fixed currentFolder issue and download issue.

Test report updated.

Next up: the delete issue with password

@PVince81
Copy link
Contributor

Everything fixed.

Now ideally we should also have the acceptance tests in this PR.

@LukasHirt @DeepDiver1975 please review the code changes

@PVince81
Copy link
Contributor

I'm looking into acceptance tests and found another issue with deletion of folders

@PVince81 PVince81 mentioned this pull request Sep 30, 2019
Closed
LukasHirt
LukasHirt commented Sep 30, 2019
View reviewed changes
apps/files/src/components/FilesAppBar.vue Outdated
<oc-icon name="ready" v-show="linkCopied" />
<oc-icon id="files-permalink-copy" name="link" v-clipboard="() => currentFolder.privateLink"
<oc-icon id="files-permalink-copy" name="link" v-clipboard="() => currentFolder && currentFolder.privateLink"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the check for current folder here necessary when we check for it already in the parent?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this solves the JS error. I guess the vue was still rendering or evaluating this piece while the current folder was not loaded yet

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, got your point. I didn't check

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@PVince81
Copy link
Contributor

I wanted to try adding tests but realized that I need a logout step, so I raised another PR for that first: #2100

@PVince81 PVince81 mentioned this pull request Sep 30, 2019
Merged
9 tasks
@PVince81
Copy link
Contributor

PVince81 commented Oct 1, 2019

as discussed, no need for logout step as we can create the link using the API.

I tried just that locally and now bumped into #1882 when running the tests

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 7c89d66 to c3a9edd Compare October 1, 2019 10:03
@PVince81 PVince81 mentioned this pull request Oct 1, 2019
Merged
9 tasks
@PVince81
Copy link
Contributor

PVince81 commented Oct 1, 2019

I've added one test for public link. This should be the base work for adding more.

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from c3a9edd to 27611d3 Compare October 1, 2019 15:02
@PVince81
Copy link
Contributor

PVince81 commented Oct 1, 2019

@individual-it please review the tests

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 27611d3 to 195d9b1 Compare October 1, 2019 15:14
@PVince81
Copy link
Contributor

PVince81 commented Oct 7, 2019

fixed failing test, it was due to a collision of assumptions when rebasing this PR

@ownclouders
Copy link
Contributor

💥 Acceptance tests XGAPortrait failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5543/

20191007-110018-392.png

@PVince81
Copy link
Contributor

PVince81 commented Oct 8, 2019

@individual-it please review again. Can we ignore the XGA failure ?

individual-it
individual-it requested changes Oct 8, 2019
View reviewed changes
Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

Something is wrong in also in the trasbin suite

15 scenarios (8 ambiguous, 7 passed)
167 steps (8 ambiguous, 64 skipped, 95 passed)
7m19.716s

tests/acceptance/stepDefinitions/filesContext.js Outdated Show resolved Hide resolved
@PVince81
Copy link
Contributor

PVince81 commented Oct 8, 2019

ah, probably a conflict resolution fail

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 9b64e28 to 5e7a8f4 Compare October 8, 2019 09:57
@PVince81
Copy link
Contributor

PVince81 commented Oct 8, 2019

solved duplicate step definition, it was a conflict resolution fail.

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 5e7a8f4 to 2f7bccc Compare October 10, 2019 12:58
@PVince81
Copy link
Contributor

Conflict resolved

individual-it
individual-it approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

just minor comments

@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 2f7bccc to 25c2d90 Compare October 11, 2019 12:36
@PVince81
Copy link
Contributor

tests passed but unpublished. => restarted build

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIFiles failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5678/

20191014-071435-071.png
20191014-071501-657.png

@PVince81
Copy link
Contributor

Ran the failing tests locally and they work fine.

Attempting another rebuild now...

Lukas Hirt and others added 3 commits October 14, 2019 13:18
Disable quota for public links and pass header for password protected…
08d3fbc 
… links upload
Fix public link page file download
a7f87bd 
Headers could not be extended when null, so we set them to an empty hash
array here.
Fix deletion in public link page
5884852
@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 25c2d90 to 95991a2 Compare October 14, 2019 11:25
Add public link tests with delete, create and password
bc984f1 
Added isActionDisabled method for checking file action button state.
Tests for deleting in public link page
Test that asserts that the delete action is disabled in a public
link share with only read permissions.
Test public link access with password.
Test public link delete when link password is set.
Test for public link upload.
@PVince81 PVince81 force-pushed the bugfix/public-link-perms branch from 95991a2 to bc984f1 Compare October 14, 2019 15:08
@PVince81
Copy link
Contributor

Resolved once again an ambiguous test which came from master through rebasing.

Hoping for a full pass now and no more conflicts...

@ownclouders
Copy link
Contributor

💥 Acceptance tests Trashbin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5705/

20191014-153338-445.png
20191014-153338-826.png
20191014-153411-182.png
20191014-153411-352.png
20191014-153509-737.png
20191014-153541-336.png
20191014-153639-205.png
20191014-153710-271.png
20191014-153730-804.png
20191014-153731-037.png
20191014-153751-599.png
20191014-153751-925.png
20191014-153812-580.png
20191014-153812-834.png
20191014-153833-443.png
20191014-153833-694.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5705/

20191014-153544-325.png
20191014-153636-824.png
20191014-153656-581.png
20191014-153755-220.png
20191014-153755-423.png
20191014-153818-039.png
20191014-153818-258.png
20191014-153840-492.png
20191014-153840-691.png
20191014-153902-789.png
20191014-153903-041.png
20191014-154144-154.png
20191014-154207-046.png
20191014-154239-141.png
20191014-154311-379.png
20191014-154332-333.png
20191014-154352-904.png
20191014-154413-588.png
20191014-154443-447.png
20191014-154503-499.png
20191014-154533-957.png
20191014-154553-040.png
20191014-154612-185.png
20191014-154641-440.png
20191014-154701-504.png
20191014-154722-399.png
20191014-154743-009.png
20191014-154833-407.png
20191014-154857-830.png
20191014-154930-504.png
20191014-154954-140.png
20191014-155020-476.png
20191014-155046-299.png
20191014-155141-252.png
20191014-155215-748.png
20191014-155239-149.png
20191014-155239-374.png
20191014-155303-453.png
20191014-155303-667.png

@PVince81 PVince81 mentioned this pull request Oct 14, 2019
Merged
22 tasks
@PVince81
Copy link
Contributor

more failures :'(

@PVince81
Copy link
Contributor

both pass locally... trying my luck again...

I hope it's not anything I changed that is now triggering these random failures ?

@PVince81 PVince81 merged commit 65dc071 into master Oct 15, 2019
@PVince81 PVince81 deleted the bugfix/public-link-perms branch October 15, 2019 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@individual-it individual-it individual-it approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@Julian1998 Julian1998 Awaiting requested review from Julian1998

Assignees

@PVince81 PVince81

Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

file operations are not possible with public links
4 participants
@LukasHirt @PVince81 @individual-it @ownclouders