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

URLs with expired sharing token prevent logged in users from editing #7309

Closed
3 tasks
hotzenklotz opened this issue Sep 5, 2023 · 4 comments · Fixed by #8139
Closed
3 tasks

URLs with expired sharing token prevent logged in users from editing #7309

hotzenklotz opened this issue Sep 5, 2023 · 4 comments · Fixed by #8139

Comments

@hotzenklotz
Copy link
Member

hotzenklotz commented Sep 5, 2023

Context

Someone shared a WK shortened link to an annotation with me. The annotation URL contains an (expired) token. Since I was logged in, I was able to open the annotation nonetheless. In this particular case the annotation was "shared" and I wanted to edit it. WK wrongly assumed I can not make any changes since the token was expired. I get a lot of warnings.

image

A likely fix could be that WK checks whether one has access to an annotation regardless if the URL has a token or not.

Expected Behavior

I should be able to edit any annotation that my user has access rights to regardless through which URL I opened it from.

Current Behavior

A (team)-shared annotation open from a shortened WK link (e.g. https://webknossos.org/links/abc) including a expired token does not let me make any changes.

Steps to Reproduce the bug

  1. Create a new shared annotation
  2. Share a link to annotation (WK will automagically include a token)
  3. Expire the token (delete from DB?)
  4. Log in, open the annotation through the shared URL
  5. Start editing --> boom
  • Cannot reproduce the bug anymore / needs deeper investigation.

Your Environment for bug

  • Browser name and version: Chrome 116
  • Operating System and version: MacOS
  • Version of WEBKNOSSOS (Release or Commit): 24599 (September 2023)
  • Specific to long-running jobs (set jobsEnabled=true in application.conf)
  • Specific to webknossos.org (set isDemoInstance=true in application.conf)
@fm3
Copy link
Member

fm3 commented Dec 5, 2023

The tracingstore auth always uses a token, cookies are not sent there. I think the frontend decides which token to send (probably the uri token has precedence).

I see a couple of options to fix this

  • (1) We could send multiple tokens to the backend (Needs changes in all tracingstore/datastore routes, though)
  • (2) The frontend could ping a separate route asking if a token is expired, and if so, discard it and send the user’s token instead
  • (3) The frontend could retry failing routes with the user’s own token

@philippotto do you have opinions on this? I guess 2 is the most viable option. There are certainly also other ways of solving this that I couldn’t think of just now

@philippotto
Copy link
Member

TLDR: A mix of 2 and 3 would probably work.

At first, I thought, (3) is the easiest, because we already have a token-retry mechanism. Currently, that mechanism is only used for non-url tokens (which have precedence as you assumed correctly), but it could easily be expanded, so that url tokens are replaced by user tokens.

Then, I noticed the following problem:
The retry-mechanism triggers when a 403 status code is detected. If the user does something which they simply cannot do (regardless of url token or private one), we will discard the url token and then use the own token from that point on. If the own token has less permissions, this will be a problem.

A potential solution for this would be to let the front-end know that the url token is really expired (this means the token is absolutely worthless and can be discarded). Either this is done by setting a special response header (or is there a dedicated status code?) OR by having a separate route which can be asked about this.

What do you think?

@fm3
Copy link
Member

fm3 commented Dec 6, 2023

Yeah, sounds about right. For the backend side it would certainly be easier to add another route so that the frontend can query this expired-ness. But it would also be feasible to introduce another status code or header in this case. I’d have to think about how to propagate that info from the webknossos backend to the datastore and tracingstore so that they can reply with that specific status.

@philippotto
Copy link
Member

Yeah, sounds about right. For the backend side it would certainly be easier to add another route so that the frontend can query this expired-ness.

Then, let's do this. The front-end can query that route in case the url-token causes 403s. I think, that's fine :) Should be rare enough to justify the extra request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants