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

Discussion for better URLS: NOT MERGE #3261

Closed
wants to merge 27 commits into from

Conversation

labkode
Copy link
Member

@labkode labkode commented Mar 3, 2022

No description provided.

diocas and others added 27 commits March 2, 2022 21:43
Signed-off-by: Christian Richter <crichter@owncloud.com@>
Co-Authored-By: Christian Richter <crichter@owncloud.com@>
* update reva to include decomposedfs nodes-per-space

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* check create space with own constraint

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* unexpected passed

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* remove unused variable

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* revert reva commit id

Co-authored-by: Florian Schade <f.schade@icloud.com>
Co-Authored-By: Christian Richter <crichter@owncloud.com>
Co-Authored-By: Willy Kloucek <wkloucek@owncloud.com>
This copies the validation code from the accounts service, also fixing a
bug in the regex that allowed adding mail addresses with whitespace and
other problematic characters to the domain part of the mail address.

Partial fix for: owncloud#3247
Similar to what the accounts service is doing, all new users get
the User role assigned now. Otherwise creating the user's personal space
upon login is not working.
* bump web rc.1

* Use web v5.2.0-rc.2

Co-authored-by: Pascal Wengerter <pascal@wengerter.info>
Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.7.3 to 2.8.0.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/master/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.7.3...v2.8.0)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@labkode labkode changed the title Discussion for better URLS Discussion for better URLS: NOT MERGE Mar 3, 2022
| https://cernbox.cern.ch/external/files-public-files/eoshome-d!2639/Microsoft%20Office?contextRouteParams.item=sstLvrDFEpxwwxL&public-token=sstLvrDFEpxwwxL | https://cernbox.cern.ch/external/public/sstLvrDFEpxwwxL/New%20file.docx?app=microsoft-office&previous=/files/public/sstLvrDFEpxwwxL |


* We strip off `personal/home` as it does not bring any value to the URL and makes the URL relative to the current user, making it impossible to just copy and paste the URL to be reused by another user.
Copy link
Contributor

@micbar micbar Mar 4, 2022

Choose a reason for hiding this comment

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

Good point. The current implementation is still WIP. personal is used to determine which view to render, eg personal or projects. The plan is to rely on the GraphAPI /drives endpoint to distinguish spaces by driveType (personal, project).

home is legacy (resolves to the old webdav URL) and will be dropped when switching to aliases.

https://cloud.owncloud.com/files/spaces/personal/einstein/relative/path/to/file.txt.
Here, personal/einstein is the alias for Albert Einsteins personal space.

Example

A technical URL would look like

https://cernbox.cern.ch/files/spaces/95b4c6f6-d8f2-4976-b0a3-1b016bdfcb7e/d/dalvesde/relative/path/to/file.txt.

To make the URL human-readable we replace the UUID with an alias.
https://cernbox.cern.ch/files/spaces/eos/users/d/dalvesde/relative/path/to/file.txt

The WebUI discovers the spaces using the /drives Endpoint. For CERNBox it can return a space with the alias eos/users and the id:95b4c6f6-d8f2-4976-b0a3-1b016bdfcb7e. This aliasing allows the WebUI to use human-readable URLs and translate the alias back into a UUID which is then used to make the actual WebDAV Requests.

The problem with aliases is, that they are not unique, can be customized and might clash. Therefore the UUID is always appended as an URL Parameter ?id=95b4c6f6-d8f2-4976-b0a3-1b016bdfcb7e

Future CERNBox URL

https://cernbox.cern.ch/files/spaces/eos/users/d/dalvesde/relative/path/to/file.txt?id=95b4c6f6-d8f2-4976-b0a3-1b016bdfcb7e

Copy link
Contributor

Choose a reason for hiding this comment

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

Important regarding timeline: the ticket owncloud/web#6448 which is part of the current sprint (to be done by 18th of March) is a requirement for making the points above possible, because one part of the ticket is to switch over from hardcoded dav endpoints to fully relying on the GraphAPI /drives endpoint for discovering dav endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

The API Endpoint https://cernbox.cern.ch/graph/v1/me/drives can be implemented for CERNBox using a static list of existing storages.

Copy link
Contributor

Choose a reason for hiding this comment

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

The beauty of it is the forward compatibility to a EOS driver which implements spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact you will be stuck with the cernbox web fork after owncloud/web#6448 is done and you don't provide the static list of existing storages that @micbar mentioned.

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

See comments



* We strip off `personal/home` as it does not bring any value to the URL and makes the URL relative to the current user, making it impossible to just copy and paste the URL to be reused by another user.
* The files app works with slashes (files/spaces/personal/home/) and the application works with dashes (files-spaces-personal-home). We find no sense to this distinction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Having the origin route name in the path of app routes is only an intermediate solution. We plan to move the origin route name to a query parameter. For apps in the web monorepo this is already very easy and straightforward to use.

The origin route name is needed to go back to where the user came from when closing an app (e.g. when closing the mediaviewer, going back to a public link or the all files page respectively).

* We strip off `personal/home` as it does not bring any value to the URL and makes the URL relative to the current user, making it impossible to just copy and paste the URL to be reused by another user.
* The files app works with slashes (files/spaces/personal/home/) and the application works with dashes (files-spaces-personal-home). We find no sense to this distinction.
Applications should not behave differently that the files app (another application).
* We find the usage of query parameters `contextRouteParams.*` confusing and we prefer to opt for a more natural value (the previous url) with the `previous` query parameter. Note that this query parameter should only be used for applications rendered inside the Web (not for external applications opening in a tab).
Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to give it a different name. I'd opt for origin, as previous implies a sequence.

In general, urls should be of the type:

* `<app-name>/</namespaced/alias></relative/path/to/resource>` or
* `<app-name>/public/<token></relative/path/to/resource>` for public links (allowing to distinguish if they need to be authenticated or not)
Copy link
Contributor

Choose a reason for hiding this comment

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

We already discussed dedicated public application routes with you in the past and opted for the solution of providing the origin route name via URL. It was a conscious design decision for oCIS to not make a difference between public and private links anymore. Actively distinguishing that in the path feels wrong to us.

Also, regarding developer experience, it seems to be an unnecessary effort for developers to define routes twice (once for authenticated, once for public context). When we have the origin route name in the query parameters it will be as simple as using a small set of composables in your self developed app to get the authentication, file retrieval and back-navigation basically for free. At the moment this is only possible for apps in the web monorepo.

@settings settings bot removed the p3-medium label Mar 11, 2022
@butonic butonic marked this pull request as draft May 20, 2022 13:01
@butonic butonic added the Priority:p2-high Escalation, on top of current planning, release blocker label Jun 10, 2022
@dschmidt
Copy link
Member

As far as I can tell, this is completely outdated as we implemented most of the ideas here already or chose different approaches in the CERN Web Merge project.

If there's anything still relevant, please open a new PR :)

@dschmidt dschmidt closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform:Web Priority:p2-high Escalation, on top of current planning, release blocker Status:Accepted Type:Design
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants