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 apps to work in public shares. #2143

Merged
merged 9 commits into from
Oct 15, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Oct 7, 2021

I extended the scope checking to make it possible to use Apps (and the App Provider) in public shares.
I don't quite like this solution but we also haven't found a better alternative yet. Public share scopes work as long as the requests are path based but that is not always the case.

Maybe we need to rethink this scope implementation. It feels a bit to hacky and unflexible.

This solution is by far not pretty. But this is the smallest change I
could find to support apps in public shares without completely rewriting
the scopes code.
If we want to use any app in a public share the app needs an
authenticated context to access the files in the share. This is working
fine since we can authenticate using the share token. But the resulting
JWT contains a scope refering to the shared resource. That means for a
folder share it contains the resource ID of the shared folder. But if
the app wants to access a specific file in the share it will request
that file by ID. Because of that the old way of the public scope checker
doesn't work because it would compare or requested ID with the ID of
the shared folder and this would fail.
For this reason I added two additional Stat requests so that I
can check if the requested file is inside the shared folder.

This is the reva part of the required changes. The ocis part can be found in this PR: owncloud/ocis#2536

@update-docs
Copy link

update-docs bot commented Oct 7, 2021

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.

@C0rby C0rby marked this pull request as ready for review October 7, 2021 12:52
@C0rby C0rby requested a review from labkode as a code owner October 7, 2021 12:52
@C0rby C0rby self-assigned this Oct 7, 2021
@C0rby C0rby requested a review from wkloucek October 7, 2021 12:52
@C0rby C0rby marked this pull request as draft October 8, 2021 11:07
@C0rby C0rby marked this pull request as ready for review October 11, 2021 07:38
wkloucek
wkloucek previously approved these changes Oct 12, 2021
Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

Confirmed, that opening a file with the CS3org WOPI server works from a public link.

@ishank011
Copy link
Contributor

@C0rby I agree about the scopes part - the current implementation requires a lot of back and forth. I'll review the PR today, and we can take a look at how to improve the scopes enforcement.

@ishank011
Copy link
Contributor

ishank011 commented Oct 14, 2021

@C0rby I see that you're trying to verify IDs lying inside a shared folder. Such expansion of scopes happens here

https://github.com/cs3org/reva/blob/master/internal/grpc/interceptors/auth/scope.go#L38

Apologies for the lack of documentation. I can create a separate PR to your PR to add that case.

@ishank011
Copy link
Contributor

@C0rby please take a look at C0rby#209
@wkloucek can you please check that apps still work after this? My tests seem to pass

David Christofas and others added 9 commits October 15, 2021 15:53
These changes are necessary to allow the user to use apps in public shares. Without these changes the requests would always fail because they couldn't be authenticated.
Up until now you couldn't see when a scope check failed because of a missing type assertion.
This solution is by far not pretty. But this is the smallest change I
could find to support apps in public shares without completely rewriting
the scopes code.
If we want to use any app in a public share the app needs an
authenticated context to access the files in the share. This is working
fine since we can authenticate using the share token. But the resulting
JWT contains a scope refering to the share resource. That means for a
folder share it contains the resource ID of the shared folder. But if
the app wants to access a specific file in the share it will request
that file by ID. Because of that the old way of the public scope checker
doesn't work because it would check if we are accessing the ID of the
shared folder and not if we are accessing any file in that folder.
Because of this problem I added two additional Stat requests so that I
can check if the requested file is inside the shared
folder.
@C0rby
Copy link
Contributor Author

C0rby commented Oct 15, 2021

We had to move certain requests to the extractRef method in the grpc interceptor but with that everything worked. 👍
From my PoV it's good to go.

@ishank011 ishank011 merged commit cddbdd4 into cs3org:master Oct 15, 2021
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.

3 participants