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

[full-ci] feat: resolve public link to actual location if possible #9299

Merged
merged 4 commits into from Jul 7, 2023
Merged

[full-ci] feat: resolve public link to actual location if possible #9299

merged 4 commits into from Jul 7, 2023

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Jun 27, 2023 •
edited by grimmoc

Description

Resolve public links to their actual location if the user has proper access to the resource (either via space or share). Also fixes an issue where opening public links in an authenticated context would prevent uploading resources.

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

@JammingBen JammingBen self-assigned this Jun 27, 2023
@owncloud owncloud deleted a comment from update-docs bot Jun 27, 2023
@ownclouders
Copy link
Contributor

ownclouders commented Jun 27, 2023 •
edited

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/36914/68/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L207.png

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L207.png

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L272.png

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L272.png

@JammingBen JammingBen changed the title feat: resolve public link to actual location of possible [full-ci] feat: resolve public link to actual location of possible Jun 27, 2023
@JammingBen JammingBen changed the title [full-ci] feat: resolve public link to actual location of possible [full-ci] feat: resolve public link to actual location if possible Jun 27, 2023
@JammingBen JammingBen force-pushed the resolve-public-to-internal branch 2 times, most recently from 3141acb to 28f67a0 Compare June 28, 2023 12:30
@ownclouders
Copy link
Contributor

Results for e2e-tests oCIS-2 https://drone.owncloud.com/owncloud/web/36848/13/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-alice-2023-6-28-12-48-02.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-anonymous-2023-6-28-12-48-35.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-brian-2023-6-28-12-48-11.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-carol-2023-6-28-12-48-24.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-david-2023-6-28-12-48-15.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36848/tracing/participant-management-edith-2023-6-28-12-48-20.zip

@ownclouders
Copy link
Contributor

ownclouders commented Jun 28, 2023 •
edited

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/36975/38/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingPublicBasic-publicLinkEdit_feature-L149.png

webUISharingPublicBasic-publicLinkEdit_feature-L149.png

@ownclouders
Copy link
Contributor

ownclouders commented Jun 28, 2023 •
edited

Results for e2e-tests oCIS-1 https://drone.owncloud.com/owncloud/web/36892/12/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36892/tracing/search-for-content-of-file-alice-2023-6-29-09-26-02.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36892/tracing/search-for-content-of-file-brian-2023-6-29-09-26-07.zip

@JammingBen JammingBen mentioned this pull request Jun 28, 2023
Closed
@JammingBen JammingBen mentioned this pull request Jun 29, 2023
Closed
@JammingBen JammingBen force-pushed the resolve-public-to-internal branch 6 times, most recently from 0c09f07 to 4d1674c Compare June 29, 2023 14:27
@kulmann kulmann mentioned this pull request Jun 30, 2023
Closed
9 tasks
@JammingBen JammingBen force-pushed the resolve-public-to-internal branch 2 times, most recently from ab9fa39 to 907311a Compare July 4, 2023 06:48
@JammingBen JammingBen marked this pull request as ready for review July 4, 2023 07:21
AlexAndBear
AlexAndBear reviewed Jul 4, 2023
View reviewed changes
packages/web-app-files/src/views/spaces/DriveResolver.vue
if (internalSpace) {
const resource = await clientService.webdav.getFileInfo(internalSpace, { path })

const resourceId = resource.type !== 'folder' ? resource.parentFolderId : resource.fileId
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this a little? Why is the parent folder of the space relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, it's relevant because in case of a single file, we want to resolve its parent folder/space and mark the file in the filelist.

@AlexAndBear AlexAndBear self-requested a review July 4, 2023 08:55
@AlexAndBear
Copy link
Contributor

AlexAndBear commented Jul 4, 2023 •
edited

Bug encountered while testing

User A shares new file.txt from Personal home with User B
User B accepts the share
User A creates a public link for the same file, User B opens public link

User B sees a empty files list, while new file.txt shows in breadcrumbs
Expectation: User B should just see the Shared with me page if the share is just a single file share and the file should be marked

@AlexAndBear
Copy link
Contributor

AlexAndBear commented Jul 4, 2023 •
edited

Bug encountered
User A creates a new folder
User A creates a quicklink for new folder with share name 'Secret File Drop'
User A opens the link themselves
User A sees the drop shortly before redirecting to the actual location

Expectation:
Drop should not be visible at all

@AlexAndBear
Copy link
Contributor

E2E tests are missing, might be added in Follow UP

JammingBen
JammingBen commented Jul 6, 2023
View reviewed changes
packages/web-runtime/src/pages/resolvePrivateLink.vue Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fyi: The changes here were made in the past but got lost because they never made it to master: https://github.com/owncloud/web/pull/8310/files#diff-a4dfebeac8039a0664a3aaf5e347a693b1d02a0304fcb203e111cd92df3c907f

AlexAndBear
AlexAndBear approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

NICE!

@JammingBen
feat: resolve public link to actual location of possible
0b26b9e 
Resolve public links to their actual location if the user has proper access to the resource (either via space or share).
@JammingBen
fix: authenticated public links breaking upload
2e23b47 
Fixes an issue where opening public links in an authenticated context would prevent uploading resources.
@JammingBen
fix: private link resolving to single shared files
0dc0499
@sonarcloud
Copy link

sonarcloud bot commented Jul 7, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

63.2% 63.2% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@JammingBen JammingBen merged commit 05f5207 into master Jul 7, 2023
3 of 4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the resolve-public-to-internal branch July 7, 2023 11:11
@micbar micbar mentioned this pull request Jul 24, 2023
Closed
68 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@dschmidt dschmidt Awaiting requested review from dschmidt

Assignees

@JammingBen JammingBen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Authenticated public links break uploads [web] Show logged in users the resource location of a public link
3 participants
@JammingBen @ownclouders @AlexAndBear