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 share path when listing a specific share #1918

Closed
wants to merge 2 commits into from
Closed

fix share path when listing a specific share #1918

wants to merge 2 commits into from

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Jul 23, 2021

The file path and target of shares where missing the share jail path when a specific share was listed.

@C0rby C0rby requested a review from labkode as a code owner July 23, 2021 14:28
@C0rby C0rby marked this pull request as draft July 23, 2021 14:28
@C0rby C0rby marked this pull request as ready for review July 23, 2021 15:27
@C0rby C0rby requested a review from ishank011 July 23, 2021 15:28
@labkode
Copy link
Member

labkode commented Jul 26, 2021

@C0rby I think this one is related with #1739
I need @phil-davis @butonic to give us some help on this part as we maintain that PR as part of our fork and
I'll like to have it settled.

@ishank011
Copy link
Contributor

@C0rby the share prefix should only be prepended if the user is a recipient of the share. For share creators who want to get a share by ID, it should be skipped.

@micbar
Copy link
Member

micbar commented Aug 2, 2021

@C0rby can you update?

@C0rby
Copy link
Contributor Author

C0rby commented Aug 2, 2021

@C0rby the share prefix should only be prepended if the user is a recipient of the share. For share creators who want to get a share by ID, it should be skipped.

Yes, you're right. I'll update the code.

@ishank011
Copy link
Contributor

The thing is this method calls GetShare which doesn't return any info about whether the share was accepted by the recipient or not. And we don't call mapState here either (since we don't even have that info), so the state is always set to 0 which corresponds to ocsStateAccepted.

@C0rby
Copy link
Contributor Author

C0rby commented Aug 2, 2021

Ah, I didn't notice that.. Yeah, that makes it a bit more difficult...

@C0rby C0rby marked this pull request as draft August 2, 2021 13:58
@C0rby C0rby force-pushed the fix-share-path branch 2 times, most recently from 8869b1e to f43f7d8 Compare August 3, 2021 13:54
@C0rby C0rby marked this pull request as ready for review August 3, 2021 14:55
@C0rby
Copy link
Contributor Author

C0rby commented Aug 3, 2021

Now I added an extra request to get the received share in the case if the current user is not the owner. The received share contains the state information. It is one additional request but shouldn't be too costly IMO.

@C0rby C0rby requested review from refs and butonic September 3, 2021 08:04
@C0rby C0rby self-assigned this Sep 3, 2021
@refs
Copy link
Member

refs commented Sep 3, 2021

Now I added an extra request to get the received share in the case if the current user is not the owner. The received share contains the state information. It is one additional request but shouldn't be too costly IMO.

Exactly. I did the same in a previous PR. 👍

labkode
labkode reviewed Sep 8, 2021
View reviewed changes
internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go Outdated

if receivedShare != nil && receivedShare.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
// Needed because received shares can be jailed in a folder in the users home
share.FileTarget = path.Join(h.sharePrefix, path.Base(info.Path))
Copy link
Member

Choose a reason for hiding this comment

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

This breaks global urls for accessing shares as you're forcing a basePath for FileTarget and Path.
It needs #1739 to be merged first and apply the same logic if I didn't miss anything.

Copy link
Member

Choose a reason for hiding this comment

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

@C0rby I just merged afa06f7, can you update this part with the same logic. If you're in the mood, it would make sense to refactor that switch logic into a function

@C0rby
Copy link
Contributor Author

C0rby commented Sep 13, 2021

AFAICT this PR is no longer necessary since the changes in addFileInfo already set the correct FileTarget.

@C0rby C0rby closed this Sep 13, 2021
@C0rby C0rby deleted the fix-share-path branch September 13, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@labkode labkode labkode left review comments

@ishank011 ishank011 Awaiting requested review from ishank011

@refs refs Awaiting requested review from refs

@butonic butonic Awaiting requested review from butonic

Assignees

@C0rby C0rby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@C0rby @labkode @ishank011 @micbar @refs