[MM-54359] Implement session validation check #620
Conversation
streamer45
added
2: Dev Review
| @@ -48,6 +48,10 @@ const ( | |||
| wsReconnectionTimeout = 10 * time.Second | |||
| ) | |||
|
|
|||
| var ( | |||
| sessionAuthCheckInterval = 10 * time.Second | |||
There was a problem hiding this comment.
@esarafianou For performance reasons I wouldn't go lower than this, let me know if it's reasonable.
server/websocket.go
Outdated
| continue | ||
| } | ||
|
|
||
| if s, appErr := p.API.GetSession(authSessionID); appErr != nil || time.Now().UnixMilli() >= s.ExpiresAt { |
There was a problem hiding this comment.
Do you have a sense of what kind of pressure this is going to put on the system?
Is there a batched version of getSession?
There was a problem hiding this comment.
Do you have a sense of what kind of pressure this is going to put on the system?
Performance should be fairly good since we cache sessions so normally that call shouldn't be hitting the database but yeah that's a concern I had as well.
Is there a batched version of getSession?
I don't believe so, definitely not exposed to the plugin side anyway.
There was a problem hiding this comment.
@cpoile To add some context, in general I felt this to be the safest approach because otherwise we'd have to implement a plugin hook that triggers on session invalidation which can definitely improve performance but it's also much easier to mess up security wise.
If performance ever becomes a problem I'd probably go down that route.
|
Sent a fix e2e tests caught. We should also support sessions with a zero |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
=======================================
+ Coverage 0 9.33% +9.33%
=======================================
Files 0 26 +26
Lines 0 5293 +5293
=======================================
+ Hits 0 494 +494
- Misses 0 4747 +4747
- Partials 0 52 +52 ☔ View full report in Codecov by Sentry. |
| @@ -379,7 +383,10 @@ func (p *Plugin) OnWebSocketDisconnect(connID, userID string) { | |||
| } | |||
| } | |||
|
|
|||
| func (p *Plugin) wsReader(us *session, handlerID string) { | |||
| func (p *Plugin) wsReader(us *session, authSessionID, handlerID string) { | |||
There was a problem hiding this comment.
Is there a relevant PR in the mattermost repo that will be sending the authSessionID?
There was a problem hiding this comment.
Indeed, that's the one I mentioned in the internal thread: mattermost/mattermost#25928
streamer45
added
the
3: Reviews Complete
Summary
PR implements a session validation check to forcefully disconnect any calls client that has either their session revoked or expired.
Ticket Link
https://mattermost.atlassian.net/browse/MM-54359