Skip to content
This repository was archived by the owner on Mar 16, 2024. It is now read-only.

change: default watchsession timeout of 1 hour #3

Draft
wants to merge 1 commit into
base: acorn
Choose a base branch
from

Conversation

iwilltry42
Copy link

As per standup discussion, we want to drop eternal WatchSessions.
Since re-evaluation of AuthZ during the websocket loop proved to be too complex, we re-considered having a general timeout for watch sessions.
This doesn't explicitly fix the underlying issue https://github.com/acorn-io/manager/issues/857, since in the worst case one would still be able to see updates in the UI for one hour if the WatchSession was started right before the token expired / got deleted, but that's OK for now (no destructive or intrusive actions can be taken anyway).

@vincent99
Copy link

vincent99 commented Oct 27, 2023

I don't see how this is improving anything.

  • It doesn't provide any information to the consumer (ui) about what's happening. So I'm still just going to keep trying to reconnect over and over after the session has expired.

  • It guarantees unnecessary drop, reconnect, and possibly expensive resync operations for the ui every hour during what are still perfectly valid sessions.

  • Doing the wrong thing for security, but for slightly less time, is still the wrong thing.

If you looked up and used the remaining life of the token as the timeout, that would at least address 2 & part of 3 (normal token life, but not early deletes).

And if it helps, signaling for 1 could be done by sending a name: "unauthorized" message on the new connection when (re)connecting and finding the token is no longer valid. Instead of trying to get a last frame into the existing connection before you close it.

@iwilltry42
Copy link
Author

iwilltry42 commented Oct 27, 2023

Let's get @ibuildthecloud into this discussion, since we discussed this yesterday.

It doesn't provide any information to the consumer (ui) about what's happening. So I'm still just going to keep trying to reconnect over and over after the session has expired.

But the next Subscribe will should return a 401, right?

Doing the wrong thing for security, but for slightly less time, is still the wrong thing.

I agree with this.

If you looked up and used the remaining life of the token as the timeout, that would at least address 2 & part of 3 (normal token life, but not early deletes).

This was discussed, but the decision was to keep it stupid and simple.

The normal token life is 7 days.

The only ways of doing this that I came up with before the meeting was

  1. Passing around the expiration time in the userInfo from token creation time
  2. Adding another watch for tokens, which requires registering the respective schema
  3. Using something like a middleware that has a client and can actually actively lookup anything

And if it helps, signaling for 1 could be done by sending a name: "unauthorized" message on the new connection when (re)connecting and finding the token is no longer valid. Instead of trying to get a last frame into the existing connection before you close it.

As per above, I think we should return 401 even before upgrading to the socket connection, if the token is invalid.

@vincent99
Copy link

But the next Subscribe will should return a 401, right?

Sure it probably does, but that is invisible to me. You cannot get any HTTP-level information out out of the browser WebSocket client. It's stupid, but that's the way it works. So your internet being down, a 200 because your company's SSL proxy ate the upgrade, a 401 because you're not logged in, a 500 because we're down, etc are all the same. I get one of bit of info, with no other detail available. "opened" or "error".

The normal token life is 7 days.

UI sessions are supposed to be 16 hours, though that doesn't seem currently true…

@thedadams
Copy link

thedadams commented Oct 27, 2023

UI sessions are supposed to be 16 hours, though that doesn't seem currently true…

We (temporarily) changed the sessions to last a week for the UI. I don't remember the exact reason, but I believe it was related to this.

@cjellick
Copy link
Member

cjellick commented Jan 3, 2024

@iwilltry42 im going to move this to draft since its sat for so long. if you dont envision it moving forward, please close

@cjellick cjellick marked this pull request as draft January 3, 2024 21:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants