Fix download of public annotation, include access ctx in user cache key #7025
Conversation
| result | ||
| } | ||
|
|
||
| def retrieve(loginInfo: LoginInfo): Future[Option[User]] = | ||
| userDAO.findOne(ObjectId(loginInfo.providerKey))(GlobalAccessContext).futureBox.map(_.toOption) | ||
| findOneCached(ObjectId(loginInfo.providerKey))(GlobalAccessContext).futureBox.map(_.toOption) |
There was a problem hiding this comment.
I noticed that this didn’t use the cache. I think it is safe for it to do so now. This method is used by silhouette when authenticating requests. This change may save us quite a few database queries
|
| @@ -411,7 +411,7 @@ Expects: | |||
| skeletonAnnotationLayer => | |||
| tracingStoreClient.getSkeletonTracing(skeletonAnnotationLayer, skeletonVersion) | |||
| } ?~> "annotation.download.fetchSkeletonLayer.failed" | |||
| user <- userService.findOneCached(annotation._user) ?~> "annotation.download.findUser.failed" | |||
| user <- userService.findOneCached(annotation._user)(GlobalAccessContext) ?~> "annotation.download.findUser.failed" | |||
There was a problem hiding this comment.
So, this failed before (ignoring the caching issue) because the requesting user didn't have access to the owning user?
Does this mean that an annotation owned by user A can be downloaded by user B (if B has access to it) even though user B might not be allowed to "read" user A? Because user A needs to be serialized into the annotation?
I'm wondering whether this circumvents the permission system a bit?
There was a problem hiding this comment.
Yes, that’s true. However, not all info about user A is serialized into the annotation. I believe we had a discussion about this some time ago and decided that it would be fair to say that a user publishing an annotation publishes their name with it. I’m open to having that discussion again, though. Also it might be worth double-checking what info is really needed for wk to show an annotation. The download only includes the name, but i think the annotation info request (which already works in the same way before this PR) also includes the email address.
There was a problem hiding this comment.
Ok, fair enough 👍
Also it might be worth double-checking what info is really needed for wk to show an annotation.
Yes, this is a good idea. One could also put in a dummy email value à la anonymous.
…ove_wkconnect * 'master' of github.com:scalableminds/webknossos: Update docker compose commands + dev install readme (#7002) Add segment groups (#6966) Add screenshot nightly test for wkorg (#7030) Workaround for WebGL crash for datasets with many segmentation layers (#6995) Fix download of public annotation, include access ctx in user cache key (#7025) Fix that changing a segment color could lead to a crash (#7000) Add more error chaining to annotation download (#7023) Guard against NaNs in shader (#7018) Store editable mappings in multiple fossildb columns+keys (#6903) Context action to move tree to group (#7005) Release 23.05.0 (#7014) Remove vault cache when reloading dataset (#7007) Fix viewing of public datasets (#7010) Update screenshots scalebar positioning (#7003) Update team members (#6999)
…ty-list-drawings * 'master' of github.com:scalableminds/webknossos: (25 commits) Fix issues with styling in dark mode on login page (#7052) Fix nightly by setting missing token (#7048) Release 23.05.1 (#7042) DRY types in update_actions.ts (#7036) Remove some spammy logging from backend (#7039) Use zarr string fill values (#7017) Fix voxel offset for Neuroglancer Precomputed datasets (#7019) Log when user is activated (#7027) Fix exception in applying UpdateTreeGroupVisibility skeleton action (#7037) Fix organization storage layouting (#7034) Update docker compose commands + dev install readme (#7002) Add segment groups (#6966) Add screenshot nightly test for wkorg (#7030) Workaround for WebGL crash for datasets with many segmentation layers (#6995) Fix download of public annotation, include access ctx in user cache key (#7025) Fix that changing a segment color could lead to a crash (#7000) Add more error chaining to annotation download (#7023) Guard against NaNs in shader (#7018) Store editable mappings in multiple fossildb columns+keys (#6903) Context action to move tree to group (#7005) ...
Fetching the user object to write the NML failed for logged-out users, since it was missing the GlobalAccessContext modifier. However, the user cache was independent of the access context, making this work sometimes nonetheless (except if the cache was expired). Instead, the user cache should take the access context as cache key, avoiding leaking user info in unexpected contexts.
Steps to test:
/api/annotations/:id/downloadIssues: