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

Add alias links to web UI #7133

Merged
merged 4 commits into from
Aug 9, 2022
Merged

Add alias links to web UI #7133

merged 4 commits into from
Aug 9, 2022

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Jun 15, 2022

Description

Cleaning up my local git stash and came across this draft. Needs concepting how to properly turn this into an oCIS-only feature. Update: alias-link capability has landed in oCIS

Related Issue

@pascalwengerter
Copy link
Contributor Author

This does now work on the link-creator side, but I run into problems when trying to open the (permissionless) link - I'd expect it to render the login form, but it shows an error page (with a 401 when listing drives)

@pascalwengerter pascalwengerter changed the title Add draft for permissionless/internal/alias-link shares for oCIS Add alias links to web UI Jun 29, 2022
@kulmann
Copy link
Member

kulmann commented Jul 7, 2022

@pascalwengerter I took the liberty of rebasing this PR, as the public link resolving has changed in the web runtime today (see #7072 for reference). However, whenever I want to create an internal link I get invalid permissions from the backend. Can you confirm that?

@pascalwengerter
Copy link
Contributor Author

pascalwengerter commented Jul 14, 2022

@pascalwengerter I took the liberty of rebasing this PR, as the public link resolving has changed in the web runtime today (see #7072 for reference). However, whenever I want to create an internal link I get invalid permissions from the backend. Can you confirm that?

Did some investigation now and can confirm it doesn't work - I thought we had implemented the necessary changes in the SDK already, but getting an invalid permissions webdav response when trying to sending e.g. a "0" as zero-permission (sending nothing doesn't work because that is the standard behavior when updating links without touching permissions). I've opened an issue in the SDK

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter I took the liberty of rebasing this PR, as the public link resolving has changed in the web runtime today (see #7072 for reference). However, whenever I want to create an internal link I get invalid permissions from the backend. Can you confirm that?

Did some investigation now and can confirm it doesn't work - I thought we had implemented the necessary changes in the SDK already, but getting an invalid permissions webdav response when trying to sending e.g. a "0" as zero-permission (sending nothing doesn't work because that is the standard behavior when updating links without touching permissions). I've opened an issue in the SDK

Turns out sending a 0 permission is enough, it does work with cs3org/reva#3079 (stopped CI, this PR needs a rebase & review once Julian's PR is merged in reva and then in oCIS and the oCIS commit ID is bumped)!

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter I took the liberty of rebasing this PR, as the public link resolving has changed in the web runtime today (see #7072 for reference). However, whenever I want to create an internal link I get invalid permissions from the backend. Can you confirm that?

Did some investigation now and can confirm it doesn't work - I thought we had implemented the necessary changes in the SDK already, but getting an invalid permissions webdav response when trying to sending e.g. a "0" as zero-permission (sending nothing doesn't work because that is the standard behavior when updating links without touching permissions). I've opened an issue in the SDK

Turns out sending a 0 permission is enough, it does work with cs3org/reva#3079 (stopped CI, this PR needs a rebase & review once Julian's PR is merged in reva and then in oCIS and the oCIS commit ID is bumped)!

Ideally we also get an E2E test for alias links in oCIS to make sure they don't break unnoticed in the near future

@kulmann
Copy link
Member

kulmann commented Jul 21, 2022

This PR handles creating internal / permissionless links, that works with updated ocis/reva as you mentioned. Respective commit id has been bumped in web already, so a rebase was sufficient. But: resolving an internal link is not handled in the public link resolve page. I'd like to merge this PR but flip the default of the capability to internal links being disabled, until we implement resolving internal links in a separate PR. Do you agree @pascalwengerter ?

@pascalwengerter
Copy link
Contributor Author

This PR handles creating internal / permissionless links, that works with updated ocis/reva as you mentioned. Respective commit id has been bumped in web already, so a rebase was sufficient. But: resolving an internal link is not handled in the public link resolve page. I'd like to merge this PR but flip the default of the capability to internal links being disabled, until we implement resolving internal links in a separate PR. Do you agree @pascalwengerter ?

Please do push to this PR as you deem necessary, I don't have any preference on how to proceed here ;)

@JammingBen
Copy link
Collaborator

I rebased and updated the PR to go along with current master. No idea why the condition coverage does not cover availableRoleOptions() though. Locally, the tests call this method and reach the updated return statement 🤷

Besides that, this should be ready for review. LGTM to me, although I'm not too invested in the topic.

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.

Some nitpicks (see comments). Also: please update the ocis commit id in .drone.env to one after the alias capability value has changed in ocis.

@JammingBen JammingBen requested a review from kulmann August 3, 2022 11:50
@JammingBen
Copy link
Collaborator

JammingBen commented Aug 3, 2022

Arghs I forgot the .drone.env, coming up next. Edit: Done.

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2022

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

65.5% 65.5% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit 38fb08c into master Aug 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the permissionsless-links branch August 9, 2022 14:52
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