Skip to content

Conversation

leowe
Copy link
Contributor

@leowe leowe commented Aug 2, 2022

URL of deployed dev instance (used for testing):

Steps to test (frontend):

  • create an annotation
  • use the dropdown next to the save button and select "zarr links"
  • create a link
  • edit the expiration date
  • remove the link
  • etc

Steps to test (backend):

First step: Get access token

  • create a new annotation in the UI
  • take the annotation ID (e.g. from the URL)
  • call POST localhost:9000/api/annotations/zarr_private with a body like:
{
    "_annotation": "<annotationId>",
    "expirationDateTime": null
}
  • in the response you get a body with the corresponding information, like
{
    "_id": {
        "id": "62ebca5ee401004f04276595"
    },
    "_annotation": {
        "id": "62ebc6cfe40100a402276591"
    },
    "accessToken": "aw-Kd5TVGosY4sTOuAFnJbNWRQSpho9o",
    "isDeleted": false
}

The relevant part here is the accessToken.

  • You can similarly test the update, delete, and get routes

Second step: Using the private link endpoints

  • There are all the endpoints for zarr_private that also exists for the "normal" zarr routes
  • a good first candidate for testing is the datasource-properties.json, since it directly shows whether the requests to the tracing-store and the internal request on the datastore works, it also checks all the access rights and does the lookup from the accessToken to the annotation on webknossos
  • request looks like this: GET localhost:9000/data/zarr_private/<accessToken>/datasource-properties.json
  • You can also check the data route (GET /zarr_private/:accessToken/:dataLayerName/:mag/:cxyz), see Zarr streaming editable mapping support #6310 for ideas on how to check this

Issues:


(Please delete unneeded items, merge only when none are left open)

leowe added 4 commits August 4, 2022 10:45
# Conflicts:
#	CHANGELOG.unreleased.md
#	app/controllers/AnnotationController.scala
#	app/controllers/UserTokenController.scala
#	app/models/annotation/AnnotationService.scala
#	tools/postgres/schema.sql
#	webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ZarrStreamingController.scala
#	webknossos-datastore/conf/com.scalableminds.webknossos.datastore.routes
#	webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes
@leowe leowe marked this pull request as ready for review August 4, 2022 13:50
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Wow, this is quite a piece of work 💪 🙂 Thanks a lot! I added a couple of notes to the code. I’ll do testing in the next round, please ping me again after you adressed the feedback 🙂

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice work!

  • @philippotto I think you mentioned that you wanted to add the possibility to share layers via zarr links in View mode. I would also vote to allow that if it's easily possible.

  • @leowe One question regarding permissions. Who is allowed to create these sharing links? Anyone who can see a layer/an annotation? I'm thinking about the use case where someone shared an annotation with me privately via link and then later revokes the link. If I created a private zarr link in between, would I still be able to access the annotation?

  • Also, the skeleton is probably never shared, right? Maybe the text in the sharing modal could be adapted to make it more clear what exactly is being shared 🤔

@philippotto
Copy link
Member

@leowe I think this PR only includes the forward sql migration, but the reversion is missing. Is this intended?

@philippotto
Copy link
Member

The missing front-end tasks should be done now. I added the dataset zarr link to the existing sharing modal for datasets, as I think, it fits there best.

@fm3
Copy link
Member

fm3 commented Aug 22, 2022

I think this PR only includes the forward sql migration, but the reversion is missing. Is this intended?

Hmm that may also have been caused by a merge. At least leo did mark this conversation as resolved some time ago #6367 (comment)

@leowe
Copy link
Contributor Author

leowe commented Aug 22, 2022

  • @leowe One question regarding permissions. Who is allowed to create these sharing links? Anyone who can see a layer/an annotation? I'm thinking about the use case where someone shared an annotation with me privately via link and then later revokes the link. If I created a private zarr link in between, would I still be able to access the annotation?

The permission check right now is against the annotation owner, i.e. you can only create / update / delete private links when you own the annotation.

@leowe
Copy link
Contributor Author

leowe commented Aug 22, 2022

I think this PR only includes the forward sql migration, but the reversion is missing. Is this intended?

Hmm that may also have been caused by a merge. At least leo did mark this conversation as resolved some time ago #6367 (comment)

Yes that seemed to have gone missing somewhere... Added it again.

@leowe
Copy link
Contributor Author

leowe commented Aug 22, 2022

I added all the changes that were requested for the backend, i.e.:

  • halved token length to 12 chars
  • removed duplicate layer names from occurring (can happen when volume layer has the same name as segmentation layer), did NOT remove the segmentation if it is the fallback layer for an annotation layer, since that information is not available in the datastore easily (the change would cause a significant overhead)
  • changed deleted link responses to 404s
  • added metadata files for directory listings similar to the other zarr routes
  • unified printed mag format in directory listings
  • usage of tracing name instead of tracingID in the datasource properties file
  • added swagger annotations for the wk endpoints for managing the private links

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Frontend LGTM 👍

leowe added 3 commits August 23, 2022 10:06
# Conflicts:
#	CHANGELOG.unreleased.md
#	app/models/binary/DataSetService.scala
#	webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ZarrStreamingController.scala
#	webknossos-datastore/app/com/scalableminds/webknossos/datastore/jzarr/ZarrHeader.scala
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

LGTM! I also did some testing again, seemed to work as intended :) Thanks for your persistence with this!

@leowe leowe merged commit 675f7ef into master Aug 23, 2022
@leowe leowe deleted the zarr-private-links branch August 23, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mapping and other parameters for zarr streaming

7 participants